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

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

On Wed, May 06, 2026 at 10:28:52AM +0200, Jan Beulich wrote:
> On 06.05.2026 09:37, Roger Pau Monne wrote:
> > @@ -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);
> 
> Don't you also need to pre-fill iommu->features?

Indeed, that's done just ahead of this chunk, in the
get_iommu_features() call.

> And with that field's use in
> disable_iommu(), won't we be at risk of leaving stuff enabled which we are
> entirely unaware of?

Possibly, yes, that's always a risk.

> Even if we fully cleared the control register (which
> would eliminate the need to fetch features), down the road a 2nd control
> register could appear.

We do clear the control register, it's indirectly done by us setting
iommu->ctrl.raw = 0 after the disable_iommu() call.

I did wonder about just doing a write of 0 to the control register,
but I think it's best if we try to gracefully disable the features (as
done in disable_iommu()), and then reset the cached control state to
0.  Future writes to the control register will clear any bits not
directly set by Xen.

> Has it become clear which register(s) or bit(s) it
> really is that are causing the observed issue? IOW is there truly something
> we may not clear?

It's not so much as not clearing something, but rather the clearing
itself putting the IOMMU in a broken state which we then can't recover
from using the initialization procedure in Xen.

Thanks, Roger.