RE: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>Hello,
>
>On 4/16/24 05:41, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>>
>>> On 4/8/24 10:12, Zhenzhong Duan wrote:
>>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>>>> container backend.
>>>>
>>>> It includes a link to VFIODevice.
>>>>
>>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    include/hw/vfio/vfio-common.h | 11 +++++++++++
>>>>    hw/vfio/container.c           | 11 ++++++++++-
>>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>>>> index b9da6c08ef..f30772f534 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -31,6 +31,7 @@
>>>>    #endif
>>>>    #include "sysemu/sysemu.h"
>>>>    #include "hw/vfio/vfio-container-base.h"
>>>> +#include "sysemu/host_iommu_device.h"
>>>>
>>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>>
>>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>>>        bool ram_block_discard_allowed;
>>>>    } VFIOGroup;
>>>>
>>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-
>legacy-
>>> vfio"
>>>
>>> I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.
>>
>> Will do.
>>
>>>
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>>> +
>>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>>> +struct HIODLegacyVFIO {
>>>
>>> same here
>>
>> Should I do the same for all the HostIOMMUDevice and
>HostIOMMUDeviceClass sub-structures?
>
>I would for type names. The main reason is for naming consistency, which is
>useful for grep and code analysis.

Got it.

>
>>
>> The reason I used 'HIOD' abbreviation is some function names become
>extremely long
>> and exceed 80 characters. E.g.:
>>
>> @@ -1148,9 +1148,9 @@ static void
>vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>   };
>>
>> -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> -                                                void *data, uint32_t len,
>> -                                                Error **errp)
>> +static int
>host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> +                                                             void *data, 
>> uint32_t len,
>> +                                                             Error **errp)
>>   {
>>       VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
>>       /* iova_ranges is a sorted list */
>> @@ -1173,7 +1173,7 @@ static void
>hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>   {
>>       HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>>
>> -    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
>> +    hioc->get_host_iommu_info =
>host_iommu_device_legacy_vfio_get_host_iommu_info;
>>   };
>>
>> I didn't find other way to make it meet the 80 chars limitation. Any
>suggestions on this?
>
>Try :
>
>@@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init(
>  {
>      HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>
>-    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
>+    hioc->get_host_iommu_info =
>+        host_iommu_device_legacy_vfio_get_host_iommu_info;
>  };
>
>  static const TypeInfo types[] = {
>
>That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix
>could be shortened to 'hiod_legacy_vfio'.

Got it.

Thanks
Zhenzhong