Linux Virtualization - Oct 2023 - [PATCH vfio 10/11] vfio/virtio: Expose admin commands over virtio device

On 10/10/2023 18:58, Michael S. Tsirkin wrote:
> On Tue, Oct 10, 2023 at 06:43:32PM +0300, Yishai Hadas wrote:
>> 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.
> On pci_dev of vf, yes? So again just move this into each command,
> that's all. I.e. pass pci_dev to each.
How about the cyclic dependencies issue inside VIRTIO that I mentioned? 
below ?

In my suggestion it's fine, VFIO will get the PF and give it to VIRTIO 
per command.

Yishai
>> 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

On Tue, Oct 10, 2023 at 07:09:08PM +0300, Yishai Hadas
wrote:
> 
> > > 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.
I just mean:
virtio_admin_legacy_io_write(sruct pci_device *,  ....)


internally it starts from vf gets the pf (or vf itself or whatever
the transport is) sends command gets status returns.

what is cyclic here?

-- 
MST