Xen devel - May 2013 - "KEXEC: correctly revert x2apic state when kexecing" broken?

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?

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).

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@suse.com>

--- 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;

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@suse.com>
>
> --- 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;
>
>
>
>