* [PATCH] x86,apic: V2 Add apic calibration hook. @ 2010-09-20 21:17 Alok Kataria 2010-09-20 22:11 ` Thomas Gleixner 0 siblings, 1 reply; 4+ messages in thread From: Alok Kataria @ 2010-09-20 21:17 UTC (permalink / raw) To: Thomas Gleixner, H. Peter Anvin, Ingo Molnar Cc: the arch/x86 maintainers, LKML Hi, I am reposting the version 2 of the apic calibration hook from the original thread [1]. Please note that I never heard back anything on this version, any review comments are much appreciated. If all looks good, please consider for inclusion. Patch applies on the tip tree. Thanks, Alok [1] http://lkml.org/lkml/2010/8/27/421 -- Add apic calibration hook. From: Alok N Kataria <akataria@vmware.com> Add a new function ptr calibrate_apic to x86_init_ops structure. On native this does the usual apic calibration. On VMware's platform we override it with a routine which gets that information from the hypervisor. Signed-off-by: Alok N Kataria <akataria@vmware.com> Cc: H. Peter Anvin <hpa@zytor.com> --- arch/x86/include/asm/apic.h | 6 +++ arch/x86/include/asm/x86_init.h | 2 + arch/x86/kernel/apic/apic.c | 88 ++++++++++++++++++++------------------- arch/x86/kernel/cpu/vmware.c | 30 ++++++++++++- arch/x86/kernel/x86_init.c | 1 5 files changed, 82 insertions(+), 45 deletions(-) diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h index 1fa03e0..5012ee5 100644 --- a/arch/x86/include/asm/apic.h +++ b/arch/x86/include/asm/apic.h @@ -17,6 +17,10 @@ #define ARCH_APICTIMER_STOPS_ON_C3 1 +/* Clock divisor */ +#define APIC_DIVISOR 16 +#define LAPIC_CAL_LOOPS (HZ/10) + /* * Debugging macros */ @@ -238,6 +242,8 @@ extern void setup_boot_APIC_clock(void); extern void setup_secondary_APIC_clock(void); extern int APIC_init_uniprocessor(void); extern void enable_NMI_through_LVT0(void); +extern int native_calibrate_apic(void); +extern int initialize_lapic(long delta, unsigned int calibration_result); /* * On 32bit this is mach-xxx local diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h index baa579c..58f982b 100644 --- a/arch/x86/include/asm/x86_init.h +++ b/arch/x86/include/asm/x86_init.h @@ -83,11 +83,13 @@ struct x86_init_paging { * boot cpu * @tsc_pre_init: platform function called before TSC init * @timer_init: initialize the platform timer (default PIT/HPET) + * @calibrate_apic: calibrate APIC bus freq (ticks per HZ) */ struct x86_init_timers { void (*setup_percpu_clockev)(void); void (*tsc_pre_init)(void); void (*timer_init)(void); + int (*calibrate_apic)(void); }; /** diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index e3b534c..ca5d084 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -176,7 +176,7 @@ static struct resource lapic_resource = { .flags = IORESOURCE_MEM | IORESOURCE_BUSY, }; -static unsigned int calibration_result; +static unsigned int apic_calibration_res; static int lapic_next_event(unsigned long delta, struct clock_event_device *evt); @@ -326,13 +326,6 @@ int lapic_get_maxlvt(void) } /* - * Local APIC timer - */ - -/* Clock divisor */ -#define APIC_DIVISOR 16 - -/* * This function sets up the local APIC timer, with a timeout of * 'clocks' APIC bus clock. During calibration we actually call * this function twice on the boot CPU, once with a bogus timeout @@ -431,7 +424,7 @@ static void lapic_timer_setup(enum clock_event_mode mode, switch (mode) { case CLOCK_EVT_MODE_PERIODIC: case CLOCK_EVT_MODE_ONESHOT: - __setup_APIC_LVTT(calibration_result, + __setup_APIC_LVTT(apic_calibration_res, mode != CLOCK_EVT_MODE_PERIODIC, 1); break; case CLOCK_EVT_MODE_UNUSED: @@ -500,8 +493,6 @@ static void __cpuinit setup_APIC_timer(void) * back to normal later in the boot process). */ -#define LAPIC_CAL_LOOPS (HZ/10) - static __initdata int lapic_cal_loops = -1; static __initdata long lapic_cal_t1, lapic_cal_t2; static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2; @@ -590,13 +581,50 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc) return 0; } -static int __init calibrate_APIC_clock(void) +int __init initialize_lapic(long delta, unsigned int calibration_result) +{ + struct clock_event_device *levt = &__get_cpu_var(lapic_events); + + /* Calculate the scaled math multiplication factor */ + lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, + lapic_clockevent.shift); + lapic_clockevent.max_delta_ns = + clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); + lapic_clockevent.min_delta_ns = + clockevent_delta2ns(0xF, &lapic_clockevent); + + apic_calibration_res = calibration_result; + + apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); + apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); + apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", + calibration_result); + + apic_printk(APIC_VERBOSE, "..... host bus clock speed is " + "%u.%04u MHz.\n", + calibration_result / (1000000 / HZ), + calibration_result % (1000000 / HZ)); + + /* + * Do a sanity check on the APIC calibration result + */ + if (calibration_result < (1000000 / HZ)) { + pr_warning("APIC frequency too slow, disabling apic timer\n"); + return -1; + } + + levt->features &= ~CLOCK_EVT_FEAT_DUMMY; + return 0; +} + +int __init native_calibrate_apic(void) { struct clock_event_device *levt = &__get_cpu_var(lapic_events); void (*real_handler)(struct clock_event_device *dev); unsigned long deltaj; long delta, deltatsc; int pm_referenced = 0; + unsigned int calibration_result; local_irq_disable(); @@ -631,20 +659,12 @@ static int __init calibrate_APIC_clock(void) pm_referenced = !calibrate_by_pmtimer(lapic_cal_pm2 - lapic_cal_pm1, &delta, &deltatsc); - /* Calculate the scaled math multiplication factor */ - lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, - lapic_clockevent.shift); - lapic_clockevent.max_delta_ns = - clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); - lapic_clockevent.min_delta_ns = - clockevent_delta2ns(0xF, &lapic_clockevent); - calibration_result = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS; - apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); - apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); - apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", - calibration_result); + if (initialize_lapic(delta, calibration_result)) { + local_irq_enable(); + return -1; + } if (cpu_has_tsc) { apic_printk(APIC_VERBOSE, "..... CPU clock speed is " @@ -653,22 +673,6 @@ static int __init calibrate_APIC_clock(void) (deltatsc / LAPIC_CAL_LOOPS) % (1000000 / HZ)); } - apic_printk(APIC_VERBOSE, "..... host bus clock speed is " - "%u.%04u MHz.\n", - calibration_result / (1000000 / HZ), - calibration_result % (1000000 / HZ)); - - /* - * Do a sanity check on the APIC calibration result - */ - if (calibration_result < (1000000 / HZ)) { - local_irq_enable(); - pr_warning("APIC frequency too slow, disabling apic timer\n"); - return -1; - } - - levt->features &= ~CLOCK_EVT_FEAT_DUMMY; - /* * PM timer calibration failed or not turned on * so lets try APIC timer based calibration @@ -706,7 +710,7 @@ static int __init calibrate_APIC_clock(void) if (levt->features & CLOCK_EVT_FEAT_DUMMY) { pr_warning("APIC timer disabled due to verification failure\n"); - return -1; + return -1; } return 0; @@ -738,7 +742,7 @@ void __init setup_boot_APIC_clock(void) apic_printk(APIC_VERBOSE, "Using local APIC timer interrupts.\n" "calibrating APIC timer ...\n"); - if (calibrate_APIC_clock()) { + if (x86_init.timers.calibrate_apic()) { /* No broadcast on UP ! */ if (num_possible_cpus() > 1) setup_APIC_timer(); diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c index 227b044..e8b760e 100644 --- a/arch/x86/kernel/cpu/vmware.c +++ b/arch/x86/kernel/cpu/vmware.c @@ -26,6 +26,7 @@ #include <asm/div64.h> #include <asm/x86_init.h> #include <asm/hypervisor.h> +#include <asm/apic.h> #define CPUID_VMWARE_INFO_LEAF 0x40000000 #define VMWARE_HYPERVISOR_MAGIC 0x564D5868 @@ -72,17 +73,40 @@ static unsigned long vmware_get_tsc_khz(void) return tsc_hz; } +static int __init vmware_calibrate_apic(void) +{ + unsigned int calibration_result; + uint32_t eax, ebx, ecx, edx; + int err; + long delta; + unsigned long flags; + + VMWARE_PORT(GETHZ, eax, ebx, ecx, edx); + BUG_ON(!ecx); + calibration_result = ecx / HZ; + + printk(KERN_INFO "APIC bus freq read from hypervisor\n"); + + delta = (calibration_result * LAPIC_CAL_LOOPS) / APIC_DIVISOR; + local_irq_save(flags); + err = initialize_lapic(delta, calibration_result); + local_irq_restore(flags); + + return err; +} + static void __init vmware_platform_setup(void) { uint32_t eax, ebx, ecx, edx; VMWARE_PORT(GETHZ, eax, ebx, ecx, edx); - if (ebx != UINT_MAX) + if (ebx != UINT_MAX) { x86_platform.calibrate_tsc = vmware_get_tsc_khz; - else + x86_init.timers.calibrate_apic = vmware_calibrate_apic; + } else printk(KERN_WARNING - "Failed to get TSC freq from the hypervisor\n"); + "Failed to setup VMware hypervisor platform\n"); } /* diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c index cd6da6b..f162e14 100644 --- a/arch/x86/kernel/x86_init.c +++ b/arch/x86/kernel/x86_init.c @@ -68,6 +68,7 @@ struct x86_init_ops x86_init __initdata = { .setup_percpu_clockev = setup_boot_APIC_clock, .tsc_pre_init = x86_init_noop, .timer_init = hpet_time_init, + .calibrate_apic = native_calibrate_apic }, .iommu = { ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86,apic: V2 Add apic calibration hook. 2010-09-20 21:17 [PATCH] x86,apic: V2 Add apic calibration hook Alok Kataria @ 2010-09-20 22:11 ` Thomas Gleixner 2010-09-20 23:34 ` Alok Kataria 0 siblings, 1 reply; 4+ messages in thread From: Thomas Gleixner @ 2010-09-20 22:11 UTC (permalink / raw) To: Alok Kataria; +Cc: H. Peter Anvin, Ingo Molnar, the arch/x86 maintainers, LKML On Mon, 20 Sep 2010, Alok Kataria wrote: > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h > index 1fa03e0..5012ee5 100644 > --- a/arch/x86/include/asm/apic.h > +++ b/arch/x86/include/asm/apic.h > @@ -17,6 +17,10 @@ > > #define ARCH_APICTIMER_STOPS_ON_C3 1 > > +/* Clock divisor */ > +#define APIC_DIVISOR 16 > +#define LAPIC_CAL_LOOPS (HZ/10) > + What's the point of this ? Oh wait, I see. Instead of fixing the apic code to use some useful units for the calibration value, you make those constants global and twiddle the (hopefully sane) return value of your hypervirus call. > /* > * Debugging macros > */ > @@ -238,6 +242,8 @@ extern void setup_boot_APIC_clock(void); > extern void setup_secondary_APIC_clock(void); > extern int APIC_init_uniprocessor(void); > extern void enable_NMI_through_LVT0(void); > +extern int native_calibrate_apic(void); > +extern int initialize_lapic(long delta, unsigned int calibration_result); Interesting, That function is initializing the local apic? Hmm, the code below makes me think it's about the local apic timer. > > /* > * On 32bit this is mach-xxx local > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h > index baa579c..58f982b 100644 > --- a/arch/x86/include/asm/x86_init.h > +++ b/arch/x86/include/asm/x86_init.h > @@ -83,11 +83,13 @@ struct x86_init_paging { > * boot cpu > * @tsc_pre_init: platform function called before TSC init > * @timer_init: initialize the platform timer (default PIT/HPET) > + * @calibrate_apic: calibrate APIC bus freq (ticks per HZ) This is not calibrating the APIC, it's calibrating the APIC timer, right ? AFAICT, you also override the TSC calibration, right? I wonder whether we could finally avoid the duplicate calibration stuff and do the TSC and the lapic together and avoid the second calibration loop. > */ > struct x86_init_timers { > void (*setup_percpu_clockev)(void); > void (*tsc_pre_init)(void); > void (*timer_init)(void); > + int (*calibrate_apic)(void); > }; > > /** > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index e3b534c..ca5d084 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -176,7 +176,7 @@ static struct resource lapic_resource = { > .flags = IORESOURCE_MEM | IORESOURCE_BUSY, > }; > > -static unsigned int calibration_result; > +static unsigned int apic_calibration_res; calibration_result is not the best choice, but please split out this cosmetic change into a separate patch. The patch probaly wants some more splitup for sanity and bisectability reasons./ > > static int lapic_next_event(unsigned long delta, > struct clock_event_device *evt); > @@ -326,13 +326,6 @@ int lapic_get_maxlvt(void) > } > > /* > - * Local APIC timer > - */ > - > -/* Clock divisor */ > -#define APIC_DIVISOR 16 > - > -/* > * This function sets up the local APIC timer, with a timeout of > * 'clocks' APIC bus clock. During calibration we actually call > * this function twice on the boot CPU, once with a bogus timeout > @@ -431,7 +424,7 @@ static void lapic_timer_setup(enum clock_event_mode mode, > switch (mode) { > case CLOCK_EVT_MODE_PERIODIC: > case CLOCK_EVT_MODE_ONESHOT: > - __setup_APIC_LVTT(calibration_result, > + __setup_APIC_LVTT(apic_calibration_res, > mode != CLOCK_EVT_MODE_PERIODIC, 1); > break; > case CLOCK_EVT_MODE_UNUSED: > @@ -500,8 +493,6 @@ static void __cpuinit setup_APIC_timer(void) > * back to normal later in the boot process). > */ > > -#define LAPIC_CAL_LOOPS (HZ/10) > - > static __initdata int lapic_cal_loops = -1; > static __initdata long lapic_cal_t1, lapic_cal_t2; > static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2; > @@ -590,13 +581,50 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc) > return 0; > } > > -static int __init calibrate_APIC_clock(void) > +int __init initialize_lapic(long delta, unsigned int calibration_result) See above > +{ > + struct clock_event_device *levt = &__get_cpu_var(lapic_events); > + > + /* Calculate the scaled math multiplication factor */ > + lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, > + lapic_clockevent.shift); > + lapic_clockevent.max_delta_ns = > + clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); > + lapic_clockevent.min_delta_ns = > + clockevent_delta2ns(0xF, &lapic_clockevent); > + > + apic_calibration_res = calibration_result; > + > + apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); > + apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); > + apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", > + calibration_result); > + > + apic_printk(APIC_VERBOSE, "..... host bus clock speed is " > + "%u.%04u MHz.\n", > + calibration_result / (1000000 / HZ), > + calibration_result % (1000000 / HZ)); > + > + /* > + * Do a sanity check on the APIC calibration result > + */ > + if (calibration_result < (1000000 / HZ)) { > + pr_warning("APIC frequency too slow, disabling apic timer\n"); > + return -1; > + } > + > + levt->features &= ~CLOCK_EVT_FEAT_DUMMY; > + return 0; > +} > + > +int __init native_calibrate_apic(void) > { > struct clock_event_device *levt = &__get_cpu_var(lapic_events); > void (*real_handler)(struct clock_event_device *dev); > unsigned long deltaj; > long delta, deltatsc; > int pm_referenced = 0; > + unsigned int calibration_result; > > local_irq_disable(); > > @@ -631,20 +659,12 @@ static int __init calibrate_APIC_clock(void) > pm_referenced = !calibrate_by_pmtimer(lapic_cal_pm2 - lapic_cal_pm1, > &delta, &deltatsc); > > - /* Calculate the scaled math multiplication factor */ > - lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, > - lapic_clockevent.shift); > - lapic_clockevent.max_delta_ns = > - clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); > - lapic_clockevent.min_delta_ns = > - clockevent_delta2ns(0xF, &lapic_clockevent); > - > calibration_result = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS; > > - apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); > - apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); > - apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", > - calibration_result); > + if (initialize_lapic(delta, calibration_result)) { > + local_irq_enable(); > + return -1; > + } > > if (cpu_has_tsc) { > apic_printk(APIC_VERBOSE, "..... CPU clock speed is " > @@ -653,22 +673,6 @@ static int __init calibrate_APIC_clock(void) > (deltatsc / LAPIC_CAL_LOOPS) % (1000000 / HZ)); > } > > - apic_printk(APIC_VERBOSE, "..... host bus clock speed is " > - "%u.%04u MHz.\n", > - calibration_result / (1000000 / HZ), > - calibration_result % (1000000 / HZ)); > - > - /* > - * Do a sanity check on the APIC calibration result > - */ > - if (calibration_result < (1000000 / HZ)) { > - local_irq_enable(); > - pr_warning("APIC frequency too slow, disabling apic timer\n"); > - return -1; > - } > - > - levt->features &= ~CLOCK_EVT_FEAT_DUMMY; > - > /* > * PM timer calibration failed or not turned on > * so lets try APIC timer based calibration > @@ -706,7 +710,7 @@ static int __init calibrate_APIC_clock(void) > > if (levt->features & CLOCK_EVT_FEAT_DUMMY) { > pr_warning("APIC timer disabled due to verification failure\n"); > - return -1; > + return -1; > } > > return 0; > @@ -738,7 +742,7 @@ void __init setup_boot_APIC_clock(void) > apic_printk(APIC_VERBOSE, "Using local APIC timer interrupts.\n" > "calibrating APIC timer ...\n"); > > - if (calibrate_APIC_clock()) { > + if (x86_init.timers.calibrate_apic()) { > /* No broadcast on UP ! */ > if (num_possible_cpus() > 1) > setup_APIC_timer(); > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c > index 227b044..e8b760e 100644 > --- a/arch/x86/kernel/cpu/vmware.c > +++ b/arch/x86/kernel/cpu/vmware.c > @@ -26,6 +26,7 @@ > #include <asm/div64.h> > #include <asm/x86_init.h> > #include <asm/hypervisor.h> > +#include <asm/apic.h> > > #define CPUID_VMWARE_INFO_LEAF 0x40000000 > #define VMWARE_HYPERVISOR_MAGIC 0x564D5868 > @@ -72,17 +73,40 @@ static unsigned long vmware_get_tsc_khz(void) > return tsc_hz; > } > > +static int __init vmware_calibrate_apic(void) > +{ > + unsigned int calibration_result; > + uint32_t eax, ebx, ecx, edx; > + int err; > + long delta; > + unsigned long flags; > + > + VMWARE_PORT(GETHZ, eax, ebx, ecx, edx); > + BUG_ON(!ecx); > + calibration_result = ecx / HZ; > + > + printk(KERN_INFO "APIC bus freq read from hypervisor\n"); > + > + delta = (calibration_result * LAPIC_CAL_LOOPS) / APIC_DIVISOR; Sigh. > + local_irq_save(flags); > + err = initialize_lapic(delta, calibration_result); Crap. You're function override is about calibration and not about initializing. Why are you calling that here. Either you override the local apic timer setup completely or just the calibration routine. Thanks, tglx ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86,apic: V2 Add apic calibration hook. 2010-09-20 22:11 ` Thomas Gleixner @ 2010-09-20 23:34 ` Alok Kataria 2010-09-20 23:59 ` Alok Kataria 0 siblings, 1 reply; 4+ messages in thread From: Alok Kataria @ 2010-09-20 23:34 UTC (permalink / raw) To: Thomas Gleixner Cc: H. Peter Anvin, Ingo Molnar, the arch/x86 maintainers, LKML Hi Thomas, Thanks for taking a look. Please find my replies below. On Mon, 2010-09-20 at 15:11 -0700, Thomas Gleixner wrote: > On Mon, 20 Sep 2010, Alok Kataria wrote: > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h > > index 1fa03e0..5012ee5 100644 > > --- a/arch/x86/include/asm/apic.h > > +++ b/arch/x86/include/asm/apic.h > > @@ -17,6 +17,10 @@ > > > > #define ARCH_APICTIMER_STOPS_ON_C3 1 > > > > +/* Clock divisor */ > > +#define APIC_DIVISOR 16 > > +#define LAPIC_CAL_LOOPS (HZ/10) > > + > > What's the point of this ? Oh wait, I see. Instead of fixing the > apic code to use some useful units for the calibration value, you > make those constants global and twiddle the (hopefully sane) return > value of your hypervirus call. Those constants are used just to calculate the delta value which is used to print useful debug information, which is more useful in the native calibration case. The calibration_result value that is used as the apic_freq equivalent is still sane and what the hypervisor returned. > > > /* > > * Debugging macros > > */ > > @@ -238,6 +242,8 @@ extern void setup_boot_APIC_clock(void); > > extern void setup_secondary_APIC_clock(void); > > extern int APIC_init_uniprocessor(void); > > extern void enable_NMI_through_LVT0(void); > > +extern int native_calibrate_apic(void); > > +extern int initialize_lapic(long delta, unsigned int calibration_result); > > Interesting, That function is initializing the local apic? Hmm, the > code below makes me think it's about the local apic timer. Yes it is initializing the local apic clockevent (timer), will call it as initialize_lapic_timer then. > > > > > /* > > * On 32bit this is mach-xxx local > > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h > > index baa579c..58f982b 100644 > > --- a/arch/x86/include/asm/x86_init.h > > +++ b/arch/x86/include/asm/x86_init.h > > @@ -83,11 +83,13 @@ struct x86_init_paging { > > * boot cpu > > * @tsc_pre_init: platform function called before TSC init > > * @timer_init: initialize the platform timer (default PIT/HPET) > > + * @calibrate_apic: calibrate APIC bus freq (ticks per HZ) > > This is not calibrating the APIC, it's calibrating the APIC timer, > right ? Right, the fact that the function pointer resided in x86_init.timers structure, made me think it would be less confusing, but I agree its better to be explicit, calibrate_apic_clock ? > > AFAICT, you also override the TSC calibration, right? > > I wonder whether we could finally avoid the duplicate calibration > stuff and do the TSC and the lapic together and avoid the second > calibration loop. Hmm, not sure how well will the apic calibration fit with all the fast TSC calibration stuff for the native case. > > > */ > > struct x86_init_timers { > > void (*setup_percpu_clockev)(void); > > void (*tsc_pre_init)(void); > > void (*timer_init)(void); > > + int (*calibrate_apic)(void); > > }; > > > > /** > > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > > index e3b534c..ca5d084 100644 > > --- a/arch/x86/kernel/apic/apic.c > > +++ b/arch/x86/kernel/apic/apic.c > > @@ -176,7 +176,7 @@ static struct resource lapic_resource = { > > .flags = IORESOURCE_MEM | IORESOURCE_BUSY, > > }; > > > > -static unsigned int calibration_result; > > +static unsigned int apic_calibration_res; > > calibration_result is not the best choice, but please split out this > cosmetic change into a separate patch. The patch probaly wants some > more splitup for sanity and bisectability reasons./ I can split-up the change in 2 parts, the first change can be just the normal cleanup for the native calibration case, and in the second one I could introduce the apic calibration hook. Below in the mail is the first (preparatory) patch. > > > > > static int lapic_next_event(unsigned long delta, > > struct clock_event_device *evt); > > @@ -326,13 +326,6 @@ int lapic_get_maxlvt(void) > > } > > > > /* > > - * Local APIC timer > > - */ > > - > > -/* Clock divisor */ > > -#define APIC_DIVISOR 16 > > - > > -/* > > * This function sets up the local APIC timer, with a timeout of > > * 'clocks' APIC bus clock. During calibration we actually call > > * this function twice on the boot CPU, once with a bogus timeout > > @@ -431,7 +424,7 @@ static void lapic_timer_setup(enum clock_event_mode mode, > > switch (mode) { > > case CLOCK_EVT_MODE_PERIODIC: > > case CLOCK_EVT_MODE_ONESHOT: > > - __setup_APIC_LVTT(calibration_result, > > + __setup_APIC_LVTT(apic_calibration_res, > > mode != CLOCK_EVT_MODE_PERIODIC, 1); > > break; > > case CLOCK_EVT_MODE_UNUSED: > > @@ -500,8 +493,6 @@ static void __cpuinit setup_APIC_timer(void) > > * back to normal later in the boot process). > > */ > > > > -#define LAPIC_CAL_LOOPS (HZ/10) > > - > > static __initdata int lapic_cal_loops = -1; > > static __initdata long lapic_cal_t1, lapic_cal_t2; > > static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2; > > @@ -590,13 +581,50 @@ calibrate_by_pmtimer(long deltapm, long *delta, long *deltatsc) > > return 0; > > } > > > > -static int __init calibrate_APIC_clock(void) > > +int __init initialize_lapic(long delta, unsigned int calibration_result) > > See above > > > +{ > > + struct clock_event_device *levt = &__get_cpu_var(lapic_events); > > + > > + /* Calculate the scaled math multiplication factor */ > > + lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, > > + lapic_clockevent.shift); > > + lapic_clockevent.max_delta_ns = > > + clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); > > + lapic_clockevent.min_delta_ns = > > + clockevent_delta2ns(0xF, &lapic_clockevent); > > + > > + apic_calibration_res = calibration_result; > > + > > + apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); > > + apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); > > + apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", > > + calibration_result); > > + > > + apic_printk(APIC_VERBOSE, "..... host bus clock speed is " > > + "%u.%04u MHz.\n", > > + calibration_result / (1000000 / HZ), > > + calibration_result % (1000000 / HZ)); > > + > > + /* > > + * Do a sanity check on the APIC calibration result > > + */ > > + if (calibration_result < (1000000 / HZ)) { > > + pr_warning("APIC frequency too slow, disabling apic timer\n"); > > + return -1; > > + } > > + > > + levt->features &= ~CLOCK_EVT_FEAT_DUMMY; > > + return 0; > > +} > > + > > +int __init native_calibrate_apic(void) > > { > > struct clock_event_device *levt = &__get_cpu_var(lapic_events); > > void (*real_handler)(struct clock_event_device *dev); > > unsigned long deltaj; > > long delta, deltatsc; > > int pm_referenced = 0; > > + unsigned int calibration_result; > > > > local_irq_disable(); > > > > @@ -631,20 +659,12 @@ static int __init calibrate_APIC_clock(void) > > pm_referenced = !calibrate_by_pmtimer(lapic_cal_pm2 - lapic_cal_pm1, > > &delta, &deltatsc); > > > > - /* Calculate the scaled math multiplication factor */ > > - lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, > > - lapic_clockevent.shift); > > - lapic_clockevent.max_delta_ns = > > - clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); > > - lapic_clockevent.min_delta_ns = > > - clockevent_delta2ns(0xF, &lapic_clockevent); > > - > > calibration_result = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS; > > > > - apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); > > - apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); > > - apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", > > - calibration_result); > > + if (initialize_lapic(delta, calibration_result)) { > > + local_irq_enable(); > > + return -1; > > + } > > > > if (cpu_has_tsc) { > > apic_printk(APIC_VERBOSE, "..... CPU clock speed is " > > @@ -653,22 +673,6 @@ static int __init calibrate_APIC_clock(void) > > (deltatsc / LAPIC_CAL_LOOPS) % (1000000 / HZ)); > > } > > > > - apic_printk(APIC_VERBOSE, "..... host bus clock speed is " > > - "%u.%04u MHz.\n", > > - calibration_result / (1000000 / HZ), > > - calibration_result % (1000000 / HZ)); > > - > > - /* > > - * Do a sanity check on the APIC calibration result > > - */ > > - if (calibration_result < (1000000 / HZ)) { > > - local_irq_enable(); > > - pr_warning("APIC frequency too slow, disabling apic timer\n"); > > - return -1; > > - } > > - > > - levt->features &= ~CLOCK_EVT_FEAT_DUMMY; > > - > > /* > > * PM timer calibration failed or not turned on > > * so lets try APIC timer based calibration > > @@ -706,7 +710,7 @@ static int __init calibrate_APIC_clock(void) > > > > if (levt->features & CLOCK_EVT_FEAT_DUMMY) { > > pr_warning("APIC timer disabled due to verification failure\n"); > > - return -1; > > + return -1; > > } > > > > return 0; > > @@ -738,7 +742,7 @@ void __init setup_boot_APIC_clock(void) > > apic_printk(APIC_VERBOSE, "Using local APIC timer interrupts.\n" > > "calibrating APIC timer ...\n"); > > > > - if (calibrate_APIC_clock()) { > > + if (x86_init.timers.calibrate_apic()) { > > /* No broadcast on UP ! */ > > if (num_possible_cpus() > 1) > > setup_APIC_timer(); > > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c > > index 227b044..e8b760e 100644 > > --- a/arch/x86/kernel/cpu/vmware.c > > +++ b/arch/x86/kernel/cpu/vmware.c > > @@ -26,6 +26,7 @@ > > #include <asm/div64.h> > > #include <asm/x86_init.h> > > #include <asm/hypervisor.h> > > +#include <asm/apic.h> > > > > #define CPUID_VMWARE_INFO_LEAF 0x40000000 > > #define VMWARE_HYPERVISOR_MAGIC 0x564D5868 > > @@ -72,17 +73,40 @@ static unsigned long vmware_get_tsc_khz(void) > > return tsc_hz; > > } > > > > +static int __init vmware_calibrate_apic(void) > > +{ > > + unsigned int calibration_result; > > + uint32_t eax, ebx, ecx, edx; > > + int err; > > + long delta; > > + unsigned long flags; > > + > > + VMWARE_PORT(GETHZ, eax, ebx, ecx, edx); > > + BUG_ON(!ecx); > > + calibration_result = ecx / HZ; > > + > > + printk(KERN_INFO "APIC bus freq read from hypervisor\n"); > > + > > + delta = (calibration_result * LAPIC_CAL_LOOPS) / APIC_DIVISOR; > > Sigh. > > > + local_irq_save(flags); > > + err = initialize_lapic(delta, calibration_result); > > Crap. You're function override is about calibration and not about > initializing. Why are you calling that here. The original calibrate_APIC_clock itself does a lot initializing so I still have to do this, to maintain consistency between both the native function and the vmware one. I could have split the initialization out of this hook to be done after the calibration is done, but in the native case the "APIC timer based verification" method relies on the calibration_result value to be already set. Hence this initialization still needs to be part of the hook. > Either you override the > local apic timer setup completely or just the calibration routine. We still want to go through the local_apic timer setup so overriding that would mean a lot of duplicate code. Below is the preparatory change, will followup with the 2nd patch which introduces the actual hook. -- Preparatory change to add apic timer calibration hook. From: Alok N Kataria <akataria@vmware.com> This change moves some APIC timer calibration code to a seperate function to facilitate adding a apic clock calibration hook. This change has no functional effect on the calibration routine. Signed-off-by: Alok N Kataria <akataria@vmware.com> Index: linux-x86-tree.git/arch/x86/kernel/apic/apic.c =================================================================== --- linux-x86-tree.git.orig/arch/x86/kernel/apic/apic.c 2010-09-20 13:45:40.000000000 -0700 +++ linux-x86-tree.git/arch/x86/kernel/apic/apic.c 2010-09-20 16:10:13.000000000 -0700 @@ -176,7 +176,7 @@ static struct resource lapic_resource = .flags = IORESOURCE_MEM | IORESOURCE_BUSY, }; -static unsigned int calibration_result; +static unsigned int apic_calibration_res; static int lapic_next_event(unsigned long delta, struct clock_event_device *evt); @@ -431,7 +431,7 @@ static void lapic_timer_setup(enum clock switch (mode) { case CLOCK_EVT_MODE_PERIODIC: case CLOCK_EVT_MODE_ONESHOT: - __setup_APIC_LVTT(calibration_result, + __setup_APIC_LVTT(apic_calibration_res, mode != CLOCK_EVT_MODE_PERIODIC, 1); break; case CLOCK_EVT_MODE_UNUSED: @@ -590,13 +590,50 @@ calibrate_by_pmtimer(long deltapm, long return 0; } -static int __init calibrate_APIC_clock(void) +int __init initialize_lapic_timer(long delta, unsigned int calibration_result) +{ + struct clock_event_device *levt = &__get_cpu_var(lapic_events); + + /* Calculate the scaled math multiplication factor */ + lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, + lapic_clockevent.shift); + lapic_clockevent.max_delta_ns = + clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); + lapic_clockevent.min_delta_ns = + clockevent_delta2ns(0xF, &lapic_clockevent); + + apic_calibration_res = calibration_result; + + apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); + apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); + apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", + calibration_result); + + apic_printk(APIC_VERBOSE, "..... host bus clock speed is " + "%u.%04u MHz.\n", + calibration_result / (1000000 / HZ), + calibration_result % (1000000 / HZ)); + + /* + * Do a sanity check on the APIC calibration result + */ + if (calibration_result < (1000000 / HZ)) { + pr_warning("APIC frequency too slow, disabling apic timer\n"); + return -1; + } + + levt->features &= ~CLOCK_EVT_FEAT_DUMMY; + return 0; +} + +int __init native_calibrate_apic_clock(void) { struct clock_event_device *levt = &__get_cpu_var(lapic_events); void (*real_handler)(struct clock_event_device *dev); unsigned long deltaj; long delta, deltatsc; int pm_referenced = 0; + unsigned int calibration_result; local_irq_disable(); @@ -631,20 +668,12 @@ static int __init calibrate_APIC_clock(v pm_referenced = !calibrate_by_pmtimer(lapic_cal_pm2 - lapic_cal_pm1, &delta, &deltatsc); - /* Calculate the scaled math multiplication factor */ - lapic_clockevent.mult = div_sc(delta, TICK_NSEC * LAPIC_CAL_LOOPS, - lapic_clockevent.shift); - lapic_clockevent.max_delta_ns = - clockevent_delta2ns(0x7FFFFF, &lapic_clockevent); - lapic_clockevent.min_delta_ns = - clockevent_delta2ns(0xF, &lapic_clockevent); - calibration_result = (delta * APIC_DIVISOR) / LAPIC_CAL_LOOPS; - apic_printk(APIC_VERBOSE, "..... delta %ld\n", delta); - apic_printk(APIC_VERBOSE, "..... mult: %u\n", lapic_clockevent.mult); - apic_printk(APIC_VERBOSE, "..... calibration result: %u\n", - calibration_result); + if (initialize_lapic_timer(delta, calibration_result)) { + local_irq_enable(); + return -1; + } if (cpu_has_tsc) { apic_printk(APIC_VERBOSE, "..... CPU clock speed is " @@ -653,22 +682,6 @@ static int __init calibrate_APIC_clock(v (deltatsc / LAPIC_CAL_LOOPS) % (1000000 / HZ)); } - apic_printk(APIC_VERBOSE, "..... host bus clock speed is " - "%u.%04u MHz.\n", - calibration_result / (1000000 / HZ), - calibration_result % (1000000 / HZ)); - - /* - * Do a sanity check on the APIC calibration result - */ - if (calibration_result < (1000000 / HZ)) { - local_irq_enable(); - pr_warning("APIC frequency too slow, disabling apic timer\n"); - return -1; - } - - levt->features &= ~CLOCK_EVT_FEAT_DUMMY; - /* * PM timer calibration failed or not turned on * so lets try APIC timer based calibration @@ -706,7 +719,7 @@ static int __init calibrate_APIC_clock(v if (levt->features & CLOCK_EVT_FEAT_DUMMY) { pr_warning("APIC timer disabled due to verification failure\n"); - return -1; + return -1; } return 0; @@ -738,7 +751,7 @@ void __init setup_boot_APIC_clock(void) apic_printk(APIC_VERBOSE, "Using local APIC timer interrupts.\n" "calibrating APIC timer ...\n"); - if (calibrate_APIC_clock()) { + if (native_calibrate_apic_clock()) { /* No broadcast on UP ! */ if (num_possible_cpus() > 1) setup_APIC_timer(); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86,apic: V2 Add apic calibration hook. 2010-09-20 23:34 ` Alok Kataria @ 2010-09-20 23:59 ` Alok Kataria 0 siblings, 0 replies; 4+ messages in thread From: Alok Kataria @ 2010-09-20 23:59 UTC (permalink / raw) To: Thomas Gleixner Cc: H. Peter Anvin, Ingo Molnar, the arch/x86 maintainers, LKML On Mon, 2010-09-20 at 16:34 -0700, Alok Kataria wrote: > Below is the preparatory change, will followup with the 2nd patch which > introduces the actual hook. Below is the 2nd patch. -- Add apic timer calibration hook. From: Alok N Kataria <akataria@vmware.com> Add a new function ptr calibrate_apic_clock to x86_init_timers structure. On native this does the usual apic calibration. On VMware's platform we override it with a routine which gets that information from the hypervisor. Signed-off-by: Alok N Kataria <akataria@vmware.com> Index: linux-x86-tree.git/arch/x86/include/asm/apic.h =================================================================== --- linux-x86-tree.git.orig/arch/x86/include/asm/apic.h 2010-09-20 16:05:58.000000000 -0700 +++ linux-x86-tree.git/arch/x86/include/asm/apic.h 2010-09-20 16:36:42.000000000 -0700 @@ -17,6 +17,10 @@ #define ARCH_APICTIMER_STOPS_ON_C3 1 +/* Clock divisor */ +#define APIC_DIVISOR 16 +#define LAPIC_CAL_LOOPS (HZ/10) + /* * Debugging macros */ @@ -238,6 +242,8 @@ extern void setup_boot_APIC_clock(void); extern void setup_secondary_APIC_clock(void); extern int APIC_init_uniprocessor(void); extern void enable_NMI_through_LVT0(void); +extern int native_calibrate_apic_clock(void); +extern int initialize_lapic_timer(long delta, unsigned int calibration_result); /* * On 32bit this is mach-xxx local Index: linux-x86-tree.git/arch/x86/include/asm/x86_init.h =================================================================== --- linux-x86-tree.git.orig/arch/x86/include/asm/x86_init.h 2010-09-20 16:06:11.000000000 -0700 +++ linux-x86-tree.git/arch/x86/include/asm/x86_init.h 2010-09-20 16:54:08.000000000 -0700 @@ -83,11 +83,13 @@ struct x86_init_paging { * boot cpu * @tsc_pre_init: platform function called before TSC init * @timer_init: initialize the platform timer (default PIT/HPET) + * @calibrate_apic_clock: calibrate APIC clock freq (ticks per HZ) */ struct x86_init_timers { void (*setup_percpu_clockev)(void); void (*tsc_pre_init)(void); void (*timer_init)(void); + int (*calibrate_apic_clock)(void); }; /** Index: linux-x86-tree.git/arch/x86/kernel/cpu/vmware.c =================================================================== --- linux-x86-tree.git.orig/arch/x86/kernel/cpu/vmware.c 2010-09-20 16:05:30.000000000 -0700 +++ linux-x86-tree.git/arch/x86/kernel/cpu/vmware.c 2010-09-20 16:40:35.000000000 -0700 @@ -26,6 +26,7 @@ #include <asm/div64.h> #include <asm/x86_init.h> #include <asm/hypervisor.h> +#include <asm/apic.h> #define CPUID_VMWARE_INFO_LEAF 0x40000000 #define VMWARE_HYPERVISOR_MAGIC 0x564D5868 @@ -72,17 +73,40 @@ static unsigned long vmware_get_tsc_khz( return tsc_hz; } +static int __init vmware_calibrate_apic_clock(void) +{ + unsigned int calibration_result; + uint32_t eax, ebx, ecx, edx; + int err; + long delta; + unsigned long flags; + + VMWARE_PORT(GETHZ, eax, ebx, ecx, edx); + BUG_ON(!ecx); + calibration_result = ecx / HZ; + + printk(KERN_INFO "APIC bus freq read from hypervisor\n"); + + delta = (calibration_result * LAPIC_CAL_LOOPS) / APIC_DIVISOR; + local_irq_save(flags); + err = initialize_lapic_timer(delta, calibration_result); + local_irq_restore(flags); + + return err; +} + static void __init vmware_platform_setup(void) { uint32_t eax, ebx, ecx, edx; VMWARE_PORT(GETHZ, eax, ebx, ecx, edx); - if (ebx != UINT_MAX) + if (ebx != UINT_MAX) { x86_platform.calibrate_tsc = vmware_get_tsc_khz; - else + x86_init.timers.calibrate_apic_clock = vmware_calibrate_apic_clock; + } else printk(KERN_WARNING - "Failed to get TSC freq from the hypervisor\n"); + "Failed to setup VMware hypervisor platform\n"); } /* Index: linux-x86-tree.git/arch/x86/kernel/x86_init.c =================================================================== --- linux-x86-tree.git.orig/arch/x86/kernel/x86_init.c 2010-09-20 16:05:07.000000000 -0700 +++ linux-x86-tree.git/arch/x86/kernel/x86_init.c 2010-09-20 16:41:21.000000000 -0700 @@ -68,6 +68,7 @@ struct x86_init_ops x86_init __initdata .setup_percpu_clockev = setup_boot_APIC_clock, .tsc_pre_init = x86_init_noop, .timer_init = hpet_time_init, + .calibrate_apic_clock = native_calibrate_apic_clock }, .iommu = { Index: linux-x86-tree.git/arch/x86/kernel/apic/apic.c =================================================================== --- linux-x86-tree.git.orig/arch/x86/kernel/apic/apic.c 2010-09-20 16:10:13.000000000 -0700 +++ linux-x86-tree.git/arch/x86/kernel/apic/apic.c 2010-09-20 16:42:12.000000000 -0700 @@ -326,13 +326,6 @@ int lapic_get_maxlvt(void) } /* - * Local APIC timer - */ - -/* Clock divisor */ -#define APIC_DIVISOR 16 - -/* * This function sets up the local APIC timer, with a timeout of * 'clocks' APIC bus clock. During calibration we actually call * this function twice on the boot CPU, once with a bogus timeout @@ -500,8 +493,6 @@ static void __cpuinit setup_APIC_timer(v * back to normal later in the boot process). */ -#define LAPIC_CAL_LOOPS (HZ/10) - static __initdata int lapic_cal_loops = -1; static __initdata long lapic_cal_t1, lapic_cal_t2; static __initdata unsigned long long lapic_cal_tsc1, lapic_cal_tsc2; ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-09-20 23:59 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-20 21:17 [PATCH] x86,apic: V2 Add apic calibration hook Alok Kataria 2010-09-20 22:11 ` Thomas Gleixner 2010-09-20 23:34 ` Alok Kataria 2010-09-20 23:59 ` Alok Kataria
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox