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 <andrew.cooper3@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home