WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

<Prev in Thread] Current Thread [Next in Thread>