Re: [PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

[Yishai Hadas <yishaih@nvidia.com>]

On 10/10/2023 18:14, Michael S. Tsirkin wrote:
> On Tue, Oct 10, 2023 at 06:09:44PM +0300, Yishai Hadas wrote:
>> On 10/10/2023 17:54, Michael S. Tsirkin wrote:
>>> On Tue, Oct 10, 2023 at 11:08:49AM -0300, Jason Gunthorpe wrote:
>>>> On Tue, Oct 10, 2023 at 09:56:00AM -0400, Michael S. Tsirkin wrote:
>>>>
>>>>>> However - the Intel GPU VFIO driver is such a bad experiance I don't
>>>>>> want to encourage people to make VFIO drivers, or code that is only
>>>>>> used by VFIO drivers, that are not under drivers/vfio review.
>>>>> So if Alex feels it makes sense to add some virtio functionality
>>>>> to vfio and is happy to maintain or let you maintain the UAPI
>>>>> then why would I say no? But we never expected devices to have
>>>>> two drivers like this does, so just exposing device pointer
>>>>> and saying "use regular virtio APIs for the rest" does not
>>>>> cut it, the new APIs have to make sense
>>>>> so virtio drivers can develop normally without fear of stepping
>>>>> on the toes of this admin driver.
>>>> Please work with Yishai to get something that make sense to you. He
>>>> can post a v2 with the accumulated comments addressed so far and then
>>>> go over what the API between the drivers is.
>>>>
>>>> Thanks,
>>>> Jason
>>> /me shrugs. I pretty much posted suggestions already. Should not be hard.
>>> Anything unclear - post on list.
>>>
>> Yes, this is the plan.
>>
>> We are working to address the comments that we got so far in both VFIO &
>> VIRTIO, retest and send the next version.
>>
>> Re the API between the modules, It looks like we have the below
>> alternatives.
>>
>> 1) Proceed with current approach where we exposed a generic API to execute
>> any admin command, however, make it much more solid inside VIRTIO.
>> 2) Expose extra APIs from VIRTIO for commands that we can consider future
>> client usage of them as of LIST_QUERY/LIST_USE, however still have the
>> generic execute admin command for others.
>> 3) Expose API per command from VIRTIO and fully drop the generic execute
>> admin command.
>>
>> Few notes:
>> Option #1 looks the most generic one, it drops the need to expose multiple
>> symbols / APIs per command and for now we have a single client for them
>> (i.e. VFIO).
>> Options #2 & #3, may still require to expose the virtio_pci_vf_get_pf_dev()
>> API to let VFIO get the VIRTIO PF (struct virtio_device *) from its PCI
>> device, each command will get it as its first argument.
>>
>> Michael,
>> What do you suggest here ?
>>
>> Thanks,
>> Yishai
> I suggest 3 but call it on the VF. commands will switch to PF
> internally as needed. For example, intel might be interested in exposing
> admin commands through a memory BAR of VF itself.
>
The driver who owns the VF is VFIO, it's not a VIRTIO one.

The ability to get the VIRTIO PF is from the PCI device (i.e. struct 
pci_dev).

In addition,
virtio_pci_vf_get_pf_dev() was implemented for now in virtio-pci as it 
worked on pci_dev.
Assuming that we'll put each command inside virtio as the generic layer, 
we won't be able to call/use this API internally to get the PF as of 
cyclic dependencies between the modules, link will fail.

So in option #3 we may still need to get outside into VFIO the VIRTIO PF 
and give it as pointer to VIRTIO upon each command.

Does it work for you ?

Yishai