lkml.org 
[lkml]   [2009]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2009-04-17 16:15    [W:4.743 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site