Re: [PATCH v2 2/2] iommu/amd-vi: do not zero IOMMU MMIO region

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/05/2026 2:55 pm, Roger Pau Monne wrote:
> Attempting to memset the whole IOMMU MMIO region to zero is dangerous to
> say the least.  We don't know what registers might be there, nor which

Sorry, one more.  "We don't know which registers might".

> values might be safe for those registers.  On a forthcoming platform doing
> the zeroing of the MMIO region does put the IOMMU in a broken state, which
> is not recoverable by the IOMMU initialization procedure in Xen.
>
> Instead attempt to forcefully disable the IOMMU ahead of enabling it.  Fold
> map_iommu_mmio_region() into it's only caller, as the function body is just
> an ioremap() call after the removal of the memset().
>
> Fixes: 0700c962ac2d ("Add AMD IOMMU support into hypervisor")
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Zero the control register after calling disable_iommu().
>  - Print a warning message if the IOMMU is handed enabled to Xen from
>    firmware.
>  - Fix commit log grammar issues.
> ---
>  xen/drivers/passthrough/amd/iommu_init.c | 31 +++++++++++++-----------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
> b/xen/drivers/passthrough/amd/iommu_init.c
> index 76ae78e5ea53..ffc041211fb5 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -42,18 +42,6 @@ static bool iommu_has_ht_flag(struct amd_iommu *iommu, u8 
> mask)
>      return iommu->ht_flags & mask;
>  }
>  
> -static int __init map_iommu_mmio_region(struct amd_iommu *iommu)
> -{
> -    iommu->mmio_base = ioremap(iommu->mmio_base_phys,
> -                               IOMMU_MMIO_REGION_LENGTH);
> -    if ( !iommu->mmio_base )
> -        return -ENOMEM;
> -
> -    memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH);
> -
> -    return 0;
> -}
> -
>  static void __init unmap_iommu_mmio_region(struct amd_iommu *iommu)
>  {
>      if ( iommu->mmio_base )
> @@ -1367,11 +1355,14 @@ static int __init amd_iommu_prepare_one(struct 
> amd_iommu *iommu)
>  {
>      int rc = alloc_ivrs_mappings(iommu->sbdf.seg);
>  
> -    if ( !rc )
> -        rc = map_iommu_mmio_region(iommu);
>      if ( rc )
>          return rc;
>  
> +    iommu->mmio_base = ioremap(iommu->mmio_base_phys,
> +                               IOMMU_MMIO_REGION_LENGTH);
> +    if ( !iommu->mmio_base )
> +        return -ENOMEM;
> +
>      get_iommu_features(iommu);
>  
>      /*
> @@ -1381,6 +1372,18 @@ static int __init amd_iommu_prepare_one(struct 
> amd_iommu *iommu)
>      if ( amd_iommu_max_paging_mode < amd_iommu_min_paging_mode )
>          return -ERANGE;
>  
> +    /* Read current control register and forcefully disable the IOMMU. */
> +    iommu->ctrl.raw = readq(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> +    if ( iommu->ctrl.iommu_en )
> +        printk(XENLOG_WARNING
> +               "AMD-Vi: IOMMU %pp enabled by firmware (%016lx)\n",
> +               &iommu->sbdf, iommu->ctrl.raw);
> +    disable_iommu(iommu, true);
> +
> +    /* With the IOMMU disabled zero the control register. */
> +    iommu->ctrl.raw = 0;
> +    writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> +
>      return 0;
>  }
>  

I don't think calling disable_iommu() is a good thing here.

It's just a cascade of clearing one/few bits in ctrl at a time, but is
is added unconditionally so that's 5 UC stores writing 0's to the same
register in the common case.

I think this logic wants to be:

@@ -1381,6 +1372,18 @@ static int __init amd_iommu_prepare_one(struct amd_iommu 
*iommu)
     if ( amd_iommu_max_paging_mode < amd_iommu_min_paging_mode )
         return -ERANGE;
 
+    /* Check if the IOMMU is active, and disable. */
+    iommu->ctrl.raw = readq(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+    if ( iommu->ctrl.iommu_en )
+    {
+        printk(XENLOG_WARNING
+               "AMD-Vi: IOMMU %pp enabled by firmware (ctrl %016lx)\n",
+               &iommu->sbdf, iommu->ctrl.raw);
+
+        iommu->ctrl.raw = 0;
+        writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+    }
+
     return 0;
 }

 

which has the advantage that it's closer to the current behaviour, and
therefore arguably a safer backport.

The only thing that disable_iommu() does which isn't editing the control
register is playing with the PCI MSI enable bit, but that really doesn't
matter when we clear ctrl.int_cap_xt_en.  (And in fact, that path is
buggy because it clears MSI enable without inhibiting interrupt
generation, which architecturally will turn into legacy line interrupt
to deal with.)

We're just about to re-set everything up, so everything's going to get
re-enabled.

In the commit message, I think it's worth highlighting that zeroing the
control register is the current behaviour, and this will need revisiting
in order to support Preboot DMA Protection.

Or alternatively, would you like me to submit a patch?  (Happy either way.)

~Andrew