[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] "KEXEC: correctly revert x2apic state when kexecing" broken?



On 22/05/13 15:25, Jan Beulich wrote:
Andrew,

we just got a report of a crash that very much looks like an effect
from this patch. In particular, by reverting to xAPIC mode,
disconnect_bsp_APIC() (called via disable_IO_APIC(), getting
invoked _after_ __stop_this_cpu(), which is what causes the fiddling
with the APIC mode by calling disable_local_APIC()) will use MMIO
accesses to the APIC page, yet that one would never have got
mapped by init_apic_mappings() if x2apic_enabled got set to
non-zero before.

The problem appears to be the use of current_cpu_data by
current_local_apic_mode(): The former becomes valid only in the
call tree coming through smp_prepare_cpus(), which is _long_
after generic_apic_probe(). Was that patch really tested on a
system with x2APIC pre-enabled?

I cant remember for certain. I think it was just on systems which turned on x2apic and left hardware set up, causing interrupt format errors for the kdump kernel.

I would certainly agree with your assessment of the problem


In any case - thoughts about the patch below? I wonder whether
always using boot_cpu_data wouldn't even be better (considering
that we do so for various other features too).

I would just go straight for boot_cpu_data. Most other things in __init functions do.

~Andrew


Jan

x86: fix boot time APIC mode detection

current_cpu_data becomes valid only relatively late in the boot
process, so looking there for a particular feature early in the game
would generally give the appearance of the feature being unavailable.

Getting this wrong means that at kexec time the system would get
returned to xAPIC mode, causing disconnect_bsp_APIC() to try to access
the APIC page, which on systems with x2APIC pre-enabled will never get
set up.

Signed-off-by: Jan Beulich<jbeulich@xxxxxxxx>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1474,7 +1474,9 @@ enum apic_mode current_local_apic_mode(v

      /* Reading EXTD bit from the MSR is only valid if CPUID
       * says so, else reserved */
-    if ( cpu_has(&current_cpu_data, X86_FEATURE_X2APIC)
+    if ( (system_state>= SYS_STATE_active ?
+          cpu_has(&current_cpu_data, X86_FEATURE_X2APIC) :
+          boot_cpu_has(X86_FEATURE_X2APIC))
           &&  (msr_contents&  MSR_IA32_APICBASE_EXTD) )
          return APIC_MODE_X2APIC;






_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.