RE: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface

[Duan, Zhenzhong]

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Thursday, October 19, 2023 8:18 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/19/23 04:29, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Wednesday, October 18, 2023 4:04 PM
>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>and
>>> targetted interface
>>>
>>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>>> Hi Cédric,
>>>>
>>>>> -----Original Message-----
>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for 
>>>>> VFIOContainer
>>> and
>>>>> targetted interface
>>>>>
>>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>>>
>>>>>> Introduce a dumb VFIOContainer base object and its targetted interface.
>>>>>> This is willingly not a QOM object because we don't want it to be
>>>>>> visible from the user interface.  The VFIOContainer will be smoothly
>>>>>> populated in subsequent patches as well as interfaces.
>>>>>>
>>>>>> No fucntional change intended.
>>>>>>
>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>>> ---
>>>>>>     include/hw/vfio/vfio-common.h         |  8 +--
>>>>>>     include/hw/vfio/vfio-container-base.h | 82
>>> +++++++++++++++++++++++++++
>>>>>>     2 files changed, 84 insertions(+), 6 deletions(-)
>>>>>>     create mode 100644 include/hw/vfio/vfio-container-base.h
>>>>>>
>>>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>>>>>> index 34648e518e..9651cf921c 100644
>>>>>> --- a/include/hw/vfio/vfio-common.h
>>>>>> +++ b/include/hw/vfio/vfio-common.h
>>>>>> @@ -30,6 +30,7 @@
>>>>>>     #include <linux/vfio.h>
>>>>>>     #endif
>>>>>>     #include "sysemu/sysemu.h"
>>>>>> +#include "hw/vfio/vfio-container-base.h"
>>>>>>
>>>>>>     #define VFIO_MSG_PREFIX "vfio %s: "
>>>>>>
>>>>>> @@ -81,6 +82,7 @@ typedef struct VFIOAddressSpace {
>>>>>>     struct VFIOGroup;
>>>>>>
>>>>>>     typedef struct VFIOLegacyContainer {
>>>>>> +    VFIOContainer bcontainer;
>>>>>
>>>>> That's the parent class, right ?
>>>>
>>>> Right.
>>>>
>>>>>
>>>>>>         VFIOAddressSpace *space;
>>>>>>         int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>>         MemoryListener listener;
>>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>>         } dmabuf;
>>>>>>     } VFIODisplay;
>>>>>>
>>>>>> -typedef struct {
>>>>>> -    unsigned long *bitmap;
>>>>>> -    hwaddr size;
>>>>>> -    hwaddr pages;
>>>>>> -} VFIOBitmap;
>>>>>> -
>>>>>>     void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>>                            hwaddr min_iova, hwaddr max_iova,
>>>>>>                            uint64_t iova_pgsizes);
>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h 
>>>>>> b/include/hw/vfio/vfio-
>>>>> container-base.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..afc8543d22
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>> @@ -0,0 +1,82 @@
>>>>>> +/*
>>>>>> + * VFIO BASE CONTAINER
>>>>>> + *
>>>>>> + * Copyright (C) 2023 Intel Corporation.
>>>>>> + * Copyright Red Hat, Inc. 2023
>>>>>> + *
>>>>>> + * Authors: Yi Liu <yi.l.liu@intel.com>
>>>>>> + *          Eric Auger <eric.auger@redhat.com>
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>> + * it under the terms of the GNU General Public License as published by
>>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>>> + * (at your option) any later version.
>>>>>> +
>>>>>> + * This program is distributed in the hope that it will be useful,
>>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>> + * GNU General Public License for more details.
>>>>>> +
>>>>>> + * You should have received a copy of the GNU General Public License
>along
>>>>>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef HW_VFIO_VFIO_BASE_CONTAINER_H
>>>>>> +#define HW_VFIO_VFIO_BASE_CONTAINER_H
>>>>>> +
>>>>>> +#include "exec/memory.h"
>>>>>> +#ifndef CONFIG_USER_ONLY
>>>>>> +#include "exec/hwaddr.h"
>>>>>> +#endif
>>>>>> +
>>>>>> +typedef struct VFIOContainer VFIOContainer;
>>>>>> +typedef struct VFIODevice VFIODevice;
>>>>>> +typedef struct VFIOIOMMUBackendOpsClass
>VFIOIOMMUBackendOpsClass;
>>>>>> +
>>>>>> +typedef struct {
>>>>>> +    unsigned long *bitmap;
>>>>>> +    hwaddr size;
>>>>>> +    hwaddr pages;
>>>>>> +} VFIOBitmap;
>>>>>> +
>>>>>> +/*
>>>>>> + * This is the base object for vfio container backends
>>>>>> + */
>>>>>> +struct VFIOContainer {
>>>>>> +    VFIOIOMMUBackendOpsClass *ops;
>>>>>
>>>>> This is unexpected.
>>>>>
>>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>>> to be introduced with a VFIOContainerClass with the ops below
>>>>> (VFIOIOMMUBackendOpsClass).
>>>>>
>>>>> Then, we would call :
>>>>>
>>>>>      VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>>
>>>>> to get the specific implementation for the current container.
>>>>>
>>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>>
>>>> The original implementation was abstract QOM model. But it wasn't
>accepted,
>>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>>>
>>> I see the idea was challenged, not rejected. I need to dig in further and 
>>> this
>>> will take time.
>> Thanks for help looking into it.
>>
>> +David, Hi David, I'm digging into your concern of using QOM to abstract base
>> container and legacy VFIOContainer:
>> "The QOM class of things is visible to the user/config layer via QMP (and
>sometimes command line).
>> It doesn't necessarily correspond to guest visible differences, but it often 
>> does."
>>
>> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE
>interface,
>> otherwise, user can't create an object of this type. Only difference is user 
>> will
>see
>> "object type '%s' isn't supported by object-add" instead of "invalid object
>type: %s".
>>
>> Is your expectation to not permit user to create this object or only want 
>> user
>> to see "invalid object type: %s".
>> If you mean the first, then I think QOM could be suitable if we don't include
>> TYPE_USER_CREATABLE interface?
>
>I was imagining some kind of QOM hierarchy under the vfio device
>with various QOM interfaces (similar to the ops) to define the
>possible IOMMU backends. The fact that we use the IOMMUFD object
>from the command line made it more plausible. I might be mistaking.

Got your point.
This way we introduce a new QOM type "vfio-pci-iommufd" for iommufd support,
and vfio-pci keep same for legacy backend, e.g:

#qemu  -object iommufd,id=iommufd0 \
              -device vfio-pci-iommufd,iommufd=iommufd0,id=vfio0... \
             -device vfio-pci,id=vfio1...

Below is a draft for vfio-pci based on your imagination. not pasted here the 
change
for platform/ap/ccw vfio device which should be similar.
Not clear if this fully match your imagination.

diff --git a/include/hw/vfio/vfio-container-base.h 
b/include/hw/vfio/vfio-container-base.h
index 5345986993..54a6ce4d73 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -124,7 +124,7 @@ DECLARE_CLASS_CHECKERS(VFIOIOMMUBackendOpsClass,

 struct VFIOIOMMUBackendOpsClass {
     /*< private >*/
-    ObjectClass parent_class;
+    InterfaceClass parent_class;

     /*< public >*/
     /* required */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index edb787d3d1..829deddc7d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3725,6 +3725,11 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, 
void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass);
+    IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
+
+    be_ops->dma_map = vfio_legacy_dma_map;
+    be_ops->dma_unmap = vfio_legacy_dma_unmap;
+    ...

     dc->reset = vfio_pci_reset;
     device_class_set_props(dc, vfio_pci_dev_properties);
@@ -3749,10 +3754,40 @@ static const TypeInfo vfio_pci_dev_info = {
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_PCIE_DEVICE },
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { INTERFACE_IOMMU_BACKEND_OPS },
         { }
     },
 };

+static Property vfio_pci_dev_iommufd_properties[] = {
+#ifdef CONFIG_IOMMUFD
+    DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
+                     TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
+#endif
+};
+
+static void vfio_pci_iommufd_dev_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
+
+    device_class_set_props(dc, vfio_pci_dev_iommufd_properties);
+
+    be_ops->dma_map = iommufd_map;
+    be_ops->dma_unmap = iommufd_unmap;
+    ...
+    /* Unimplemented ops */
+    be_ops->set_dirty_page_tracking = NULL;
+    ...
+}
+
+static const TypeInfo vfio_pci_iommufd_dev_info = {
+    .name = TYPE_VFIO_PCI_IOMMUFD,
+    .parent = TYPE_VFIO_PCI,
+    .instance_size = sizeof(VFIOPCIDevice),
+    .class_init = vfio_pci_iommufd_dev_class_init,
+};
+
 static Property vfio_pci_dev_nohotplug_properties[] = {
     DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
     DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
@@ -3770,13 +3805,20 @@ static void 
vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)

 static const TypeInfo vfio_pci_nohotplug_dev_info = {
     .name = TYPE_VFIO_PCI_NOHOTPLUG,
-    .parent = TYPE_VFIO_PCI,
+    .parent = TYPE_VFIO_PCI_IOMMUFD,
     .instance_size = sizeof(VFIOPCIDevice),
     .class_init = vfio_pci_nohotplug_dev_class_init,
 };

 static void register_vfio_pci_dev_type(void)
 {
+    static const TypeInfo iommu_be_ops_interface_info = {
+        .name          = TYPE_IOMMU_BACKEND_OPS,
+        .parent        = TYPE_INTERFACE,
+        .class_size = sizeof(VFIOIOMMUBackendOpsClass),
+    };
+
+    type_register_static(&iommu_be_ops_interface_info);
     type_register_static(&vfio_pci_dev_info);
     type_register_static(&vfio_pci_nohotplug_dev_info);
 }

Thanks
Zhenzhong

>
>Anyhow, the series looks pretty good. There are other aspect to
>check, like are all this iommu ops well suited for the need ?
>Let's stress the models in parallel of the reviews. If we could get
>some of it for 8.2 that'd be nice. It's on top of my list now.
>
>Thanks,
>
>C.
>
>
>> Thanks
>> Zhenzhong