>-----Original Message----- >From: Cédric Le Goater <c...@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....@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....@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