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 11:17:25AM +0200, Jan Beulich wrote:
> On 06.05.2026 10:43, Roger Pau Monné wrote:
> > 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.
> 
> Maybe better to explicitly write out that 0 right away, even if you want
> to keep using disable_iommu()?

Yeah, I also considered that.  So after disable_iommu() set the cached
control state to 0 and also zero the control register right there.
Can do in the next version, unless there are further objections.

Thanks, Roger.