On Thu, Mar 31, 2022 at 11:29:44AM +0200, Jan Beulich wrote:
> Use the original calibration against PIT only when the platform timer
> is PIT. This implicitly excludes the "xen_guest" case from using the PIT
> logic (init_pit() fails there, and as of 5e73b2594c54 ["x86/time: minor
> adjustments to init_pit()"] using_pit also isn't being set too early
> anymore), so the respective hack there can be dropped at the same time.
> This also reduces calibration time from 100ms to 50ms, albeit this step
> is being skipped as of 0731a56c7c72 ("x86/APIC: no need for timer
> calibration when using TDT") anyway.
>
> While re-indenting the PIT logic in calibrate_APIC_clock(), besides
> adjusting style also switch around the 2nd TSC/TMCCT read pair, to match
> the order of the 1st one, yielding more consistent deltas.
>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> ---
> Open-coding apic_read() in apic_tmcct_read() isn't overly nice, but I
> wanted to avoid x2apic_enabled being evaluated twice in close
> succession. And I also wouldn't want to have the barrier there even for
> the (uncached) MMIO read.
>
> Unlike the CPU frequencies enumerated in CPUID leaf 0x16 (which aren't
> precise), using CPUID[0x15].ECX - if populated - may be an option to
> skip calibration altogether. Aiui the value there is precise, but using
> the systems I have easy access to I cannot verify this: In the sample
> of three I have, none have ECX populated.
>
> I wonder whether the secondary CPU freq measurement (used for display
> purposes only) wouldn't better be dropped at this occasion.
I don't have a strong opinion re those informative messages.
> --- a/xen/arch/x86/include/asm/apic.h
> +++ b/xen/arch/x86/include/asm/apic.h
> @@ -192,6 +192,9 @@ extern void record_boot_APIC_mode(void);
> extern enum apic_mode current_local_apic_mode(void);
> extern void check_for_unexpected_msi(unsigned int vector);
>
> +uint64_t calibrate_apic_timer(void);
> +uint32_t apic_tmcct_read(void);
> +
> extern void check_nmi_watchdog(void);
>
> extern unsigned int nmi_watchdog;
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -26,6 +26,7 @@
> #include <xen/symbols.h>
> #include <xen/keyhandler.h>
> #include <xen/guest_access.h>
> +#include <asm/apic.h>
> #include <asm/io.h>
> #include <asm/iocap.h>
> #include <asm/msr.h>
> @@ -1018,6 +1019,67 @@ static u64 __init init_platform_timer(vo
> return rc;
> }
>
> +static uint64_t __init read_pt_and_tmcct(uint32_t *tmcct)
> +{
> + uint32_t tmcct_prev = *tmcct = apic_tmcct_read(), tmcct_min = ~0;
> + uint64_t best = best;
> + unsigned int i;
> +
> + for ( i = 0; ; ++i )
> + {
> + uint64_t pt = plt_src.read_counter();
> + uint32_t tmcct_cur = apic_tmcct_read();
> + uint32_t tmcct_delta = tmcct_prev - tmcct_cur;
> +
> + if ( tmcct_delta < tmcct_min )
> + {
> + tmcct_min = tmcct_delta;
> + *tmcct = tmcct_cur;
> + best = pt;
> + }
> + else if ( i > 2 )
> + break;
> +
> + tmcct_prev = tmcct_cur;
> + }
> +
> + return best;
> +}
> +
> +uint64_t __init calibrate_apic_timer(void)
> +{
> + uint32_t start, end;
> + uint64_t count = read_pt_and_tmcct(&start), elapsed;
> + uint64_t target = CALIBRATE_VALUE(plt_src.frequency), actual;
> + uint64_t mask = (uint64_t)~0 >> (64 - plt_src.counter_bits);
> +
> + /*
> + * PIT cannot be used here as it requires the timer interrupt to maintain
> + * its 32-bit software counter, yet here we run with IRQs disabled.
> + */
> + if ( using_pit )
> + return 0;
> +
> + while ( ((plt_src.read_counter() - count) & mask) < target )
> + continue;
> +
> + actual = read_pt_and_tmcct(&end) - count;
Don't you need to apply the pt mask here?
> + elapsed = start - end;
> +
> + if ( likely(actual > target) )
> + {
> + /* See the comment in calibrate_tsc(). */
I would maybe add that the counters used here might have > 32 bits,
and hence we need to trim the values for muldiv64 to scale properly.
Thanks, Roger.
