> From: Yishai Hadas <yishaih@xxxxxxxxxx> > 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.
