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

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

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
>