Re: [RFC PATCH RESEND 3/5] amd-iommu: Add support for set/unset IOMMU for VFIO PCI devices

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

Hi,

On 11/18/25 5:15 AM, Sairaj Kodilkar wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> "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 <suravee.suthikulpanit@amd.com>
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> ---
>  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;