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 <jbeul...@suse.com> > --- > 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.