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

[Eric Auger]

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. 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.

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