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 <[email protected]>
---
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.
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
