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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2026 12:21, Roger Pau Monné wrote:
> On Thu, May 07, 2026 at 10:51:18AM +0200, Jan Beulich wrote:
>> 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 how do we know what needs to be zeroed?  We are then in the same
> position where the introduction of a new control register would cause
> the zeroing to no longer be accurate.

An option may be to zero everything we don't know about (plus perhaps
everything we know about, but don't otherwise use), on the assumption
that new (writable) registers added are okay to zero.

>>> 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.
> 
> Do we really support booting from such environments?  We would need
> much more careful handling of enabled features IMO, as blindly zeroing
> the whole MMIO register area is likely to not make the IOMMU happy if
> it was in an enabled state.
> 
> Note for example how Xen was zeroing the command and log buffer
> pointers ahead of disabling the features in the control register, just
> because those register are ahead of the control register in the MMIO
> space.

Hmm, yes, such ordering issues could also appear with new registers.
Then again, with the IOMMU as a whole disabled (which we would still
want to do up front), perhaps the order of other stores can be assumed
to not matter?

Jan