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

[Sairaj Kodilkar <sarunkod@amd.com>]

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