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/
> + 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.
> + 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.
> 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
> 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;
