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

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

On Mon, Apr 04, 2022 at 01:55:56PM +0200, Jan Beulich wrote:
> On 04.04.2022 13:46, Roger Pau Monné wrote:
> > On Thu, Mar 31, 2022 at 11:29:44AM +0200, Jan Beulich wrote:
> >> +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?
> 
> Oh, yes, of course. I guess I did clone an earlier mistake from
> calibrate_tsc().
> 
> >> +    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.
> 
> Sure - I've added "But first scale down values to actually fit
> muldiv64()'s input range."

With those taken care of:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.