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

[Sairaj Kodilkar]

On 11/27/2025 3:46 AM, Alejandro Jimenez wrote:


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 <sarun...@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.


I think .needed only useful when the field is rarely used. For our use 
case the guest will always
try to enable IOMMU XT interrupts. So we will anyway endup breaking the 
migration from newer
to older.

I think we can forbid the migration from newer to older. As you said the 
added complexity is not
worth it.

Thanks
Sairaj