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>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
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
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)
--
2.34.1
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 <[sarunkod@amd.com](mailto:sarunkod@amd.com)>
> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com](mailto:vasant.hegde@amd.com)>
> ---
> 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)
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 <[sarunkod@amd.com](mailto:sarunkod@amd.com)>
>> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com](mailto:vasant.hegde@amd.com)>
>> ---
>> 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.
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)
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 <[sarunkod@amd.com]
>>> (mailto:sarunkod@amd.com)>
>>> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com]
>>> (mailto:vasant.hegde@amd.com)>
>>> ---
>>> 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
> 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)
>
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 <[sarunkod@amd.com]
>>>> (mailto:sarunkod@amd.com)>
>>>> Reviewed-by: Vasant Hegde <[vasant.hegde@amd.com]
>>>> (mailto:vasant.hegde@amd.com)>
>>>> ---
>>>> 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)
On 1/29/26 5:28 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>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
> 1 file changed, 27 insertions(+), 38 deletions(-)
>
I'd like to also remove the unused AMDVI_MMIO_REGS_{LOW,HIGH} definitions
and amdvi_mmio_trace_{read,write} helpers as I did on the example diff in:
https://lore.kernel.org/qemu-devel/eaf49cf3-e56b-40f1-974d-207969c7371e@oracle.com/
assuming you agree with it, no need to send a new revision, I can add those
changes to the current patch.
Otherwise:
Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> 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
>
> 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)
On 1/30/2026 3:20 AM, Alejandro Jimenez wrote:
>
> On 1/29/26 5:28 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>
>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> hw/i386/amd_iommu.c | 65 +++++++++++++++++++--------------------------
>> 1 file changed, 27 insertions(+), 38 deletions(-)
>>
> I'd like to also remove the unused AMDVI_MMIO_REGS_{LOW,HIGH} definitions
> and amdvi_mmio_trace_{read,write} helpers as I did on the example diff in:
>
> https://lore.kernel.org/qemu-devel/eaf49cf3-e56b-40f1-974d-207969c7371e@oracle.com/
>
> assuming you agree with it, no need to send a new revision, I can add those
> changes to the current patch.
Sure go ahead,
Thanks
Sairaj
>
> Otherwise:
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>
>> 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
>>
>> 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)
© 2016 - 2026 Red Hat, Inc.