[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 4 of 7] APIC: record local APIC state on boot



>>> On 14.06.11 at 12:48, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2011-06-14 at 09:57 +0100, Jan Beulich wrote:
>> >>> On 13.06.11 at 19:02, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> > Xen does not store the boot local APIC state which leads to problems
>> > when shutting down for a kexec jump.  This patch records the boot
>> > state so we can return to the boot state when kexecing.
>> > 
>> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> 
>> With a couple of minor adjustments (see inline comments),
>> 
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxxxx>
>> 
>> > diff -r 68efd418b6f1 -r 615db56671fa xen/arch/x86/apic.c
>> > --- a/xen/arch/x86/apic.c  Mon Jun 13 17:45:43 2011 +0100
>> > +++ b/xen/arch/x86/apic.c  Mon Jun 13 17:45:43 2011 +0100
>> > @@ -74,6 +74,8 @@ u8 __read_mostly apic_verbosity;
>> >  static bool_t __initdata opt_x2apic = 1;
>> >  boolean_param("x2apic", opt_x2apic);
>> >  
>> > +enum apic_mode apic_boot_mode = APIC_MODE_INVALID;
>> 
>> static ... __read_mostly ...
> 
> I think I suggested that Andrew remove the read mostly annotiation since
> this variable isn't really read mostly -- more of an access almost never
> variable (it's written on boot and only read on the kexec path).

Indeed, the "static" part is the more important one.

> Does optimising for this variable really help anything? I suspect
> (without evidence to backup my feeling) that over using this annotation
> will actually push actual frequently read __read_mostly variables into
> taking up more cache lines and actually hurt...

Agreed.

Jan

> Ian.
> 
>> > +
>> >  bool_t __read_mostly x2apic_enabled = 0;
>> >  bool_t __read_mostly directed_eoi_enabled = 0;
>> >  
>> > @@ -1438,6 +1440,62 @@ int __init APIC_init_uniprocessor (void)
>> >      return 0;
>> >  }
>> >  
>> > +/* Needs to be called during startup.  It records the state the BIOS
>> > + * leaves the local APIC so we can undo upon kexec.
>> > + */
>> > +void __init record_boot_APIC_mode(void)
>> > +{
>> > +    /* Sanity check - we should only ever run once, but could possibly
>> > +     * be called several times */
>> > +    if ( APIC_MODE_INVALID != apic_boot_mode )
>> > +        return;
>> > +
>> > +    apic_boot_mode = current_local_apic_mode();
>> > +
>> > +    apic_printk(APIC_DEBUG, "APIC boot state is '%s'\n",
>> > +                apic_mode_to_str(apic_boot_mode));
>> > +}
>> > +
>> > +/* Look at the bits in MSR_IA32_APICBASE and work out which
>> > + * APIC mode we are in */
>> > +enum apic_mode current_local_apic_mode(void)
>> 
>> Logically (within this patch) this function should also be static, but
>> I see it is being referenced by a later patch. Read on there.
>> 
>> > +{
>> > +    u64 msr_contents;
>> > +
>> > +    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) )
>> > +        return APIC_MODE_X2APIC;
>> > +
>> > +    /* EN bit should always be valid as long as we can read the MSR
>> > +     */
>> > +    if ( msr_contents & MSR_IA32_APICBASE_ENABLE )
>> > +        return APIC_MODE_XAPIC;
>> > +
>> > +    return APIC_MODE_DISABLED;
>> > +}
>> > +
>> > +
>> > +const char * apic_mode_to_str(const enum apic_mode mode)
>> 
>> static ... __init ...
>> 
>> Jan
>> 
>> > +{
>> > +    switch ( mode )
>> > +    {
>> > +        case APIC_MODE_INVALID:
>> > +            return "invalid";
>> > +        case APIC_MODE_DISABLED:
>> > +            return "disabled";
>> > +        case APIC_MODE_XAPIC:
>> > +            return "xapic";
>> > +        case APIC_MODE_X2APIC:
>> > +            return "x2apic";
>> > +        default:
>> > +            return "unrecognised";
>> > +    }
>> > +}
>> > +
>> >  void check_for_unexpected_msi(unsigned int vector)
>> >  {
>> >      unsigned long v = apic_read(APIC_ISR + ((vector & ~0x1f) >> 1));
>> > diff -r 68efd418b6f1 -r 615db56671fa xen/arch/x86/genapic/probe.c
>> > --- a/xen/arch/x86/genapic/probe.c Mon Jun 13 17:45:43 2011 +0100
>> > +++ b/xen/arch/x86/genapic/probe.c Mon Jun 13 17:45:43 2011 +0100
>> > @@ -60,6 +60,8 @@ void __init generic_apic_probe(void)
>> >  { 
>> >    int i, changed;
>> >  
>> > +  record_boot_APIC_mode();
>> > +
>> >    check_x2apic_preenabled();
>> >    cmdline_apic = changed = (genapic != NULL);
>> >  
>> > diff -r 68efd418b6f1 -r 615db56671fa xen/include/asm-x86/apic.h
>> > --- a/xen/include/asm-x86/apic.h   Mon Jun 13 17:45:43 2011 +0100
>> > +++ b/xen/include/asm-x86/apic.h   Mon Jun 13 17:45:43 2011 +0100
>> > @@ -21,6 +21,23 @@
>> >  #define IO_APIC_REDIR_DEST_LOGICAL        0x00800
>> >  #define IO_APIC_REDIR_DEST_PHYSICAL       0x00000
>> >  
>> > +/* Possible APIC states */
>> > +enum apic_mode { APIC_MODE_INVALID,  /* Not set yet */
>> > +                 APIC_MODE_DISABLED, /* If uniprocessor, or smp in
>> > +                                      * uniprocessor mode */
>> > +                 APIC_MODE_XAPIC,    /* xAPIC mode - default upon
>> > +                                      * chipset reset */
>> > +                 APIC_MODE_X2APIC    /* x2APIC mode - common for large
>> > +                                      * smp machines */
>> > +};
>> > +
>> > +/* Bootstrap processor local APIC boot mode - so we can undo our changes
>> > + * to the APIC state */
>> > +extern enum apic_mode apic_boot_mode;
>> > +
>> > +/* enum apic_mode -> str function for logging/debug */
>> > +const char * apic_mode_to_str(const enum apic_mode);
>> > +
>> >  extern u8 apic_verbosity;
>> >  extern bool_t x2apic_enabled;
>> >  extern bool_t directed_eoi_enabled;
>> > @@ -203,6 +220,8 @@ extern void disable_APIC_timer(void);
>> >  extern void enable_APIC_timer(void);
>> >  extern int lapic_suspend(void);
>> >  extern int lapic_resume(void);
>> > +extern void record_boot_APIC_mode(void);
>> > +extern enum apic_mode current_local_apic_mode(void);
>> >  
>> >  extern int check_nmi_watchdog (void);
>> >  
>> > 
>> > _______________________________________________
>> > 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 



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.