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

[Duan, Zhenzhong]

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?

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?

>
>> +    /*< 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)