Re: [PATCH v3 2/4] x86/APIC: calibrate against platform timer when possible

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

On Mon, Feb 14, 2022 at 10:25:11AM +0100, 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 read_tmcct() isn't overly nice, but I wanted
> to avoid x2apic_enabled being evaluated twice in close succession. (The
> barrier is there just in case only anyway: While this RDMSR isn't
> serializing, I'm unaware of any statement whether it can also be
> executed speculatively, like RDTSC can.) An option might be to move the
> function to apic.c such that it would also be used by
> calibrate_APIC_clock().

I think that would make sense. Or else it's kind of orthogonal that we
use a barrier in calibrate_apic_timer but not in calibrate_APIC_clock.
But maybe we can get rid of the open-coded PIT calibration in
calibrate_APIC_clock? (see below)

> --- 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>
> @@ -1004,6 +1005,78 @@ static u64 __init init_platform_timer(vo
>      return rc;
>  }
>  
> +static uint32_t __init read_tmcct(void)
> +{
> +    if ( x2apic_enabled )
> +    {
> +        alternative("lfence", "mfence", X86_FEATURE_MFENCE_RDTSC);
> +        return apic_rdmsr(APIC_TMCCT);
> +    }
> +
> +    return apic_mem_read(APIC_TMCCT);
> +}
> +
> +static uint64_t __init read_pt_and_tmcct(uint32_t *tmcct)
> +{
> +    uint32_t tmcct_prev = *tmcct = read_tmcct(), 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 = read_tmcct();
> +        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.
> +     */

The reasoning in calibrate_APIC_clock to have interrupts disabled
doesn't apply anymore I would think (interrupts are already enabled
when we get there), and hence it seems to me that calibrate_APIC_clock
could be called with interrupts enabled and we could remove the
open-coded usage of the PIT in calibrate_APIC_clock.

Thanks, Roger.