Messages in this thread Patch in this message | | | Date | Sun, 24 Oct 2010 12:15:27 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 15/15] x86: Disabling x2apic if nox2apic is specified |
| |
On Sat, 23 Oct 2010, Yinghai Lu wrote:
> For > 1. x2apic preenabled system > 2. first kernel have x2apic enabled, and try to boot second kernel with "nox2apic" > > Will put back cpu with apic id < 255 into xapic mode, instead of panic. > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > --- > arch/x86/include/asm/apic.h | 6 ++++ > arch/x86/include/asm/apicdef.h | 1 + > arch/x86/kernel/acpi/boot.c | 10 ++++++- > arch/x86/kernel/apic/apic.c | 54 +++++++++++++++++++++++++++++++-------- > arch/x86/mm/srat_64.c | 12 ++++++++- > 5 files changed, 69 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h > index 69879dd..522f39b 100644 > --- a/arch/x86/include/asm/apic.h > +++ b/arch/x86/include/asm/apic.h > @@ -176,6 +176,7 @@ static inline u64 native_x2apic_icr_read(void) > } > > extern int x2apic_phys; > +extern int nox2apic;
Can you please use a sensible variable name like x2apic_disabled ?
> extern void check_x2apic(void); > extern void enable_x2apic(void); > extern void x2apic_icr_write(u32 low, u32 id); > @@ -186,6 +187,10 @@ static inline int x2apic_enabled(void) > if (!cpu_has_x2apic) > return 0; > > + /* avoid to read msr */
That comment is useless. I wish you would add comments to complex code not to obvious one.
Also it can be folded into the above check
if (!cpu_has_x2apic || x2apic_disabled)
> + if (nox2apic) > + return 0; > + > rdmsr(MSR_IA32_APICBASE, msr, msr2); > if (msr & X2APIC_ENABLE) > return 1;
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index d286db1..ebb13e8 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -138,15 +138,14 @@ int x2apic_mode; > #ifdef CONFIG_X86_X2APIC > /* x2apic enabled before OS handover */ > static int x2apic_preenabled; > +int nox2apic; > static __init int setup_nox2apic(char *str) > { > - if (x2apic_enabled()) { > - pr_warning("Bios already enabled x2apic, " > - "can't enforce nox2apic"); > - return 0; > - } > + if (x2apic_enabled()) > + pr_warning("Bios already enabled x2apic, will disable it"); > + > + nox2apic = 1; > > - setup_clear_cpu_cap(X86_FEATURE_X2APIC);
Why is this removed ?
> return 0; > } > early_param("nox2apic", setup_nox2apic); > @@ -1393,8 +1392,33 @@ void __cpuinit end_local_APIC_setup(void) > } > > #ifdef CONFIG_X86_X2APIC > + > +static void disable_x2apic(void) > +{ > + int msr, msr2; > + > + if (!cpu_has_x2apic) > + return; > + > + rdmsr(MSR_IA32_APICBASE, msr, msr2); > + if (msr & X2APIC_ENABLE) { > + pr_info("Disabling x2apic\n"); > + /* > + * Need to disable xapic and x2apic at the same time at first > + * then enable xapic > + */ > + wrmsr(MSR_IA32_APICBASE, msr & ~(X2APIC_ENABLE | XAPIC_ENABLE), > + 0); > + wrmsr(MSR_IA32_APICBASE, msr & ~X2APIC_ENABLE, 0); > + } > +} > void check_x2apic(void) > { > + if (nox2apic) { > + disable_x2apic(); > + return; > + } > + > if (x2apic_enabled()) { > pr_info("x2apic enabled by BIOS, switching to x2apic ops\n"); > x2apic_preenabled = x2apic_mode = 1; > @@ -1405,6 +1429,11 @@ void enable_x2apic(void) > { > int msr, msr2; > > + if (nox2apic) { > + disable_x2apic(); > + return; > + } > + > if (!x2apic_mode) > return; > > @@ -1430,7 +1459,7 @@ int __init enable_IR(void) > return 0; > } > > - if (enable_intr_remapping(x2apic_supported())) > + if (enable_intr_remapping(x2apic_supported() && !nox2apic))
Do we really need all these extra checks ? Can't we simply make all this one variable wich is set to 1 when x2apic is available and not disabled on the kernel command line ?
> diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c > index a35cb9d..baa9eab 100644 > --- a/arch/x86/mm/srat_64.c > +++ b/arch/x86/mm/srat_64.c > @@ -126,6 +126,13 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa) > if ((pa->flags & ACPI_SRAT_CPU_ENABLED) == 0) > return; > pxm = pa->proximity_domain; > + apic_id = pa->apic_id; > +#ifdef CONFIG_X86_X2APIC
Why conditional? No need to duplicate the printk. It just needs some thought.
> + if (nox2apic && (apic_id >= 0xff)) { > + printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n", > + pxm, apic_id); > + return; > + } > node = setup_node(pxm); > if (node < 0) { > printk(KERN_ERR "SRAT: Too many proximity domains %x\n", pxm); > @@ -133,12 +140,15 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa) > return; > } > > - apic_id = pa->apic_id; > apicid_to_node[apic_id] = node; > node_set(node, cpu_nodes_parsed); > acpi_numa = 1; > printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u\n", > pxm, apic_id, node); > +#else > + printk(KERN_INFO "SRAT: PXM %u -> X2APIC 0x%04x ignored\n", > + pxm, apic_id); > +#endif > } > > /* Callback for Proximity Domain -> LAPIC mapping */ > -- > 1.7.1 >
| |