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

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

On 11/19/25 11:43 PM, Sairaj Kodilkar wrote:
> 
> 
> On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
>> Hi Sairaj,
>>
>> On 11/18/25 3:24 AM, 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>
>>> ---
>>>   hw/i386/amd_iommu.c | 76 +++++++++++++++++++++++----------------------
>>>   1 file changed, 39 insertions(+), 37 deletions(-)
>>>
>>
>> [...]
>>
>>> +#define MMIO_REG_TO_STRING(mmio_reg, name) {\
>>> +    case mmio_reg: \
>>> +        name = #mmio_reg; \
>>> +        break; \
>>> +}
>>> +
>>> +#define MMIO_NAME_SIZE 50
>>>     struct AMDVIAddressSpace {
>>>       PCIBus *bus;                /* PCIBus (for bus 
>>> number)              */
>>> @@ -1484,31 +1469,48 @@ 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 void amdvi_mmio_get_name(hwaddr addr,
>>> +                                       char mmio_name[MMIO_NAME_SIZE])
>>> +{
>>> +    const char *name = NULL;
>>> +
>>> +    switch (addr) {
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_CONTROL, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXCL_LIMIT, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EXT_FEATURES, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_HEAD, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_COMMAND_TAIL, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_HEAD, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_EVENT_TAIL, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_STATUS, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD, name)
>>> +    MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL, name)
>>> +    default:
>>> +        name = "UNHANDLED";
>>>       }
>>>   -    return index;
>>> +    strncpy(mmio_name, name, MMIO_NAME_SIZE);
>>>   }
>>
>> While I don't believe there is a correctness issue, and it is a clever 
>> construct to reduce code repetition, I had some concerns with the 
>> implementation above, mostly on coding style and maintainability. I 
>> can go into each of the issues, but as I was trying to think of fixes 
>> it just became easier to write the code so...
>>
>> I think these changes preserve your original idea while fixing the 
>> problems and removing unnecessary code. Rather than diff from your 
>> patch, I'm sharing a diff for the full patch. I am still working 
>> through the other patches but the upcoming changes should fit in with 
>> no issues.
>> Let me know if you agree with the changes, or if there is something I 
>> missed.
>>
>> Alejandro
>>
>> ---
>> (compile tested only)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index d689a06eca..6fd9e2670a 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
>>
>>  struct AMDVIAddressSpace {
>>      PCIBus *bus;                /* PCIBus (for bus 
>> number)              */
>> @@ -1484,31 +1463,27 @@ 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 const char *amdvi_mmio_get_name(hwaddr addr)
>> +{
>> +    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";
>>      }
> 
> Hi Alejandro
> Although this approach looks good and faster (since you are
> returning a pointer without copy), it has a major flaw.
> You are returning a pointer to the "local string" 

Not quite. While you are right this could be issue in the initial 
version if you returned the local *name, it is not a problem above since 
the strings created using the macro stringification operator (#) are 
literal strings and not local i.e. they do not live in the stack frame 
that gets destroyed when the function returns. In all cases the returned 
pointer will point to a string literal that is available for the life of 
the program in the (ro)data section.
You can check it yourself by building the binary and looking at the data 
section with something like  objdump, but that would only show some 
fragments due to alignment of the output. After a bit of searching, it 
looks like readelf has an option that works best:

$ readelf -p .rodata build/qemu-system-x86_64 | grep AMDVI_MMIO
   [ a1c37]  AMDVI_MMIO_DEVICE_TABLE
   [ a1c4f]  AMDVI_MMIO_COMMAND_BASE
   [ a1c67]  AMDVI_MMIO_EVENT_BASE
   [ a1c7d]  AMDVI_MMIO_CONTROL
   [ a1c90]  AMDVI_MMIO_EXCL_BASE
   [ a1ca5]  AMDVI_MMIO_EXCL_LIMIT
   [ a1cbb]  AMDVI_MMIO_EXT_FEATURES
   [ a1cd3]  AMDVI_MMIO_COMMAND_HEAD
   [ a1ceb]  AMDVI_MMIO_COMMAND_TAIL
   [ a1d03]  AMDVI_MMIO_EVENT_HEAD
   [ a1d19]  AMDVI_MMIO_EVENT_TAIL
   [ a1d2f]  AMDVI_MMIO_STATUS
   [ a1d41]  AMDVI_MMIO_PPR_BASE
   [ a1d55]  AMDVI_MMIO_PPR_HEAD
   [ a1d69]  AMDVI_MMIO_PPR_TAIL


which can
> lead to all sorts of nasty issues. This is why I am copying
> the entire string to the destination.
> 
FYI, this copy is one of the reasons that made me look for an 
alternative implemention. Using strncpy is explicitly forbidden in the 
QEMU coding style:
https://qemu-project.gitlab.io/qemu/devel/style.html#string-manipulation
and while there are alternatives, in this case the copy is not really 
necessary.

Alejandro

> Thanks
> Sairaj
> 
>> -
>> -    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);
>> -}
>> -
>> -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);
>>  }
>>
>>  static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned 
>> size)
>> @@ -1528,7 +1503,7 @@ static uint64_t amdvi_mmio_read(void *opaque, 
>> hwaddr addr, unsigned size)
>>      } else if (size == 8) {
>>          val = amdvi_readq(s, addr);
>>      }
>> -    amdvi_mmio_trace_read(addr, size);
>> +    trace_amdvi_mmio_read(amdvi_mmio_get_name(addr), addr, size, addr 
>> & ~0x07);
>>
>>      return val;
>>  }
>> @@ -1684,7 +1659,9 @@ static void amdvi_mmio_write(void *opaque, 
>> hwaddr addr, uint64_t val,
>>          return;
>>      }
>>
>> -    amdvi_mmio_trace_write(addr, size, val);
>> +    trace_amdvi_mmio_write(amdvi_mmio_get_name(addr), addr, size, val,
>> +                          addr & ~0x07);
>> +
>>      switch (addr & ~0x07) {
>>      case AMDVI_MMIO_CONTROL:
>>          amdvi_mmio_reg_write(s, size, val, addr);
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index 302ccca512..ca4ff9fffe 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -45,10 +45,6 @@
>>  #define AMDVI_CAPAB_FLAG_IOTLBSUP     (1 << 24)
>>  #define AMDVI_CAPAB_INIT_TYPE         (3 << 16)
>>
>> -/* No. of used MMIO registers */
>> -#define AMDVI_MMIO_REGS_HIGH  7
>> -#define AMDVI_MMIO_REGS_LOW   8
>> -
>>  /* MMIO registers */
>>  #define AMDVI_MMIO_DEVICE_TABLE       0x0000
>>  #define AMDVI_MMIO_COMMAND_BASE       0x0008
>>
> 
> 
>