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>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
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;
+
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);
--
2.47.1On 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 <zhenzhong.duan@intel.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> 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? :)
> 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 <yi.l.liu@intel.com>
>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>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 <zhenzhong.duan@intel.com>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> 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 <yi.l.liu@intel.com>
On 2025/8/28 17:17, Duan, Zhenzhong wrote:
>
>
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> 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 <zhenzhong.duan@intel.com>
>>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>> 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.
got it. it's needed by following patch.
Hi Zhenzhong/Yi,
On Fri, Aug 22, 2025 at 02:40:45AM -0400, Zhenzhong Duan wrote:
> @@ -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,
I wonder if the bus/devfn here would always reflect the actual BDF
numbers in this function, on an x86 VM.
With ARM, when the device is attached to a pxb bus, the bus/devfn
here are both 0, so PCI_BUILD_BDF() using these two returns 0 too.
QEMU command for the device:
-device pxb-pcie,id=pcie.1,bus_nr=1,bus=pcie.0 \
-device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1,accel=on \
-device pcie-root-port,id=pcie.port1,bus=pcie.1,chassis=1,io-reserve=0 \
-device vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.port1,rombar=0,id=dev0,iommufd=iommufd0
QEMU log:
smmuv3_accel_set_iommu_device: bus=0, devfn=0, sid=0
The set_iommu_device op is invoked by vfio_pci_realize() where the
the BDF number won't get ready for this kind of PCI setup until a
later stage that I can't identify yet..
Given that VTD wants the BDF number too, I start to wonder whether
the set_iommu_device op is invoked in the right place or not..
Maybe VTD works because it saves the bus pointer v.s. bus_num(=0),
so its bus_num would be updated when later code calculates the BDF
number using the saved bus pointer (in the key). Nonetheless, the
saved devfn (in the key) is 0, which wouldn't be updated later as
the bus_num. So, if the device is supposed to have a devfn (!=0),
this wouldn't work?
Thanks
Nicolin
Hi Nicolin,
On 8/26/25 7:21 PM, Nicolin Chen wrote:
> Hi Zhenzhong/Yi,
>
> On Fri, Aug 22, 2025 at 02:40:45AM -0400, Zhenzhong Duan wrote:
>> @@ -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,
> I wonder if the bus/devfn here would always reflect the actual BDF
> numbers in this function, on an x86 VM.
>
> With ARM, when the device is attached to a pxb bus, the bus/devfn
> here are both 0, so PCI_BUILD_BDF() using these two returns 0 too.
>
> QEMU command for the device:
> -device pxb-pcie,id=pcie.1,bus_nr=1,bus=pcie.0 \
> -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1,accel=on \
> -device pcie-root-port,id=pcie.port1,bus=pcie.1,chassis=1,io-reserve=0 \
> -device vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.port1,rombar=0,id=dev0,iommufd=iommufd0
>
> QEMU log:
> smmuv3_accel_set_iommu_device: bus=0, devfn=0, sid=0
>
> The set_iommu_device op is invoked by vfio_pci_realize() where the
> the BDF number won't get ready for this kind of PCI setup until a
> later stage that I can't identify yet..
>
> Given that VTD wants the BDF number too, I start to wonder whether
> the set_iommu_device op is invoked in the right place or not..
>
> Maybe VTD works because it saves the bus pointer v.s. bus_num(=0),
> so its bus_num would be updated when later code calculates the BDF
> number using the saved bus pointer (in the key). Nonetheless, the
> saved devfn (in the key) is 0, which wouldn't be updated later as
> the bus_num. So, if the device is supposed to have a devfn (!=0),
> this wouldn't work?
in hw/arm/smmu-common.c, along with smmu_find_smmu_pcibus() there is a
comment about late computation of bus number. This looks like a safe
place where the bus_num is known.
Thanks
Eric
>
> Thanks
> Nicolin
>
Hi Eric, On Wed, Aug 27, 2025 at 06:36:09PM +0200, Eric Auger wrote: > On 8/26/25 7:21 PM, Nicolin Chen wrote: > > QEMU log: > > smmuv3_accel_set_iommu_device: bus=0, devfn=0, sid=0 > > > > The set_iommu_device op is invoked by vfio_pci_realize() where the > > the BDF number won't get ready for this kind of PCI setup until a > > later stage that I can't identify yet.. > > > > Given that VTD wants the BDF number too, I start to wonder whether > > the set_iommu_device op is invoked in the right place or not.. > > > > Maybe VTD works because it saves the bus pointer v.s. bus_num(=0), > > so its bus_num would be updated when later code calculates the BDF > > number using the saved bus pointer (in the key). Nonetheless, the > > saved devfn (in the key) is 0, which wouldn't be updated later as > > the bus_num. So, if the device is supposed to have a devfn (!=0), > > this wouldn't work? > > in hw/arm/smmu-common.c, along with smmu_find_smmu_pcibus() there is a > comment about late computation of bus number. This looks like a safe > place where the bus_num is known. Yea, sid is a parameter of that smmu_find_smmu_pcibus() function, so bus_num must be known. What I want here is to allocate a vDEVICE (needs vSID), as early as possible. This will be potentially a requirement for CCA. Thanks Nicolin
Hi
>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [PATCH v5 07/21] intel_iommu: Introduce a new structure
>VTDHostIOMMUDevice
>
>Hi Zhenzhong/Yi,
>
>On Fri, Aug 22, 2025 at 02:40:45AM -0400, Zhenzhong Duan wrote:
>> @@ -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,
>
>I wonder if the bus/devfn here would always reflect the actual BDF
>numbers in this function, on an x86 VM.
devfn is enumerated by QEMU, see do_pci_register_device(), bus number is enumerated in BIOS or kernel.
So we can't use BDF number as key, we use PCIBus pointer + devfn as the key instead.
>
>With ARM, when the device is attached to a pxb bus, the bus/devfn
>here are both 0, so PCI_BUILD_BDF() using these two returns 0 too.
>
>QEMU command for the device:
> -device pxb-pcie,id=pcie.1,bus_nr=1,bus=pcie.0 \
> -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1,accel=on \
> -device pcie-root-port,id=pcie.port1,bus=pcie.1,chassis=1,io-reserve=0 \
> -device
>vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.port1,rombar=0,id=dev0,iom
>mufd=iommufd0
>
>QEMU log:
>smmuv3_accel_set_iommu_device: bus=0, devfn=0, sid=0
There is only one device under pcie.port1, devfn is initialized to 0, bus number isn't enumerated yet during realize() so 0.
>
>The set_iommu_device op is invoked by vfio_pci_realize() where the
>the BDF number won't get ready for this kind of PCI setup until a
>later stage that I can't identify yet..
>
>Given that VTD wants the BDF number too, I start to wonder whether
>the set_iommu_device op is invoked in the right place or not..
>
>Maybe VTD works because it saves the bus pointer v.s. bus_num(=0),
>so its bus_num would be updated when later code calculates the BDF
>number using the saved bus pointer (in the key). Nonetheless, the
>saved devfn (in the key) is 0, which wouldn't be updated later as
>the bus_num. So, if the device is supposed to have a devfn (!=0),
>this wouldn't work?
Both PCIBus pointer and devfn are fixed value for a QEMU instance, never changed.
Thanks
Zhenzhong
On Wed, Aug 27, 2025 at 06:45:42AM +0000, Duan, Zhenzhong wrote:
> Hi
>
> >-----Original Message-----
> >From: Nicolin Chen <nicolinc@nvidia.com>
> >Subject: Re: [PATCH v5 07/21] intel_iommu: Introduce a new structure
> >VTDHostIOMMUDevice
> >
> >Hi Zhenzhong/Yi,
> >
> >On Fri, Aug 22, 2025 at 02:40:45AM -0400, Zhenzhong Duan wrote:
> >> @@ -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,
> >
> >I wonder if the bus/devfn here would always reflect the actual BDF
> >numbers in this function, on an x86 VM.
>
> devfn is enumerated by QEMU, see do_pci_register_device(),
Oh, thanks for the direction.
> bus number is enumerated in BIOS or kernel.
> So we can't use BDF number as key, we use PCIBus pointer + devfn
> as the key instead.
Yea, I figured that out.
> >With ARM, when the device is attached to a pxb bus, the bus/devfn
> >here are both 0, so PCI_BUILD_BDF() using these two returns 0 too.
> >
> >QEMU command for the device:
> > -device pxb-pcie,id=pcie.1,bus_nr=1,bus=pcie.0 \
> > -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.1,accel=on \
> > -device pcie-root-port,id=pcie.port1,bus=pcie.1,chassis=1,io-reserve=0 \
> > -device
> >vfio-pci-nohotplug,host=0009:01:00.0,bus=pcie.port1,rombar=0,id=dev0,iom
> >mufd=iommufd0
> >
> >QEMU log:
> >smmuv3_accel_set_iommu_device: bus=0, devfn=0, sid=0
>
> There is only one device under pcie.port1, devfn is initialized to 0,
> bus number isn't enumerated yet during realize() so 0.
That's a pain for ARM... It needs to set BDF number early for some
use case. Shameer's current solution is doing after the guest kernel
boots, very late. So we might want to move it forward..
So, it'd be very ideal to have BDF in the set_iommu_device callback.
Otherwise, we'd have to add something like set_iommu_vdevice op to
invoke in the PCI core.
> >The set_iommu_device op is invoked by vfio_pci_realize() where the
> >the BDF number won't get ready for this kind of PCI setup until a
> >later stage that I can't identify yet..
> >
> >Given that VTD wants the BDF number too, I start to wonder whether
> >the set_iommu_device op is invoked in the right place or not..
> >
> >Maybe VTD works because it saves the bus pointer v.s. bus_num(=0),
> >so its bus_num would be updated when later code calculates the BDF
> >number using the saved bus pointer (in the key). Nonetheless, the
> >saved devfn (in the key) is 0, which wouldn't be updated later as
> >the bus_num. So, if the device is supposed to have a devfn (!=0),
> >this wouldn't work?
>
> Both PCIBus pointer and devfn are fixed value for a QEMU instance,
> never changed.
I see. devfn wouldn't be changed. Only the bus_num will be updated
in the later stage. So, it's not a problem for Intel.
Thanks
Nicolin
On Fri, Aug 22, 2025 at 02:40:45AM -0400, 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 <zhenzhong.duan@intel.com> > Reviewed-by: Eric Auger <eric.auger@redhat.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
© 2016 - 2025 Red Hat, Inc.