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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/05/2026 9:58 am, Roger Pau Monné wrote:
> On Wed, May 06, 2026 at 09:20:07AM +0100, Andrew Cooper wrote:
>> On 06/05/2026 8:37 am, 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, neither what
>>> values might be safe for those registers.
>> Minor grammar.  "there, nor which values".
>>
>>> On a forthcoming platform doing
>>> the zeroing of the MMIO region can put the IOMMU in a broken state,
>> "does put"
>>
>>> which is not recovered by the IOMMU initialization procedure in Xen.
>> "recoverable".
>>
>>> diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
>>> b/xen/drivers/passthrough/amd/iommu_init.c
>>> index 76ae78e5ea53..8bf5ca4de18f 100644
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -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.
> But is it safe to backport the memset without also backporting the
> disabling side?  We might then be dealing with an enabled IOMMU which
> could lead to all sorts of fun.

I would absolutely take working with a potentially active IOMMU over
zeroing the registers which don't appear in the public documentation.

This is where BIOS norms save us.  The IOMMU can't be enabled without
active OS negotiation, or DoS wouldn't be able to boot.  It is only in
the past couple of years where this has not been the base compatibility
case on all systems.

But I see your point.  Given that we were clearing the main enable bit,
we ought to continue to do so for backport purposes.

It would be nice to have a printk_once() in there so we can spot when
the IOMMU is enabled.  I expect it not to trigger but if it does
trigger, we've got more investigation to do.

>> 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.
> Note that Linux (when not booted from kdump) will do a similar sequence of
> what I'm attempting to do here for Xen and will call iommu_disable()
> ahead of attempting to enable the IOMMU.

Lovely...  That's broken.

The AMD IOMMU makes this especially easy to do.  Each of the ring
buffers have enable bits separate to the general IOMMU enable, so you
can temporary disable, move the buffer and clear the ring pointers, then
re-enable.

What is critical for Preboot DMA protection is that DMA translation
doesn't get turned off.

~Andrew