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 <[[email protected]](mailto:[email protected])>
> Reviewed-by: Vasant Hegde
> <[[email protected]](mailto:[email protected])>
> ---
> 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.
>
> 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)
