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

[Yishai Hadas <yishaih@nvidia.com>]

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
>