Re: [PATCH v4 1/4] x86/APIC: calibrate against platform timer when possible

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <jbeulich@xxxxxxxx>
> ---
> 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.