Hi Cédric,
>-----Original Message-----
>From: Cédric Le Goater <c...@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.au...@redhat.com>
>> Suggested-by: Cédric Le Goater <c...@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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)
