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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, May 06, 2026 at 09:20:07AM +0100, Andrew Cooper wrote:
> On 06/05/2026 8:37 am, 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, neither what
> > values might be safe for those registers.
> 
> Minor grammar.  "there, nor which values".
> 
> > On a forthcoming platform doing
> > the zeroing of the MMIO region can put the IOMMU in a broken state,
> 
> "does put"
> 
> > which is not recovered by the IOMMU initialization procedure in Xen.
> 
> "recoverable".
> 
> > diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> > index 76ae78e5ea53..8bf5ca4de18f 100644
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -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,11 @@ 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);
> > +    disable_iommu(iommu, true);
> > +    iommu->ctrl.raw = 0;
> > +
> >      return 0;
> >  } 
> 
> These two things are unrelated at want splitting into separate patches
> at a minimum.  The removal of memset() critically needs backporting.

But is it safe to backport the memset without also backporting the
disabling side?  We might then be dealing with an enabled IOMMU which
could lead to all sorts of fun.

> As for disabling the IOMMU, I'm not certain it's wise.
> 
> Linux can already "bring up" an already-live IOMMU and Xen needs to gain
> this ability in due course.  This is mainly for supporting PreBoot DMA
> Protection, but also for things like the kexec environment.

Note that Linux (when not booted from kdump) will do a similar sequence of
what I'm attempting to do here for Xen and will call iommu_disable()
ahead of attempting to enable the IOMMU.

I understand that when Xen supports kdump or preboot DMA protection we
would need to be more careful there, but going from zeroing the whole
MMIO area (which disables everything) to not doing any kind of
disabling is IMO a dangerous approach, and I would rather backport a
change that at least attempts to make sure the IOMMU is disabled.

Thanks, Roger.