[PATCH 0/2] iommu/amd-vi: remove zeroing of MMIO region

[PATCH 0/2] iommu/amd-vi: remove zeroing of MMIO region

[Roger Pau Monne]

Hello,

Unconditionally zeroing the whole IOMMU MMIO region is dangerous, at
least on an upcoming platform this puts the IOMMU in a broken state
that's not recovered by the init procedure in Xen.

Get rid of the zeroing, and instead attempt to disable the IOMMU ahead
of enabling it.

Thanks, Roger.

Roger Pau Monne (2):
  iommu/amd-vi: allow disable_iommu() against non-initialized IOMMUs
  iommu/amd-vi: do not zero IOMMU MMIO region

 xen/drivers/passthrough/amd/iommu_init.c | 34 +++++++++++-------------
 1 file changed, 15 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH 1/2] iommu/amd-vi: allow disable_iommu() against non-initialized IOMMUs

[Roger Pau Monne]

Introduce a force option to disable_iommu() that allows it to disable the
IOMMU, even when ->enabled is not set.  While there remove the unlikely(),
this is not a hot path anyway.

No functional change, as there are no current callers that pass force ==
true.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/amd/iommu_init.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index d77dd8511288..76ae78e5ea53 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -929,13 +929,13 @@ static void enable_iommu(struct amd_iommu *iommu)
         amd_iommu_flush_all_caches(iommu);
 }
 
-static void disable_iommu(struct amd_iommu *iommu)
+static void disable_iommu(struct amd_iommu *iommu, bool force)
 {
     unsigned long flags;
 
     spin_lock_irqsave(&iommu->lock, flags);
 
-    if ( unlikely(!iommu->enabled) )
+    if ( !iommu->enabled && !force )
         goto out;
 
     if ( !iommu->ctrl.int_cap_xt_en )
@@ -1139,7 +1139,7 @@ static void __init amd_iommu_init_cleanup(void)
         iommu->ctrl.int_cap_xt_en = 0;
 
         if ( iommu->enabled )
-            disable_iommu(iommu);
+            disable_iommu(iommu, false);
         else if ( iommu->mmio_base )
             writeq(iommu->ctrl.raw,
                    iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
@@ -1584,7 +1584,7 @@ void cf_check amd_iommu_crash_shutdown(void)
     struct amd_iommu *iommu;
 
     for_each_amd_iommu ( iommu )
-        disable_iommu(iommu);
+        disable_iommu(iommu, false);
 }
 
 void cf_check amd_iommu_resume(void)
@@ -1598,7 +1598,7 @@ void cf_check amd_iommu_resume(void)
         * To make sure that iommus have not been touched 
         * before re-enablement
         */
-        disable_iommu(iommu);
+        disable_iommu(iommu, false);
         enable_iommu(iommu);
         if ( !iommu->features.flds.ia_sup )
             invalidate_all = false;
-- 
2.53.0

Re: [PATCH 1/2] iommu/amd-vi: allow disable_iommu() against non-initialized IOMMUs

[Jan Beulich]

On 06.05.2026 09:37, Roger Pau Monne wrote:
> Introduce a force option to disable_iommu() that allows it to disable the
> IOMMU, even when ->enabled is not set.  While there remove the unlikely(),
> this is not a hot path anyway.
> 
> No functional change, as there are no current callers that pass force ==
> true.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

[PATCH 2/2] 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, neither what
values might be safe for those registers.  On a forthcoming platform doing
the zeroing of the MMIO region can put the IOMMU in a broken state, which
is not recovered by the IOMMU initialization procedure in Xen.

Instead attempt to forcefully disable the IOMMU ahead of enabling it.  Fold
map_iommu_mmio_region() into it's 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>
---
 xen/drivers/passthrough/amd/iommu_init.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

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
@@ -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,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;
 }
 
-- 
2.53.0

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

[Andrew Cooper]

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.

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.

~Andrew

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

[Jan Beulich]

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.

Jan

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

[Andrew Cooper]

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

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

[Jan Beulich]

On 06.05.2026 11:02, Andrew Cooper wrote:
> 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.

From a very general perspective I agree. However, when adding new registers
(or new bits in existing ones), having them default to 0 (and hence making
0 be a valid value) is common practice.

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

Well, okay. For firmware settings I think we can assume these would indeed
be settings, not enables of any features which would typically require
driving by an OS. Yet that still leaves the question (along the lines of
what I had raised before) of how we'd deal with being invoked with unknown
to us features enabled. We need to disable them, yet how do you suggest
doing that without blindly clearing most (if not all) registers? The only
clean way of doing that would look to be a "soft reset" command to the
IOMMU (of course not to be issued via the command queue). I'm unaware of
anything like this, though.

Jan

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

[Roger Pau Monné]

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.

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

I understand that when Xen supports kdump or preboot DMA protection we
would need to be more careful there, but going from zeroing the whole
MMIO area (which disables everything) to not doing any kind of
disabling is IMO a dangerous approach, and I would rather backport a
change that at least attempts to make sure the IOMMU is disabled.

Thanks, Roger.

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

[Andrew Cooper]

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

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

[Jan Beulich]

On 06.05.2026 09:37, Roger Pau Monne wrote:
> @@ -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);

Don't you also need to pre-fill iommu->features? And with that field's use in
disable_iommu(), won't we be at risk of leaving stuff enabled which we are
entirely unaware of? Even if we fully cleared the control register (which
would eliminate the need to fetch features), down the road a 2nd control
register could appear. Has it become clear which register(s) or bit(s) it
really is that are causing the observed issue? IOW is there truly something
we may not clear? Or is it maybe that memset() really isn't suitable for use
against MMIO, especially after having switched to use of REP MOVSB when ERMS
is available?

Jan

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

[Roger Pau Monné]

On Wed, May 06, 2026 at 10:28:52AM +0200, Jan Beulich wrote:
> On 06.05.2026 09:37, Roger Pau Monne wrote:
> > @@ -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);
> 
> Don't you also need to pre-fill iommu->features?

Indeed, that's done just ahead of this chunk, in the
get_iommu_features() call.

> And with that field's use in
> disable_iommu(), won't we be at risk of leaving stuff enabled which we are
> entirely unaware of?

Possibly, yes, that's always a risk.

> Even if we fully cleared the control register (which
> would eliminate the need to fetch features), down the road a 2nd control
> register could appear.

We do clear the control register, it's indirectly done by us setting
iommu->ctrl.raw = 0 after the disable_iommu() call.

I did wonder about just doing a write of 0 to the control register,
but I think it's best if we try to gracefully disable the features (as
done in disable_iommu()), and then reset the cached control state to
0.  Future writes to the control register will clear any bits not
directly set by Xen.

> Has it become clear which register(s) or bit(s) it
> really is that are causing the observed issue? IOW is there truly something
> we may not clear?

It's not so much as not clearing something, but rather the clearing
itself putting the IOMMU in a broken state which we then can't recover
from using the initialization procedure in Xen.

Thanks, Roger.

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

[Jan Beulich]

On 06.05.2026 10:43, Roger Pau Monné wrote:
> On Wed, May 06, 2026 at 10:28:52AM +0200, Jan Beulich wrote:
>> On 06.05.2026 09:37, Roger Pau Monne wrote:
>>> @@ -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);
>>
>> Don't you also need to pre-fill iommu->features?
> 
> Indeed, that's done just ahead of this chunk, in the
> get_iommu_features() call.
> 
>> And with that field's use in
>> disable_iommu(), won't we be at risk of leaving stuff enabled which we are
>> entirely unaware of?
> 
> Possibly, yes, that's always a risk.
> 
>> Even if we fully cleared the control register (which
>> would eliminate the need to fetch features), down the road a 2nd control
>> register could appear.
> 
> We do clear the control register, it's indirectly done by us setting
> iommu->ctrl.raw = 0 after the disable_iommu() call.
> 
> I did wonder about just doing a write of 0 to the control register,
> but I think it's best if we try to gracefully disable the features (as
> done in disable_iommu()), and then reset the cached control state to
> 0.  Future writes to the control register will clear any bits not
> directly set by Xen.

Maybe better to explicitly write out that 0 right away, even if you want
to keep using disable_iommu()?

Jan

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

[Roger Pau Monné]

On Wed, May 06, 2026 at 11:17:25AM +0200, Jan Beulich wrote:
> On 06.05.2026 10:43, Roger Pau Monné wrote:
> > On Wed, May 06, 2026 at 10:28:52AM +0200, Jan Beulich wrote:
> >> On 06.05.2026 09:37, Roger Pau Monne wrote:
> >>> @@ -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);
> >>
> >> Don't you also need to pre-fill iommu->features?
> > 
> > Indeed, that's done just ahead of this chunk, in the
> > get_iommu_features() call.
> > 
> >> And with that field's use in
> >> disable_iommu(), won't we be at risk of leaving stuff enabled which we are
> >> entirely unaware of?
> > 
> > Possibly, yes, that's always a risk.
> > 
> >> Even if we fully cleared the control register (which
> >> would eliminate the need to fetch features), down the road a 2nd control
> >> register could appear.
> > 
> > We do clear the control register, it's indirectly done by us setting
> > iommu->ctrl.raw = 0 after the disable_iommu() call.
> > 
> > I did wonder about just doing a write of 0 to the control register,
> > but I think it's best if we try to gracefully disable the features (as
> > done in disable_iommu()), and then reset the cached control state to
> > 0.  Future writes to the control register will clear any bits not
> > directly set by Xen.
> 
> Maybe better to explicitly write out that 0 right away, even if you want
> to keep using disable_iommu()?

Yeah, I also considered that.  So after disable_iommu() set the cached
control state to 0 and also zero the control register right there.
Can do in the next version, unless there are further objections.

Thanks, Roger.

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

[Jan Beulich]

On 06.05.2026 11:20, Roger Pau Monné wrote:
> On Wed, May 06, 2026 at 11:17:25AM +0200, Jan Beulich wrote:
>> On 06.05.2026 10:43, Roger Pau Monné wrote:
>>> On Wed, May 06, 2026 at 10:28:52AM +0200, Jan Beulich wrote:
>>>> On 06.05.2026 09:37, Roger Pau Monne wrote:
>>>>> @@ -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);
>>>>
>>>> Don't you also need to pre-fill iommu->features?
>>>
>>> Indeed, that's done just ahead of this chunk, in the
>>> get_iommu_features() call.
>>>
>>>> And with that field's use in
>>>> disable_iommu(), won't we be at risk of leaving stuff enabled which we are
>>>> entirely unaware of?
>>>
>>> Possibly, yes, that's always a risk.
>>>
>>>> Even if we fully cleared the control register (which
>>>> would eliminate the need to fetch features), down the road a 2nd control
>>>> register could appear.
>>>
>>> We do clear the control register, it's indirectly done by us setting
>>> iommu->ctrl.raw = 0 after the disable_iommu() call.
>>>
>>> I did wonder about just doing a write of 0 to the control register,
>>> but I think it's best if we try to gracefully disable the features (as
>>> done in disable_iommu()), and then reset the cached control state to
>>> 0.  Future writes to the control register will clear any bits not
>>> directly set by Xen.
>>
>> Maybe better to explicitly write out that 0 right away, even if you want
>> to keep using disable_iommu()?
> 
> Yeah, I also considered that.  So after disable_iommu() set the cached
> control state to 0 and also zero the control register right there.
> Can do in the next version, unless there are further objections.

Well, as said - I'm wary of fully dropping the memset(). It may help now,
but it could easily cause issues later.

Jan