When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU itself are
sent based on the programming in XT IOMMU Interrupt Control Registers in MMIO
0x170-0x180 instead of the programming in the IOMMU's MSI capability registers.
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>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
 hw/i386/amd_iommu.h  | 17 ++++++++++++++++
 hw/i386/trace-events |  1 +
 3 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 4a86e62a3b92..bd24fdee7cfe 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -192,18 +192,38 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
    amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
 }
 
+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)
@@ -1482,6 +1502,7 @@ const char *amdvi_mmio_get_name(hwaddr addr)
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_BASE);
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_HEAD);
     MMIO_REG_TO_STRING(AMDVI_MMIO_PPR_TAIL);
+    MMIO_REG_TO_STRING(AMDVI_MMIO_XT_GEN_INTR);
 #undef MMIO_REG_TO_STRING
     default:
         return "UNHANDLED";
@@ -1525,6 +1546,15 @@ static void amdvi_handle_control_write(AMDVIState *s)
     s->ga_enabled = !!(control & AMDVI_MMIO_CONTROL_GAEN);
     s->xten = !!(control & AMDVI_MMIO_CONTROL_XTEN) && s->xtsup &&
               s->ga_enabled;
+    /*
+     * IntCapXTEn controls whether IOMMU-originated interrupts are sent based
+     * on the information in XT IOMMU Interrupt Control Registers rather than
+     * the IOMMU’s MSI capability registers. Therefore it requires IOMMU
+     * x2APIC support capabilities (i.e. XTSup=1), but it is independent of
+     * whether a driver chooses to enable x2APIC mode for interrupt remapping
+     * (i.e. XTEn=1).
+     */
+    s->intcapxten = !!(control & AMDVI_MMIO_CONTROL_INTCAPXTEN) && s->xtsup;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -1732,6 +1762,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;
     }
 }
 
@@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
     s->enabled = false;
     s->cmdbuf_enabled = false;
     s->xten = false;
+    s->intcapxten = false;
 
     /* reset MMIO */
     memset(s->mmior, 0, AMDVI_MMIO_SIZE);
@@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
        .minimum_version_id = 1,
        .fields = (VMStateField[]) {
            VMSTATE_BOOL(xten, AMDVIState),
+           VMSTATE_BOOL(intcapxten, AMDVIState),
            VMSTATE_END_OF_LIST()
        }
 };
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index d39019b216af..72139bb0c70b 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -53,6 +53,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
@@ -103,6 +104,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)
@@ -339,6 +341,20 @@ struct irte_ga {
   union irte_ga_hi hi;
 };
 
+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;
+    };
+};
+
 #define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
 OBJECT_DECLARE_SIMPLE_TYPE(AMDVIState, AMD_IOMMU_DEVICE)
 
@@ -417,6 +433,7 @@ struct AMDVIState {
     bool ga_enabled;
     bool xtsup;     /* xtsup=on command line */
     bool xten;      /* guest controlled, x2apic mode enabled */
+    bool intcapxten; /* guest controlled, IOMMU x2apic interrupts enabled */
 
     /* DMA address translation */
     bool dma_remap;
diff --git a/hw/i386/trace-events b/hw/i386/trace-events
index 5fa5e93b68dc..a1dfade20f18 100644
--- a/hw/i386/trace-events
+++ b/hw/i386/trace-events
@@ -118,6 +118,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

On 3/2/2026 5:21 PM, Sairaj Kodilkar wrote:
> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU itself are
> sent based on the programming in XT IOMMU Interrupt Control Registers in MMIO
> 0x170-0x180 instead of the programming in the IOMMU's MSI capability registers.
> 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>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>  hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>  hw/i386/trace-events |  1 +
>  3 files changed, 59 insertions(+), 6 deletions(-)
> 

.../...

>  
>      /* update the flags depending on the control register */
>      if (s->cmdbuf_enabled) {
> @@ -1732,6 +1762,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;
>      }
>  }
>  
> @@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
>      s->enabled = false;
>      s->cmdbuf_enabled = false;
>      s->xten = false;
> +    s->intcapxten = false;
>  
>      /* reset MMIO */
>      memset(s->mmior, 0, AMDVI_MMIO_SIZE);
> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>         .minimum_version_id = 1,
>         .fields = (VMStateField[]) {
>             VMSTATE_BOOL(xten, AMDVIState),
> +           VMSTATE_BOOL(intcapxten, AMDVIState),

Do we need to increase the version no?

-Vasant

On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>
> On 3/2/2026 5:21 PM, Sairaj Kodilkar wrote:
>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU itself are
>> sent based on the programming in XT IOMMU Interrupt Control Registers in MMIO
>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability registers.
>> 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>
>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>   hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>   hw/i386/trace-events |  1 +
>>   3 files changed, 59 insertions(+), 6 deletions(-)
>>
> .../...
>
>>   
>>       /* update the flags depending on the control register */
>>       if (s->cmdbuf_enabled) {
>> @@ -1732,6 +1762,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;
>>       }
>>   }
>>   
>> @@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
>>       s->enabled = false;
>>       s->cmdbuf_enabled = false;
>>       s->xten = false;
>> +    s->intcapxten = false;
>>   
>>       /* reset MMIO */
>>       memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>          .minimum_version_id = 1,
>>          .fields = (VMStateField[]) {
>>              VMSTATE_BOOL(xten, AMDVIState),
>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
> Do we need to increase the version no?

No, because we have introduced a separate subsection, the older and newer
qemu are compatible.

Thanks
Sairaj

On 3/3/26 4:55 AM, Sairaj Kodilkar wrote:
> 
> 
> On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>>
>> On 3/2/2026 5:21 PM, Sairaj Kodilkar wrote:
>>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU
>>> itself are
>>> sent based on the programming in XT IOMMU Interrupt Control Registers in
>>> MMIO
>>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability
>>> registers.
>>> 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>
>>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>> ---
>>>   hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>   hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>   hw/i386/trace-events |  1 +
>>>   3 files changed, 59 insertions(+), 6 deletions(-)
>>>
>> .../...
>>
>>>         /* update the flags depending on the control register */
>>>       if (s->cmdbuf_enabled) {
>>> @@ -1732,6 +1762,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;
>>>       }
>>>   }
>>>   @@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
>>>       s->enabled = false;
>>>       s->cmdbuf_enabled = false;
>>>       s->xten = false;
>>> +    s->intcapxten = false;
>>>         /* reset MMIO */
>>>       memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>          .minimum_version_id = 1,
>>>          .fields = (VMStateField[]) {
>>>              VMSTATE_BOOL(xten, AMDVIState),
>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>> Do we need to increase the version no?
> 
> No, because we have introduced a separate subsection, the older and newer
> qemu are compatible.
> 
I thought that was the case because the changes will still be part of the
same "release". I don't believe we guarantee migration compatibility
between random/intermediate commits, but...

If we are going to follow the guidelines strictly then I think Vasant's
observation is correct. The patch changes the layout of the subsection so
we are in the same scenario that lead us to include a subsection to begin with.

Because the two new values are still part of the same xt support domain, I
think it makes sense to keep them in the same subsection and the simplest
way is to do:

 static const VMStateDescription vmstate_xt = {
     .name = "amd-iommu-xt",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_BOOL(xten, AMDVIState),
-        VMSTATE_BOOL(intcapxten, AMDVIState),
+        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
         VMSTATE_END_OF_LIST()
     }
 };

That change is on top of your current patch. There seems to be precedent
for this based on my search in the git log. If you are ok with this
approach let me know and I'll apply it, no need to send a new revision.

Thank you,
Alejandro


> Thanks
> Sairaj

On 3/13/2026 5:46 AM, Alejandro Jimenez wrote:
>
> On 3/3/26 4:55 AM, Sairaj Kodilkar wrote:
>>
>> On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>>> On 3/2/2026 5:21 PM, Sairaj Kodilkar wrote:
>>>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU
>>>> itself are
>>>> sent based on the programming in XT IOMMU Interrupt Control Registers in
>>>> MMIO
>>>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability
>>>> registers.
>>>> 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>
>>>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>> ---
>>>>    hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>>    hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>>    hw/i386/trace-events |  1 +
>>>>    3 files changed, 59 insertions(+), 6 deletions(-)
>>>>
>>> .../...
>>>
>>>>          /* update the flags depending on the control register */
>>>>        if (s->cmdbuf_enabled) {
>>>> @@ -1732,6 +1762,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;
>>>>        }
>>>>    }
>>>>    @@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
>>>>        s->enabled = false;
>>>>        s->cmdbuf_enabled = false;
>>>>        s->xten = false;
>>>> +    s->intcapxten = false;
>>>>          /* reset MMIO */
>>>>        memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>>           .minimum_version_id = 1,
>>>>           .fields = (VMStateField[]) {
>>>>               VMSTATE_BOOL(xten, AMDVIState),
>>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>> Do we need to increase the version no?
>> No, because we have introduced a separate subsection, the older and newer
>> qemu are compatible.
>>
> I thought that was the case because the changes will still be part of the
> same "release". I don't believe we guarantee migration compatibility
> between random/intermediate commits, but...
>
> If we are going to follow the guidelines strictly then I think Vasant's
> observation is correct. The patch changes the layout of the subsection so
> we are in the same scenario that lead us to include a subsection to begin with.
>
> Because the two new values are still part of the same xt support domain, I
> think it makes sense to keep them in the same subsection and the simplest
> way is to do:
>
>   static const VMStateDescription vmstate_xt = {
>       .name = "amd-iommu-xt",
> -    .version_id = 1,
> +    .version_id = 2,
>       .minimum_version_id = 1,
>       .fields = (VMStateField[]) {
>           VMSTATE_BOOL(xten, AMDVIState),
> -        VMSTATE_BOOL(intcapxten, AMDVIState),
> +        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
>           VMSTATE_END_OF_LIST()
>       }
>   };
>
> That change is on top of your current patch. There seems to be precedent
> for this based on my search in the git log. If you are ok with this
> approach let me know and I'll apply it, no need to send a new revision.

I am not sure if it is really useful here. Because without this patch
xt-support will not work and migrating from vm which only has first two
patches to the vm which has all three patches does not make sense to me.
I think we should treat three patches as a single unit, let me know what
do you think about this ?

Or we can do something like [1], which introduces new subsections
for each capbility.
https://lore.kernel.org/qemu-devel/20180119050005.29392-1-sjitindarsingh@gmail.com/

But this makes sense only when you can use each capability separately.

Thanks
Sairaj
>
> Thank you,
> Alejandro
>
>
>> Thanks
>> Sairaj

On 3/13/26 1:20 AM, Sairaj Kodilkar wrote:
> 
> 
> On 3/13/2026 5:46 AM, Alejandro Jimenez wrote:
>>
>> On 3/3/26 4:55 AM, Sairaj Kodilkar wrote:
>>>
>>> On 3/3/2026 3:17 PM, Vasant Hegde wrote:
>>>> On 3/2/2026 5:21 PM, Sairaj Kodilkar wrote:
>>>>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU
>>>>> itself are
>>>>> sent based on the programming in XT IOMMU Interrupt Control Registers in
>>>>> MMIO
>>>>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability
>>>>> registers.
>>>>> 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>
>>>>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>>>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>>> ---
>>>>>    hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>>>    hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>>>    hw/i386/trace-events |  1 +
>>>>>    3 files changed, 59 insertions(+), 6 deletions(-)
>>>>>
>>>> .../...
>>>>
>>>>>          /* update the flags depending on the control register */
>>>>>        if (s->cmdbuf_enabled) {
>>>>> @@ -1732,6 +1762,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;
>>>>>        }
>>>>>    }
>>>>>    @@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
>>>>>        s->enabled = false;
>>>>>        s->cmdbuf_enabled = false;
>>>>>        s->xten = false;
>>>>> +    s->intcapxten = false;
>>>>>          /* reset MMIO */
>>>>>        memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>>>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>>>           .minimum_version_id = 1,
>>>>>           .fields = (VMStateField[]) {
>>>>>               VMSTATE_BOOL(xten, AMDVIState),
>>>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>>> Do we need to increase the version no?
>>> No, because we have introduced a separate subsection, the older and newer
>>> qemu are compatible.
>>>
>> I thought that was the case because the changes will still be part of the
>> same "release". I don't believe we guarantee migration compatibility
>> between random/intermediate commits, but...
>>
>> If we are going to follow the guidelines strictly then I think Vasant's
>> observation is correct. The patch changes the layout of the subsection so
>> we are in the same scenario that lead us to include a subsection to begin
>> with.
>>
>> Because the two new values are still part of the same xt support domain, I
>> think it makes sense to keep them in the same subsection and the simplest
>> way is to do:
>>
>>   static const VMStateDescription vmstate_xt = {
>>       .name = "amd-iommu-xt",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>       .minimum_version_id = 1,
>>       .fields = (VMStateField[]) {
>>           VMSTATE_BOOL(xten, AMDVIState),
>> -        VMSTATE_BOOL(intcapxten, AMDVIState),
>> +        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
>>           VMSTATE_END_OF_LIST()
>>       }
>>   };
>>
>> That change is on top of your current patch. There seems to be precedent
>> for this based on my search in the git log. If you are ok with this
>> approach let me know and I'll apply it, no need to send a new revision.
> 
> I am not sure if it is really useful here. Because without this patch
> xt-support will not work and migrating from vm which only has first two
> patches to the vm which has all three patches does not make sense to me.

I agree it doesn't make much sense from a practical standpoint, unless we
are bisecting a migration issue, and we don't want to fail between these
two commits.
But again, adding a new field to the subsection does change the payload
that is sent "on the wire", and increasing the version like in my example
above is the minimum change that keeps it all fully correct for migrations
going forward (just like we ensured when adding the subsection in the first
place).

> I think we should treat three patches as a single unit, let me know what
> do you think about this ?
> 
I considered just moving the creation of the entire subsection to the last
patch, but I think it is unnecessary. The approach of incrementing the
version ID above doesn't have any downsides other than little additional
complexity, and it is the "technically correct" way to do it.

I don't want to introduce new subsections, since that has basically the
same effect as the method I proposed, and it makes more sense to keep all
of the XT-related fields in the same subsection (appropriately named
"amd-iommu-xt")

So unless you have any correctness concerns, we should go with my initial
suggestion (actually it was Vasant's). I get the usefulness is limited, but
it is the proper way to do this based on my interpretation of the docs.

Thank you,
Alejandro

> Or we can do something like [1], which introduces new subsections
> for each capbility.
> https://lore.kernel.org/qemu-devel/20180119050005.29392-1-
> sjitindarsingh@gmail.com/
> 
> But this makes sense only when you can use each capability separately.
> 
> Thanks
> Sairaj
>>
>> Thank you,
>> Alejandro
>>
>>
>>> Thanks
>>> Sairaj
> 

>>>>>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU
>>>>>> itself are
>>>>>> sent based on the programming in XT IOMMU Interrupt Control Registers in
>>>>>> MMIO
>>>>>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability
>>>>>> registers.
>>>>>> 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>
>>>>>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>>>>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>>>> ---
>>>>>>     hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>>>>     hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>>>>     hw/i386/trace-events |  1 +
>>>>>>     3 files changed, 59 insertions(+), 6 deletions(-)
>>>>>>
>>>>> .../...
>>>>>
>>>>>>           /* update the flags depending on the control register */
>>>>>>         if (s->cmdbuf_enabled) {
>>>>>> @@ -1732,6 +1762,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;
>>>>>>         }
>>>>>>     }
>>>>>>     @@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
>>>>>>         s->enabled = false;
>>>>>>         s->cmdbuf_enabled = false;
>>>>>>         s->xten = false;
>>>>>> +    s->intcapxten = false;
>>>>>>           /* reset MMIO */
>>>>>>         memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>>>>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>>>>            .minimum_version_id = 1,
>>>>>>            .fields = (VMStateField[]) {
>>>>>>                VMSTATE_BOOL(xten, AMDVIState),
>>>>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>>>> Do we need to increase the version no?
>>>> No, because we have introduced a separate subsection, the older and newer
>>>> qemu are compatible.
>>>>
>>> I thought that was the case because the changes will still be part of the
>>> same "release". I don't believe we guarantee migration compatibility
>>> between random/intermediate commits, but...
>>>
>>> If we are going to follow the guidelines strictly then I think Vasant's
>>> observation is correct. The patch changes the layout of the subsection so
>>> we are in the same scenario that lead us to include a subsection to begin
>>> with.
>>>
>>> Because the two new values are still part of the same xt support domain, I
>>> think it makes sense to keep them in the same subsection and the simplest
>>> way is to do:
>>>
>>>    static const VMStateDescription vmstate_xt = {
>>>        .name = "amd-iommu-xt",
>>> -    .version_id = 1,
>>> +    .version_id = 2,
>>>        .minimum_version_id = 1,
>>>        .fields = (VMStateField[]) {
>>>            VMSTATE_BOOL(xten, AMDVIState),
>>> -        VMSTATE_BOOL(intcapxten, AMDVIState),
>>> +        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
>>>            VMSTATE_END_OF_LIST()
>>>        }
>>>    };
>>>
>>> That change is on top of your current patch. There seems to be precedent
>>> for this based on my search in the git log. If you are ok with this
>>> approach let me know and I'll apply it, no need to send a new revision.
>> I am not sure if it is really useful here. Because without this patch
>> xt-support will not work and migrating from vm which only has first two
>> patches to the vm which has all three patches does not make sense to me.
> I agree it doesn't make much sense from a practical standpoint, unless we
> are bisecting a migration issue, and we don't want to fail between these
> two commits.
> But again, adding a new field to the subsection does change the payload
> that is sent "on the wire", and increasing the version like in my example
> above is the minimum change that keeps it all fully correct for migrations
> going forward (just like we ensured when adding the subsection in the first
> place).
>
>> I think we should treat three patches as a single unit, let me know what
>> do you think about this ?
>>
> I considered just moving the creation of the entire subsection to the last
> patch, but I think it is unnecessary. The approach of incrementing the
> version ID above doesn't have any downsides other than little additional
> complexity, and it is the "technically correct" way to do it.
>
> I don't want to introduce new subsections, since that has basically the
> same effect as the method I proposed, and it makes more sense to keep all
> of the XT-related fields in the same subsection (appropriately named
> "amd-iommu-xt")
>
> So unless you have any correctness concerns, we should go with my initial
> suggestion (actually it was Vasant's). I get the usefulness is limited, but
> it is the proper way to do this based on my interpretation of the docs.
Hi Alejandro,

I don't have any correctness concerns. I was trying to understand pros and
cons of this method. I am ok with this change.

Thanks
Sairaj