>-----Original Message-----
>From: Liu, Yi L <[email protected]>
>Subject: Re: [PATCH v5 07/21] intel_iommu: Introduce a new structure
>VTDHostIOMMUDevice
>
>On 2025/8/22 14:40, Zhenzhong Duan wrote:
>> 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 <[email protected]>
>> Reviewed-by: Eric Auger <[email protected]>
>> ---
>> hw/i386/intel_iommu_internal.h | 7 +++++++
>> include/hw/i386/intel_iommu.h | 2 +-
>> hw/i386/intel_iommu.c | 15 +++++++++++++--
>> 3 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h
>b/hw/i386/intel_iommu_internal.h
>> index 360e937989..c7046eb4e2 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 e3b871de70..512ca4fdc5 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,
>> @@ -4371,6 +4374,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,
>> @@ -4387,7 +4391,14 @@ 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;
>
>how about moving it after the below if branch? :)
They will be used in vtd_check_hiod(), so need to initialize them early.
Thanks
Zhenzhong
>
>> if (!vtd_check_hiod(s, hiod, errp)) {
>> + g_free(vtd_hiod);
>> vtd_iommu_unlock(s);
>> return false;
>> }
>> @@ -4397,7 +4408,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);
>>
>
>LGTM.
>
>Reviewed-by: Yi Liu <[email protected]>
