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

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

[Roger Pau Monne]

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@citrix.com>
---
Changes since v2:
 - Avoid the disable_iommu() dance.
 - Expand commit message a bit.

Changes since v1:
 - Zero the control register after calling disable_iommu().
 - Print a warning message if the IOMMU is handed enabled to Xen from
   firmware.
 - Fix commit log grammar issues.
---
 xen/drivers/passthrough/amd/iommu_init.c | 33 ++++++++++++++----------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index d77dd8511288..e0c8925c33f7 100644
--- 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;
-}
-
 static void __init unmap_iommu_mmio_region(struct amd_iommu *iommu)
 {
     if ( iommu->mmio_base )
@@ -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,20 @@ static int __init amd_iommu_prepare_one(struct amd_iommu *iommu)
     if ( amd_iommu_max_paging_mode < amd_iommu_min_paging_mode )
         return -ERANGE;
 
+    /*
+     * Check whether the IOMMU is already enabled and unconditionally disable
+     * it (zero the control register) ahead of Xen setup.  Needs to be
+     * revisited to support Preboot DMA Protection.
+     */
+    iommu->ctrl.raw = readq(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+    if ( iommu->ctrl.iommu_en )
+        printk(XENLOG_WARNING
+               "AMD-Vi: IOMMU %pp enabled by firmware (ctrl %016lx)\n",
+               &iommu->sbdf, iommu->ctrl.raw);
+
+    iommu->ctrl.raw = 0;
+    writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+
     return 0;
 }
 
-- 
2.53.0

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

[Andrew Cooper]

On 06/05/2026 5:51 pm, 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

what->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@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper@citrix.com>

Thanks.

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

[Jan Beulich]

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@citrix.com>

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.

Jan

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

[Roger Pau Monné]

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@citrix.com>
> 
> 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 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.

Regards, Roger.

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

[Jan Beulich]

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@citrix.com>
>>
>> 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

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

[Roger Pau Monné]

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@citrix.com>
> >>
> >> 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.

> > 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.

Thanks, Roger.

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

[Jan Beulich]

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@citrix.com>
>>>>
>>>> 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

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

[Roger Pau Monné]

On Thu, May 07, 2026 at 01:20:25PM +0200, Jan Beulich wrote:
> 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@citrix.com>
> >>>>
> >>>> 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.

I don't know, I wouldn't feel very comfortable in zeroing everything
we don't know about - there's a risk of zeroing hidden registers set
up by the firmware.

> >>> 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?

I would assume so, yes, but for the issue here the order of the writes
did matter, even when the IOMMU was fully disabled.

I've inquired to see if there's a recommended way to clear any
previous state from an IOMMU.

Thanks, Roger.