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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2026 11:02, Andrew Cooper wrote:
> On 06/05/2026 9:32 am, Jan Beulich wrote:
>> On 06.05.2026 10:20, Andrew Cooper wrote:
>>> On 06/05/2026 8:37 am, Roger Pau Monne wrote:
>>>> @@ -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.
>>>
>>> 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.
>> While I agree we would better support this, as per my reply to Roger: How
>> is that going to work if the IOMMU has features enabled we may not even
>> be aware of? We'd still need to blindly clear everything we can't drive
>> ourselves.
> 
> Zeroing 16k of unknown MMIO is completely unreasonable.  It is not RAM,
> and 0 is not a safe thing to write into an unknown register.

>From a very general perspective I agree. However, when adding new registers
(or new bits in existing ones), having them default to 0 (and hence making
0 be a valid value) is common practice.

> But to the AMD IOMMU specifically, the spec makes it clear that there
> are registers configured by firmware that we are expected to leave alone.

Well, okay. For firmware settings I think we can assume these would indeed
be settings, not enables of any features which would typically require
driving by an OS. Yet that still leaves the question (along the lines of
what I had raised before) of how we'd deal with being invoked with unknown
to us features enabled. We need to disable them, yet how do you suggest
doing that without blindly clearing most (if not all) registers? The only
clean way of doing that would look to be a "soft reset" command to the
IOMMU (of course not to be issued via the command queue). I'm unaware of
anything like this, though.

Jan