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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 1/28/2026 7:10 AM, Alejandro Jimenez wrote:
> 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/

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;