Re: [PATCH v1 04/15] intel_iommu: Introduce a new structure VTDHostIOMMUDevice
On 6/13/25 11:08 AM, Duan, Zhenzhong wrote:
> Hi Clement,
>
>> -Original Message-
>> From: CLEMENT MATHIEU--DRIF
>> 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
>>> ---
>>> 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
>
> BRs,
> Zhenzhong
>
RE: [PATCH v1 04/15] intel_iommu: Introduce a new structure VTDHostIOMMUDevice
Hi Clement,
>-Original Message-
>From: CLEMENT MATHIEU--DRIF
>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
>> ---
>> 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.
BRs,
Zhenzhong
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
> ---
> 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
>
