RE: [PATCH v1 13/22] vfio: Add base container

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Wednesday, September 20, 2023 1:24 AM
>Subject: Re: [PATCH v1 13/22] vfio: Add base container
>
>On 8/30/23 12:37, Zhenzhong Duan wrote:
>> From: Yi Liu <yi.l.liu@intel.com>
>>
>> Abstract the VFIOContainer to be a base object. It is supposed to be
>> embedded by legacy VFIO container and later on, into the new iommufd
>> based container.
>>
>> The base container implements generic code such as code related to
>> memory_listener and address space management. The VFIOContainerOps
>> implements callbacks that depend on the kernel user space being used.
>>
>> 'common.c' and vfio device code only manipulates the base container with
>> wrapper functions that calls the functions defined in VFIOContainerOpsClass.
>> Existing 'container.c' code is converted to implement the legacy container
>> ops functions.
>>
>> Below is the base container. It's named as VFIOContainer, old VFIOContainer
>> is replaced with VFIOLegacyContainer.
>
>Usualy, we introduce the new interface solely, port the current models
>on top of the new interface, wire the new models in the current
>implementation and remove the old implementation. Then, we can start
>adding extensions to support other implementations.

Not sure if I understand your point correctly. Do you mean to introduce
a new type for the base container as below:

static const TypeInfo vfio_container_info = {
    .parent             = TYPE_OBJECT,
    .name               = TYPE_VFIO_CONTAINER,
    .class_size         = sizeof(VFIOContainerClass),
    .instance_size      = sizeof(VFIOContainer),
    .abstract           = true,
    .interfaces = (InterfaceInfo[]) {
        { TYPE_VFIO_IOMMU_BACKEND_OPS },
        { }
    }
};

and a new interface as below:

static const TypeInfo nvram_info = {
    .name = TYPE_VFIO_IOMMU_BACKEND_OPS,
    .parent = TYPE_INTERFACE,
    .class_size = sizeof(VFIOIOMMUBackendOpsClass),
};

struct VFIOIOMMUBackendOpsClass {
    InterfaceClass parent;
    VFIODevice *(*dev_iter_next)(VFIOContainer *container, VFIODevice *curr);
    int (*dma_map)(VFIOContainer *container,
    ......
};

and legacy container on top of TYPE_VFIO_CONTAINER?

static const TypeInfo vfio_legacy_container_info = {
    .parent = TYPE_VFIO_CONTAINER,
    .name = TYPE_VFIO_LEGACY_CONTAINER,
    .class_init = vfio_legacy_container_class_init,
};

This object style is rejected early in RFCv1.
See https://lore.kernel.org/kvm/20220414104710.28534-8-yi.l.liu@intel.com/

>
>spapr should be taken care of separatly following the principle above.
>With my PPC hat, I would not even read such a massive change, too risky
>for the subsystem. This path will need (much) further splitting to be
>understandable and acceptable.

I'll digging into this and try to split it. Meanwhile, there are many changes
just renaming the parameter or function name for code readability.
For example:

-int vfio_dma_unmap(VFIOContainer *container, hwaddr iova,
-                   ram_addr_t size, IOMMUTLBEntry *iotlb)
+static int vfio_legacy_dma_unmap(VFIOContainer *bcontainer, hwaddr iova,
+                          ram_addr_t size, IOMMUTLBEntry *iotlb)

-        ret = vfio_get_dirty_bitmap(container, iova, size,
+        ret = vfio_get_dirty_bitmap(bcontainer, iova, size,

Let me know if you think such changes are unnecessary which could reduce
this patch largely.

>
>Also, please include the .h file first, it helps in reading.

Do you mean to put struct declaration earlier in patch description?

> Have you considered using an InterfaceClass ?

See above, with object style rejected, it looks hard to use InterfaceClass.

Thanks
Zhenzhong