Xen devel - Oct 2012 - [PATCH v3] IOMMU: keep disabled until iommu_setup() is called

The iommu is enabled by default when xen is booting and later disabled
in iommu_setup() when no iommu is present.

But under some circumstances iommu code can be called before
iommu_setup() is processed. If there is no iommu available xen crashes.

This can happen for example when panic(...) is called as introduced
with the patch "x86-64: detect processors subject to AMD erratum #121
and refuse to boot" since xen 4.1.3, resulting in
find_iommu_for_device() to be called in the context of
disable_IO_APIC() / __stop_this_cpu().

This patch fixes this by keeping the iommu disabled until iommu_setup()
is entered.

Originally-by: Ronny Hegewald <ronny.hegewald@online.de>

In order for iommu_enable to be off initially, iommu_supports_eim()
must not depend on it anymore, nor must acpi_parse_dmar(). The former
in turn requires that iommu_intremap gets uncoupled from iommu_enabled
(in particular, failure during IOMMU setup should no longer result in
iommu_intremap getting cleared by generic code; IOMMU specific code
can still do so provided in can live with the consequences).

This could have the nice side effect of allowing to use "iommu=off"
even when x2APIC was pre-enabled by the BIOS (in which case interrupt
remapping is a requirement, but DMA translation [obviously] isn''t), but
that doesn''t currently work (and hence x2apic_bsp_setup() forces the
IOMMU on rather than just interrupt remapping).

For consistency with VT-d, make the AMD IOMMU code also skip all ACPI
table parsing when neither iommu_enable nor iommu_intremap are set.

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

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -975,6 +975,8 @@ void __init x2apic_bsp_setup(void)
         goto restore_out;
     }
 
+    force_iommu = 1;
+
     genapic = apic_x2apic_probe();
     printk("Switched to APIC driver %s.\n", genapic->name);
 
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -164,9 +164,13 @@ int __init amd_iov_detect(void)
 {
     INIT_LIST_HEAD(&amd_iommu_head);
 
+    if ( !iommu_enable && !iommu_intremap )
+        return 0;
+
     if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
     {
         printk("AMD-Vi: IOMMU not found!\n");
+        iommu_intremap = 0;
         return -ENODEV;
     }
 
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -41,7 +41,8 @@ static void iommu_dump_p2m_table(unsigne
  *   no-intremap                Disable VT-d Interrupt Remapping
  */
 custom_param("iommu", parse_iommu_param);
-bool_t __read_mostly iommu_enabled = 1;
+bool_t __initdata iommu_enable = 1;
+bool_t __read_mostly iommu_enabled;
 bool_t __read_mostly force_iommu;
 bool_t __initdata iommu_dom0_strict;
 bool_t __read_mostly iommu_verbose;
@@ -77,7 +78,7 @@ static void __init parse_iommu_param(cha
             *ss = ''\0'';
 
         if ( !parse_bool(s) )
-            iommu_enabled = 0;
+            iommu_enable = 0;
         else if ( !strcmp(s, "force") || !strcmp(s,
"required") )
             force_iommu = val;
         else if ( !strcmp(s, "workaround_bios_bug") )
@@ -395,7 +396,7 @@ int __init iommu_setup(void)
     if ( iommu_dom0_strict )
         iommu_passthrough = 0;
 
-    if ( iommu_enabled )
+    if ( iommu_enable )
     {
         rc = iommu_hardware_setup();
         iommu_enabled = (rc == 0);
@@ -409,8 +410,6 @@ int __init iommu_setup(void)
     if ( !iommu_enabled )
     {
         iommu_snoop = 0;
-        iommu_qinval = 0;
-        iommu_intremap = 0;
         iommu_passthrough = 0;
         iommu_dom0_strict = 0;
     }
@@ -419,6 +418,8 @@ int __init iommu_setup(void)
         printk(" - Dom0 mode: %s\n",
                iommu_passthrough ? "Passthrough" :
                iommu_dom0_strict ? "Strict" : "Relaxed");
+    else if ( iommu_intremap )
+        printk("Interrupt remapping enabled\n");
 
     return rc;
 }
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -744,7 +744,7 @@ static int __init acpi_parse_dmar(struct
     dmar = (struct acpi_table_dmar *)table;
     dmar_flags = dmar->flags;
 
-    if ( !iommu_enabled )
+    if ( !iommu_enable && !iommu_intremap )
     {
         ret = -EINVAL;
         goto out;
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -149,8 +149,7 @@ int iommu_supports_eim(void)
     struct acpi_drhd_unit *drhd;
     int apic;
 
-    if ( !iommu_enabled || !iommu_qinval || !iommu_intremap ||
-         list_empty(&acpi_drhd_units) )
+    if ( !iommu_qinval || !iommu_intremap || list_empty(&acpi_drhd_units) )
         return 0;
 
     /* We MUST have a DRHD unit for each IOAPIC. */
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2086,7 +2086,10 @@ int __init intel_vtd_setup(void)
     int ret;
 
     if ( list_empty(&acpi_drhd_units) )
-        return -ENODEV;
+    {
+        ret = -ENODEV;
+        goto error;
+    }
 
     platform_quirks_init();
 
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -26,7 +26,7 @@
 #include <public/hvm/ioreq.h>
 #include <public/domctl.h>
 
-extern bool_t iommu_enabled;
+extern bool_t iommu_enable, iommu_enabled;
 extern bool_t force_iommu, iommu_verbose;
 extern bool_t iommu_workaround_bios_bug, iommu_passthrough;
 extern bool_t iommu_snoop, iommu_qinval, iommu_intremap;




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

>>> On 24.10.12 at 17:49, "Jan Beulich"
<JBeulich@suse.com> wrote:
> The iommu is enabled by default when xen is booting and later disabled
> in iommu_setup() when no iommu is present.
> 
> But under some circumstances iommu code can be called before
> iommu_setup() is processed. If there is no iommu available xen crashes.
> 
> This can happen for example when panic(...) is called as introduced
> with the patch "x86-64: detect processors subject to AMD erratum #121
> and refuse to boot" since xen 4.1.3, resulting in
> find_iommu_for_device() to be called in the context of
> disable_IO_APIC() / __stop_this_cpu().
> 
> This patch fixes this by keeping the iommu disabled until iommu_setup()
> is entered.
> 
> Originally-by: Ronny Hegewald <ronny.hegewald@online.de>
> 
> In order for iommu_enable to be off initially, iommu_supports_eim()
> must not depend on it anymore, nor must acpi_parse_dmar(). The former
> in turn requires that iommu_intremap gets uncoupled from iommu_enabled
> (in particular, failure during IOMMU setup should no longer result in
> iommu_intremap getting cleared by generic code; IOMMU specific code
> can still do so provided in can live with the consequences).
> 
> This could have the nice side effect of allowing to use
"iommu=off"
> even when x2APIC was pre-enabled by the BIOS (in which case interrupt
> remapping is a requirement, but DMA translation [obviously]
isn''t), but
> that doesn''t currently work (and hence x2apic_bsp_setup() forces
the
> IOMMU on rather than just interrupt remapping).
> 
> For consistency with VT-d, make the AMD IOMMU code also skip all ACPI
> table parsing when neither iommu_enable nor iommu_intremap are set.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -975,6 +975,8 @@ void __init x2apic_bsp_setup(void)
>          goto restore_out;
>      }
>  
> +    force_iommu = 1;
> +
>      genapic = apic_x2apic_probe();
>      printk("Switched to APIC driver %s.\n", genapic->name);
>  
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -164,9 +164,13 @@ int __init amd_iov_detect(void)
>  {
>      INIT_LIST_HEAD(&amd_iommu_head);
>  
> +    if ( !iommu_enable && !iommu_intremap )
> +        return 0;
> +
>      if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
>      {
>          printk("AMD-Vi: IOMMU not found!\n");
> +        iommu_intremap = 0;
>          return -ENODEV;
>      }
>  
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -41,7 +41,8 @@ static void iommu_dump_p2m_table(unsigne
>   *   no-intremap                Disable VT-d Interrupt Remapping
>   */
>  custom_param("iommu", parse_iommu_param);
> -bool_t __read_mostly iommu_enabled = 1;
> +bool_t __initdata iommu_enable = 1;
> +bool_t __read_mostly iommu_enabled;
>  bool_t __read_mostly force_iommu;
>  bool_t __initdata iommu_dom0_strict;
>  bool_t __read_mostly iommu_verbose;
> @@ -77,7 +78,7 @@ static void __init parse_iommu_param(cha
>              *ss = ''\0'';
>  
>          if ( !parse_bool(s) )
> -            iommu_enabled = 0;
> +            iommu_enable = 0;
>          else if ( !strcmp(s, "force") || !strcmp(s,
"required") )
>              force_iommu = val;
>          else if ( !strcmp(s, "workaround_bios_bug") )
> @@ -395,7 +396,7 @@ int __init iommu_setup(void)
>      if ( iommu_dom0_strict )
>          iommu_passthrough = 0;
>  
> -    if ( iommu_enabled )
> +    if ( iommu_enable )
>      {
>          rc = iommu_hardware_setup();
>          iommu_enabled = (rc == 0);
> @@ -409,8 +410,6 @@ int __init iommu_setup(void)
>      if ( !iommu_enabled )
>      {
>          iommu_snoop = 0;
> -        iommu_qinval = 0;
> -        iommu_intremap = 0;
>          iommu_passthrough = 0;
>          iommu_dom0_strict = 0;
>      }
> @@ -419,6 +418,8 @@ int __init iommu_setup(void)
>          printk(" - Dom0 mode: %s\n",
>                 iommu_passthrough ? "Passthrough" :
>                 iommu_dom0_strict ? "Strict" :
"Relaxed");
> +    else if ( iommu_intremap )
> +        printk("Interrupt remapping enabled\n");
>  
>      return rc;
>  }
Please consider this one hunk absent for the purposes of reviewing
(it''s a leftover from the previous failed attempt to make interrupt
remapping work without the full IOMMU enabling and without much
more intrusive changes), I already dropped it from what I hope to
commit eventually.

Jan
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -744,7 +744,7 @@ static int __init acpi_parse_dmar(struct
>      dmar = (struct acpi_table_dmar *)table;
>      dmar_flags = dmar->flags;
>  
> -    if ( !iommu_enabled )
> +    if ( !iommu_enable && !iommu_intremap )
>      {
>          ret = -EINVAL;
>          goto out;
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -149,8 +149,7 @@ int iommu_supports_eim(void)
>      struct acpi_drhd_unit *drhd;
>      int apic;
>  
> -    if ( !iommu_enabled || !iommu_qinval || !iommu_intremap ||
> -         list_empty(&acpi_drhd_units) )
> +    if ( !iommu_qinval || !iommu_intremap ||
list_empty(&acpi_drhd_units) )
>          return 0;
>  
>      /* We MUST have a DRHD unit for each IOAPIC. */
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2086,7 +2086,10 @@ int __init intel_vtd_setup(void)
>      int ret;
>  
>      if ( list_empty(&acpi_drhd_units) )
> -        return -ENODEV;
> +    {
> +        ret = -ENODEV;
> +        goto error;
> +    }
>  
>      platform_quirks_init();
>  
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -26,7 +26,7 @@
>  #include <public/hvm/ioreq.h>
>  #include <public/domctl.h>
>  
> -extern bool_t iommu_enabled;
> +extern bool_t iommu_enable, iommu_enabled;
>  extern bool_t force_iommu, iommu_verbose;
>  extern bool_t iommu_workaround_bios_bug, iommu_passthrough;
>  extern bool_t iommu_snoop, iommu_qinval, iommu_intremap;

Looks fine, Thanks!
Acked-by: Xiantao Zhang <xiantao.zhang@intel.com>
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, October 24, 2012 11:50 PM
> To: xen-devel
> Cc: Wei Wang; Zhang, Xiantao; Ronny Hegewald
> Subject: [PATCH v3] IOMMU: keep disabled until iommu_setup() is called
> 
> The iommu is enabled by default when xen is booting and later disabled in
> iommu_setup() when no iommu is present.
> 
> But under some circumstances iommu code can be called before
> iommu_setup() is processed. If there is no iommu available xen crashes.
> 
> This can happen for example when panic(...) is called as introduced with
the
> patch "x86-64: detect processors subject to AMD erratum #121 and
refuse to
> boot" since xen 4.1.3, resulting in
> find_iommu_for_device() to be called in the context of
> disable_IO_APIC() / __stop_this_cpu().
> 
> This patch fixes this by keeping the iommu disabled until iommu_setup() is
> entered.
> 
> Originally-by: Ronny Hegewald <ronny.hegewald@online.de>
> 
> In order for iommu_enable to be off initially, iommu_supports_eim() must
> not depend on it anymore, nor must acpi_parse_dmar(). The former in turn
> requires that iommu_intremap gets uncoupled from iommu_enabled (in
> particular, failure during IOMMU setup should no longer result in
> iommu_intremap getting cleared by generic code; IOMMU specific code can
> still do so provided in can live with the consequences).
> 
> This could have the nice side effect of allowing to use
"iommu=off"
> even when x2APIC was pre-enabled by the BIOS (in which case interrupt
> remapping is a requirement, but DMA translation [obviously]
isn''t), but that
> doesn''t currently work (and hence x2apic_bsp_setup() forces the
IOMMU
> on rather than just interrupt remapping).
> 
> For consistency with VT-d, make the AMD IOMMU code also skip all ACPI
> table parsing when neither iommu_enable nor iommu_intremap are set.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -975,6 +975,8 @@ void __init x2apic_bsp_setup(void)
>          goto restore_out;
>      }
> 
> +    force_iommu = 1;
> +
>      genapic = apic_x2apic_probe();
>      printk("Switched to APIC driver %s.\n", genapic->name);
> 
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -164,9 +164,13 @@ int __init amd_iov_detect(void)  {
>      INIT_LIST_HEAD(&amd_iommu_head);
> 
> +    if ( !iommu_enable && !iommu_intremap )
> +        return 0;
> +
>      if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
>      {
>          printk("AMD-Vi: IOMMU not found!\n");
> +        iommu_intremap = 0;
>          return -ENODEV;
>      }
> 
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -41,7 +41,8 @@ static void iommu_dump_p2m_table(unsigne
>   *   no-intremap                Disable VT-d Interrupt Remapping
>   */
>  custom_param("iommu", parse_iommu_param); -bool_t __read_mostly
> iommu_enabled = 1;
> +bool_t __initdata iommu_enable = 1;
> +bool_t __read_mostly iommu_enabled;
>  bool_t __read_mostly force_iommu;
>  bool_t __initdata iommu_dom0_strict;
>  bool_t __read_mostly iommu_verbose;
> @@ -77,7 +78,7 @@ static void __init parse_iommu_param(cha
>              *ss = ''\0'';
> 
>          if ( !parse_bool(s) )
> -            iommu_enabled = 0;
> +            iommu_enable = 0;
>          else if ( !strcmp(s, "force") || !strcmp(s,
"required") )
>              force_iommu = val;
>          else if ( !strcmp(s, "workaround_bios_bug") ) @@ -395,7
+396,7 @@ int
> __init iommu_setup(void)
>      if ( iommu_dom0_strict )
>          iommu_passthrough = 0;
> 
> -    if ( iommu_enabled )
> +    if ( iommu_enable )
>      {
>          rc = iommu_hardware_setup();
>          iommu_enabled = (rc == 0);
> @@ -409,8 +410,6 @@ int __init iommu_setup(void)
>      if ( !iommu_enabled )
>      {
>          iommu_snoop = 0;
> -        iommu_qinval = 0;
> -        iommu_intremap = 0;
>          iommu_passthrough = 0;
>          iommu_dom0_strict = 0;
>      }
> @@ -419,6 +418,8 @@ int __init iommu_setup(void)
>          printk(" - Dom0 mode: %s\n",
>                 iommu_passthrough ? "Passthrough" :
>                 iommu_dom0_strict ? "Strict" :
"Relaxed");
> +    else if ( iommu_intremap )
> +        printk("Interrupt remapping enabled\n");
> 
>      return rc;
>  }
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -744,7 +744,7 @@ static int __init acpi_parse_dmar(struct
>      dmar = (struct acpi_table_dmar *)table;
>      dmar_flags = dmar->flags;
> 
> -    if ( !iommu_enabled )
> +    if ( !iommu_enable && !iommu_intremap )
>      {
>          ret = -EINVAL;
>          goto out;
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -149,8 +149,7 @@ int iommu_supports_eim(void)
>      struct acpi_drhd_unit *drhd;
>      int apic;
> 
> -    if ( !iommu_enabled || !iommu_qinval || !iommu_intremap ||
> -         list_empty(&acpi_drhd_units) )
> +    if ( !iommu_qinval || !iommu_intremap ||
> + list_empty(&acpi_drhd_units) )
>          return 0;
> 
>      /* We MUST have a DRHD unit for each IOAPIC. */
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2086,7 +2086,10 @@ int __init intel_vtd_setup(void)
>      int ret;
> 
>      if ( list_empty(&acpi_drhd_units) )
> -        return -ENODEV;
> +    {
> +        ret = -ENODEV;
> +        goto error;
> +    }
> 
>      platform_quirks_init();
> 
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -26,7 +26,7 @@
>  #include <public/hvm/ioreq.h>
>  #include <public/domctl.h>
> 
> -extern bool_t iommu_enabled;
> +extern bool_t iommu_enable, iommu_enabled;
>  extern bool_t force_iommu, iommu_verbose;  extern bool_t
> iommu_workaround_bios_bug, iommu_passthrough;  extern bool_t
> iommu_snoop, iommu_qinval, iommu_intremap;
>

>>> On 24.10.12 at 17:49, Jan Beulich wrote:
> The iommu is enabled by default when xen is booting and later disabled
> in iommu_setup() when no iommu is present.
> 
> But under some circumstances iommu code can be called before
> iommu_setup() is processed. If there is no iommu available xen crashes.
> 
> This can happen for example when panic(...) is called as introduced
> with the patch "x86-64: detect processors subject to AMD erratum #121
> and refuse to boot" since xen 4.1.3, resulting in
> find_iommu_for_device() to be called in the context of
> disable_IO_APIC() / __stop_this_cpu().
> 
> This patch fixes this by keeping the iommu disabled until iommu_setup()
> is entered.
> 
> Originally-by: Ronny Hegewald <ronny.hegewald@online.de>
> 
> In order for iommu_enable to be off initially, iommu_supports_eim()
> must not depend on it anymore, nor must acpi_parse_dmar(). The former
> in turn requires that iommu_intremap gets uncoupled from iommu_enabled
> (in particular, failure during IOMMU setup should no longer result in
> iommu_intremap getting cleared by generic code; IOMMU specific code
> can still do so provided in can live with the consequences).
> 
> This could have the nice side effect of allowing to use
"iommu=off"
> even when x2APIC was pre-enabled by the BIOS (in which case interrupt
> remapping is a requirement, but DMA translation [obviously]
isn''t), but
> that doesn''t currently work (and hence x2apic_bsp_setup() forces
the
> IOMMU on rather than just interrupt remapping).
> 
> For consistency with VT-d, make the AMD IOMMU code also skip all ACPI
> table parsing when neither iommu_enable nor iommu_intremap are set.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
So after having got acks from both Intel and AMD, I committed
this to -unstable, but I''m rather hesitant to backport this onto the
4.x branches (not the least because in the end this is just
cosmetic for the problem you reported - the box would not boot
in either case, just that with the patch it is more obvious why).

Jan

> So after having got acks from both Intel and AMD, I committed
> this to -unstable, but I''m rather hesitant to backport this onto
the
> 4.x branches (not the least because in the end this is just
> cosmetic for the problem you reported - the box would not boot
> in either case, just that with the patch it is more obvious why).
Actually the affected box can still boot xen. But you have to add the 
"allow_unsafe" option to the boot-parameters, which you are informed
about as
long the intended panic message is printed. 

Without the patch their is just the crash and the stacktrace.

But this problem seem only to hit some older AMD cpus for socket 754,939 and 
940 (as noted in CVE-2012-2934), and i didnt find any other report on the web 
about this xen crash. 

So it looks minor enough to not trouble the stable-branches with it imho too.

Acked-by: Wei Wang weiwang.dd@gmail.com
and sorry for late reply.

Thanks,
Wei
2012/11/3 Ronny Hegewald <ronny.hegewald@online.de>
> > So after having got acks from both Intel and AMD, I committed
> > this to -unstable, but I''m rather hesitant to backport this
onto the
> > 4.x branches (not the least because in the end this is just
> > cosmetic for the problem you reported - the box would not boot
> > in either case, just that with the patch it is more obvious why).
>
> Actually the affected box can still boot xen. But you have to add the
> "allow_unsafe" option to the boot-parameters, which you are
informed about
> as
> long the intended panic message is printed.
>
> Without the patch their is just the crash and the stacktrace.
>
> But this problem seem only to hit some older AMD cpus for socket 754,939
> and
> 940 (as noted in CVE-2012-2934), and i didnt find any other report on the
> web
> about this xen crash.
>
> So it looks minor enough to not trouble the stable-branches with it imho
> too.
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel