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 <[email protected]>
---
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
