Messages in this thread Patch in this message | | | Date | Fri, 17 Apr 2009 16:13:10 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 3/5] x86, intr-remap: enable interrupt remapping early |
| |
* Weidong Han <weidong.han@intel.com> wrote:
> --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -118,6 +118,8 @@ static int x2apic_preenabled; > static int disable_x2apic; > static __init int setup_nox2apic(char *str) > { > + if (x2apic_enabled()) > + panic("Bios already enabled x2apic, can't enforce nox2apic");
Could you please turn that into something like:
printk(KERN_WARNING "Bios already enabled x2apic, can't enforce nox2apic"); return 1;
panic-ing the box just because we cannot meet a boot option is not good.
> +#ifdef CONFIG_X86_X2APIC > + if (cpu_has_x2apic) > + ret = enable_intr_remapping(EIM_32BIT_APIC_ID); > + else > +#endif > + ret = enable_intr_remapping(EIM_8BIT_APIC_ID);
That construct looks rather ugly.
Why not clear x2apic from the CPU flags if CONFIG_X86_X2APIC is disabled. (and print a one-liner during bootup that we did so)
Then this could be written as:
if (cpu_has_x2apic) ret = enable_intr_remapping(EIM_32BIT_APIC_ID); else ret = enable_intr_remapping(EIM_8BIT_APIC_ID);
which looks far more nice.
> +#ifdef CONFIG_X86_X2APIC > + if (cpu_has_x2apic && !x2apic) { > x2apic = 1; > enable_x2apic(); > + pr_info("Enabled x2apic\n"); > } > +#endif
ditto - this #ifdef could go away with the cpuflags trick.
> +ir_failed: > + if (x2apic_preenabled) > + panic("x2apic enabled by bios. But IR enabling failed");
What is the likelyhood that we can continue in compat mode? If there's some chance, we should rather print a KERN_WARNING and should try to continue. If IRQs are not coming we'll hang shortly afterwards anyway.
> panic("x2apic enabled prior OS handover," > - " enable CONFIG_INTR_REMAP");
ditto.
> +++ b/drivers/pci/intel-iommu.c > @@ -1968,15 +1968,6 @@ static int __init init_dmars(void) > } > } > > -#ifdef CONFIG_INTR_REMAP > - if (!intr_remapping_enabled) { > - ret = enable_intr_remapping(0); > - if (ret) > - printk(KERN_ERR > - "IOMMU: enable interrupt remapping failed\n"); > - } > -#endif
David, is this fine with you? Doing ir-remap setup in the ioapic code and early on is the obviously right thing to do.
> --- a/drivers/pci/intr_remapping.c > +++ b/drivers/pci/intr_remapping.c > @@ -423,20 +423,6 @@ static void iommu_set_intr_remapping(struct intel_iommu *iommu, int mode) > readl, (sts & DMA_GSTS_IRTPS), sts); > spin_unlock_irqrestore(&iommu->register_lock, flags); > > - if (mode == 0) { > - spin_lock_irqsave(&iommu->register_lock, flags); > - > - /* enable comaptiblity format interrupt pass through */
(this removal also fixes a typo ;-)
> - cmd = iommu->gcmd | DMA_GCMD_CFI; > - iommu->gcmd |= DMA_GCMD_CFI; > - writel(cmd, iommu->reg + DMAR_GCMD_REG); > - > - IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, > - readl, (sts & DMA_GSTS_CFIS), sts); > - > - spin_unlock_irqrestore(&iommu->register_lock, flags); > - }
Btw., switching on compat mode might be worthwile to do in one of the failure paths? I.e. we try to switch to IR mode but fail - we should then try to switch to compat pass-through instead of leaving the controller in limbo. Does it matter in your opinion?
> - > /* > * global invalidation of interrupt entry cache before enabling > * interrupt-remapping. > @@ -516,6 +502,20 @@ end: > spin_unlock_irqrestore(&iommu->register_lock, flags); > } > > +int __init intr_remapping_supported(void) > +{ > + struct dmar_drhd_unit *drhd; > + > + for_each_drhd_unit(drhd) { > + struct intel_iommu *iommu = drhd->iommu; > + > + if (!ecap_ir_support(iommu->ecap)) > + return 0; > + } > + > + return 1; > +}
Jesse, are these bits fine with you? The layering is still a bit incestous but it's a marked improvement over what we had there before.
Ingo
| |