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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Friday, October 20, 2023 4:19 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>Hi,
>On 10/20/23 07:48, Duan, Zhenzhong wrote:
>> 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...
>you would need to do that for all types for vfio devices, ap, ccw,
>platform. Looks heavy to me.

Yes, this is a try out following Cédric's imagination, pasted here just for 
discuss.

>Why would we need to use a different
>vfio-pci-* device while we could switch the iommu backend according to
>the "iommufd" prop presence. The initial discussion was about QOMyfying
>the container instead.

We indeed are switching backend according to "iommufd" prop though not
QOMtfying the container in this series.

Thanks
Zhenzhong

>
>Thanks
>
>Eric
>
>>
>> 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