Re: [PATCH v1 04/15] intel_iommu: Introduce a new structure VTDHostIOMMUDevice

[CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>]

Hi,

On 06/06/2025 12:04 pm, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> 
> 
> Introduce a new structure VTDHostIOMMUDevice which replaces
> HostIOMMUDevice to be stored in hash table.
> 
> It includes a reference to HostIOMMUDevice and IntelIOMMUState,
> also includes BDF information which will be used in future
> patches.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h |  7 +++++++
>   include/hw/i386/intel_iommu.h  |  2 +-
>   hw/i386/intel_iommu.c          | 14 ++++++++++++--
>   3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 2cda744786..18bc22fc72 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -28,6 +28,7 @@
>   #ifndef HW_I386_INTEL_IOMMU_INTERNAL_H
>   #define HW_I386_INTEL_IOMMU_INTERNAL_H
>   #include "hw/i386/intel_iommu.h"
> +#include "system/host_iommu_device.h"
> 
>   /*
>    * Intel IOMMU register specification
> @@ -608,4 +609,10 @@ typedef struct VTDRootEntry VTDRootEntry;
>   /* Bits to decide the offset for each level */
>   #define VTD_LEVEL_BITS           9
> 
> +typedef struct VTDHostIOMMUDevice {
> +    IntelIOMMUState *iommu_state;
> +    PCIBus *bus;
> +    uint8_t devfn;
> +    HostIOMMUDevice *hiod;
> +} VTDHostIOMMUDevice;
>   #endif
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index e95477e855..50f9b27a45 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -295,7 +295,7 @@ struct IntelIOMMUState {
>       /* list of registered notifiers */
>       QLIST_HEAD(, VTDAddressSpace) vtd_as_with_notifiers;
> 
> -    GHashTable *vtd_host_iommu_dev;             /* HostIOMMUDevice */
> +    GHashTable *vtd_host_iommu_dev;             /* VTDHostIOMMUDevice */
> 
>       /* interrupt remapping */
>       bool intr_enabled;              /* Whether guest enabled IR */
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c42ef83ddc..796b71605c 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -281,7 +281,10 @@ static gboolean vtd_hiod_equal(gconstpointer v1, gconstpointer v2)
> 
>   static void vtd_hiod_destroy(gpointer v)
>   {
> -    object_unref(v);
> +    VTDHostIOMMUDevice *vtd_hiod = v;
> +
> +    object_unref(vtd_hiod->hiod);
> +    g_free(vtd_hiod);
>   }
> 
>   static gboolean vtd_hash_remove_by_domain(gpointer key, gpointer value,
> @@ -4397,6 +4400,7 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>                                        HostIOMMUDevice *hiod, Error **errp)
>   {
>       IntelIOMMUState *s = opaque;
> +    VTDHostIOMMUDevice *vtd_hiod;
>       struct vtd_as_key key = {
>           .bus = bus,
>           .devfn = devfn,
> @@ -4413,6 +4417,12 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>           return false;
>       }
> 
> +    vtd_hiod = g_malloc0(sizeof(VTDHostIOMMUDevice));
> +    vtd_hiod->bus = bus;
> +    vtd_hiod->devfn = (uint8_t)devfn;
> +    vtd_hiod->iommu_state = s;
> +    vtd_hiod->hiod = hiod;
> +
>       if (!vtd_check_hiod(s, hiod, errp)) {

Shouldn't we free vtd_hiod here?

>           vtd_iommu_unlock(s);
>           return false;
> @@ -4423,7 +4433,7 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>       new_key->devfn = devfn;
> 
>       object_ref(hiod);
> -    g_hash_table_insert(s->vtd_host_iommu_dev, new_key, hiod);
> +    g_hash_table_insert(s->vtd_host_iommu_dev, new_key, vtd_hiod);
> 
>       vtd_iommu_unlock(s);
> 
> --
> 2.34.1
>