From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 2 of 3] apic: remove 'enabled_via_apicbase' variable Date: Wed, 18 May 2011 14:53:39 -0400 Message-ID: <20110518185339.GD14013@dumpdata.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Andrew Cooper Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Wed, May 18, 2011 at 07:08:15PM +0100, Andrew Cooper wrote: > The use of this varable was only sensible when the choice for lapic mode variable > was between enabled or disabled. Now that x2apic is about, it is wrong, > and causes a protection fault in certain cases when trying to tare down tare? > x2apic mode. > > The only place where its use is relevent in the code is in disable_local_APIC ^- is ^^^ take that out. > which has been changed to correctly tare down the local APIC without a teardown? > protection fault (which leads to a general protection fault). So if you don't have x2apic, then it is wrong to disable the LAPIC mode? What about older hardware? > > Signed-off-by: Andrew Cooper > > diff -r 62a8ce6595ad -r e80b5280fe2f xen/arch/x86/apic.c > --- a/xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 > +++ b/xen/arch/x86/apic.c Wed May 18 19:00:13 2011 +0100 > @@ -165,8 +165,6 @@ void __init apic_intr_init(void) > /* Using APIC to generate smp_local_timer_interrupt? */ > static bool_t __read_mostly using_apic_timer; > > -static bool_t __read_mostly enabled_via_apicbase; > - > int get_physical_broadcast(void) > { > if (modern_apic()) > @@ -330,6 +328,9 @@ void disconnect_bsp_APIC(int virt_wire_s > > void disable_local_APIC(void) > { > + u64 msr_contents; > + enum apic_mode current_mode; > + > clear_local_APIC(); > > /* > @@ -339,10 +340,54 @@ void disable_local_APIC(void) > apic_write_around(APIC_SPIV, > apic_read(APIC_SPIV) & ~APIC_SPIV_APIC_ENABLED); > > - if (enabled_via_apicbase) { > - uint64_t msr_content; > - rdmsrl(MSR_IA32_APICBASE, msr_content); > - wrmsrl(MSR_IA32_APICBASE, msr_content & ~MSR_IA32_APICBASE_ENABLE); > + /* Work out what apic mode we are in */ > + rdmsrl(MSR_IA32_APICBASE, msr_contents); > + > + /* Reading EXTD bit from the MSR is only valid if CPUID says so, else reserved */ > + if ( cpu_has(¤t_cpu_data, X86_FEATURE_X2APIC) > + && (msr_contents & MSR_IA32_APICBASE_EXTD) ) > + current_mode = APIC_MODE_X2APIC; > + else > + { > + /* EN bit should always be valid as long as we can read the MSR > + * Can anyone confirm this? Might want to email Vivek Goyal.. > + */ > + if ( msr_contents & MSR_IA32_APICBASE_ENABLE ) > + current_mode = APIC_MODE_XAPIC; > + else > + current_mode = APIC_MODE_DISABLED; > + } > + > + /* See what (if anything) we need to do to revert to boot mode */ > + if( current_mode != this_cpu(apic_boot_mode) ) > + { > + rdmsrl(MSR_IA32_APICBASE, msr_contents); > + msr_contents &= ~ ( MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD ); > + wrmsrl(MSR_IA32_APICBASE, msr_contents); > + > + switch(this_cpu(apic_boot_mode)) > + { > + case APIC_MODE_DISABLED: > + break; /* Nothing to do - we did this above */ > + case APIC_MODE_XAPIC: > + { > + msr_contents |= MSR_IA32_APICBASE_ENABLE; > + wrmsrl(MSR_IA32_APICBASE, msr_contents); > + break; > + } > + case APIC_MODE_X2APIC: > + { > + msr_contents |= ( MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD ); > + wrmsrl(MSR_IA32_APICBASE, msr_contents); > + break; > + } > + default: > + { > + printk("Hit default case when reverting lapic to boot state on core #%d\n", > + smp_processor_id()); > + break; > + } > + } > } > } > > @@ -874,7 +919,6 @@ static int __init detect_init_APIC (void > wrmsrl(MSR_IA32_APICBASE, > msr_content | MSR_IA32_APICBASE_ENABLE > | APIC_DEFAULT_PHYS_BASE); > - enabled_via_apicbase = 1; > } > } > /* > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel