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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2026 10:46, Roger Pau Monné wrote:
> On Thu, May 07, 2026 at 10:03:05AM +0200, Jan Beulich wrote:
>> On 06.05.2026 18:51, 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, nor which
>>> values might be safe for those registers.  On a forthcoming platform doing
>>> the zeroing of the MMIO region does put the IOMMU in a broken state, which
>>> is not recoverable by the IOMMU initialization procedure in Xen.
>>>
>>> Instead just zero the control register, which mimics the current behavior
>>> with regards to how the control register is handled, and ensures the IOMU
>>> setup is done with the unit disabled.  This approach will need revisiting
>>> in order to support Preboot DMA Protection.
>>>
>>> Fold map_iommu_mmio_region() into its only caller, as the function body is
>>> just an ioremap() call after the removal of the memset().
>>>
>>> Fixes: 0700c962ac2d ("Add AMD IOMMU support into hypervisor")
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> While you got Andrew's R-b, I don't view that as enough to commit it. My
>> prior concern towards ...
>>
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -42,18 +42,6 @@ static bool iommu_has_ht_flag(struct amd_iommu *iommu, 
>>> u8 mask)
>>>      return iommu->ht_flags & mask;
>>>  }
>>>  
>>> -static int __init map_iommu_mmio_region(struct amd_iommu *iommu)
>>> -{
>>> -    iommu->mmio_base = ioremap(iommu->mmio_base_phys,
>>> -                               IOMMU_MMIO_REGION_LENGTH);
>>> -    if ( !iommu->mmio_base )
>>> -        return -ENOMEM;
>>> -
>>> -    memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH);
>>> -
>>> -    return 0;
>>> -}
>>
>> ... this part of the change wasn't addressed, neither verbally nor by an
>> adjustment to the description of what was committed. As previously stated,
>> blindly memset()-ing the entire area may not be the best of all options,
>> but the downsides of not doing this need to somehow be addressed. As
>> indicated, once they run out of bits in the main control register, they
>> likely will add a 2nd one. That'll then also need clearing, yet we have
>> no code to do so anymore.
> 
> I could introduce an opt-in command line option that forces the
> zeroing of the MMIO region (to have the option to resort to the
> previous behavior),

But we don't want to fully go back to this. We'd need a form that zeroes
what may be zeroed, without causing the issue you're trying to address.

> but I was (wrongly) under the impression that we
> have agreement the proposed approach was the least bad of the ones
> available, sorry.
> 
> Note how VT-d also doesn't zero the IOMMU registers MMIO page either,
> neither does it seems to zero the Global Command Register either,
> which I'm not saying it's correct, but is at least a (possibly wrong)
> precedent.  I don't think there's much we can do with the handling of
> enabled bits in possibly registers not know/handled by Xen.  Like on
> VT-d, we possibly need to rely on the firmware to handle the IOMMU in
> a half-sane configuration, with no enabled features on registers Xen
> doesn't know about.

As indicated before, for firmware we can likely rely on that. Pre-boot
non-firmware environments and especially Xen being kexec-ed (or being
run past something which was kexec-ed) may be of more concern.

Jan