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).
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...
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(¤t_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
|
Home