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

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

On Thu, May 07, 2026 at 10:03:05AM +0200, Jan Beulich wrote:
> On 06.05.2026 18:51, 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
> > 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 just zero the control register, which mimics the current behavior
> > with regards to how the control register is handled, and ensures the IOMU
> > setup is done with the unit disabled.  This approach will need revisiting
> > in order to support Preboot DMA Protection.
> > 
> > Fold map_iommu_mmio_region() into its 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>
> 
> While you got Andrew's R-b, I don't view that as enough to commit it. My
> prior concern towards ...
> 
> > --- 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;
> > -}
> 
> ... this part of the change wasn't addressed, neither verbally nor by an
> adjustment to the description of what was committed. As previously stated,
> blindly memset()-ing the entire area may not be the best of all options,
> but the downsides of not doing this need to somehow be addressed. As
> indicated, once they run out of bits in the main control register, they
> likely will add a 2nd one. That'll then also need clearing, yet we have
> no code to do so anymore.

I could introduce an opt-in command line option that forces the
zeroing of the MMIO region (to have the option to resort to the
previous behavior), but I was (wrongly) under the impression that we
have agreement the proposed approach was the least bad of the ones
available, sorry.

Note how VT-d also doesn't zero the IOMMU registers MMIO page either,
neither does it seems to zero the Global Command Register either,
which I'm not saying it's correct, but is at least a (possibly wrong)
precedent.  I don't think there's much we can do with the handling of
enabled bits in possibly registers not know/handled by Xen.  Like on
VT-d, we possibly need to rely on the firmware to handle the IOMMU in
a half-sane configuration, with no enabled features on registers Xen
doesn't know about.

Regards, Roger.