[v1] amd_iommu: Support Generation of IOMMU XT interrupts

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

Posted by Sairaj Kodilkar 1 week, 3 days ago

Current code uses 32 bit cpu destination irrespective of the fact that
guest has enabled x2APIC support through control register[XTEn] and
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). Also, add a new subsection in
`VMStateDescription` for backward compatibility during vm migration.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 21 +++++++++++++++++++--
 hw/i386/amd_iommu.h |  4 +++-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 62175cc366ac..850d3920a76d 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1535,6 +1535,8 @@ 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 &&
+              s->ga_enabled;
 
     /* update the flags depending on the control register */
     if (s->cmdbuf_enabled) {
@@ -2007,7 +2009,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 {
@@ -2390,6 +2392,7 @@ static void amdvi_init(AMDVIState *s)
     s->mmio_enabled = false;
     s->enabled = false;
     s->cmdbuf_enabled = false;
+    s->xten = false;
 
     /* reset MMIO */
     memset(s->mmior, 0, AMDVI_MMIO_SIZE);
@@ -2454,6 +2457,16 @@ static void amdvi_sysbus_reset(DeviceState *dev)
     amdvi_reset_address_translation_all(s);
 }
 
+static const VMStateDescription vmstate_xt = {
+       .name = "amd-iommu-xt",
+       .version_id = 1,
+       .minimum_version_id = 1,
+       .fields = (VMStateField[]) {
+           VMSTATE_BOOL(xten, AMDVIState),
+           VMSTATE_END_OF_LIST()
+       }
+};
+
 static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
     .name = "amd-iommu",
     .version_id = 1,
@@ -2498,7 +2511,11 @@ static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
       VMSTATE_UINT8_ARRAY(romask, AMDVIState, AMDVI_MMIO_SIZE),
       VMSTATE_UINT8_ARRAY(w1cmask, AMDVIState, AMDVI_MMIO_SIZE),
       VMSTATE_END_OF_LIST()
-    }
+    },
+    .subsections = (const VMStateDescription *const []) {
+                    &vmstate_xt,
+                    NULL
+     }
 };
 
 static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 302ccca5121f..e9401f3a5c27 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;      /* guest controlled, x2apic mode enabled */
 
     /* DMA address translation */
     bool dma_remap;
-- 
2.34.1

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

Posted by Alejandro Jimenez 6 days, 3 hours ago

One minor nit that I mentioned on v1:

On 1/29/26 5:28 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"

> guest has enabled x2APIC support through control register[XTEn] and
> 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). Also, add a new subsection in
> `VMStateDescription` for backward compatibility during vm migration.
> 
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

> ---
>  hw/i386/amd_iommu.c | 21 +++++++++++++++++++--
>  hw/i386/amd_iommu.h |  4 +++-
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 62175cc366ac..850d3920a76d 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1535,6 +1535,8 @@ 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 &&
> +              s->ga_enabled;
>  
>      /* update the flags depending on the control register */
>      if (s->cmdbuf_enabled) {
> @@ -2007,7 +2009,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 {
> @@ -2390,6 +2392,7 @@ static void amdvi_init(AMDVIState *s)
>      s->mmio_enabled = false;
>      s->enabled = false;
>      s->cmdbuf_enabled = false;
> +    s->xten = false;
>  
>      /* reset MMIO */
>      memset(s->mmior, 0, AMDVI_MMIO_SIZE);
> @@ -2454,6 +2457,16 @@ static void amdvi_sysbus_reset(DeviceState *dev)
>      amdvi_reset_address_translation_all(s);
>  }
>  
> +static const VMStateDescription vmstate_xt = {
> +       .name = "amd-iommu-xt",
> +       .version_id = 1,
> +       .minimum_version_id = 1,
> +       .fields = (VMStateField[]) {
> +           VMSTATE_BOOL(xten, AMDVIState),
> +           VMSTATE_END_OF_LIST()
> +       }
> +};
> +
>  static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
>      .name = "amd-iommu",
>      .version_id = 1,
> @@ -2498,7 +2511,11 @@ static const VMStateDescription vmstate_amdvi_sysbus_migratable = {
>        VMSTATE_UINT8_ARRAY(romask, AMDVIState, AMDVI_MMIO_SIZE),
>        VMSTATE_UINT8_ARRAY(w1cmask, AMDVIState, AMDVI_MMIO_SIZE),
>        VMSTATE_END_OF_LIST()
> -    }
> +    },
> +    .subsections = (const VMStateDescription *const []) {
> +                    &vmstate_xt,
> +                    NULL
> +     }
>  };
>  
>  static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 302ccca5121f..e9401f3a5c27 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;      /* guest controlled, x2apic mode enabled */
>  
>      /* DMA address translation */
>      bool dma_remap;

[PATCH v2 1/3] amd_iommu: Use switch case to determine mmio register name
[PATCH v2 2/3] amd_iommu: Turn on XT support only when guest has enabled it
[PATCH v2 3/3] amd_iommu: Generate XT interrupts when xt support is enabled