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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 06, 2026 at 05:18:40PM +0100, Andrew Cooper wrote:
> 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.

The approach to use disable_iommu() is because that's closer to what
Linux does in iommu_disable(), which seems to explicitly disable one
feature at a time instead of writing zero to the command register in
one go.  I've been cautious in taking the same approach on Xen.

I don't mind doing a plain write of 0, let me test to ensure this is
OK.

> 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);
> +    }

In your snippet above, I think we want to unconditionally set
iommu->ctrl.raw = 0, and also propagate that 0 to the register,
otherwise we will inherit set bits from whatever is currently in the
control register:

> +    /* 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);

Thanks, Roger.