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

["Cédric Le Goater" <clg@redhat.com>]

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.

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


Thanks,

C.



> 
>>
>>> +    /*< private >*/
>>> +    HostIOMMUDevice parent;
>>> +    VFIODevice *vdev;
>>
>> It seems to me that the back pointer should be on the container instead.
>> Looks more correct conceptually.
> 
> Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all saved in bcontainer.
> 
>>
>>
>>> +};
>>> +
>>>    typedef struct VFIODMABuf {
>>>        QemuDmaBuf buf;
>>>        uint32_t pos_x, pos_y, pos_updates;
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index 77bdec276e..44018ef085 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -1143,12 +1143,21 @@ static void
>> vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>>        vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>>    };
>>>
>>> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +};
>>
>> Is it preferable to introduce routines when they are actually useful.
>> Please drop the .class_init definition.
> 
> Sure.
> 
> Thanks
> Zhenzhong
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>> +
>>>    static const TypeInfo types[] = {
>>>        {
>>>            .name = TYPE_VFIO_IOMMU_LEGACY,
>>>            .parent = TYPE_VFIO_IOMMU,
>>>            .class_init = vfio_iommu_legacy_class_init,
>>> -    },
>>> +    }, {
>>> +        .name = TYPE_HIOD_LEGACY_VFIO,
>>> +        .parent = TYPE_HOST_IOMMU_DEVICE,
>>> +        .instance_size = sizeof(HIODLegacyVFIO),
>>> +        .class_init = hiod_legacy_vfio_class_init,
>>> +    }
>>>    };
>>>
>>>    DEFINE_TYPES(types)
>