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

[Cédric Le Goater]

On 9/20/23 10:48, Duan, Zhenzhong wrote:
> > -----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/

ouch. this is long ago and I was not aware :/ Bare with me, I will
probably ask the same questions. Nevertheless, we could improve the
cover and the flow of changes in the patchset to help the reader.

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

I know I am asking for a lot of work. Thanks for that.

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

Cleanups, renames, some code reshuffling, anything preparing ground for
the new abstraction is good to have first and can be merged very quickly
if there are no functional changes. It reduces the overall patchset and
ease the coming reviews.

You can send such series independently. That's fine.

> > Also, please include the .h file first, it helps in reading.
>
> Do you mean to put struct declaration earlier in patch description?

Just add to your .gitconfig :

[diff]
        orderFile = /path/to/qemu/scripts/git.orderfile

It should be enough

> > Have you considered using an InterfaceClass ?
>
> See above, with object style rejected, it looks hard to use InterfaceClass.

I am not convinced by the QOM approach. I will dig in the past arguments
and let's see what we come with.

Thanks,

C.


> Thanks
> Zhenzhong