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

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

On 1/29/26 5:28 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>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 38 deletions(-)
> 

I'd like to also remove the unused AMDVI_MMIO_REGS_{LOW,HIGH} definitions
and amdvi_mmio_trace_{read,write} helpers as I did on the example diff in:

https://lore.kernel.org/qemu-devel/eaf49cf3-e56b-40f1-974d-207969c7371e@oracle.com/

assuming you agree with it, no need to send a new revision, I can add those
changes to the current patch.

Otherwise:
Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

> 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
>  
>  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)