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>
Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
 hw/i386/amd_iommu.h |  4 ---
 2 files changed, 27 insertions(+), 53 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 789e09d6f2bc..f5aa9c763d02 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -35,29 +35,6 @@
 #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"
-};
-
 struct AMDVIAddressSpace {
     PCIBus *bus;                /* PCIBus (for bus number)              */
     uint8_t devfn;              /* device function                      */
@@ -1484,31 +1461,31 @@ 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) {
+#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_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);
+#undef MMIO_REG_TO_STRING
+    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 +1505,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 +1661,8 @@ 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, offset);
+
     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 302ccca5121f..ca4ff9fffee3 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
-- 
2.34.1

Hi Sairaj,

On 3/2/26 6:51 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>
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
>  hw/i386/amd_iommu.h |  4 ---
>  2 files changed, 27 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 789e09d6f2bc..f5aa9c763d02 100644

[...]

> -
> -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 +1505,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 +1661,8 @@ 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, offset);
> +

There is a small change in the trace output here from the previous patches,
and it is because the use of offset in the last argument vs the previous
patches which still use (addr & ~0x07).
My understanding is that the offset the trace uses is the MMIO offset
(which gives a mapping between the name of the register we print and its
offset in the MMIO space).

The offset variable that amdvi_mmio_write() calculates earlier is the byte
offset within the MMIO register, and it is needed for those special cases
(i.e. hacks) where a guest driver does not write with a full 64-bit write
to the register, so that we need to delay the side effect until the MMIO
register has been fully updated. That is my understanding of the code that
does:

1709         if (offset || (size == 8)) {

1710             amdvi_handle_devtab_write(s);

1711         }


i.e. don't apply the architectural side effects unless we write to the full
64-bit register at once, or offset is not zero. This last condition meaning
we do word writes, and write to the high word last i.e. offset !=0.

So in short, I will fix the call to trace_amdvi_mmio_write() to continue to
use (addr & ~0x07), and will also change the switch condition in
amdvi_mmio_get_name() to use the same.
No need to send another revision, but I just wanted to highlight the
difference so you are aware of the change.

Thank you,
Alejandro


>      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 302ccca5121f..ca4ff9fffee3 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

On 3/13/2026 3:29 AM, Alejandro Jimenez wrote:
> Hi Sairaj,
>
> On 3/2/26 6:51 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>
>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
>>   hw/i386/amd_iommu.h |  4 ---
>>   2 files changed, 27 insertions(+), 53 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 789e09d6f2bc..f5aa9c763d02 100644
> [...]
>
>> -
>> -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 +1505,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 +1661,8 @@ 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, offset);
>> +
> There is a small change in the trace output here from the previous patches,
> and it is because the use of offset in the last argument vs the previous
> patches which still use (addr & ~0x07).
> My understanding is that the offset the trace uses is the MMIO offset
> (which gives a mapping between the name of the register we print and its
> offset in the MMIO space).
>
> The offset variable that amdvi_mmio_write() calculates earlier is the byte
> offset within the MMIO register, and it is needed for those special cases
> (i.e. hacks) where a guest driver does not write with a full 64-bit write
> to the register, so that we need to delay the side effect until the MMIO
> register has been fully updated. That is my understanding of the code that
> does:
>
> 1709         if (offset || (size == 8)) {
>
> 1710             amdvi_handle_devtab_write(s);
>
> 1711         }
>
>
> i.e. don't apply the architectural side effects unless we write to the full
> 64-bit register at once, or offset is not zero. This last condition meaning
> we do word writes, and write to the high word last i.e. offset !=0.
>
> So in short, I will fix the call to trace_amdvi_mmio_write() to continue to
> use (addr & ~0x07), and will also change the switch condition in
> amdvi_mmio_get_name() to use the same.
> No need to send another revision, but I just wanted to highlight the
> difference so you are aware of the change.
>
> Thank you,
> Alejandro
>

Hey Alejandro,

Thanks, for pointing this out. I got confused because of the same naming
i.e. offset in both the cases and missed '~'.
I should have used addr & ~0x07 directly.

Thanks
Sairaj

>>       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 302ccca5121f..ca4ff9fffee3 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

On 3/12/26 5:59 PM, Alejandro Jimenez wrote:
> Hi Sairaj,
> 
> On 3/2/26 6:51 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>
>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>  hw/i386/amd_iommu.c | 76 ++++++++++++++++-----------------------------
>>  hw/i386/amd_iommu.h |  4 ---
>>  2 files changed, 27 insertions(+), 53 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 789e09d6f2bc..f5aa9c763d02 100644
> 
> [...]
> 
>> -
>> -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 +1505,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 +1661,8 @@ 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, offset);
>> +
> 
> There is a small change in the trace output here from the previous patches,
> and it is because the use of offset in the last argument vs the previous
> patches which still use (addr & ~0x07).
> My understanding is that the offset the trace uses is the MMIO offset
> (which gives a mapping between the name of the register we print and its
> offset in the MMIO space).
> 
> The offset variable that amdvi_mmio_write() calculates earlier is the byte
> offset within the MMIO register, and it is needed for those special cases
> (i.e. hacks) where a guest driver does not write with a full 64-bit write
> to the register, so that we need to delay the side effect until the MMIO
> register has been fully updated. That is my understanding of the code that
> does:
> 
> 1709         if (offset || (size == 8)) {
> 
> 1710             amdvi_handle_devtab_write(s);
> 
> 1711         }
> 
> 
> i.e. don't apply the architectural side effects unless we write to the full
> 64-bit register at once, or offset is not zero. This last condition meaning
> we do word writes, and write to the high word last i.e. offset !=0.
> 
To clarify because I made a mistake above and this is confusing enough:

I should have written dword (i.e. 32-bits) instead of word. So the hack is
accounting for two 32-bit writes, first to low and then to higher 32bits of
these registers mapped to the MMIO space. We should probably guard against
more finely grained accesses, since IIUC the spec says byte accesses are
possible... but that is a different can of worms.

Hopefully less confusing that before...
Alejandro

> So in short, I will fix the call to trace_amdvi_mmio_write() to continue to
> use (addr & ~0x07), and will also change the switch condition in
> amdvi_mmio_get_name() to use the same.
> No need to send another revision, but I just wanted to highlight the
> difference so you are aware of the change.
> 
> Thank you,
> Alejandro
> 
> 
>>      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 302ccca5121f..ca4ff9fffee3 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
> 
>