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