Re: [Xen-devel] [PATCH 1/1] keep iommu disabled until iommu_setup is called

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.10.12 at 02:49, Ronny Hegewald <ronny.hegewald@xxxxxxxxx> 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@xxxxxxxxx 
> 
> --- 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@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 




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