On 1/28/2026 7:10 AM, Alejandro Jimenez wrote:
Hi,
On 11/18/25 5:15 AM, Sairaj Kodilkar wrote:
From: Suravee Suthikulpanit <[email protected]>
"Set" function tracks VFIO devices in the hash table. This is useful when
looking up per-device host IOMMU information later on.
Signed-off-by: Suravee Suthikulpanit <[email protected]>
Signed-off-by: Sairaj Kodilkar <[email protected]>
---
hw/i386/amd_iommu.c | 71 +++++++++++++++++++++++++++++++++++++++++++++
hw/i386/amd_iommu.h | 8 +++++
2 files changed, 79 insertions(+)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 378e0cb55eab..8b146f4d33d2 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -382,6 +382,22 @@ static guint amdvi_uint64_hash(gconstpointer v)
return (guint)*(const uint64_t *)v;
}
+static guint amdvi_dte_hash(gconstpointer v)
+{
+ const struct AMDVI_dte_key *key = v;
+ guint value = (guint)(uintptr_t)key->bus;
+
+ return (guint)(value << 8 | key->devfn);
+}
+
+static gboolean amdvi_dte_equal(gconstpointer v1, gconstpointer v2)
+{
+ const struct AMDVI_dte_key *key1 = v1;
+ const struct AMDVI_dte_key *key2 = v2;
+
+ return (key1->bus == key2->bus) && (key1->devfn == key2->devfn);
+}
+
static AMDVIIOTLBEntry *amdvi_iotlb_lookup(AMDVIState *s, hwaddr addr,
uint64_t devid)
{
@@ -2291,8 +2307,60 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus,
void *opaque, int devfn)
return &iommu_as[devfn]->as;
}
+static bool amdvi_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
+ HostIOMMUDevice *hiod, Error **errp)
+{
+ AMDVIState *s = opaque;
+ struct AMDVI_dte_key *new_key;
+ struct AMDVI_dte_key key = {
+ .bus = bus,
+ .devfn = devfn,
+ };
+
+ assert(hiod);
+ assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
+
+ if (g_hash_table_lookup(s->hiod_hash, &key)) {
+ error_setg(errp, "Host IOMMU device already exist");
nit: s/exist/exists/
Sure
+ return false;
+ }
+
+ if (hiod->caps.type != IOMMU_HW_INFO_TYPE_AMD &&
+ hiod->caps.type != IOMMU_HW_INFO_TYPE_DEFAULT) {
+ error_setg(errp, "IOMMU hardware is not compatible");
+ return false;
+ }
+
+ new_key = g_malloc(sizeof(*new_key));
When allocating the new key, use g_new0() instead of g_malloc(), matches
the current code better e.g.
new_key = g_new0(AMDVIHIODKey, 1);
*the AMDVIHIODKey type comes from a suggestion I make later.
Right
+ new_key->bus = bus;
+ new_key->devfn = devfn;
+
+ object_ref(hiod);
+ g_hash_table_insert(s->hiod_hash, new_key, hiod);
+
+ return true;
+}
+
+static void amdvi_unset_iommu_device(PCIBus *bus, void *opaque,
+ int devfn)
+{
+ AMDVIState *s = opaque;
+ struct AMDVI_dte_key key = {
+ .bus = bus,
+ .devfn = devfn,
+ };
+
+ if (!g_hash_table_lookup(s->hiod_hash, &key)) {
+ return;
+ }
+
+ g_hash_table_remove(s->hiod_hash, &key);
+}
+
I think we have to explicitly decrement the reference count for the hiod
object when removing the last entry from s->hiod_hash.
It looks like the best approach is to pass a custom value_destroy_func
callback for it when calling g_hash_table_new_full() to create the table.
Both the VT-d and virtio IOMMU implementations do it via that method.
Good catch, Will do this !
static const PCIIOMMUOps amdvi_iommu_ops = {
.get_address_space = amdvi_host_dma_iommu,
+ .set_iommu_device = amdvi_set_iommu_device,
+ .unset_iommu_device = amdvi_unset_iommu_device,
};
static const MemoryRegionOps mmio_mem_ops = {
@@ -2510,6 +2578,9 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error
**errp)
s->iotlb = g_hash_table_new_full(amdvi_uint64_hash,
amdvi_uint64_equal, g_free, g_free);
+ s->hiod_hash = g_hash_table_new_full(amdvi_dte_hash,
+ amdvi_dte_equal, g_free, g_free);
+
As I mentioned above, I think the last parameter to g_hash_table_new_full()
should be a custom destroy function with a call to:
object_unref((HostIOMMUDevice *)v);
/* set up MMIO */
memory_region_init_io(&s->mr_mmio, OBJECT(s), &mmio_mem_ops, s,
"amdvi-mmio", AMDVI_MMIO_SIZE);
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index daf82fc85f96..e6f6902fe06d 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -358,6 +358,11 @@ struct AMDVIPCIState {
uint32_t capab_offset; /* capability offset pointer */
};
+struct AMDVI_dte_key {
+ PCIBus *bus;
+ uint8_t devfn;
+};
+
For consistency with earlier usage, use a typedef and CamelCase for the new
AMDVI_dte_key definition i.e.
typedef struct AMDVIDTEKey {
PCIBus *bus;
uint8_t devfn;
} AMDVIDTEKey;
having it in the header file is best I think. I will send a patch moving
other definitions to amd_iommu.h as well.
But I am not sure that using "dte" in this case is the best choice. I had
this comment written for another section, fits better here:
hiod_hash and amdvi_dte_hash() should probably use a similar naming to
signal their relationship. Maybe they can all be 'hiod' based i.e.
amdvi_hiod_hash().
This seems to be the choice the VT-d implementation made, and it also
signals we are using the same HostIOMMUDevice abstraction/model. I get that
the device the HostIOMMUDevice represents is identified by a unique DTE on
the host side IOMMU structures, so I am not arguing the naming is
incorrect, but since we also have many places in the code that act on the
guest DTE (e.g. amdvi_get_dte()), it would be better to avoid overloading
'dte' to avoid confusion.
If the above makes sense, then we should also use `typedef struct
AMDVIHIODKey` instead...
Thank you,
Alejandro
Yeah I am in favour of HIOD base naming, will update it in V2
Thanks
Sairaj
struct AMDVIState {
X86IOMMUState iommu; /* IOMMU bus device */
AMDVIPCIState *pci; /* IOMMU PCI device */
@@ -416,6 +421,9 @@ struct AMDVIState {
/* IOTLB */
GHashTable *iotlb;
+ /* HostIOMMUDevice hash table*/
+ GHashTable *hiod_hash;
+
/* Interrupt remapping */
bool ga_enabled;
bool xtsup;
