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

[PATCH v2 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 does put the IOMMU in a broken state
that's not recoverable 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 | 41 +++++++++++++-----------
 1 file changed, 22 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH v2 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>
Acked-by: Jan Beulich <jbeulich@suse.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

[PATCH v2 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, 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 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>
---
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 | 31 +++++++++++++-----------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 76ae78e5ea53..ffc041211fb5 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,18 @@ 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);
+    if ( iommu->ctrl.iommu_en )
+        printk(XENLOG_WARNING
+               "AMD-Vi: IOMMU %pp enabled by firmware (%016lx)\n",
+               &iommu->sbdf, iommu->ctrl.raw);
+    disable_iommu(iommu, true);
+
+    /* With the IOMMU disabled zero the control register. */
+    iommu->ctrl.raw = 0;
+    writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
+
     return 0;
 }
 
-- 
2.53.0

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

[Andrew Cooper]

On 06/05/2026 2:55 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

Sorry, one more.  "We don't know which registers might".

> 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 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>
> ---
> 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 | 31 +++++++++++++-----------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index 76ae78e5ea53..ffc041211fb5 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,18 @@ 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);
> +    if ( iommu->ctrl.iommu_en )
> +        printk(XENLOG_WARNING
> +               "AMD-Vi: IOMMU %pp enabled by firmware (%016lx)\n",
> +               &iommu->sbdf, iommu->ctrl.raw);
> +    disable_iommu(iommu, true);
> +
> +    /* With the IOMMU disabled zero the control register. */
> +    iommu->ctrl.raw = 0;
> +    writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> +
>      return 0;
>  }
>  

I don't think calling disable_iommu() is a good thing here.

It's just a cascade of clearing one/few bits in ctrl at a time, but is
is added unconditionally so that's 5 UC stores writing 0's to the same
register in the common case.

I think this logic wants to be:

@@ -1381,6 +1372,18 @@ 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 if the IOMMU is active, and disable. */
+    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;
 }

 

which has the advantage that it's closer to the current behaviour, and
therefore arguably a safer backport.

The only thing that disable_iommu() does which isn't editing the control
register is playing with the PCI MSI enable bit, but that really doesn't
matter when we clear ctrl.int_cap_xt_en.  (And in fact, that path is
buggy because it clears MSI enable without inhibiting interrupt
generation, which architecturally will turn into legacy line interrupt
to deal with.)

We're just about to re-set everything up, so everything's going to get
re-enabled.

In the commit message, I think it's worth highlighting that zeroing the
control register is the current behaviour, and this will need revisiting
in order to support Preboot DMA Protection.

Or alternatively, would you like me to submit a patch?  (Happy either way.)

~Andrew

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

[Roger Pau Monné]

On Wed, May 06, 2026 at 05:18:40PM +0100, Andrew Cooper wrote:
> On 06/05/2026 2:55 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
> 
> Sorry, one more.  "We don't know which registers might".
> 
> > 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 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>
> > ---
> > 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 | 31 +++++++++++++-----------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> > index 76ae78e5ea53..ffc041211fb5 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,18 @@ 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);
> > +    if ( iommu->ctrl.iommu_en )
> > +        printk(XENLOG_WARNING
> > +               "AMD-Vi: IOMMU %pp enabled by firmware (%016lx)\n",
> > +               &iommu->sbdf, iommu->ctrl.raw);
> > +    disable_iommu(iommu, true);
> > +
> > +    /* With the IOMMU disabled zero the control register. */
> > +    iommu->ctrl.raw = 0;
> > +    writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> > +
> >      return 0;
> >  }
> >  
> 
> I don't think calling disable_iommu() is a good thing here.
> 
> It's just a cascade of clearing one/few bits in ctrl at a time, but is
> is added unconditionally so that's 5 UC stores writing 0's to the same
> register in the common case.

The approach to use disable_iommu() is because that's closer to what
Linux does in iommu_disable(), which seems to explicitly disable one
feature at a time instead of writing zero to the command register in
one go.  I've been cautious in taking the same approach on Xen.

I don't mind doing a plain write of 0, let me test to ensure this is
OK.

> I think this logic wants to be:
> 
> @@ -1381,6 +1372,18 @@ 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 if the IOMMU is active, and disable. */
> +    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);
> +    }

In your snippet above, I think we want to unconditionally set
iommu->ctrl.raw = 0, and also propagate that 0 to the register,
otherwise we will inherit set bits from whatever is currently in the
control register:

> +    /* Check if the IOMMU is active, and disable. */
> +    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);

Thanks, Roger.

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Wed, May 06, 2026 at 03:55:12PM +0200, Roger Pau Monne wrote:
> Hello,
> 
> Unconditionally zeroing the whole IOMMU MMIO region is dangerous, at
> least on an upcoming platform this does put the IOMMU in a broken state
> that's not recoverable by the init procedure in Xen.
> 
> Get rid of the zeroing, and instead attempt to disable the IOMMU ahead
> of enabling it.

Just to mention it here, while this looks like a step in the right
direction, even better would be to not disable IOMMU at boot, but
reconfigure it - to preserve uninterrupted protection when boot time DMA
protection is enabled in firmware. But that's definitely more work...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Roger Pau Monné]

On Wed, May 06, 2026 at 06:49:47PM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, May 06, 2026 at 03:55:12PM +0200, Roger Pau Monne wrote:
> > Hello,
> > 
> > Unconditionally zeroing the whole IOMMU MMIO region is dangerous, at
> > least on an upcoming platform this does put the IOMMU in a broken state
> > that's not recoverable by the init procedure in Xen.
> > 
> > Get rid of the zeroing, and instead attempt to disable the IOMMU ahead
> > of enabling it.
> 
> Just to mention it here, while this looks like a step in the right
> direction, even better would be to not disable IOMMU at boot, but
> reconfigure it - to preserve uninterrupted protection when boot time DMA
> protection is enabled in firmware. But that's definitely more work...

Yes, we are aware of this.  But IMO booting with the IOMMU enabled
will be a new feature, this is strictly a fix that we can "safely"
backport to stable branches to deal with broken hardware.

Thanks, Roger.

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

[Jan Beulich]

On 06.05.2026 18:18, Andrew Cooper wrote:
> On 06/05/2026 2:55 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
> 
> Sorry, one more.  "We don't know which registers might".
> 
>> 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 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>
>> ---
>> 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 | 31 +++++++++++++-----------
>>  1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
>> index 76ae78e5ea53..ffc041211fb5 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,18 @@ 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);
>> +    if ( iommu->ctrl.iommu_en )
>> +        printk(XENLOG_WARNING
>> +               "AMD-Vi: IOMMU %pp enabled by firmware (%016lx)\n",
>> +               &iommu->sbdf, iommu->ctrl.raw);
>> +    disable_iommu(iommu, true);
>> +
>> +    /* With the IOMMU disabled zero the control register. */
>> +    iommu->ctrl.raw = 0;
>> +    writeq(0, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
>> +
>>      return 0;
>>  }
>>  
> 
> I don't think calling disable_iommu() is a good thing here.
> 
> It's just a cascade of clearing one/few bits in ctrl at a time, but is
> is added unconditionally so that's 5 UC stores writing 0's to the same
> register in the common case.
> 
> I think this logic wants to be:
> 
> @@ -1381,6 +1372,18 @@ 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 if the IOMMU is active, and disable. */
> +    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;
>  }
> 
>  
> 
> which has the advantage that it's closer to the current behaviour, and
> therefore arguably a safer backport.
> 
> The only thing that disable_iommu() does which isn't editing the control
> register is playing with the PCI MSI enable bit, but that really doesn't
> matter when we clear ctrl.int_cap_xt_en.  (And in fact, that path is
> buggy because it clears MSI enable without inhibiting interrupt
> generation, which architecturally will turn into legacy line interrupt
> to deal with.)

Except there's no line interrupt associated with this?

Jan