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

From: Yishai Hadas <yishaih@nvidia.com>
To: "Michael S. Tsirkin" <mst@redhat.com>,
	Alex Williamson <alex.williamson@redhat.com>
Cc: <jasowang@redhat.com>, <jgg@nvidia.com>, <kvm@vger.kernel.org>,
	<virtualization@lists.linux-foundation.org>, <parav@nvidia.com>,
	<feliu@nvidia.com>, <jiri@nvidia.com>, <kevin.tian@intel.com>,
	<joao.m.martins@oracle.com>, <si-wei.liu@oracle.com>,
	<leonro@nvidia.com>, <maorg@nvidia.com>
Subject: Re: [PATCH V7 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices
Date: Thu, 14 Dec 2023 18:03:30 +0200	[thread overview]
Message-ID: <5b596e34-aac9-4786-8f13-4d85986803f0@nvidia.com> (raw)
In-Reply-To: <20231214100403-mutt-send-email-mst@kernel.org>

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:
>>
>>> 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);
>>
>> 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.

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.

As of that, we may expect that VIRTIO will consider its limitation and 
return false in systems that are not X86 and won't expose its 
concerns/knowledge to other modules.

As of the above, I believe that better staying with the solution inside 
VIRTIO as was suggested here and is part of V8 already.

Makes sense ?

Yishai