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

Yishai Hadas
2023-12-14T09:37:36Z

On 14/12/2023 11:19, Michael S. Tsirkin wrote:
> On Thu, Dec 14, 2023 at 11:03:49AM +0200, Yishai Hadas wrote:
>> On 14/12/2023 8:38, Michael S. Tsirkin wrote:
>>> On Thu, Dec 07, 2023 at 12:28:20PM +0200, Yishai Hadas wrote:
>>>> Introduce a vfio driver over virtio devices to support the legacy
>>>> interface functionality for VFs.
>>>>
>>>> Background, from the virtio spec [1].
>>>> --------------------------------------------------------------------
>>>> In some systems, there is a need to support a virtio legacy driver with
>>>> a device that does not directly support the legacy interface. In such
>>>> scenarios, a group owner device can provide the legacy interface
>>>> functionality for the group member devices. The driver of the owner
>>>> device can then access the legacy interface of a member device on behalf
>>>> of the legacy member device driver.
>>>>
>>>> For example, with the SR-IOV group type, group members (VFs) can not
>>>> present the legacy interface in an I/O BAR in BAR0 as expected by the
>>>> legacy pci driver. If the legacy driver is running inside a virtual
>>>> machine, the hypervisor executing the virtual machine can present a
>>>> virtual device with an I/O BAR in BAR0. The hypervisor intercepts the
>>>> legacy driver accesses to this I/O BAR and forwards them to the group
>>>> owner device (PF) using group administration commands.
>>>> --------------------------------------------------------------------
>>>>
>>>> Specifically, this driver adds support for a virtio-net VF to be exposed
>>>> as a transitional device to a guest driver and allows the legacy IO BAR
>>>> functionality on top.
>>>>
>>>> This allows a VM which uses a legacy virtio-net driver in the guest to
>>>> work transparently over a VF which its driver in the host is that new
>>>> driver.
>>>>
>>>> The driver can be extended easily to support some other types of virtio
>>>> devices (e.g virtio-blk), by adding in a few places the specific type
>>>> properties as was done for virtio-net.
>>>>
>>>> For now, only the virtio-net use case was tested and as such we introduce
>>>> the support only for such a device.
>>>>
>>>> Practically,
>>>> Upon probing a VF for a virtio-net device, in case its PF supports
>>>> legacy access over the virtio admin commands and the VF doesn't have BAR
>>>> 0, we set some specific 'vfio_device_ops' to be able to simulate in SW a
>>>> transitional device with I/O BAR in BAR 0.
>>>>
>>>> The existence of the simulated I/O bar is reported later on by
>>>> overwriting the VFIO_DEVICE_GET_REGION_INFO command and the device
>>>> exposes itself as a transitional device by overwriting some properties
>>>> upon reading its config space.
>>>>
>>>> Once we report the existence of I/O BAR as BAR 0 a legacy driver in the
>>>> guest may use it via read/write calls according to the virtio
>>>> specification.
>>>>
>>>> Any read/write towards the control parts of the BAR will be captured by
>>>> the new driver and will be translated into admin commands towards the
>>>> device.
>>>>
>>>> Any data path read/write access (i.e. virtio driver notifications) will
>>>> be forwarded to the physical BAR which its properties were supplied by
>>>> the admin command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO upon the
>>>> probing/init flow.
>>>>
>>>> With that code in place a legacy driver in the guest has the look and
>>>> feel as if having a transitional device with legacy support for both its
>>>> control and data path flows.
>>>>
>>>> [1]
>>>> https://github.com/oasis-tcs/virtio-spec/commit/03c2d32e5093ca9f2a17797242fbef88efe94b8c
>>>>
>>>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>>>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>>>> ---
>>>> MAINTAINERS | 7 +
>>>> drivers/vfio/pci/Kconfig | 2 +
>>>> drivers/vfio/pci/Makefile | 2 +
>>>> drivers/vfio/pci/virtio/Kconfig | 16 +
>>>> drivers/vfio/pci/virtio/Makefile | 4 +
>>>> drivers/vfio/pci/virtio/main.c | 567 +++++++++++++++++++++++++++++++
>>>> 6 files changed, 598 insertions(+)
>>>> create mode 100644 drivers/vfio/pci/virtio/Kconfig
>>>> create mode 100644 drivers/vfio/pci/virtio/Makefile
>>>> create mode 100644 drivers/vfio/pci/virtio/main.c
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 012df8ccf34e..b246b769092d 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -22872,6 +22872,13 @@ L: kvm@vger.kernel.org
>>>> S: Maintained
>>>> F: drivers/vfio/pci/mlx5/
>>>> +VFIO VIRTIO PCI DRIVER
>>>> +M: Yishai Hadas <yishaih@nvidia.com>
>>>> +L: kvm@vger.kernel.org
>>>> +L: virtualization@lists.linux-foundation.org
>>>> +S: Maintained
>>>> +F: drivers/vfio/pci/virtio
>>>> +
>>>> VFIO PCI DEVICE SPECIFIC DRIVERS
>>>> R: Jason Gunthorpe <jgg@nvidia.com>
>>>> R: Yishai Hadas <yishaih@nvidia.com>
>>>> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
>>>> index 8125e5f37832..18c397df566d 100644
>>>> --- a/drivers/vfio/pci/Kconfig
>>>> +++ b/drivers/vfio/pci/Kconfig
>>>> @@ -65,4 +65,6 @@ source "drivers/vfio/pci/hisilicon/Kconfig"
>>>> source "drivers/vfio/pci/pds/Kconfig"
>>>> +source "drivers/vfio/pci/virtio/Kconfig"
>>>> +
>>>> endmenu
>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>> index 45167be462d8..046139a4eca5 100644
>>>> --- a/drivers/vfio/pci/Makefile
>>>> +++ b/drivers/vfio/pci/Makefile
>>>> @@ -13,3 +13,5 @@ obj-$(CONFIG_MLX5_VFIO_PCI) += mlx5/
>>>> obj-$(CONFIG_HISI_ACC_VFIO_PCI) += hisilicon/
>>>> obj-$(CONFIG_PDS_VFIO_PCI) += pds/
>>>> +
>>>> +obj-$(CONFIG_VIRTIO_VFIO_PCI) += virtio/
>>>> diff --git a/drivers/vfio/pci/virtio/Kconfig b/drivers/vfio/pci/virtio/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..3a6707639220
>>>> --- /dev/null
>>>> +++ b/drivers/vfio/pci/virtio/Kconfig
>>>> @@ -0,0 +1,16 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>> +config VIRTIO_VFIO_PCI
>>>> + tristate "VFIO support for VIRTIO NET PCI devices"
>>>> + depends on VIRTIO_PCI
>>>> + select VFIO_PCI_CORE
>>>> + help
>>>> + This provides support for exposing VIRTIO NET VF devices which support
>>>> + legacy IO access, using the VFIO framework that can work with a legacy
>>>> + virtio driver in the guest.
>>>> + Based on PCIe spec, VFs do not support I/O Space; thus, VF BARs shall
>>>> + not indicate I/O Space.
>>>> + As of that this driver emulated I/O BAR in software to let a VF be
>>>> + seen as a transitional device in the guest and let it work with
>>>> + a legacy driver.
>>>> +
>>>> + If you don't know what to do here, say N.
>>>
>>> BTW shouldn't this driver be limited to X86? Things like lack of memory
>>> barriers will make legacy virtio racy on e.g. ARM will they not?
>>> And endian-ness will be broken on PPC ...
>>>
>>
>> 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);

Yishai