Xen devel - Oct 2012 - [PATCH 1/1] keep iommu 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 that got introduced with
patch "x86-64: detect processors subject to AMD erratum #121 and refuse to 
boot." since xen 4.1.3 and results in the following stacktrace:

   find_iommu_for_device
   amd_iommu_ioapic_update_ire
   timer_interrupt
   enable_8259_A_irq
   do_IRQ
   printk_start_of_line
   acpi_os_printf
   io_apic_write
   __ioapic_write_entry
   ioapic_write_entry
   __clear_IO_APIC_pin
   clear_IO_APIC
   disable_IO_APIC
   __stop_this_cpu
   smp_send_stop
   machine_restart
   panic
   tasklet_schedule_on_cpu
   display_cacheinfo
   init_amd
   generic_identify
   identify_cpu
   _start_xen
   _high_start


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

Signed-off-by: Ronny Hegewald@online.de

--- xen/drivers/passthrough/iommu.c.org	2012-10-05 03:38:33.000000000 +0000
+++ xen/drivers/passthrough/iommu.c	2012-10-17 22:58:07.000000000 +0000
@@ -38,7 +38,7 @@
  *   no-intremap                Disable VT-d Interrupt Remapping
  */
 custom_param("iommu", parse_iommu_param);
-bool_t __read_mostly iommu_enabled = 1;
+bool_t __read_mostly iommu_enabled = 0;
 bool_t __read_mostly force_iommu;
 bool_t __initdata iommu_dom0_strict;
 bool_t __read_mostly iommu_verbose;
@@ -51,6 +51,8 @@
 bool_t __read_mostly amd_iommu_debug;
 bool_t __read_mostly amd_iommu_perdev_intremap;
 
+bool_t iommu_enabled_default = 1;
+
 static void __init parse_iommu_param(char *s)
 {
     char *ss;
@@ -61,7 +63,7 @@
             *ss = ''\0'';
 
         if ( !parse_bool(s) )
-            iommu_enabled = 0;
+            iommu_enabled_default = 0;
         else if ( !strcmp(s, "force") || !strcmp(s,
"required") )
             force_iommu = 1;
         else if ( !strcmp(s, "workaround_bios_bug") )
@@ -312,6 +314,7 @@
 {
     int rc = -ENODEV;
     bool_t force_intremap = force_iommu && iommu_intremap;
+    iommu_enabled = iommu_enabled_default;
 
     if ( iommu_dom0_strict )
         iommu_passthrough = 0;

>>> On 18.10.12 at 02:49, Ronny Hegewald
<ronny.hegewald@online.de> 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 that got introduced 
> with
> patch "x86-64: detect processors subject to AMD erratum #121 and
refuse to
> boot." since xen 4.1.3 and results in the following stacktrace:
> 
>    find_iommu_for_device
>    amd_iommu_ioapic_update_ire
>    timer_interrupt
>    enable_8259_A_irq
>    do_IRQ
>    printk_start_of_line
>    acpi_os_printf
>    io_apic_write
>    __ioapic_write_entry
>    ioapic_write_entry
>    __clear_IO_APIC_pin
>    clear_IO_APIC
>    disable_IO_APIC
>    __stop_this_cpu
>    smp_send_stop
>    machine_restart
>    panic
>    tasklet_schedule_on_cpu
>    display_cacheinfo
>    init_amd
>    generic_identify
>    identify_cpu
>    _start_xen
>    _high_start
> 
> 
> This patch fixes this by keeping the iommu disabled until iommu_setup() is 
> entered. 
The concept is plausible, but I''m not convinced that there
aren''t
any users of iommu_enabled that actually depend on it having
its final value after command line parsing, yet before
iommu_setup() gets called. Did you fully audit all users?

As to the patch itself, ...
> Signed-off-by: Ronny Hegewald@online.de 
> 
> --- xen/drivers/passthrough/iommu.c.org	2012-10-05 03:38:33.000000000 +0000
> +++ xen/drivers/passthrough/iommu.c	2012-10-17 22:58:07.000000000 +0000
> @@ -38,7 +38,7 @@
>   *   no-intremap                Disable VT-d Interrupt Remapping
>   */
>  custom_param("iommu", parse_iommu_param);
> -bool_t __read_mostly iommu_enabled = 1;
> +bool_t __read_mostly iommu_enabled = 0;
No initializer needed.
>  bool_t __read_mostly force_iommu;
>  bool_t __initdata iommu_dom0_strict;
>  bool_t __read_mostly iommu_verbose;
> @@ -51,6 +51,8 @@
>  bool_t __read_mostly amd_iommu_debug;
>  bool_t __read_mostly amd_iommu_perdev_intremap;
>  
> +bool_t iommu_enabled_default = 1;
__initdata. Also better placed alongside iommu_enabled (see
below for the naming).
> +
>  static void __init parse_iommu_param(char *s)
>  {
>      char *ss;
> @@ -61,7 +63,7 @@
>              *ss = ''\0'';
>  
>          if ( !parse_bool(s) )
> -            iommu_enabled = 0;
> +            iommu_enabled_default = 0;
>          else if ( !strcmp(s, "force") || !strcmp(s,
"required") )
>              force_iommu = 1;
>          else if ( !strcmp(s, "workaround_bios_bug") )
> @@ -312,6 +314,7 @@
>  {
>      int rc = -ENODEV;
>      bool_t force_intremap = force_iommu && iommu_intremap;
> +    iommu_enabled = iommu_enabled_default;
Misplaced and pointless - just make the subsequent conditional
check the new variable instead of iommu_enabled (making quite
obvious that the chosen name is somewhat odd - iommu_enable
would probably be better).

Jan
>  
>      if ( iommu_dom0_strict )
>          iommu_passthrough = 0;
> 
> 
> 
> 
> 
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

> The concept is plausible, but I''m not convinced that there
aren''t
> any users of iommu_enabled that actually depend on it having
> its final value after command line parsing, yet before
> iommu_setup() gets called. 
But that is currently only true for systems with a iommu. For the ones without 
one have iommu_enabled=1 first, until iommu_setup() is called and 
iommu_enabled is set to 0 because of the not detectable iommu.

So, with or without my patch, a similiar problem seems to be present, just for 
different systems.
> Did you fully audit all users?
No, because there is just no way i can make a meaningfull judgement how the 
iommu code interdepends with all the other parts that check iommu_enabled. 
> As to the patch itself, ...
I will send a 2. version which addresses your comments as soon its clear if 
the approach in the patch is the right one to fix the crash.

>>> On 19.10.12 at 02:35, Ronny Hegewald
<ronny.hegewald@online.de> wrote:
>>  The concept is plausible, but I''m not convinced that there
aren''t
>> any users of iommu_enabled that actually depend on it having
>> its final value after command line parsing, yet before
>> iommu_setup() gets called. 
> 
> But that is currently only true for systems with a iommu. For the ones 
> without 
> one have iommu_enabled=1 first, until iommu_setup() is called and 
> iommu_enabled is set to 0 because of the not detectable iommu.
That''s a valid argument.
> So, with or without my patch, a similiar problem seems to be present, just 
> for different systems.
> 
>> Did you fully audit all users?
> 
> No, because there is just no way i can make a meaningfull judgement how the
> iommu code interdepends with all the other parts that check iommu_enabled. 
I did a brief check, and it seems to be okay. I guess we''ll have to
deal with eventual fallout then.
>> As to the patch itself, ...
> 
> I will send a 2. version which addresses your comments as soon its clear if
> the approach in the patch is the right one to fix the crash.
Please do.

Jan