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

[Cédric Le Goater]

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.

> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
> +
> +/* Abstraction of VFIO legacy host IOMMU device */
> +struct HIODLegacyVFIO {

same here

> +    /*< private >*/
> +    HostIOMMUDevice parent;
> +    VFIODevice *vdev;

It seems to me that the back pointer should be on the container instead.
Looks more correct conceptually.


> +};
> +
>   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.

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)