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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

~Andrew