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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 11/26/2025 4:34 AM, Alejandro Jimenez wrote:
> Hi Sairaj,
>
> I have a couple of suggestions, and one addition I believe is needed 
> in the code, but overall looks good.
>
> On 11/18/25 3:24 AM, Sairaj Kodilkar wrote:
>> Current code uses 32 bit cpu destination irrespective of the fact that
>
> s/"32 bit cpu destination"/"32-bit destination ID"
>
> I think it fits the language used by the spec slightly better.
>
>> guest has enabled xt support through control register[XTEn] and
>
> a guest has enabled x2APIC support ...
>
> I think it is better to replace "xt" above with "x2APIC", which 
> describes what the XT feature is/does.
>
>> 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;
>
> I think we should also include a new xten field in 
> vmstate_amdvi_sysbus_migratable, to ensure the remapping behavior 
> stays consistent after migration. i.e.

Right I completely missed this

>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 9bf36ef608..5940011ef1 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -2452,6 +2452,7 @@ static const VMStateDescription 
> vmstate_amdvi_sysbus_migratable = {
>        /* Updated in  amdvi_handle_control_write() */
>        VMSTATE_BOOL(enabled, AMDVIState),
>        VMSTATE_BOOL(ga_enabled, AMDVIState),
> +      VMSTATE_BOOL(xten, AMDVIState),
>        /* bool ats_enabled is obsolete */
>        VMSTATE_UNUSED(1), /* was ats_enabled */
>        VMSTATE_BOOL(cmdbuf_enabled, AMDVIState),
>
> There is more work to do there, but this seems straightforward.
>
>>       /* 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 */
>
> I'd reword the comment to indicate this what the guest toggles for 
> enabling x2apic. e.g.
>
> bool xten;      /* guest controlled, x2apic mode enabled */
>
> I am aware that other fields that are also "guest controlled" don't 
> have similar comments. My idea is to highlight that the xten is 
> "toggled" at runtime, and is different from xtsup, which is a 
> capability inherent to the hardware being emulated and set during 
> initialization.

Yep that makes sense

Thanks
Sairaj