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
> 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)
>
