| Message ID | 34662095-6f58-4471-8bbf-1bdf67650fb2@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/time: TSC calibration improvements | expand |
On Wed, Jan 12, 2022 at 09:55:17AM +0100, Jan Beulich wrote: > Looping until reaching/exceeding a certain value is error prone: If the > target value is close enough to the wrapping point, the loop may not > terminate at all. Switch to using delta values, which then allows to > fold the two loops each into just one. > > Fixes: 93340297802b ("x86/time: calibrate TSC against platform timer") > Reported-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 12.01.2022 09:55, Jan Beulich wrote: > @@ -504,11 +501,8 @@ static s64 __init init_pmtimer(struct pl > > count = inl(pmtmr_ioport) & mask; > start = rdtsc_ordered(); > - target = count + CALIBRATE_VALUE(ACPI_PM_FREQUENCY); > - if ( target < count ) > - while ( (inl(pmtmr_ioport) & mask) >= count ) > - continue; > - while ( (inl(pmtmr_ioport) & mask) < target ) > + target = CALIBRATE_VALUE(ACPI_PM_FREQUENCY); > + while ( (elapsed = (inl(pmtmr_ioport) & mask) - count) < target ) I think this is wrong, and instead needs to be while ( (elapsed = (inl(pmtmr_ioport) - count) & mask) < target ) There no similar issue with HPET as there we always have full 32 bits available. Roger - you gave your R-b. If you agree, I'd like to retain that with the fix in place. But I'm not going to commit either variant ahead of hearing back from you. Jan > continue; > > return (rdtsc_ordered() - start) * CALIBRATE_FRAC; > >
On Thu, Jan 13, 2022 at 10:37:53AM +0100, Jan Beulich wrote: > On 12.01.2022 09:55, Jan Beulich wrote: > > @@ -504,11 +501,8 @@ static s64 __init init_pmtimer(struct pl > > > > count = inl(pmtmr_ioport) & mask; > > start = rdtsc_ordered(); > > - target = count + CALIBRATE_VALUE(ACPI_PM_FREQUENCY); > > - if ( target < count ) > > - while ( (inl(pmtmr_ioport) & mask) >= count ) > > - continue; > > - while ( (inl(pmtmr_ioport) & mask) < target ) > > + target = CALIBRATE_VALUE(ACPI_PM_FREQUENCY); > > + while ( (elapsed = (inl(pmtmr_ioport) & mask) - count) < target ) > > I think this is wrong, and instead needs to be > > while ( (elapsed = (inl(pmtmr_ioport) - count) & mask) < target ) > > There no similar issue with HPET as there we always have full 32 bits > available. > > Roger - you gave your R-b. If you agree, I'd like to retain that with > the fix in place. But I'm not going to commit either variant ahead of > hearing back from you. Indeed, or else overflows past the mask boundary could make the loop exit early. Please keep the R-b. Roger.
--- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -378,7 +378,7 @@ static u64 read_hpet_count(void) static int64_t __init init_hpet(struct platform_timesource *pts) { uint64_t hpet_rate, start; - uint32_t count, target; + uint32_t count, target, elapsed; /* * Allow HPET to be setup, but report a frequency of 0 so it's not selected * as a timer source. This is required so it can be used in legacy @@ -451,11 +451,8 @@ static int64_t __init init_hpet(struct p count = hpet_read32(HPET_COUNTER); start = rdtsc_ordered(); - target = count + CALIBRATE_VALUE(hpet_rate); - if ( target < count ) - while ( hpet_read32(HPET_COUNTER) >= count ) - continue; - while ( hpet_read32(HPET_COUNTER) < target ) + target = CALIBRATE_VALUE(hpet_rate); + while ( (elapsed = hpet_read32(HPET_COUNTER) - count) < target ) continue; return (rdtsc_ordered() - start) * CALIBRATE_FRAC; @@ -493,8 +490,8 @@ static u64 read_pmtimer_count(void) static s64 __init init_pmtimer(struct platform_timesource *pts) { - u64 start; - u32 count, target, mask; + uint64_t start; + uint32_t count, target, mask, elapsed; if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) ) return 0; @@ -504,11 +501,8 @@ static s64 __init init_pmtimer(struct pl count = inl(pmtmr_ioport) & mask; start = rdtsc_ordered(); - target = count + CALIBRATE_VALUE(ACPI_PM_FREQUENCY); - if ( target < count ) - while ( (inl(pmtmr_ioport) & mask) >= count ) - continue; - while ( (inl(pmtmr_ioport) & mask) < target ) + target = CALIBRATE_VALUE(ACPI_PM_FREQUENCY); + while ( (elapsed = (inl(pmtmr_ioport) & mask) - count) < target ) continue; return (rdtsc_ordered() - start) * CALIBRATE_FRAC;
Looping until reaching/exceeding a certain value is error prone: If the target value is close enough to the wrapping point, the loop may not terminate at all. Switch to using delta values, which then allows to fold the two loops each into just one. Fixes: 93340297802b ("x86/time: calibrate TSC against platform timer") Reported-by: Roger Pau Monné <roger.pau@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com>