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

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

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";
      }
-
-    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