|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |