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

Yishai Hadas
2023-12-17T13:20:58Z

On 17/12/2023 14:20, Michael S. Tsirkin wrote:
> On Sun, Dec 17, 2023 at 12:39:48PM +0200, Yishai Hadas wrote:
>> On 14/12/2023 18:40, Michael S. Tsirkin wrote:
>>> 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.
>>>
>>>
>>
>> In VFIO we already agreed to add a dependency on X86 [1] as Alex asked.
>>
>> As VIRTIO should be ready for other clients and be self contained, I thought
>> to keep things simple and just return false from
>> virtio_pci_admin_has_legacy_io() in non X86 systems as was sent in V8.
>>
>> However, we can go with your approach as well and compile out all the legacy
>> IO stuff in non X86 systems by moving its code to a separate file (i.e.
>> virtio_pci_admin_legacy_io.c) and control this file upon the Makefile. In
>> addition, you suggested to control the 'supported_cmds' by an ifdef. This
>> will let the device know that we don't support legacy IO as well on non X86
>> systems.
>>
>> Please be aware that the above approach requires another ifdef on the H file
>> which exposes the 6 exported symbols and some further changes inside virtio
>> as of making vp_modern_admin_cmd_exec() non static as now we move the legacy
>> IO stuff to another C file, etc.
>>
>> Please see below how [2] it will look like.
>>
>> If you prefer that, so OK, it will be part of V9.
>> Please let me know.
>>
>>
>> [1] diff --git a/drivers/vfio/pci/virtio/Kconfig
>> b/drivers/vfio/pci/virtio/Kconfig
>> index 050473b0e5df..a3e5d8ea22a0 100644
>> --- a/drivers/vfio/pci/virtio/Kconfig
>> +++ b/drivers/vfio/pci/virtio/Kconfig
>> @@ -1,7 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> config VIRTIO_VFIO_PCI
>> tristate "VFIO support for VIRTIO NET PCI devices"
>> - depends on VIRTIO_PCI
>> + depends on X86 && VIRTIO_PCI
>> select VFIO_PCI_CORE
>> help
>> This provides support for exposing VIRTIO NET VF devices which
>> support
>>
>> [2] diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
>> index 8e98d24917cc..a73358bb4ebb 100644
>> --- a/drivers/virtio/Makefile
>> +++ b/drivers/virtio/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
>> obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
>> virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
>> virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
>> +virtio_pci-$(CONFIG_X86) += virtio_pci_admin_legacy_io.o
>> obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
>> obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
>> obj-$(CONFIG_VIRTIO_VDPA) += virtio_vdpa.o
>> diff --git a/drivers/virtio/virtio_pci_common.h
>> b/drivers/virtio/virtio_pci_common.h
>> index af676b3b9907..9963e5d0e881 100644
>> --- a/drivers/virtio/virtio_pci_common.h
>> +++ b/drivers/virtio/virtio_pci_common.h
>> @@ -158,4 +158,14 @@ void virtio_pci_modern_remove(struct virtio_pci_device
>> *);
>>
>> struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
>>
>> +#define VIRTIO_LEGACY_ADMIN_CMD_BITMAP \
>> + (BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE) | \
>> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ) | \
>> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE) | \
>> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ) | \
>> + BIT_ULL(VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO))
>> +
>
>
> I'd add something like:
>
> #ifdef CONFIG_X86
> #define VIRTIO_ADMIN_CMD_BITMAP VIRTIO_LEGACY_ADMIN_CMD_BITMAP
> #else
> #define VIRTIO_ADMIN_CMD_BITMAP 0
> #endif
>

This new macro (i.e. VIRTIO_ADMIN_CMD_BITMAP) will hold in the future
another sets of supported bits (e.g. Live migration), right ?

> Add a comment explaining why, please.

OK

How about the below ?

"As of some limitations in the legacy driver (e.g. lack of memory
barriers in ARM, endian-ness is broken in PPC) the supported legacy IO
commands are masked out on non x86 systems."

>
>
>> +int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
>> + struct virtio_admin_cmd *cmd);
>> +
>> #endif
>> diff --git a/drivers/virtio/virtio_pci_modern.c
>> b/drivers/virtio/virtio_pci_modern.c
>> index 53e29824d404..defb6282e1d7 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
>> @@ -75,8 +75,8 @@ static int virtqueue_exec_admin_cmd(struct
>> virtio_pci_admin_vq *admin_vq,
>> return 0;
>> }
>>
>> -static int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
>> - struct virtio_admin_cmd *cmd)
>> +int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
>> + struct virtio_admin_cmd *cmd)
>> {
>> struct scatterlist *sgs[VIRTIO_AVQ_SGS_MAX], hdr, stat;
>> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> @@ -172,6 +172,9 @@ static void virtio_pci_admin_cmd_list_init(struct
>> virtio_device *virtio_dev)
>> if (ret)
>> goto end;
>>
>> +#ifndef CONFIG_X86
>> + *data &= ~(cpu_to_le64(VIRTIO_LEGACY_ADMIN_CMD_BITMAP));
>> +#endif
>
> Then here we don't need an ifdef just use VIRTIO_ADMIN_CMD_BITMAP.
>

Following your suggestion, this will become,
*data &= cpu_to_le64(VIRTIO_ADMIN_CMD_BITMAP);

(no ifdef, and no ~).

Yishai