[PATCH v2 0/3] x86/IOMMU: enabled / intremap handling

[PATCH v2 0/3] x86/IOMMU: enabled / intremap handling

[Jan Beulich]

In the course of reading the response to v1 (patch 1 only) I realized
that not only that patch needs further adjustment, but that also
further changes are needed (and there's likely yet more amiss).

1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
2: x86/APIC: avoid iommu_supports_x2apic() on error path
3: AMD/IOMMU: iommu_enable vs iommu_intremap

Jan

[PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing

[Jan Beulich]

x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC
mode (physical vs clustered) depends on iommu_intremap, that variable
needs to be set to off as soon as we know we can't / won't enable
interrupt remapping, i.e. in particular when parsing of the respective
ACPI tables failed. Move the turning off of iommu_intremap from AMD
specific code into acpi_iommu_init(), accompanying it by clearing of
iommu_enable.

Take the opportunity and also fully skip ACPI table parsing logic on
VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway,
like was already the case for AMD.

The tag below only references the commit uncovering a pre-existing
anomaly.

Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While the change here deals with apic_x2apic_probe() as called from
x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be
similarly affected. That call occurs before acpi_boot_init(), which is
what calls acpi_iommu_init(). The ordering in setup.c is in part
relatively fragile, which is why for the moment I'm still hesitant to
move the generic_apic_probe() call down. Plus I don't have easy access
to a suitable system to test this case. Thoughts?
---
v2: Treat iommu_enable and iommu_intremap as separate options.

--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -183,9 +183,6 @@ int __init acpi_ivrs_init(void)
 {
     int rc;
 
-    if ( !iommu_enable && !iommu_intremap )
-        return 0;
-
     rc = amd_iommu_get_supported_ivhd_type();
     if ( rc < 0 )
         return rc;
@@ -193,10 +190,7 @@ int __init acpi_ivrs_init(void)
     ivhd_type = rc;
 
     if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
-    {
-        iommu_intremap = iommu_intremap_off;
         return -ENODEV;
-    }
 
     iommu_init_ops = &_iommu_init_ops;
 
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -777,11 +777,7 @@ static int __init acpi_parse_dmar(struct
     dmar = (struct acpi_table_dmar *)table;
     dmar_flags = dmar->flags;
 
-    if ( !iommu_enable && !iommu_intremap )
-    {
-        ret = -EINVAL;
-        goto out;
-    }
+    ASSERT(iommu_enable || iommu_intremap);
 
     if ( !dmar->width )
     {
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -41,6 +41,24 @@ enum iommu_intremap __read_mostly iommu_
 bool __read_mostly iommu_intpost;
 #endif
 
+void __init acpi_iommu_init(void)
+{
+    int ret;
+
+    if ( !iommu_enable && !iommu_intremap )
+        return;
+
+    ret = acpi_dmar_init();
+    if ( ret == -ENODEV )
+        ret = acpi_ivrs_init();
+
+    if ( ret )
+    {
+        iommu_enable = false;
+        iommu_intremap = iommu_intremap_off;
+    }
+}
+
 int __init iommu_hardware_setup(void)
 {
     struct IO_APIC_route_entry **ioapic_entries = NULL;
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -141,16 +141,10 @@ extern u32 x86_acpiid_to_apicid[];
 extern u32 pmtmr_ioport;
 extern unsigned int pmtmr_width;
 
+void acpi_iommu_init(void);
 int acpi_dmar_init(void);
 int acpi_ivrs_init(void);
 
-static inline int acpi_iommu_init(void)
-{
-    int ret = acpi_dmar_init();
-
-    return ret == -ENODEV ? acpi_ivrs_init() : ret;
-}
-
 void acpi_mmcfg_init(void);
 
 /* Incremented whenever we transition through S3. Value is 1 during boot. */

[PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path

[Jan Beulich]

The value it returns may change from true to false in case
iommu_enable_x2apic() fails and, as a side effect, clears iommu_intremap
(as can happen at least on AMD). Latch the return value from the first
invocation to replace the second one.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -865,6 +865,7 @@ void x2apic_ap_setup(void)
 void __init x2apic_bsp_setup(void)
 {
     struct IO_APIC_route_entry **ioapic_entries = NULL;
+    bool iommu_x2apic;
     const char *orig_name;
 
     if ( !cpu_has_x2apic )
@@ -880,7 +881,8 @@ void __init x2apic_bsp_setup(void)
         printk("x2APIC: Already enabled by BIOS: Ignoring cmdline disable.\n");
     }
 
-    if ( iommu_supports_x2apic() )
+    iommu_x2apic = iommu_supports_x2apic();
+    if ( iommu_x2apic )
     {
         if ( (ioapic_entries = alloc_ioapic_entries()) == NULL )
         {
@@ -933,8 +935,11 @@ void __init x2apic_bsp_setup(void)
         printk("Switched to APIC driver %s\n", genapic.name);
 
 restore_out:
-    /* iommu_x2apic_enabled cannot be used here in the error case. */
-    if ( iommu_supports_x2apic() )
+    /*
+     * iommu_x2apic_enabled and iommu_supports_x2apic() cannot be used here
+     * in the error case.
+     */
+    if ( iommu_x2apic )
     {
         /*
          * NB: do not use raw mode when restoring entries if the iommu has

[PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Jan Beulich]

The two are really meant to be independent settings; iov_supports_xt()
using || instead of && was simply wrong. The corrected check is,
however, redundant, just like the (correct) one in iov_detect(): These
hook functions are unreachable without acpi_ivrs_init() installing the
iommu_init_ops pointer, which it does only upon success. (Unlike for
VT-d there is no late clearing of iommu_enable due to quirks, and any
possible clearing of iommu_intremap happens only after iov_supports_xt()
has run.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
In fact in iov_detect() it could be iommu_enable alone which gets
checked, but this felt overly aggressive to me. Instead I'm getting the
impression that the function may wrongly not get called when "iommu=off"
but interrupt remapping is in use: We'd not get the interrupt handler
installed, and hence interrupt remapping related events would never get
reported. (Same on VT-d, FTAOD.)

For iov_supports_xt() the question is whether, like VT-d's
intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
alone (in which case it would need to remain a check rather than getting
converted to ASSERT()).
---
v2: New.

--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -731,8 +731,7 @@ bool __init iov_supports_xt(void)
 {
     unsigned int apic;
 
-    if ( !iommu_enable || !iommu_intremap )
-        return false;
+    ASSERT(iommu_enable || iommu_intremap);
 
     if ( amd_iommu_prepare(true) )
         return false;
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -199,8 +199,7 @@ int __init acpi_ivrs_init(void)
 
 static int __init iov_detect(void)
 {
-    if ( !iommu_enable && !iommu_intremap )
-        return 0;
+    ASSERT(iommu_enable || iommu_intremap);
 
     if ( (init_done ? amd_iommu_init_late()
                     : amd_iommu_init(false)) != 0 )

Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing

[Jan Beulich]

On 21.10.2021 11:58, Jan Beulich wrote:
> x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC
> mode (physical vs clustered) depends on iommu_intremap, that variable
> needs to be set to off as soon as we know we can't / won't enable
> interrupt remapping, i.e. in particular when parsing of the respective
> ACPI tables failed. Move the turning off of iommu_intremap from AMD
> specific code into acpi_iommu_init(), accompanying it by clearing of
> iommu_enable.
> 
> Take the opportunity and also fully skip ACPI table parsing logic on
> VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway,
> like was already the case for AMD.
> 
> The tag below only references the commit uncovering a pre-existing
> anomaly.
> 
> Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ouch, forgot to Cc Kevin; now added.

Jan

> ---
> While the change here deals with apic_x2apic_probe() as called from
> x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be
> similarly affected. That call occurs before acpi_boot_init(), which is
> what calls acpi_iommu_init(). The ordering in setup.c is in part
> relatively fragile, which is why for the moment I'm still hesitant to
> move the generic_apic_probe() call down. Plus I don't have easy access
> to a suitable system to test this case. Thoughts?
> ---
> v2: Treat iommu_enable and iommu_intremap as separate options.
> 
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -183,9 +183,6 @@ int __init acpi_ivrs_init(void)
>  {
>      int rc;
>  
> -    if ( !iommu_enable && !iommu_intremap )
> -        return 0;
> -
>      rc = amd_iommu_get_supported_ivhd_type();
>      if ( rc < 0 )
>          return rc;
> @@ -193,10 +190,7 @@ int __init acpi_ivrs_init(void)
>      ivhd_type = rc;
>  
>      if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
> -    {
> -        iommu_intremap = iommu_intremap_off;
>          return -ENODEV;
> -    }
>  
>      iommu_init_ops = &_iommu_init_ops;
>  
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -777,11 +777,7 @@ static int __init acpi_parse_dmar(struct
>      dmar = (struct acpi_table_dmar *)table;
>      dmar_flags = dmar->flags;
>  
> -    if ( !iommu_enable && !iommu_intremap )
> -    {
> -        ret = -EINVAL;
> -        goto out;
> -    }
> +    ASSERT(iommu_enable || iommu_intremap);
>  
>      if ( !dmar->width )
>      {
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -41,6 +41,24 @@ enum iommu_intremap __read_mostly iommu_
>  bool __read_mostly iommu_intpost;
>  #endif
>  
> +void __init acpi_iommu_init(void)
> +{
> +    int ret;
> +
> +    if ( !iommu_enable && !iommu_intremap )
> +        return;
> +
> +    ret = acpi_dmar_init();
> +    if ( ret == -ENODEV )
> +        ret = acpi_ivrs_init();
> +
> +    if ( ret )
> +    {
> +        iommu_enable = false;
> +        iommu_intremap = iommu_intremap_off;
> +    }
> +}
> +
>  int __init iommu_hardware_setup(void)
>  {
>      struct IO_APIC_route_entry **ioapic_entries = NULL;
> --- a/xen/include/asm-x86/acpi.h
> +++ b/xen/include/asm-x86/acpi.h
> @@ -141,16 +141,10 @@ extern u32 x86_acpiid_to_apicid[];
>  extern u32 pmtmr_ioport;
>  extern unsigned int pmtmr_width;
>  
> +void acpi_iommu_init(void);
>  int acpi_dmar_init(void);
>  int acpi_ivrs_init(void);
>  
> -static inline int acpi_iommu_init(void)
> -{
> -    int ret = acpi_dmar_init();
> -
> -    return ret == -ENODEV ? acpi_ivrs_init() : ret;
> -}
> -
>  void acpi_mmcfg_init(void);
>  
>  /* Incremented whenever we transition through S3. Value is 1 during boot. */
> 
> 

Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing

[Roger Pau Monné]

On Thu, Oct 21, 2021 at 11:58:18AM +0200, Jan Beulich wrote:
> x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC
> mode (physical vs clustered) depends on iommu_intremap, that variable
> needs to be set to off as soon as we know we can't / won't enable
> interrupt remapping, i.e. in particular when parsing of the respective
> ACPI tables failed. Move the turning off of iommu_intremap from AMD
> specific code into acpi_iommu_init(), accompanying it by clearing of
> iommu_enable.
> 
> Take the opportunity and also fully skip ACPI table parsing logic on
> VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway,
> like was already the case for AMD.
> 
> The tag below only references the commit uncovering a pre-existing
> anomaly.
> 
> Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> While the change here deals with apic_x2apic_probe() as called from
> x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be
> similarly affected. That call occurs before acpi_boot_init(), which is
> what calls acpi_iommu_init(). The ordering in setup.c is in part
> relatively fragile, which is why for the moment I'm still hesitant to
> move the generic_apic_probe() call down. Plus I don't have easy access
> to a suitable system to test this case. Thoughts?

Indeed, that seems it could go quite wrong, as apic_x2apic_probe will
see iommu_intremap == iommu_intremap_full (the default value) and thus
could choose cluster mode without real interrupt remapping support.

At first sight it would seem possible to move lower down, but as you
say, this is all quite fragile. It's even made worse because we lack a
strict ordering discipline or any kind of dependency checking, so even
if we mess up the order it's likely to go unnoticed unless someone
tests on an affected system.

While we can try to solve this for the upcoming release, long term we
need a stricter ordering, either as a comment, or even better enforced
somehow in code. The x2APIC vs IOMMU ordering has bitten us multiple
times and we should see about implementing a more robust solution.

Thanks, Roger.

Re: [PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path

[Roger Pau Monné]

On Thu, Oct 21, 2021 at 11:58:37AM +0200, Jan Beulich wrote:
> The value it returns may change from true to false in case
> iommu_enable_x2apic() fails and, as a side effect, clears iommu_intremap
> (as can happen at least on AMD). Latch the return value from the first
> invocation to replace the second one.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Roger Pau Monné]

On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
> The two are really meant to be independent settings; iov_supports_xt()
> using || instead of && was simply wrong. The corrected check is,
> however, redundant, just like the (correct) one in iov_detect(): These
> hook functions are unreachable without acpi_ivrs_init() installing the
> iommu_init_ops pointer, which it does only upon success. (Unlike for
> VT-d there is no late clearing of iommu_enable due to quirks, and any
> possible clearing of iommu_intremap happens only after iov_supports_xt()
> has run.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> In fact in iov_detect() it could be iommu_enable alone which gets
> checked, but this felt overly aggressive to me. Instead I'm getting the
> impression that the function may wrongly not get called when "iommu=off"
> but interrupt remapping is in use: We'd not get the interrupt handler
> installed, and hence interrupt remapping related events would never get
> reported. (Same on VT-d, FTAOD.)

I've spend a non-trivial amount of time looking into this before
reading this note. AFAICT you could set iommu=off and still get x2APIC
enabled and relying on interrupt remapping.

> For iov_supports_xt() the question is whether, like VT-d's
> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
> alone (in which case it would need to remain a check rather than getting
> converted to ASSERT()).

Hm, no, I don't think so. I think iommu_enable should take precedence
over iommu_intremap, and having iommu_enable == false should force
interrupt remapping to be reported as disabled. Note that disabling it
in iommu_setup is too late, as the APIC initialization will have
already taken place.

It's my reading of the command line parameter documentation that
setting iommu=off should disable all usage of the IOMMU, and that
includes the interrupt remapping support (ie: a user should not need
to set iommu=off,no-intremap)

> ---
> v2: New.
> 
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -731,8 +731,7 @@ bool __init iov_supports_xt(void)
>  {
>      unsigned int apic;
>  
> -    if ( !iommu_enable || !iommu_intremap )
> -        return false;
> +    ASSERT(iommu_enable || iommu_intremap);

I think this should be && in order to match my comments above.

Thanks, Roger.

RE: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, October 22, 2021 1:59 PM
> 
> On 21.10.2021 11:58, Jan Beulich wrote:
> > x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC
> > mode (physical vs clustered) depends on iommu_intremap, that variable
> > needs to be set to off as soon as we know we can't / won't enable
> > interrupt remapping, i.e. in particular when parsing of the respective
> > ACPI tables failed. Move the turning off of iommu_intremap from AMD
> > specific code into acpi_iommu_init(), accompanying it by clearing of
> > iommu_enable.
> >
> > Take the opportunity and also fully skip ACPI table parsing logic on
> > VT-d when both "iommu=off" and "iommu=no-intremap" are in effect
> anyway,
> > like was already the case for AMD.
> >
> > The tag below only references the commit uncovering a pre-existing
> > anomaly.
> >
> > Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier")
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Ouch, forgot to Cc Kevin; now added.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> Jan
> 
> > ---
> > While the change here deals with apic_x2apic_probe() as called from
> > x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be
> > similarly affected. That call occurs before acpi_boot_init(), which is
> > what calls acpi_iommu_init(). The ordering in setup.c is in part
> > relatively fragile, which is why for the moment I'm still hesitant to
> > move the generic_apic_probe() call down. Plus I don't have easy access
> > to a suitable system to test this case. Thoughts?
> > ---
> > v2: Treat iommu_enable and iommu_intremap as separate options.
> >
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -183,9 +183,6 @@ int __init acpi_ivrs_init(void)
> >  {
> >      int rc;
> >
> > -    if ( !iommu_enable && !iommu_intremap )
> > -        return 0;
> > -
> >      rc = amd_iommu_get_supported_ivhd_type();
> >      if ( rc < 0 )
> >          return rc;
> > @@ -193,10 +190,7 @@ int __init acpi_ivrs_init(void)
> >      ivhd_type = rc;
> >
> >      if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
> > -    {
> > -        iommu_intremap = iommu_intremap_off;
> >          return -ENODEV;
> > -    }
> >
> >      iommu_init_ops = &_iommu_init_ops;
> >
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -777,11 +777,7 @@ static int __init acpi_parse_dmar(struct
> >      dmar = (struct acpi_table_dmar *)table;
> >      dmar_flags = dmar->flags;
> >
> > -    if ( !iommu_enable && !iommu_intremap )
> > -    {
> > -        ret = -EINVAL;
> > -        goto out;
> > -    }
> > +    ASSERT(iommu_enable || iommu_intremap);
> >
> >      if ( !dmar->width )
> >      {
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -41,6 +41,24 @@ enum iommu_intremap __read_mostly iommu_
> >  bool __read_mostly iommu_intpost;
> >  #endif
> >
> > +void __init acpi_iommu_init(void)
> > +{
> > +    int ret;
> > +
> > +    if ( !iommu_enable && !iommu_intremap )
> > +        return;
> > +
> > +    ret = acpi_dmar_init();
> > +    if ( ret == -ENODEV )
> > +        ret = acpi_ivrs_init();
> > +
> > +    if ( ret )
> > +    {
> > +        iommu_enable = false;
> > +        iommu_intremap = iommu_intremap_off;
> > +    }
> > +}
> > +
> >  int __init iommu_hardware_setup(void)
> >  {
> >      struct IO_APIC_route_entry **ioapic_entries = NULL;
> > --- a/xen/include/asm-x86/acpi.h
> > +++ b/xen/include/asm-x86/acpi.h
> > @@ -141,16 +141,10 @@ extern u32 x86_acpiid_to_apicid[];
> >  extern u32 pmtmr_ioport;
> >  extern unsigned int pmtmr_width;
> >
> > +void acpi_iommu_init(void);
> >  int acpi_dmar_init(void);
> >  int acpi_ivrs_init(void);
> >
> > -static inline int acpi_iommu_init(void)
> > -{
> > -    int ret = acpi_dmar_init();
> > -
> > -    return ret == -ENODEV ? acpi_ivrs_init() : ret;
> > -}
> > -
> >  void acpi_mmcfg_init(void);
> >
> >  /* Incremented whenever we transition through S3. Value is 1 during boot.
> */
> >
> >

Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing

[Jan Beulich]

On 22.10.2021 17:52, Roger Pau Monné wrote:
> On Thu, Oct 21, 2021 at 11:58:18AM +0200, Jan Beulich wrote:
>> x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC
>> mode (physical vs clustered) depends on iommu_intremap, that variable
>> needs to be set to off as soon as we know we can't / won't enable
>> interrupt remapping, i.e. in particular when parsing of the respective
>> ACPI tables failed. Move the turning off of iommu_intremap from AMD
>> specific code into acpi_iommu_init(), accompanying it by clearing of
>> iommu_enable.
>>
>> Take the opportunity and also fully skip ACPI table parsing logic on
>> VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway,
>> like was already the case for AMD.
>>
>> The tag below only references the commit uncovering a pre-existing
>> anomaly.
>>
>> Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier")
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> While the change here deals with apic_x2apic_probe() as called from
>> x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be
>> similarly affected. That call occurs before acpi_boot_init(), which is
>> what calls acpi_iommu_init(). The ordering in setup.c is in part
>> relatively fragile, which is why for the moment I'm still hesitant to
>> move the generic_apic_probe() call down. Plus I don't have easy access
>> to a suitable system to test this case. Thoughts?
> 
> Indeed, that seems it could go quite wrong, as apic_x2apic_probe will
> see iommu_intremap == iommu_intremap_full (the default value) and thus
> could choose cluster mode without real interrupt remapping support.
> 
> At first sight it would seem possible to move lower down, but as you
> say, this is all quite fragile. It's even made worse because we lack a
> strict ordering discipline or any kind of dependency checking, so even
> if we mess up the order it's likely to go unnoticed unless someone
> tests on an affected system.
> 
> While we can try to solve this for the upcoming release, long term we
> need a stricter ordering, either as a comment, or even better enforced
> somehow in code. The x2APIC vs IOMMU ordering has bitten us multiple
> times and we should see about implementing a more robust solution.

So what's your thought then: Make the change (in another patch), or rather
leave the code as is? I'm slightly in favor of making the change seeing
that you agree that the rearrangement looks to be correct.

Jan

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Jan Beulich]

On 25.10.2021 12:28, Roger Pau Monné wrote:
> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
>> The two are really meant to be independent settings; iov_supports_xt()
>> using || instead of && was simply wrong. The corrected check is,
>> however, redundant, just like the (correct) one in iov_detect(): These
>> hook functions are unreachable without acpi_ivrs_init() installing the
>> iommu_init_ops pointer, which it does only upon success. (Unlike for
>> VT-d there is no late clearing of iommu_enable due to quirks, and any
>> possible clearing of iommu_intremap happens only after iov_supports_xt()
>> has run.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> In fact in iov_detect() it could be iommu_enable alone which gets
>> checked, but this felt overly aggressive to me. Instead I'm getting the
>> impression that the function may wrongly not get called when "iommu=off"
>> but interrupt remapping is in use: We'd not get the interrupt handler
>> installed, and hence interrupt remapping related events would never get
>> reported. (Same on VT-d, FTAOD.)
> 
> I've spend a non-trivial amount of time looking into this before
> reading this note. AFAICT you could set iommu=off and still get x2APIC
> enabled and relying on interrupt remapping.

Right, contrary to ...

>> For iov_supports_xt() the question is whether, like VT-d's
>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
>> alone (in which case it would need to remain a check rather than getting
>> converted to ASSERT()).
> 
> Hm, no, I don't think so. I think iommu_enable should take precedence
> over iommu_intremap, and having iommu_enable == false should force
> interrupt remapping to be reported as disabled. Note that disabling it
> in iommu_setup is too late, as the APIC initialization will have
> already taken place.
> 
> It's my reading of the command line parameter documentation that
> setting iommu=off should disable all usage of the IOMMU, and that
> includes the interrupt remapping support (ie: a user should not need
> to set iommu=off,no-intremap)

... that documentation. But I think it's the documentation that
wants fixing, such that iommu=off really only control DMA remap.
With that ...

>> ---
>> v2: New.
>>
>> --- a/xen/drivers/passthrough/amd/iommu_intr.c
>> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
>> @@ -731,8 +731,7 @@ bool __init iov_supports_xt(void)
>>  {
>>      unsigned int apic;
>>  
>> -    if ( !iommu_enable || !iommu_intremap )
>> -        return false;
>> +    ASSERT(iommu_enable || iommu_intremap);
> 
> I think this should be && in order to match my comments above.

... I think || is correct to use here.

Jan

Re: [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling

[Jan Beulich]

On 21.10.2021 11:57, Jan Beulich wrote:
> In the course of reading the response to v1 (patch 1 only) I realized
> that not only that patch needs further adjustment, but that also
> further changes are needed (and there's likely yet more amiss).
> 
> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
> 3: AMD/IOMMU: iommu_enable vs iommu_intremap

Ian, while we further discuss / refine patch 3, the first two have the
needed R-b, but will now need you release-ack aiui.

Andrew, did you perhaps have a chance to actually try v2 of patch 1? It
works for me when suitably configuring the BIOS on my Skylake, so I
wouldn't feel uncertain in committing without a Tested-by, but it would
feel even better if I had one.

Thanks, Jan

Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing

[Roger Pau Monné]

On Tue, Nov 02, 2021 at 11:07:25AM +0100, Jan Beulich wrote:
> On 22.10.2021 17:52, Roger Pau Monné wrote:
> > On Thu, Oct 21, 2021 at 11:58:18AM +0200, Jan Beulich wrote:
> >> x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC
> >> mode (physical vs clustered) depends on iommu_intremap, that variable
> >> needs to be set to off as soon as we know we can't / won't enable
> >> interrupt remapping, i.e. in particular when parsing of the respective
> >> ACPI tables failed. Move the turning off of iommu_intremap from AMD
> >> specific code into acpi_iommu_init(), accompanying it by clearing of
> >> iommu_enable.
> >>
> >> Take the opportunity and also fully skip ACPI table parsing logic on
> >> VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway,
> >> like was already the case for AMD.
> >>
> >> The tag below only references the commit uncovering a pre-existing
> >> anomaly.
> >>
> >> Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier")
> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> ---
> >> While the change here deals with apic_x2apic_probe() as called from
> >> x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be
> >> similarly affected. That call occurs before acpi_boot_init(), which is
> >> what calls acpi_iommu_init(). The ordering in setup.c is in part
> >> relatively fragile, which is why for the moment I'm still hesitant to
> >> move the generic_apic_probe() call down. Plus I don't have easy access
> >> to a suitable system to test this case. Thoughts?
> > 
> > Indeed, that seems it could go quite wrong, as apic_x2apic_probe will
> > see iommu_intremap == iommu_intremap_full (the default value) and thus
> > could choose cluster mode without real interrupt remapping support.
> > 
> > At first sight it would seem possible to move lower down, but as you
> > say, this is all quite fragile. It's even made worse because we lack a
> > strict ordering discipline or any kind of dependency checking, so even
> > if we mess up the order it's likely to go unnoticed unless someone
> > tests on an affected system.
> > 
> > While we can try to solve this for the upcoming release, long term we
> > need a stricter ordering, either as a comment, or even better enforced
> > somehow in code. The x2APIC vs IOMMU ordering has bitten us multiple
> > times and we should see about implementing a more robust solution.
> 
> So what's your thought then: Make the change (in another patch), or rather
> leave the code as is? I'm slightly in favor of making the change seeing
> that you agree that the rearrangement looks to be correct.

Sorry this wasn't clear, I've rambled too much: making the change in
another patch would be my preferred option.

I still wonder whether we could place some kind of checks to ensure
the logic of interrupt remap vs APIC initialization is executed in the
right order, but that's not release material.

Thanks, Roger.

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Roger Pau Monné]

On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
> On 25.10.2021 12:28, Roger Pau Monné wrote:
> > On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
> >> The two are really meant to be independent settings; iov_supports_xt()
> >> using || instead of && was simply wrong. The corrected check is,
> >> however, redundant, just like the (correct) one in iov_detect(): These
> >> hook functions are unreachable without acpi_ivrs_init() installing the
> >> iommu_init_ops pointer, which it does only upon success. (Unlike for
> >> VT-d there is no late clearing of iommu_enable due to quirks, and any
> >> possible clearing of iommu_intremap happens only after iov_supports_xt()
> >> has run.)
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> In fact in iov_detect() it could be iommu_enable alone which gets
> >> checked, but this felt overly aggressive to me. Instead I'm getting the
> >> impression that the function may wrongly not get called when "iommu=off"
> >> but interrupt remapping is in use: We'd not get the interrupt handler
> >> installed, and hence interrupt remapping related events would never get
> >> reported. (Same on VT-d, FTAOD.)
> > 
> > I've spend a non-trivial amount of time looking into this before
> > reading this note. AFAICT you could set iommu=off and still get x2APIC
> > enabled and relying on interrupt remapping.
> 
> Right, contrary to ...
> 
> >> For iov_supports_xt() the question is whether, like VT-d's
> >> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
> >> alone (in which case it would need to remain a check rather than getting
> >> converted to ASSERT()).
> > 
> > Hm, no, I don't think so. I think iommu_enable should take precedence
> > over iommu_intremap, and having iommu_enable == false should force
> > interrupt remapping to be reported as disabled. Note that disabling it
> > in iommu_setup is too late, as the APIC initialization will have
> > already taken place.
> > 
> > It's my reading of the command line parameter documentation that
> > setting iommu=off should disable all usage of the IOMMU, and that
> > includes the interrupt remapping support (ie: a user should not need
> > to set iommu=off,no-intremap)
> 
> ... that documentation. But I think it's the documentation that
> wants fixing, such that iommu=off really only control DMA remap.

IMO I think it's confusing to have sub-options that could be enabled
when you set the global one to off. I would expect `iommu=off` to
disable all the iommu related options, and I think it's fair for
people to expect that behavior.

I'm unsure whether it's fair to change the documentation now, we
should instead fix the code, so that people using `iommu=off` get the
expected behavior. Then we would likely need to introduce a way to
disable just dma remapping (dmaremap, similar to intremap). That
would make a much better and saner interface IMO.

Thanks, Roger.

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Jan Beulich]

On 02.11.2021 12:03, Roger Pau Monné wrote:
> On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
>> On 25.10.2021 12:28, Roger Pau Monné wrote:
>>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
>>>> The two are really meant to be independent settings; iov_supports_xt()
>>>> using || instead of && was simply wrong. The corrected check is,
>>>> however, redundant, just like the (correct) one in iov_detect(): These
>>>> hook functions are unreachable without acpi_ivrs_init() installing the
>>>> iommu_init_ops pointer, which it does only upon success. (Unlike for
>>>> VT-d there is no late clearing of iommu_enable due to quirks, and any
>>>> possible clearing of iommu_intremap happens only after iov_supports_xt()
>>>> has run.)
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> In fact in iov_detect() it could be iommu_enable alone which gets
>>>> checked, but this felt overly aggressive to me. Instead I'm getting the
>>>> impression that the function may wrongly not get called when "iommu=off"
>>>> but interrupt remapping is in use: We'd not get the interrupt handler
>>>> installed, and hence interrupt remapping related events would never get
>>>> reported. (Same on VT-d, FTAOD.)
>>>
>>> I've spend a non-trivial amount of time looking into this before
>>> reading this note. AFAICT you could set iommu=off and still get x2APIC
>>> enabled and relying on interrupt remapping.
>>
>> Right, contrary to ...
>>
>>>> For iov_supports_xt() the question is whether, like VT-d's
>>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
>>>> alone (in which case it would need to remain a check rather than getting
>>>> converted to ASSERT()).
>>>
>>> Hm, no, I don't think so. I think iommu_enable should take precedence
>>> over iommu_intremap, and having iommu_enable == false should force
>>> interrupt remapping to be reported as disabled. Note that disabling it
>>> in iommu_setup is too late, as the APIC initialization will have
>>> already taken place.
>>>
>>> It's my reading of the command line parameter documentation that
>>> setting iommu=off should disable all usage of the IOMMU, and that
>>> includes the interrupt remapping support (ie: a user should not need
>>> to set iommu=off,no-intremap)
>>
>> ... that documentation. But I think it's the documentation that
>> wants fixing, such that iommu=off really only control DMA remap.
> 
> IMO I think it's confusing to have sub-options that could be enabled
> when you set the global one to off. I would expect `iommu=off` to
> disable all the iommu related options, and I think it's fair for
> people to expect that behavior.
> 
> I'm unsure whether it's fair to change the documentation now, we
> should instead fix the code, so that people using `iommu=off` get the
> expected behavior. Then we would likely need to introduce a way to
> disable just dma remapping (dmaremap, similar to intremap). That
> would make a much better and saner interface IMO.

But from an x2APIC perspective it is a problem to have "iommu=off"
also turn off intremap. And indeed the option has never (fully)
worked that way: It clears iommu_enable, but not iommu_intremap
(nor any of the other sub-options, but there it's less of a problem
because they're not used in isolation), and iommu_intremap only
may have happened to either get turned off later or to not get
evaluated in at least some of the case.

Jan

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Roger Pau Monné]

On Tue, Nov 02, 2021 at 03:00:24PM +0100, Jan Beulich wrote:
> On 02.11.2021 12:03, Roger Pau Monné wrote:
> > On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
> >> On 25.10.2021 12:28, Roger Pau Monné wrote:
> >>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
> >>>> The two are really meant to be independent settings; iov_supports_xt()
> >>>> using || instead of && was simply wrong. The corrected check is,
> >>>> however, redundant, just like the (correct) one in iov_detect(): These
> >>>> hook functions are unreachable without acpi_ivrs_init() installing the
> >>>> iommu_init_ops pointer, which it does only upon success. (Unlike for
> >>>> VT-d there is no late clearing of iommu_enable due to quirks, and any
> >>>> possible clearing of iommu_intremap happens only after iov_supports_xt()
> >>>> has run.)
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> In fact in iov_detect() it could be iommu_enable alone which gets
> >>>> checked, but this felt overly aggressive to me. Instead I'm getting the
> >>>> impression that the function may wrongly not get called when "iommu=off"
> >>>> but interrupt remapping is in use: We'd not get the interrupt handler
> >>>> installed, and hence interrupt remapping related events would never get
> >>>> reported. (Same on VT-d, FTAOD.)
> >>>
> >>> I've spend a non-trivial amount of time looking into this before
> >>> reading this note. AFAICT you could set iommu=off and still get x2APIC
> >>> enabled and relying on interrupt remapping.
> >>
> >> Right, contrary to ...
> >>
> >>>> For iov_supports_xt() the question is whether, like VT-d's
> >>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
> >>>> alone (in which case it would need to remain a check rather than getting
> >>>> converted to ASSERT()).
> >>>
> >>> Hm, no, I don't think so. I think iommu_enable should take precedence
> >>> over iommu_intremap, and having iommu_enable == false should force
> >>> interrupt remapping to be reported as disabled. Note that disabling it
> >>> in iommu_setup is too late, as the APIC initialization will have
> >>> already taken place.
> >>>
> >>> It's my reading of the command line parameter documentation that
> >>> setting iommu=off should disable all usage of the IOMMU, and that
> >>> includes the interrupt remapping support (ie: a user should not need
> >>> to set iommu=off,no-intremap)
> >>
> >> ... that documentation. But I think it's the documentation that
> >> wants fixing, such that iommu=off really only control DMA remap.
> > 
> > IMO I think it's confusing to have sub-options that could be enabled
> > when you set the global one to off. I would expect `iommu=off` to
> > disable all the iommu related options, and I think it's fair for
> > people to expect that behavior.
> > 
> > I'm unsure whether it's fair to change the documentation now, we
> > should instead fix the code, so that people using `iommu=off` get the
> > expected behavior. Then we would likely need to introduce a way to
> > disable just dma remapping (dmaremap, similar to intremap). That
> > would make a much better and saner interface IMO.
> 
> But from an x2APIC perspective it is a problem to have "iommu=off"
> also turn off intremap.

I think we could log a message in that case? (x2APIC could be enabled
but iommu explicitly disabled)

And maybe expand the documentation to notice that disabling the iommu
or interrupt remapping will disable x2APIC support.

> And indeed the option has never (fully)
> worked that way: It clears iommu_enable, but not iommu_intremap
> (nor any of the other sub-options, but there it's less of a problem
> because they're not used in isolation), and iommu_intremap only
> may have happened to either get turned off later or to not get
> evaluated in at least some of the case.

While I understand there's some baggage here, I'm not sure keeping the
current behavior is correct. I would rather have iommu=off to cover
all iommu functionality, and then we should add dmaremap sub-option to
disable remapping only. I think that would be a sane and logic
interface for users to understand.

We should also note the implications of disabling interrupt remapping
regarding x2APIC support in the documentation.

Thanks, Roger.

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Jan Beulich]

On 02.11.2021 12:03, Roger Pau Monné wrote:
> On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
>> On 25.10.2021 12:28, Roger Pau Monné wrote:
>>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
>>>> The two are really meant to be independent settings; iov_supports_xt()
>>>> using || instead of && was simply wrong. The corrected check is,
>>>> however, redundant, just like the (correct) one in iov_detect(): These
>>>> hook functions are unreachable without acpi_ivrs_init() installing the
>>>> iommu_init_ops pointer, which it does only upon success. (Unlike for
>>>> VT-d there is no late clearing of iommu_enable due to quirks, and any
>>>> possible clearing of iommu_intremap happens only after iov_supports_xt()
>>>> has run.)
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> In fact in iov_detect() it could be iommu_enable alone which gets
>>>> checked, but this felt overly aggressive to me. Instead I'm getting the
>>>> impression that the function may wrongly not get called when "iommu=off"
>>>> but interrupt remapping is in use: We'd not get the interrupt handler
>>>> installed, and hence interrupt remapping related events would never get
>>>> reported. (Same on VT-d, FTAOD.)
>>>
>>> I've spend a non-trivial amount of time looking into this before
>>> reading this note. AFAICT you could set iommu=off and still get x2APIC
>>> enabled and relying on interrupt remapping.
>>
>> Right, contrary to ...
>>
>>>> For iov_supports_xt() the question is whether, like VT-d's
>>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
>>>> alone (in which case it would need to remain a check rather than getting
>>>> converted to ASSERT()).
>>>
>>> Hm, no, I don't think so. I think iommu_enable should take precedence
>>> over iommu_intremap, and having iommu_enable == false should force
>>> interrupt remapping to be reported as disabled. Note that disabling it
>>> in iommu_setup is too late, as the APIC initialization will have
>>> already taken place.
>>>
>>> It's my reading of the command line parameter documentation that
>>> setting iommu=off should disable all usage of the IOMMU, and that
>>> includes the interrupt remapping support (ie: a user should not need
>>> to set iommu=off,no-intremap)
>>
>> ... that documentation. But I think it's the documentation that
>> wants fixing, such that iommu=off really only control DMA remap.
> 
> IMO I think it's confusing to have sub-options that could be enabled
> when you set the global one to off. I would expect `iommu=off` to
> disable all the iommu related options, and I think it's fair for
> people to expect that behavior.

It occurs to me that this reply of yours here contradicts your R-b
on patch 1, in particular with its revision log saying:

v2: Treat iommu_enable and iommu_intremap as separate options.

Even in case I receive a release ack from Ian, I'll try to remember
to hold off committing that until this apparent (to me) confusion
was sorted.

Jan

Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling

[Jan Beulich]

On 02.11.2021 11:17, Jan Beulich wrote:
> On 21.10.2021 11:57, Jan Beulich wrote:
>> In the course of reading the response to v1 (patch 1 only) I realized
>> that not only that patch needs further adjustment, but that also
>> further changes are needed (and there's likely yet more amiss).
>>
>> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
>> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
>> 3: AMD/IOMMU: iommu_enable vs iommu_intremap
> 
> Ian, while we further discuss / refine patch 3, the first two have the
> needed R-b, but will now need you release-ack aiui.

Seeing your reply on IRC, here an attempt at a release justification
(the patches were ready by Oct 29, but no-one cared to commit them
in my absence, so I thought I'd get away without such a write-up):

Patch 1 addresses a regression identified by Andrew. The main risk I
see here (which has turned up only very recently) is disagreement on
patch 3 which imo has an effect also on what patch 1 does, as to the
(non-)effects of "iommu=off" on the hypervisor command line. This,
however, is not an effect of the patch, but pre-existing behavior.
The behavioral change (in this regard) is in patch 3, which is still
under discussion.

Patch 2 corrects an (unlikely but not impossible to be taken) error
path, supposedly making systems functional again in case they would
in fact cause that error path to be taken. The risk looks low to me,
given that two function calls with previously assumed to be
identical results now get folded into one with the result latched.

Jan

> Andrew, did you perhaps have a chance to actually try v2 of patch 1? It
> works for me when suitably configuring the BIOS on my Skylake, so I
> wouldn't feel uncertain in committing without a Tested-by, but it would
> feel even better if I had one.
> 
> Thanks, Jan
> 

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Roger Pau Monné]

On Wed, Nov 03, 2021 at 10:46:40AM +0100, Jan Beulich wrote:
> On 02.11.2021 12:03, Roger Pau Monné wrote:
> > On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
> >> On 25.10.2021 12:28, Roger Pau Monné wrote:
> >>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
> >>>> The two are really meant to be independent settings; iov_supports_xt()
> >>>> using || instead of && was simply wrong. The corrected check is,
> >>>> however, redundant, just like the (correct) one in iov_detect(): These
> >>>> hook functions are unreachable without acpi_ivrs_init() installing the
> >>>> iommu_init_ops pointer, which it does only upon success. (Unlike for
> >>>> VT-d there is no late clearing of iommu_enable due to quirks, and any
> >>>> possible clearing of iommu_intremap happens only after iov_supports_xt()
> >>>> has run.)
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> In fact in iov_detect() it could be iommu_enable alone which gets
> >>>> checked, but this felt overly aggressive to me. Instead I'm getting the
> >>>> impression that the function may wrongly not get called when "iommu=off"
> >>>> but interrupt remapping is in use: We'd not get the interrupt handler
> >>>> installed, and hence interrupt remapping related events would never get
> >>>> reported. (Same on VT-d, FTAOD.)
> >>>
> >>> I've spend a non-trivial amount of time looking into this before
> >>> reading this note. AFAICT you could set iommu=off and still get x2APIC
> >>> enabled and relying on interrupt remapping.
> >>
> >> Right, contrary to ...
> >>
> >>>> For iov_supports_xt() the question is whether, like VT-d's
> >>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
> >>>> alone (in which case it would need to remain a check rather than getting
> >>>> converted to ASSERT()).
> >>>
> >>> Hm, no, I don't think so. I think iommu_enable should take precedence
> >>> over iommu_intremap, and having iommu_enable == false should force
> >>> interrupt remapping to be reported as disabled. Note that disabling it
> >>> in iommu_setup is too late, as the APIC initialization will have
> >>> already taken place.
> >>>
> >>> It's my reading of the command line parameter documentation that
> >>> setting iommu=off should disable all usage of the IOMMU, and that
> >>> includes the interrupt remapping support (ie: a user should not need
> >>> to set iommu=off,no-intremap)
> >>
> >> ... that documentation. But I think it's the documentation that
> >> wants fixing, such that iommu=off really only control DMA remap.
> > 
> > IMO I think it's confusing to have sub-options that could be enabled
> > when you set the global one to off. I would expect `iommu=off` to
> > disable all the iommu related options, and I think it's fair for
> > people to expect that behavior.
> 
> It occurs to me that this reply of yours here contradicts your R-b
> on patch 1, in particular with its revision log saying:
> 
> v2: Treat iommu_enable and iommu_intremap as separate options.

Right, I see. patch 1 uses

if ( !iommu_enable && !iommu_intremap )
    return;

Which I think should be:

if ( !iommu_enable )
    return;

Sorry I didn't realize in that context. I think we need to decide
whether we want to fix the documentation to match the code, or whether
we should fix the code to match the documentation.

My preference would be for the latter, because I think the resulting
interface would be clearer. That will require introducing a new
dmaremap iommu suboption, but again I think this will result in a
better interface overall.

Thanks, Roger.

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Jan Beulich]

On 03.11.2021 16:06, Roger Pau Monné wrote:
> On Wed, Nov 03, 2021 at 10:46:40AM +0100, Jan Beulich wrote:
>> On 02.11.2021 12:03, Roger Pau Monné wrote:
>>> On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
>>>> On 25.10.2021 12:28, Roger Pau Monné wrote:
>>>>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
>>>>>> The two are really meant to be independent settings; iov_supports_xt()
>>>>>> using || instead of && was simply wrong. The corrected check is,
>>>>>> however, redundant, just like the (correct) one in iov_detect(): These
>>>>>> hook functions are unreachable without acpi_ivrs_init() installing the
>>>>>> iommu_init_ops pointer, which it does only upon success. (Unlike for
>>>>>> VT-d there is no late clearing of iommu_enable due to quirks, and any
>>>>>> possible clearing of iommu_intremap happens only after iov_supports_xt()
>>>>>> has run.)
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> ---
>>>>>> In fact in iov_detect() it could be iommu_enable alone which gets
>>>>>> checked, but this felt overly aggressive to me. Instead I'm getting the
>>>>>> impression that the function may wrongly not get called when "iommu=off"
>>>>>> but interrupt remapping is in use: We'd not get the interrupt handler
>>>>>> installed, and hence interrupt remapping related events would never get
>>>>>> reported. (Same on VT-d, FTAOD.)
>>>>>
>>>>> I've spend a non-trivial amount of time looking into this before
>>>>> reading this note. AFAICT you could set iommu=off and still get x2APIC
>>>>> enabled and relying on interrupt remapping.
>>>>
>>>> Right, contrary to ...
>>>>
>>>>>> For iov_supports_xt() the question is whether, like VT-d's
>>>>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
>>>>>> alone (in which case it would need to remain a check rather than getting
>>>>>> converted to ASSERT()).
>>>>>
>>>>> Hm, no, I don't think so. I think iommu_enable should take precedence
>>>>> over iommu_intremap, and having iommu_enable == false should force
>>>>> interrupt remapping to be reported as disabled. Note that disabling it
>>>>> in iommu_setup is too late, as the APIC initialization will have
>>>>> already taken place.
>>>>>
>>>>> It's my reading of the command line parameter documentation that
>>>>> setting iommu=off should disable all usage of the IOMMU, and that
>>>>> includes the interrupt remapping support (ie: a user should not need
>>>>> to set iommu=off,no-intremap)
>>>>
>>>> ... that documentation. But I think it's the documentation that
>>>> wants fixing, such that iommu=off really only control DMA remap.
>>>
>>> IMO I think it's confusing to have sub-options that could be enabled
>>> when you set the global one to off. I would expect `iommu=off` to
>>> disable all the iommu related options, and I think it's fair for
>>> people to expect that behavior.
>>
>> It occurs to me that this reply of yours here contradicts your R-b
>> on patch 1, in particular with its revision log saying:
>>
>> v2: Treat iommu_enable and iommu_intremap as separate options.
> 
> Right, I see. patch 1 uses
> 
> if ( !iommu_enable && !iommu_intremap )
>     return;
> 
> Which I think should be:
> 
> if ( !iommu_enable )
>     return;
> 
> Sorry I didn't realize in that context. I think we need to decide
> whether we want to fix the documentation to match the code, or whether
> we should fix the code to match the documentation.

Except that adjusting the conditional(s) in patch 1 would then
be a functional change that's not really the purpose of that
patch - it really only folds acpi_ivrs_init()'s and
acpi_parse_dmar()'s into a vendor-independent instance in
acpi_iommu_init(). Alternatively we could adjust the conditional
here (in patch 3), but that would feel unrelated once again, as
this change is supposed to be AMD-specific.

> My preference would be for the latter, because I think the resulting
> interface would be clearer. That will require introducing a new
> dmaremap iommu suboption, but again I think this will result in a
> better interface overall.

I guess we could do with a 3rd opinion: Paul, any chance?

In any event I hope that we can agree that patches 1 and 2 are
okay for 4.16 in their present shape, and patch 3 (plus whichever
further ones) would better wait for post-4.16?

Jan

Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"):
> On 02.11.2021 11:17, Jan Beulich wrote:
> > On 21.10.2021 11:57, Jan Beulich wrote:
> >> In the course of reading the response to v1 (patch 1 only) I realized
> >> that not only that patch needs further adjustment, but that also
> >> further changes are needed (and there's likely yet more amiss).
> >>
> >> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
> >> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
> >> 3: AMD/IOMMU: iommu_enable vs iommu_intremap
> > 
> > Ian, while we further discuss / refine patch 3, the first two have the
> > needed R-b, but will now need you release-ack aiui.
> 
> Seeing your reply on IRC, here an attempt at a release justification
> (the patches were ready by Oct 29, but no-one cared to commit them
> in my absence, so I thought I'd get away without such a write-up):
> 
> Patch 1 addresses a regression identified by Andrew. The main risk I
> see here (which has turned up only very recently) is disagreement on
> patch 3 which imo has an effect also on what patch 1 does, as to the
> (non-)effects of "iommu=off" on the hypervisor command line. This,
> however, is not an effect of the patch, but pre-existing behavior.
> The behavioral change (in this regard) is in patch 3, which is still
> under discussion.

Thank you.  I also went to the list and read the thread there.

Patch 1:

Reviewed-by: Ian Jackson <iwj@xenproject.org>

> Patch 2 corrects an (unlikely but not impossible to be taken) error
> path, supposedly making systems functional again in case they would
> in fact cause that error path to be taken. The risk looks low to me,
> given that two function calls with previously assumed to be
> identical results now get folded into one with the result latched.

This one also:

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

I think, from reading the thread, that patch 3 is not targeting 4.16.

Thanks,
Ian.

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap

[Roger Pau Monné]

On Wed, Nov 03, 2021 at 04:15:52PM +0100, Jan Beulich wrote:
> On 03.11.2021 16:06, Roger Pau Monné wrote:
> > On Wed, Nov 03, 2021 at 10:46:40AM +0100, Jan Beulich wrote:
> >> On 02.11.2021 12:03, Roger Pau Monné wrote:
> >>> On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
> >>>> On 25.10.2021 12:28, Roger Pau Monné wrote:
> >>>>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
> >>>>>> The two are really meant to be independent settings; iov_supports_xt()
> >>>>>> using || instead of && was simply wrong. The corrected check is,
> >>>>>> however, redundant, just like the (correct) one in iov_detect(): These
> >>>>>> hook functions are unreachable without acpi_ivrs_init() installing the
> >>>>>> iommu_init_ops pointer, which it does only upon success. (Unlike for
> >>>>>> VT-d there is no late clearing of iommu_enable due to quirks, and any
> >>>>>> possible clearing of iommu_intremap happens only after iov_supports_xt()
> >>>>>> has run.)
> >>>>>>
> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>> ---
> >>>>>> In fact in iov_detect() it could be iommu_enable alone which gets
> >>>>>> checked, but this felt overly aggressive to me. Instead I'm getting the
> >>>>>> impression that the function may wrongly not get called when "iommu=off"
> >>>>>> but interrupt remapping is in use: We'd not get the interrupt handler
> >>>>>> installed, and hence interrupt remapping related events would never get
> >>>>>> reported. (Same on VT-d, FTAOD.)
> >>>>>
> >>>>> I've spend a non-trivial amount of time looking into this before
> >>>>> reading this note. AFAICT you could set iommu=off and still get x2APIC
> >>>>> enabled and relying on interrupt remapping.
> >>>>
> >>>> Right, contrary to ...
> >>>>
> >>>>>> For iov_supports_xt() the question is whether, like VT-d's
> >>>>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
> >>>>>> alone (in which case it would need to remain a check rather than getting
> >>>>>> converted to ASSERT()).
> >>>>>
> >>>>> Hm, no, I don't think so. I think iommu_enable should take precedence
> >>>>> over iommu_intremap, and having iommu_enable == false should force
> >>>>> interrupt remapping to be reported as disabled. Note that disabling it
> >>>>> in iommu_setup is too late, as the APIC initialization will have
> >>>>> already taken place.
> >>>>>
> >>>>> It's my reading of the command line parameter documentation that
> >>>>> setting iommu=off should disable all usage of the IOMMU, and that
> >>>>> includes the interrupt remapping support (ie: a user should not need
> >>>>> to set iommu=off,no-intremap)
> >>>>
> >>>> ... that documentation. But I think it's the documentation that
> >>>> wants fixing, such that iommu=off really only control DMA remap.
> >>>
> >>> IMO I think it's confusing to have sub-options that could be enabled
> >>> when you set the global one to off. I would expect `iommu=off` to
> >>> disable all the iommu related options, and I think it's fair for
> >>> people to expect that behavior.
> >>
> >> It occurs to me that this reply of yours here contradicts your R-b
> >> on patch 1, in particular with its revision log saying:
> >>
> >> v2: Treat iommu_enable and iommu_intremap as separate options.
> > 
> > Right, I see. patch 1 uses
> > 
> > if ( !iommu_enable && !iommu_intremap )
> >     return;
> > 
> > Which I think should be:
> > 
> > if ( !iommu_enable )
> >     return;
> > 
> > Sorry I didn't realize in that context. I think we need to decide
> > whether we want to fix the documentation to match the code, or whether
> > we should fix the code to match the documentation.
> 
> Except that adjusting the conditional(s) in patch 1 would then
> be a functional change that's not really the purpose of that
> patch - it really only folds acpi_ivrs_init()'s and
> acpi_parse_dmar()'s into a vendor-independent instance in
> acpi_iommu_init().

Right.

> Alternatively we could adjust the conditional
> here (in patch 3), but that would feel unrelated once again, as
> this change is supposed to be AMD-specific.

Depending on what we end up doing regarding interrupt remapping being
disabled with iommu=off we might want to rework patch 3.

> > My preference would be for the latter, because I think the resulting
> > interface would be clearer. That will require introducing a new
> > dmaremap iommu suboption, but again I think this will result in a
> > better interface overall.
> 
> I guess we could do with a 3rd opinion: Paul, any chance?
> 
> In any event I hope that we can agree that patches 1 and 2 are
> okay for 4.16 in their present shape, and patch 3 (plus whichever
> further ones) would better wait for post-4.16?

I consider the issues either a bug in the documentation or the code,
so it's likely I would suggest whatever fix we end up doing to be
backported. At which point it might make sense to add to the release.

I don't think it should be a blocked though, as this hasn't been
introduced in this release.

Thanks, Roger.

Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling

[Jan Beulich]

On 03.11.2021 17:19, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"):
>> On 02.11.2021 11:17, Jan Beulich wrote:
>>> On 21.10.2021 11:57, Jan Beulich wrote:
>>>> In the course of reading the response to v1 (patch 1 only) I realized
>>>> that not only that patch needs further adjustment, but that also
>>>> further changes are needed (and there's likely yet more amiss).
>>>>
>>>> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
>>>> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
>>>> 3: AMD/IOMMU: iommu_enable vs iommu_intremap
>>>
>>> Ian, while we further discuss / refine patch 3, the first two have the
>>> needed R-b, but will now need you release-ack aiui.
>>
>> Seeing your reply on IRC, here an attempt at a release justification
>> (the patches were ready by Oct 29, but no-one cared to commit them
>> in my absence, so I thought I'd get away without such a write-up):
>>
>> Patch 1 addresses a regression identified by Andrew. The main risk I
>> see here (which has turned up only very recently) is disagreement on
>> patch 3 which imo has an effect also on what patch 1 does, as to the
>> (non-)effects of "iommu=off" on the hypervisor command line. This,
>> however, is not an effect of the patch, but pre-existing behavior.
>> The behavioral change (in this regard) is in patch 3, which is still
>> under discussion.
> 
> Thank you.  I also went to the list and read the thread there.
> 
> Patch 1:
> 
> Reviewed-by: Ian Jackson <iwj@xenproject.org>
> 
>> Patch 2 corrects an (unlikely but not impossible to be taken) error
>> path, supposedly making systems functional again in case they would
>> in fact cause that error path to be taken. The risk looks low to me,
>> given that two function calls with previously assumed to be
>> identical results now get folded into one with the result latched.
> 
> This one also:
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks, but your reply leaves me a little confused: Your use of "also"
may mean R-b for both patches but R-a-b only for patch 2. But I could
also find a variety of other interpretations, including that the
first R-b really was meant to be R-a-b (which otherwise I'd need on
top of the R-b anyway). Please clarify.

> I think, from reading the thread, that patch 3 is not targeting 4.16.

Correct. The other related one now targeting 4.16 is the separately
submitted "x86/x2APIC: defer probe until after IOMMU ACPI table
parsing". But I can see reasons for you to prefer to have that
deferred.

Jan

Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"):
> > Reviewed-by: Ian Jackson <iwj@xenproject.org>
> > 
> >> Patch 2 corrects an (unlikely but not impossible to be taken) error
> >> path, supposedly making systems functional again in case they would
> >> in fact cause that error path to be taken. The risk looks low to me,
> >> given that two function calls with previously assumed to be
> >> identical results now get folded into one with the result latched.
> > 
> > This one also:
> > 
> > Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> Thanks, but your reply leaves me a little confused: Your use of "also"
> may mean R-b for both patches but R-a-b only for patch 2. But I could
> also find a variety of other interpretations, including that the
> first R-b really was meant to be R-a-b (which otherwise I'd need on
> top of the R-b anyway). Please clarify.

Um.  Well spotted.  I meant release-acked-by for both.  I botched the
keystroke in my MUA.  Sorry for the confusion.

So FTAAD I have *not* "Reviewed" either patch (although I did read it,
I don't consider myself qualified to give an R-b).

> > I think, from reading the thread, that patch 3 is not targeting 4.16.
> 
> Correct. The other related one now targeting 4.16 is the separately
> submitted "x86/x2APIC: defer probe until after IOMMU ACPI table
> parsing". But I can see reasons for you to prefer to have that
> deferred.

Thanks,
Ian.