Re: [PATCH V7 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Yishai Hadas <yishaih@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	jasowang@redhat.com, jgg@nvidia.com, kvm@vger.kernel.org,
	virtualization@lists.linux-foundation.org, parav@nvidia.com,
	feliu@nvidia.com, jiri@nvidia.com, kevin.tian@intel.com,
	joao.m.martins@oracle.com, si-wei.liu@oracle.com,
	leonro@nvidia.com, maorg@nvidia.com
Subject: Re: [PATCH V7 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices
Date: Thu, 14 Dec 2023 11:40:03 -0500	[thread overview]
Message-ID: <20231214113649-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <c838ad5a-e6ba-4b8e-a9a2-5d43da0ba5a4@nvidia.com>

On Thu, Dec 14, 2023 at 06:25:25PM +0200, Yishai Hadas wrote:
> On 14/12/2023 18:15, Alex Williamson wrote:
> > On Thu, 14 Dec 2023 18:03:30 +0200
> > Yishai Hadas <yishaih@nvidia.com> wrote:
> > 
> > > On 14/12/2023 17:05, Michael S. Tsirkin wrote:
> > > > On Thu, Dec 14, 2023 at 07:59:05AM -0700, Alex Williamson wrote:
> > > > > On Thu, 14 Dec 2023 11:37:10 +0200
> > > > > Yishai Hadas <yishaih@nvidia.com> wrote:
> > > > > > > > OK, if so, we can come with the below extra code.
> > > > > > > > Makes sense ?
> > > > > > > > 
> > > > > > > > I'll squash it as part of V8 to the relevant patch.
> > > > > > > > 
> > > > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c
> > > > > > > > b/drivers/virtio/virtio_pci_modern.c
> > > > > > > > index 37a0035f8381..b652e91b9df4 100644
> > > > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > > > @@ -794,6 +794,9 @@ bool virtio_pci_admin_has_legacy_io(struct pci_dev
> > > > > > > > *pdev)
> > > > > > > >            struct virtio_device *virtio_dev = virtio_pci_vf_get_pf_dev(pdev);
> > > > > > > >            struct virtio_pci_device *vp_dev;
> > > > > > > > 
> > > > > > > > +#ifndef CONFIG_X86
> > > > > > > > +       return false;
> > > > > > > > +#endif
> > > > > > > >            if (!virtio_dev)
> > > > > > > >                    return false;
> > > > > > > > 
> > > > > > > > Yishai
> > > > > > > 
> > > > > > > Isn't there going to be a bunch more dead code that compiler won't be
> > > > > > > able to elide?
> > > > > > 
> > > > > > On my setup the compiler didn't complain about dead-code (I simulated it
> > > > > > by using ifdef CONFIG_X86 return false).
> > > > > > 
> > > > > > However, if we suspect that some compiler might complain, we can come
> > > > > > with the below instead.
> > > > > > 
> > > > > > Do you prefer that ?
> > > > > > 
> > > > > > index 37a0035f8381..53e29824d404 100644
> > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > @@ -782,6 +782,7 @@ static void vp_modern_destroy_avq(struct
> > > > > > virtio_device *vdev)
> > > > > >             BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ) | \
> > > > > >             BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO))
> > > > > > 
> > > > > > +#ifdef CONFIG_X86
> > > > > >     /*
> > > > > >      * virtio_pci_admin_has_legacy_io - Checks whether the legacy IO
> > > > > >      * commands are supported
> > > > > > @@ -807,6 +808,12 @@ bool virtio_pci_admin_has_legacy_io(struct pci_dev
> > > > > > *pdev)
> > > > > >                    return true;
> > > > > >            return false;
> > > > > >     }
> > > > > > +#else
> > > > > > +bool virtio_pci_admin_has_legacy_io(struct pci_dev *pdev)
> > > > > > +{
> > > > > > +       return false;
> > > > > > +}
> > > > > > +#endif
> > > > > >     EXPORT_SYMBOL_GPL(virtio_pci_admin_has_legacy_io);
> > > > > 
> > > > > Doesn't this also raise the question of the purpose of virtio-vfio-pci
> > > > > on non-x86?  Without any other features it offers nothing over vfio-pci
> > > > > and we should likely adjust the Kconfig for x86 or COMPILE_TEST.
> > > > > Thanks,
> > > > > 
> > > > > Alex
> > > > 
> > > > Kconfig dependency is what I had in mind, yes. The X86 specific code in
> > > > virtio_pci_modern.c can be moved to a separate file then use makefile
> > > > tricks to skip it on other platforms.
> > > 
> > > The next feature for that driver will be the live migration support over
> > > virtio, once the specification which is WIP those day will be accepted.
> > > 
> > > The migration functionality is not X86 dependent and doesn't have the
> > > legacy virtio driver limitations that enforced us to run only on X86.
> > > 
> > > So, by that time we may need to enable in VFIO the loading of
> > > virtio-vfio-pci driver and put back the ifdef X86 inside VIRTIO, only on
> > > the legacy IO API, as I did already in V8.
> > > 
> > > So using a KCONFIG solution in VFIO is a short term one, which will be
> > > reverted just later on.
> > 
> > I understand the intent, but I don't think that justifies building a
> > driver that serves no purpose in the interim.  IF and when migration
> > support becomes a reality, it's trivial to update the depends line.
> > 
> 
> OK, so I'll add a KCONFIG dependency on X86 as you suggested as part of V9
> inside VFIO.
> 
> > > In addition, the virtio_pci_admin_has_legacy_io() API can be used in the
> > > future not only by VFIO, this was one of the reasons to put it inside
> > > VIRTIO.
> > 
> > Maybe this should be governed by a new Kconfig option which would be
> > selected by drivers like this.  Thanks,
> > 
> 
> We can still keep the simple ifdef X86 inside VIRTIO for future users/usage
> which is not only VFIO.
> 
> Michael,
> Can that work for you ?
> 
> Yishai
> 
> > Alex
> > 

I am not sure what is proposed exactly. General admin q infrastructure
can be kept as is. The legacy things however can never work outside X86.
Best way to limit it to x86 is to move it to a separate file and
only build that on X86. This way the only ifdef we need is where
we set the flags to enable legacy commands.

-- 
MST