Nouveau - Oct 2024 - [RFC 18/29] nvkm/vgpu: introduce pci_driver.sriov_configure() in nvkm

On 27/09/2024 1.56, Jason Gunthorpe wrote:
> On Sun, Sep 22, 2024 at 05:49:40AM -0700, Zhi Wang wrote:
> 
>> diff --git a/include/drm/nvkm_vgpu_mgr_vfio.h
b/include/drm/nvkm_vgpu_mgr_vfio.h
>> index d9ed2cd202ff..5c2c650c2df9 100644
>> --- a/include/drm/nvkm_vgpu_mgr_vfio.h
>> +++ b/include/drm/nvkm_vgpu_mgr_vfio.h
>> @@ -6,8 +6,13 @@
>>   #ifndef __NVKM_VGPU_MGR_VFIO_H__
>>   #define __NVKM_VGPU_MGR_VFIO_H__
>>   
>> +enum {
>> +	NVIDIA_VGPU_EVENT_PCI_SRIOV_CONFIGURE = 0,
>> +};
>> +
>>   struct nvidia_vgpu_vfio_handle_data {
>>   	void *priv;
>> +	struct notifier_block notifier;
>>   };
> 
> Nothing references this? Why would you need it?
> 
Oops, these are the leftovers of the discard changes. Will remove them 
accordingly in the next iteration. Thanks so much for catching this.
> It looks approx correct to me to just directly put your function in
> the sriov_configure callback.
> 
> This is the callback that indicates the admin has decided to turn on
> the SRIOV feature.
Turning on the SRIOV feature is just a part of the process enabling a 
vGPU. The VF is not instantly usable before a vGPU type is chosen via 
another userspace interface (e.g. fwctl).

Besides, admin has to enable the vGPU support by some means (e.g. a 
kernel parameter is just one candidate) and GSP firmware needs to be 
configured accordingly when being loaded.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 

As this is related to user space interface, I am leaning towards putting 
some restriction/checks for the pre-condition in the 
driver.sriov_configure(), so admin would know there is something wrong 
in his configuration as early as possible, instead of he failed to 
creating vGPUs again and again, then he found he forgot to enable the 
vGPU support.

Thanks,
Zhi.
> Jason

On Mon, Oct 14, 2024 at 08:32:03AM +0000, Zhi Wang wrote:
> Turning on the SRIOV feature is just a part of the process enabling a 
> vGPU. The VF is not instantly usable before a vGPU type is chosen via 
> another userspace interface (e.g. fwctl).
That's OK, that has become pretty normal now that VFs are just empty
handles when they are created until they are properly profiled.
> Besides, admin has to enable the vGPU support by some means (e.g. a 
> kernel parameter is just one candidate) and GSP firmware needs to be 
> configured accordingly when being loaded.
Definitely not a kernel parameter..
> As this is related to user space interface, I am leaning towards putting 
> some restriction/checks for the pre-condition in the 
> driver.sriov_configure(), so admin would know there is something wrong 
> in his configuration as early as possible, instead of he failed to 
> creating vGPUs again and again, then he found he forgot to enable the 
> vGPU support.
Well, as I've said, this is poor, you shouldn't have a FW SRIOV enable
bit at all, or at least it shouldn't be user configurable.

If the PCI function supports SRIOV then it should work to turn it on.

Jason