On 2/2/2026 8:47 PM, Alejandro Jimenez wrote:
On 1/30/26 3:04 AM, Sairaj Kodilkar wrote:
On 1/30/2026 1:09 PM, CLEMENT MATHIEU--DRIF wrote:
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.
I agree with the above. I think it is a good idea given the ad-hoc nature
of this macro to keep the definition and its usage together and undef it
right after to avoid any confusion.
I was a bit reluctant when I proposed the macro because it affects control
flow (the kernel coding style frowns on that even if QEMU doesn't
explicitly forbids it), but I think this is clean and easy to parse:
static inline
const char *amdvi_mmio_get_name(hwaddr addr)
{
/* Return MMIO names as string literals */
switch (addr) {
#define MMIO_REG_TO_STRING(mmio_reg) case mmio_reg: return #mmio_reg
MMIO_REG_TO_STRING(AMDVI_MMIO_DEVICE_TABLE);
[...]
MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
#undef MMIO_REG_TO_STRING
default:
return "UNHANDLED";
}
}
Sairaj: if you don't want to sign off on this specific pattern, I can send
a patch for it and we can review it separately.
Alejandro
Sure, I'll send the V3 by tommorrow with all the changes
Thanks
Sairaj
Hi
I think this is probably okay as its unlikely to cause any issues in future.
Thanks
-Sairaj
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)
