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.
> 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,
Alex
