[PATCH 0/3] amd_iommu: Support Generation of IOMMU XT interrupts

[PATCH 0/3] amd_iommu: Support Generation of IOMMU XT interrupts

[Sairaj Kodilkar]

AMD IOMMU uses MMIO registers 0x170-0x180 to generate the interrupts when guest
has enabled xt support through control register. The guest programs these
registers with appropriate vector and destination ID instead of writing to PCI
MSI capability.

Until now enabling the xt support through command line "xtsup=on" provided
support for 128 bit IRTE. But it has few limitations:

1. It does not consider if guest has actually enabled xt support through MMIO
   control register (0x18). This may cause problems for the guests which do
   not enable this support.
2. The vIOMMU is not capable of generating interrupts using vector and
   destinatio ID in IOMMU x2APIC Control Registers (not supporting event log
   interrupts).

To overcome above limitations, this patch series introduces new internal flag 
"intcapxten" which is set when guest writes "1" to MMIO control register (0x18)
bit 51 (IntCapXTEn) and adds support to generate event log interrupt using
vector and 32 bit destination ID in XT MMIO register 0x170.

-------------------------------------------------------------------------------

The patches are based on top of upstream qemu master (e88510fcdc13)

-------------------------------------------------------------------------------

Sairaj Kodilkar (3):
  amd_iommu: Use switch case to determine mmio register name
  amd_iommu: Turn on XT support only when guest has enabled it
  amd_iommu: Generate XT interrupts when xt support is enabled

 hw/i386/amd_iommu.c  | 130 ++++++++++++++++++++++++++++---------------
 hw/i386/amd_iommu.h  |   7 ++-
 hw/i386/trace-events |   1 +
 3 files changed, 93 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar]

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>
---
 hw/i386/amd_iommu.c | 76 +++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index d689a06eca46..a9ee7150ef17 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -35,28 +35,13 @@
 #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, 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);
 }
 
 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);
+    char mmio_name[MMIO_NAME_SIZE];
+
+    amdvi_mmio_get_name(addr, mmio_name);
+    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);
+    char mmio_name[MMIO_NAME_SIZE];
+
+    amdvi_mmio_get_name(addr, mmio_name);
+    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

Re: [PATCH 1/3] amd_iommu: Use switch case to determine mmio register name

[Alejandro Jimenez]

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 <sarunkod@amd.com>
> ---
>   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

Re: [PATCH 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar]

On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
> 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 <sarunkod@amd.com>
>> ---
>>   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";
>      }

Hi Alejandro
Although this approach looks good and faster (since you are
returning a pointer without copy), it has a major flaw.
You are returning a pointer to the "local string" which can
lead to all sorts of nasty issues. This is why I am copying
the entire string to the destination.

Thanks
Sairaj

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

Re: [PATCH 1/3] amd_iommu: Use switch case to determine mmio register name

[Alejandro Jimenez]

On 11/19/25 11:43 PM, Sairaj Kodilkar wrote:
> 
> 
> On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
>> 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 <sarunkod@amd.com>
>>> ---
>>>   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";
>>      }
> 
> Hi Alejandro
> Although this approach looks good and faster (since you are
> returning a pointer without copy), it has a major flaw.
> You are returning a pointer to the "local string" 

Not quite. While you are right this could be issue in the initial 
version if you returned the local *name, it is not a problem above since 
the strings created using the macro stringification operator (#) are 
literal strings and not local i.e. they do not live in the stack frame 
that gets destroyed when the function returns. In all cases the returned 
pointer will point to a string literal that is available for the life of 
the program in the (ro)data section.
You can check it yourself by building the binary and looking at the data 
section with something like  objdump, but that would only show some 
fragments due to alignment of the output. After a bit of searching, it 
looks like readelf has an option that works best:

$ readelf -p .rodata build/qemu-system-x86_64 | grep AMDVI_MMIO
   [ a1c37]  AMDVI_MMIO_DEVICE_TABLE
   [ a1c4f]  AMDVI_MMIO_COMMAND_BASE
   [ a1c67]  AMDVI_MMIO_EVENT_BASE
   [ a1c7d]  AMDVI_MMIO_CONTROL
   [ a1c90]  AMDVI_MMIO_EXCL_BASE
   [ a1ca5]  AMDVI_MMIO_EXCL_LIMIT
   [ a1cbb]  AMDVI_MMIO_EXT_FEATURES
   [ a1cd3]  AMDVI_MMIO_COMMAND_HEAD
   [ a1ceb]  AMDVI_MMIO_COMMAND_TAIL
   [ a1d03]  AMDVI_MMIO_EVENT_HEAD
   [ a1d19]  AMDVI_MMIO_EVENT_TAIL
   [ a1d2f]  AMDVI_MMIO_STATUS
   [ a1d41]  AMDVI_MMIO_PPR_BASE
   [ a1d55]  AMDVI_MMIO_PPR_HEAD
   [ a1d69]  AMDVI_MMIO_PPR_TAIL


which can
> lead to all sorts of nasty issues. This is why I am copying
> the entire string to the destination.
> 
FYI, this copy is one of the reasons that made me look for an 
alternative implemention. Using strncpy is explicitly forbidden in the 
QEMU coding style:
https://qemu-project.gitlab.io/qemu/devel/style.html#string-manipulation
and while there are alternatives, in this case the copy is not really 
necessary.

Alejandro

> Thanks
> Sairaj
> 
>> -
>> -    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
>>
> 
> 
> 

Re: [PATCH 1/3] amd_iommu: Use switch case to determine mmio register name

[Sairaj Kodilkar]

On 11/20/2025 7:01 PM, Alejandro Jimenez wrote:
>
>
> On 11/19/25 11:43 PM, Sairaj Kodilkar wrote:
>>
>>
>> On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
>>> 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 <sarunkod@amd.com>
>>>> ---
>>>>   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";
>>>      }
>>
>> Hi Alejandro
>> Although this approach looks good and faster (since you are
>> returning a pointer without copy), it has a major flaw.
>> You are returning a pointer to the "local string" 
>
> Not quite. While you are right this could be issue in the initial 
> version if you returned the local *name, it is not a problem above 
> since the strings created using the macro stringification operator (#) 
> are literal strings and not local i.e. they do not live in the stack 
> frame that gets destroyed when the function returns. In all cases the 
> returned pointer will point to a string literal that is available for 
> the life of the program in the (ro)data section.
> You can check it yourself by building the binary and looking at the 
> data section with something like  objdump, but that would only show 
> some fragments due to alignment of the output. After a bit of 
> searching, it looks like readelf has an option that works best:
>
> $ readelf -p .rodata build/qemu-system-x86_64 | grep AMDVI_MMIO
>   [ a1c37]  AMDVI_MMIO_DEVICE_TABLE
>   [ a1c4f]  AMDVI_MMIO_COMMAND_BASE
>   [ a1c67]  AMDVI_MMIO_EVENT_BASE
>   [ a1c7d]  AMDVI_MMIO_CONTROL
>   [ a1c90]  AMDVI_MMIO_EXCL_BASE
>   [ a1ca5]  AMDVI_MMIO_EXCL_LIMIT
>   [ a1cbb]  AMDVI_MMIO_EXT_FEATURES
>   [ a1cd3]  AMDVI_MMIO_COMMAND_HEAD
>   [ a1ceb]  AMDVI_MMIO_COMMAND_TAIL
>   [ a1d03]  AMDVI_MMIO_EVENT_HEAD
>   [ a1d19]  AMDVI_MMIO_EVENT_TAIL
>   [ a1d2f]  AMDVI_MMIO_STATUS
>   [ a1d41]  AMDVI_MMIO_PPR_BASE
>   [ a1d55]  AMDVI_MMIO_PPR_HEAD
>   [ a1d69]  AMDVI_MMIO_PPR_TAIL
>
>
> which can
>> lead to all sorts of nasty issues. This is why I am copying
>> the entire string to the destination.
>>
> FYI, this copy is one of the reasons that made me look for an 
> alternative implemention. Using strncpy is explicitly forbidden in the 
> QEMU coding style:
> https://qemu-project.gitlab.io/qemu/devel/style.html#string-manipulation
> and while there are alternatives, in this case the copy is not really 
> necessary.
>
> Alejandro

Ahh right I missed that its string literal and will be stored in ro data 
section. Thanks for clarifying the things
I am in favor of this approach. Will do this in the next patch version. 
Could you please provide your signed-off-by ?

Thanks
Sairaj

.../...

Re: [PATCH 1/3] amd_iommu: Use switch case to determine mmio register name

[Alejandro Jimenez]

On 11/21/25 12:20 AM, Sairaj Kodilkar wrote:
> 
> 
> On 11/20/2025 7:01 PM, Alejandro Jimenez wrote:
>>
>>
>> On 11/19/25 11:43 PM, Sairaj Kodilkar wrote:
>>>
>>>
>>> On 11/20/2025 7:06 AM, Alejandro Jimenez wrote:
>>>> 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 <sarunkod@amd.com>
>>>>> ---
>>>>>   hw/i386/amd_iommu.c | 76 ++++++++++++++++++++++ 
>>>>> +----------------------
>>>>>   1 file changed, 39 insertions(+), 37 deletions(-)
>>>>>
>>>>

[...]


> I am in favor of this approach. Will do this in the next patch version. 
> Could you please provide your signed-off-by ?
> 

No need. I suggested some adjustments, but I think your original idea 
still stands and Vasant's R-b still applies (unless he has an objection 
to the proposed changes...).

I'm still reviewing the other patches in the series, but might not get 
to reply until early next week.

Thank you,
Alejandro

> Thanks
> Sairaj
> 
> .../...

[PATCH 2/3] amd_iommu: Turn on XT support only when guest has enabled it

[Sairaj Kodilkar]

Current code uses 32 bit cpu destination irrespective of the fact that
guest has enabled xt support through control register[XTEn] and
completely depends on command line parameter xtsup=on. This is not a
correct hardware behaviour and can cause problems in the guest which has
not enabled XT mode.

Introduce new flag "xten", which is enabled when guest writes 1 to the
control register bit 50 (XTEn).

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 hw/i386/amd_iommu.c | 3 ++-
 hw/i386/amd_iommu.h | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index a9ee7150ef17..7f08fc31111a 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1548,6 +1548,7 @@ static void amdvi_handle_control_write(AMDVIState *s)
     s->cmdbuf_enabled = s->enabled && !!(control &
                         AMDVI_MMIO_CONTROL_CMDBUFLEN);
     s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
+    s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -2020,7 +2021,7 @@ static int amdvi_int_remap_ga(AMDVIState *iommu,
     irq->vector = irte.hi.fields.vector;
     irq->dest_mode = irte.lo.fields_remap.dm;
     irq->redir_hint = irte.lo.fields_remap.rq_eoi;
-    if (iommu->xtsup) {
+    if (iommu->xten) {
         irq->dest = irte.lo.fields_remap.destination |
                     (irte.hi.fields.destination_hi << 24);
     } else {
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 302ccca5121f..32467d0bc241 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -106,6 +106,7 @@
 #define AMDVI_MMIO_CONTROL_COMWAITINTEN   (1ULL << 4)
 #define AMDVI_MMIO_CONTROL_CMDBUFLEN      (1ULL << 12)
 #define AMDVI_MMIO_CONTROL_GAEN           (1ULL << 17)
+#define AMDVI_MMIO_CONTROL_XTEN           (1ULL << 50)
 
 /* MMIO status register bits */
 #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
@@ -418,7 +419,8 @@ struct AMDVIState {
 
     /* Interrupt remapping */
     bool ga_enabled;
-    bool xtsup;
+    bool xtsup;     /* xtsup=on command line */
+    bool xten;      /* Enable x2apic */
 
     /* DMA address translation */
     bool dma_remap;
-- 
2.34.1

[PATCH 3/3] amd_iommu: Generate XT interrupts when xt support is enabled

[Sairaj Kodilkar]

AMD IOMMU uses MMIO registers 0x170-0x180 to generate the interrupts
when guest enables xt support through control register [IntCapXTEn]. The
guest programs these registers with appropriate vector and destination
ID instead of writing to PCI MSI capability.

Current AMD vIOMMU is capable of generating interrupts only through PCI
MSI capability and does not care about xt mode. Because of this AMD
vIOMMU cannot generate event log interrupts when the guest has enabled
xt mode.

Introduce a new flag "intcapxten" which is set when guest writes control
register [IntCapXTEn] (bit 51) and use vector and destination field in
the XT MMIO register (0x170) to support XT mode.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 hw/i386/amd_iommu.c  | 51 ++++++++++++++++++++++++++++++++++++++------
 hw/i386/amd_iommu.h  |  3 +++
 hw/i386/trace-events |  1 +
 3 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 7f08fc31111a..c6bc13c93679 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -200,18 +200,52 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
    amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
 }
 
+union mmio_xt_intr {
+    uint64_t val;
+    struct {
+        uint64_t rsvd_1:2,
+                 destination_mode:1,
+                 rsvd_2:5,
+                 destination_lo:24,
+                 vector:8,
+                 delivery_mode:1,
+                 rsvd_3:15,
+                 destination_hi:8;
+    };
+};
+
+static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
+{
+    union mmio_xt_intr xt_reg;
+    struct X86IOMMUIrq irq;
+
+    xt_reg.val = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
+
+    irq.vector = xt_reg.vector;
+    irq.delivery_mode = xt_reg.delivery_mode;
+    irq.dest_mode = xt_reg.destination_mode;
+    irq.dest = (xt_reg.destination_hi << 24) | xt_reg.destination_lo;
+    irq.trigger_mode = 0;
+    irq.redir_hint = 0;
+
+    x86_iommu_irq_to_msi_message(&irq, msg);
+}
+
 static void amdvi_generate_msi_interrupt(AMDVIState *s)
 {
     MSIMessage msg = {};
-    MemTxAttrs attrs = {
-        .requester_id = pci_requester_id(&s->pci->dev)
-    };
 
-    if (msi_enabled(&s->pci->dev)) {
+    if (s->intcapxten) {
+        trace_amdvi_generate_msi_interrupt("XT GEN");
+        amdvi_build_xt_msi_msg(s, &msg);
+    } else if (msi_enabled(&s->pci->dev)) {
+        trace_amdvi_generate_msi_interrupt("MSI");
         msg = msi_get_message(&s->pci->dev, 0);
-        address_space_stl_le(&address_space_memory, msg.address, msg.data,
-                             attrs, NULL);
+    } else {
+        trace_amdvi_generate_msi_interrupt("NO MSI");
+        return;
     }
+    apic_get_class(NULL)->send_msi(&msg);
 }
 
 static uint32_t get_next_eventlog_entry(AMDVIState *s)
@@ -1490,6 +1524,7 @@ static inline void amdvi_mmio_get_name(hwaddr addr,
     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)
+    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR, name)
     default:
         name = "UNHANDLED";
     }
@@ -1549,6 +1584,7 @@ static void amdvi_handle_control_write(AMDVIState *s)
                         AMDVI_MMIO_CONTROL_CMDBUFLEN);
     s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
     s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup;
+    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -1755,6 +1791,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
     case AMDVI_MMIO_STATUS:
         amdvi_mmio_reg_write(s, size, val, addr);
         break;
+    case AMDVI_MMIO_XT_GEN_INTR:
+        amdvi_mmio_reg_write(s, size, val, addr);
+        break;
     }
 }
 
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 32467d0bc241..399a4fb748e5 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -57,6 +57,7 @@
 #define AMDVI_MMIO_EXCL_BASE          0x0020
 #define AMDVI_MMIO_EXCL_LIMIT         0x0028
 #define AMDVI_MMIO_EXT_FEATURES       0x0030
+#define AMDVI_MMIO_XT_GEN_INTR        0x0170
 #define AMDVI_MMIO_COMMAND_HEAD       0x2000
 #define AMDVI_MMIO_COMMAND_TAIL       0x2008
 #define AMDVI_MMIO_EVENT_HEAD         0x2010
@@ -107,6 +108,7 @@
 #define AMDVI_MMIO_CONTROL_CMDBUFLEN      (1ULL << 12)
 #define AMDVI_MMIO_CONTROL_GAEN           (1ULL << 17)
 #define AMDVI_MMIO_CONTROL_XTEN           (1ULL << 50)
+#define AMDVI_MMIO_CONTROL_INTCAPXTEN     (1ULL << 51)
 
 /* MMIO status register bits */
 #define AMDVI_MMIO_STATUS_CMDBUF_RUN  (1 << 4)
@@ -421,6 +423,7 @@ struct AMDVIState {
     bool ga_enabled;
     bool xtsup;     /* xtsup=on command line */
     bool xten;      /* Enable x2apic */
+    bool intcapxten; /* Enable IOMMU x2apic interrupt generation */
 
     /* DMA address translation */
     bool dma_remap;
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index b704f4f90c3d..fe7aea4507ae 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -114,6 +114,7 @@ amdvi_ir_intctl(uint8_t val) "int_ctl 0x%"PRIx8
 amdvi_ir_target_abort(const char *str) "%s"
 amdvi_ir_delivery_mode(const char *str) "%s"
 amdvi_ir_irte_ga_val(uint64_t hi, uint64_t lo) "hi 0x%"PRIx64" lo 0x%"PRIx64
+amdvi_generate_msi_interrupt(const char *str) "Mode: %s"
 
 # vmport.c
 vmport_register(unsigned char command, void *func, void *opaque) "command: 0x%02x func: %p opaque: %p"
-- 
2.34.1

Re: [PATCH 0/3] amd_iommu: Support Generation of IOMMU XT interrupts

[Vasant Hegde]

On 11/18/2025 1:54 PM, Sairaj Kodilkar wrote:
> AMD IOMMU uses MMIO registers 0x170-0x180 to generate the interrupts when guest
> has enabled xt support through control register. The guest programs these
> registers with appropriate vector and destination ID instead of writing to PCI
> MSI capability.
> 
> Until now enabling the xt support through command line "xtsup=on" provided
> support for 128 bit IRTE. But it has few limitations:
> 
> 1. It does not consider if guest has actually enabled xt support through MMIO
>    control register (0x18). This may cause problems for the guests which do
>    not enable this support.
> 2. The vIOMMU is not capable of generating interrupts using vector and
>    destinatio ID in IOMMU x2APIC Control Registers (not supporting event log
>    interrupts).
> 
> To overcome above limitations, this patch series introduces new internal flag 
> "intcapxten" which is set when guest writes "1" to MMIO control register (0x18)
> bit 51 (IntCapXTEn) and adds support to generate event log interrupt using
> vector and 32 bit destination ID in XT MMIO register 0x170.

I have reviewed this series and it looks good. For entire series,

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>


-Vasant