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

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

On 11/26/25 7:13 AM, Sairaj Kodilkar wrote:
> 
> 
> 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.
>>
>> 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),
> 
> Hi Alejandro,
> 
> I don't think adding a VMSTATE_BOOL directly is a good idea because 
> it'll break backward
> compatibility (kernel running on older qemu won't be able to migrate on 
> newer qemu because
> change in the stream length of the migration data). In this case we will 
> have to increment
> the min_version_number to stop migration from older qemu
> I am thinking to add a new subsection as it'll not break the 
> compatibility. Let me know what
> do you think about this.
> 

You are right, the subsection approach is the right way of doing this.
Given the lack of updates to the code until very recently, I think it is 
unlikely anyone is using migration... but since the feature is 
officially available, we need to do the right thing.

A problem remains if we need to support migration from newer to older 
(i.e. missing xten) QEMU versions. Then we need to have a .needed() 
method to avoid sending the subsection, or otherwise the migration fails 
due to the dest (i.e. the old version) not knowing about it. We can try 
to find a workaround, but my thinking is that the added complexity is 
not worth it, again considering the extremely low likelihood of actually 
breaking a user setup.

I don't know if there is a precedent for this type of choice, so let me 
do some research, and hopefully others in thread will also comment with 
their opinion/guidance.

Thank you,
Alejandro

> Something like this...
> 
> static const VMStateDescription vmstate_xt = {
>        .name = "amd-iommu-xt",
>        .version_id = 1,
>        .minimum_version_id = 1,
>        .fields = (VMStateField[]) {
>            VMSTATE_BOOL(xten, AMDVIState),
>        }
> };
> 
> static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
>      ....
>      .subsections = (const VMStateDescription *const []) {
>                     &vmstate_xt,
>                     NULL
>      }
> };
> 
> I have tested above code.
> 
> Thanks
> Sairaj