From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: "KEXEC: correctly revert x2apic state when kexecing" broken? Date: Wed, 22 May 2013 18:36:51 +0100 Message-ID: <519D0233.300@citrix.com> References: <519CF16802000078000D8252@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <519CF16802000078000D8252@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org 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 > > --- 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(¤t_cpu_data, X86_FEATURE_X2APIC) > + if ( (system_state>= SYS_STATE_active ? > + cpu_has(¤t_cpu_data, X86_FEATURE_X2APIC) : > + boot_cpu_has(X86_FEATURE_X2APIC)) > && (msr_contents& MSR_IA32_APICBASE_EXTD) ) > return APIC_MODE_X2APIC; > > > >