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@xxxxxxxxxx> 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@xxxxxxxxxx> 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
