RE: [Xen-devel] [PATCH]x86:x2apic: Disable x2apic on x86-32 permanently

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> On 09.02.11 at 09:43, "Wei, Gang" <gang.wei@xxxxxxxxx> wrote:
> Jan Beulich wrote on 2011-02-09:
>>>>> On 09.02.11 at 07:57, "Wei, Gang" <gang.wei@xxxxxxxxx> wrote:
>>> Jan Beulich wrote on 2011-01-31:
>>>> How does this (namely the x2apic_enabled part) play together with
>>>> the selection of the APIC driver, which in this case would have
>>>> happened quite a bit earlier (from generic_apic_probe())?
>>>> 
>>>> I would therefore think that this change really belongs into
>>>> check_x2apic_preenabled().
>>> 
>>> It is really a problem. But I do think we shall simply remove below
>>> line from check_x2apic_preenabled() fn:
>>> 
>>>          genapic = apic_x2apic_probe();
>>> The same line exists in x2apic_bsp_setup() fn.
>> 
>> No, that would be exactly the wrong way round - we need genapic to be
>> set early in case x2apic was pre-enabled (see -unstable c/s 22707).
> 
> I could not get the reason to set genapic as x2apic in case x2apic was 
> pre-enabled while referring to c/s 22707. Genapic->xxx was never called 
> before 
> calling x2apic_bsp_setup(). Are you trying to keep consistence (x2apic 
> pre-enabled, so genapic have to be x2apic as early as possible)?

Without setting genapic here, generic_apic_probe() would set it
to &apic_default, which is obviously wrong (even if it later gets
overridden) - just look at the various APIC driver related
messages that can result on such a system (if you're trying to
analyze a problem, seeing the APIC driver change perhaps
multiple time in the log is certainly not helpful).

Apart from that - what problem do you see with just moving the
change you did into check_x2apic_preenabled()? We're using
below patch on 4.0.x as replacement/extension to your
-unstable c/s 22789 (i.e. a patch against current -unstable would
need to revert some of what your change did).

Jan

****************************************************

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -958,6 +958,10 @@ void x2apic_setup(void)
     if ( !cpu_has_x2apic )
         return;
 
+#ifdef __i386__
+    BUG();
+#else
+
     if ( !opt_x2apic )
     {
         if ( !x2apic_enabled )
@@ -1019,6 +1023,7 @@ restore_out:
     unmask_8259A();
 
 out:
+#endif /* !__i386__ */
     if ( ioapic_entries )
         free_ioapic_entries(ioapic_entries);
 }
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -25,6 +25,8 @@
 #include <xen/smp.h>
 #include <asm/mach-default/mach_mpparse.h>
 
+#ifndef __i386__
+
 static int x2apic_phys; /* By default we use logical cluster mode. */
 boolean_param("x2apic_phys", x2apic_phys);
 
@@ -137,6 +139,8 @@ const struct genapic *apic_x2apic_probe(
     return x2apic_phys ? &apic_x2apic_phys : &apic_x2apic_cluster;
 }
 
+#endif /* !__i386__ */
+
 void __init check_x2apic_preenabled(void)
 {
     u32 lo, hi;
@@ -149,7 +153,19 @@ void __init check_x2apic_preenabled(void
     if ( lo & MSR_IA32_APICBASE_EXTD )
     {
         printk("x2APIC mode is already enabled by BIOS.\n");
+#ifndef __i386__
         x2apic_enabled = 1;
         genapic = apic_x2apic_probe();
+#else
+        lo &= ~(MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD);
+        wrmsr(MSR_IA32_APICBASE, lo, hi);
+        lo |= MSR_IA32_APICBASE_ENABLE;
+        wrmsr(MSR_IA32_APICBASE, lo, hi);
+        printk("x2APIC disabled permanently on x86_32.\n");
+#endif
     }
+
+#ifdef __i386__
+    clear_bit(X86_FEATURE_X2APIC, boot_cpu_data.x86_capability);
+#endif
 }



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel