Re: "KEXEC: correctly revert x2apic state when kexecing" broken?

[Andrew Cooper <andrew.cooper3@citrix.com>]

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