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

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

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),

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.

Thank you,
Alejandro

>       /* DMA address translation */
>       bool dma_remap;