[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 24.10.12 at 17:49, "Jan Beulich" <JBeulich@xxxxxxxx> 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@xxxxxxxxx>
> 
> 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@xxxxxxxx>
> 
> --- 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;



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.