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

[Eric Auger <eric.auger@redhat.com>]

Hi Cédric,

On 9/20/23 14:57, Cédric Le Goater wrote:
> 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.

Here is the reference:
https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/

Eric

>
> Thanks,
>
> C.
>
>
>> Thanks
>> Zhenzhong
>>
>