Re: [Xen-devel] [PATCH 2 of 3] apic: remove 'enabled_via_apicbase' variable

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/05/11 19:53, Konrad Rzeszutek Wilk wrote:
> 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?
I guess I wasn't very clear in my description.  In older hardware 
without x2apic, it is correct to simply twiddle the ENABLE bit in the 
APICBASE MSR.  However, with x2apic mode enabled, setting the ENABLE bit 
from 1 to 0 while leaving the EXTD bit set will result in a protection 
fault which will propagate to a general protection fault the same 
codepath will be called in the fault handler.  As a result, the current 
code in disable_local_APIC will result in a GPF if the BIOS boots with 
LAPICs disabled (fine as per the spec for compatibility) and xen decided 
to take advantage of x2apic mode.
> > 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(&current_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
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel