On 6/13/25 11:08 AM, Duan, Zhenzhong wrote:
> Hi Clement,
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com>
>> Subject: Re: [PATCH v1 04/15] intel_iommu: Introduce a new structure
>> VTDHostIOMMUDevice
>>
>> 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.d...@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?
> Good catch, Thanks.
> I put g_free in patch10, but it should be in this patch, will fix.
With this fixed,
Reviewed-by: Eric Auger <eric.au...@redhat.com>
Eric
>
> BRs,
> Zhenzhong
>
