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

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

> From: Yishai Hadas <yishaih at nvidia.com>
> Sent: Tuesday, October 10, 2023 9:14 PM
> 
> 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.
>
I think,
For #3 the virtio level API signature should be,

virtio_admin_legacy_xyz_cmd(struct virtio_device *dev, u64 group_member_id,
....);

This maintains the right abstraction needed between vfio, generic virtio and
virtio transport (pci) layer.

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.
> 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 06:43:32PM +0300, Yishai Hadas
wrote:
> > 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.
And to loop back into my previous discussion: that's the fundamental
problem here.  If it is owned by the virtio subsystem, which just
calls into vfio we would not have this problem, including the
circular loops and exposed APIs.