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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux