Re: [PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar <sarunkod@amd.com>]

On 1/30/2026 1:09 PM, CLEMENT MATHIEU--DRIF wrote:
>
> On Thu, 2026-01-29 at 15:58 +0530, Sairaj Kodilkar wrote:
>> This makes it easier to add new MMIO registers for tracing and removes
>> the unnecessary complexity introduced by amdvi_mmio_(low/high) array.
>>
>> Signed-off-by: Sairaj Kodilkar <[sarunkod@amd.com](mailto:sarunkod@amd.com)>
>> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com](mailto:vasant.hegde@amd.com)>
>> ---
>>   hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>>   1 file changed, 27 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 789e09d6f2bc..62175cc366ac 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -35,28 +35,7 @@
>>   #include "kvm/kvm_i386.h"
>>   #include "qemu/iova-tree.h"
>>     
>> -/* used AMD-Vi MMIO registers */
>> -const char *amdvi_mmio_low[] = {
>> -    "AMDVI_MMIO_DEVTAB_BASE",
>> -    "AMDVI_MMIO_CMDBUF_BASE",
>> -    "AMDVI_MMIO_EVTLOG_BASE",
>> -    "AMDVI_MMIO_CONTROL",
>> -    "AMDVI_MMIO_EXCL_BASE",
>> -    "AMDVI_MMIO_EXCL_LIMIT",
>> -    "AMDVI_MMIO_EXT_FEATURES",
>> -    "AMDVI_MMIO_PPR_BASE",
>> -    "UNHANDLED"
>> -};
>> -const char *amdvi_mmio_high[] = {
>> -    "AMDVI_MMIO_COMMAND_HEAD",
>> -    "AMDVI_MMIO_COMMAND_TAIL",
>> -    "AMDVI_MMIO_EVTLOG_HEAD",
>> -    "AMDVI_MMIO_EVTLOG_TAIL",
>> -    "AMDVI_MMIO_STATUS",
>> -    "AMDVI_MMIO_PPR_HEAD",
>> -    "AMDVI_MMIO_PPR_TAIL",
>> -    "UNHANDLED"
>> -};
>> +#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
> Hi Sairaj,
>
> Shouldn't we define this inside the mmio_get_name function and undef it after the return statement?
> I think it would be cleanup to make the scope of this a bit smaller as it is specifically written for this function.

Hi
I think this is probably okay as its unlikely to cause any issues in future.

Thanks
-Sairaj

>
>>     
>>   struct AMDVIAddressSpace {
>>       PCIBus *bus;                /* PCIBus (for bus number)              */
>> @@ -1484,31 +1463,41 @@ static void amdvi_cmdbuf_run(AMDVIState *s)
>>       }
>>   }
>>     
>> -static inline uint8_t amdvi_mmio_get_index(hwaddr addr)
>> -{
>> -    uint8_t index = (addr & ~0x2000) / 8;
>> -
>> -    if ((addr & 0x2000)) {
>> -        /* high table */
>> -        index = index >= AMDVI_MMIO_REGS_HIGH ? AMDVI_MMIO_REGS_HIGH : index;
>> -    } else {
>> -        index = index >= AMDVI_MMIO_REGS_LOW ? AMDVI_MMIO_REGS_LOW : index;
>> +static inline
>> +const char *amdvi_mmio_get_name(hwaddr addr)
>> +{
>> +    /* Return MMIO names as string literals */
>> +    switch (addr) {
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
>> +    default:
>> +        return "UNHANDLED";
>>       }
>> -
>> -    return index;
>>   }
>>     
>>   static void amdvi_mmio_trace_read(hwaddr addr, unsigned size)
>>   {
>> -    uint8_t index = amdvi_mmio_get_index(addr);
>> -    trace_amdvi_mmio_read(amdvi_mmio_low[index], addr, size, addr & ~0x07);
>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>> +    trace_amdvi_mmio_read(mmio_name, addr, size, addr & ~0x07);
>>   }
>>     
>>   static void amdvi_mmio_trace_write(hwaddr addr, unsigned size, uint64_t val)
>>   {
>> -    uint8_t index = amdvi_mmio_get_index(addr);
>> -    trace_amdvi_mmio_write(amdvi_mmio_low[index], addr, size, val,
>> -                           addr & ~0x07);
>> +    const char *mmio_name = amdvi_mmio_get_name(addr);
>> +    trace_amdvi_mmio_write(mmio_name, addr, size, val, addr & ~0x07);
>>   }
>>     
>>   static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)