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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Friday, September 22, 2023 1:20 AM
>Subject: Re: [PATCH v1 13/22] vfio: Add base container
>
>Hi Zhenzhong,
>On 9/21/23 05:35, Duan, Zhenzhong wrote:
>> Hi Eric,
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.auger@redhat.com>
>>> Sent: Thursday, September 21, 2023 1:31 AM
>>> Subject: Re: [PATCH v1 13/22] vfio: Add base container
>>>
>>> Hi Zhenzhong,
>>>
>>> On 9/19/23 19:23, Cédric Le Goater wrote:
>>>> 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.
>>>>
>>>> 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.
>>> We might split this patch by
>>> 1) introducing VFIOLegacyContainer encapsulating the base VFIOContainer,
>>> without using the ops in a first place:
>>>  common.c would call vfio_container_* with harcoded legacy
>>> implementation, ie. retrieving the legacy container with container_of.
>>> 2) we would introduce the BE interface without using it.
>>> 3) we would use the new BE interface
>>>
>>> Obviously this needs to be further tried out. If you wish I can try to
>>> split it that way ... Please let me know
>> Sure, thanks for your help, glad that I can cooperate with you to move
>> this series forward.
>> I just updated the branch which rebased to newest upstream for you to pick at
>https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_cdev_v1_rebased
>
>I have spent most of my day reshuffling this single patch into numerous
>ones (16!). This should help the review.
>I was short of time. This compiles, the end code should be identical to
>the original one. Besides this deserves some additional review on your
>end, commit msg tuning, ...
>
>But at least it is a move forward. Feel free to incorporate that in your
>next respin.
>
>Please find that work on the following branch
>
>https://github.com/eauger/qemu/tree/iommufd_cdev_v1_rebased_split

Thanks Eric, you have done a so quick and awesome work. Let me learn
your change and integrate with my other changes. Will get back to you
then.

BRs.
Zhenzhong