Re: [PATCH v2 1/4] x86/time: further improve TSC / CPU freq calibration accuracy

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.01.2022 09:25, Jan Beulich wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -287,9 +287,47 @@ static char *freq_string(u64 freq)
>      return s;
>  }
>  
> -static uint64_t adjust_elapsed(uint64_t elapsed, uint32_t actual,
> -                               uint32_t target)
> +static uint32_t __init read_pt_and_tsc(uint64_t *tsc,
> +                                       const struct platform_timesource *pts)
>  {
> +    uint64_t tsc_prev = *tsc = rdtsc_ordered(), tsc_min = ~0;
> +    uint32_t best = best;
> +    unsigned int i;
> +
> +    for ( i = 0; ; ++i )
> +    {
> +        uint32_t pt = pts->read_counter();
> +        uint64_t tsc_cur = rdtsc_ordered();
> +        uint64_t tsc_delta = tsc_cur - tsc_prev;
> +
> +        if ( tsc_delta < tsc_min )
> +        {
> +            tsc_min = tsc_delta;
> +            *tsc = tsc_cur;
> +            best = pt;
> +        }
> +        else if ( i > 2 )
> +            break;
> +
> +        tsc_prev = tsc_cur;
> +    }
> +
> +    return best;
> +}
> +
> +static uint64_t __init calibrate_tsc(const struct platform_timesource *pts)
> +{
> +    uint64_t start, end, elapsed;
> +    uint32_t count = read_pt_and_tsc(&start, pts);
> +    uint32_t target = CALIBRATE_VALUE(pts->frequency), actual;
> +    uint32_t mask = (uint32_t)~0 >> (32 - pts->counter_bits);
> +
> +    while ( ((pts->read_counter() - count) & mask) < target )
> +        continue;
> +
> +    actual = read_pt_and_tsc(&end, pts) - count;

Having run into a case where the resulting CPU freq was 141 kHz (and
boot failing slightly later because of this), I've spotted that this
also needs masking by "mask", to guard against a 24-bit PM timer
wrapping between the earlier read and this one. The original code ...

> @@ -508,22 +539,12 @@ static u64 read_pmtimer_count(void)
>  
>  static s64 __init init_pmtimer(struct platform_timesource *pts)
>  {
> -    uint64_t start;
> -    uint32_t count, target, mask, elapsed;
> -
>      if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) )
>          return 0;
>  
>      pts->counter_bits = pmtmr_width;
> -    mask = 0xffffffff >> (32 - pmtmr_width);
> -
> -    count = inl(pmtmr_ioport);
> -    start = rdtsc_ordered();
> -    target = CALIBRATE_VALUE(ACPI_PM_FREQUENCY);
> -    while ( (elapsed = (inl(pmtmr_ioport) - count) & mask) < target )
> -        continue;

... ended up requiring use of "mask" just once.

Jan

> -    return adjust_elapsed(rdtsc_ordered() - start, elapsed, target);
> +    return calibrate_tsc(pts);
>  }
>  
>  static struct platform_timesource __initdata plt_pmtimer =
> 
>