Messages in this thread | | | From | Oliver Upton <> | Date | Sat, 7 Aug 2021 11:35:21 -0700 | Subject | Re: [RESEND PATCH] clocksource/arm_arch_timer: Fix masking for high freq counters |
| |
Hi Marc,
On Sat, Aug 7, 2021 at 3:52 AM Marc Zyngier <maz@kernel.org> wrote: > > Hi Oliver, > > On Fri, 06 Aug 2021 19:21:26 +0100, > Oliver Upton <oupton@google.com> wrote: > > > > Unfortunately, the architecture provides no means to determine the bit > > width of the system counter. However, we do know the following from the > > specification: > > > > - the system counter is at least 56 bits wide > > - Roll-over time of not less than 40 years > > > > To date, the arch timer driver has depended on the first property, > > assuming any system counter to be 56 bits wide and masking off the rest. > > However, combining a narrow clocksource mask with a high frequency > > counter could result in prematurely wrapping the system counter by a > > significant margin. For example, a 56 bit wide, 1GHz system counter > > would wrap in a mere 2.28 years! > > > > This is a problem for two reasons: v8.6+ implementations are required to > > provide a 64 bit, 1GHz system counter. Furthermore, before v8.6, > > implementers may select a counter frequency of their choosing. > > > > Fix the issue by deriving a valid clock mask based on the second > > property from above. Set the floor at 56 bits, since we know no system > > counter is narrower than that. > > > > Suggested-by: Marc Zyngier <maz@kernel.org> > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > This patch was tested with QEMU, tweaked to provide a 1GHz system > > counter frequency, as I could not easily figure out how to tweak the > > base FVP to provide a 1GHz counter. > > <FVP> > "bp.refcounter.base_frequency" is the property you are looking for. In > general, passing --list-params to the model reveals a treasure trove > of weird and wonderful options that can be used to configure the model > to your liking. > </FVP>
Ah! Thanks for the tip. I didn't find the fast models documentation to be too terribly helpful. > > > > +/* > > + * The minimum amount of time a generic timer is guaranteed to not roll over > > nit: s/timer/counter/ >
Ack.
> > + * (40 years) > > For later reference, could you add the section of the ARMv8 ARM where > this is mentioned? Something like 'ARM DDI 0487G.a D11.1.2 ("The > system counter")', either here or in the comment further down. >
Sure thing. I think I'll drop it on the function below since we depend on two properties of the system counter there.
> > +static int __init arch_counter_get_width(void) > > +{ > > + u64 min_cycles = MIN_ROLLOVER_SECS * arch_timer_get_cntfrq(); > > That's unfortunately wishful thinking. We have stupidly broken systems > out there that do not set CNTFRQ_EL0, and instead rely on DT > properties to describe the counter frequency. You're likely to end up > with a glorious zero as a result, with interesting consequences...
Blech. I didn't read the DT hooks for the driver. That's awful.
> Use arch_timer_rate instead, which will be set as by the time you > register the counter.
Ack.
> > > + > > + /* guarantee the returned width is within the valid range */ > > + return max(56, min(64, ilog2(min_cycles))); > > Maybe better written as "clamp_val(ilog2(min_cycles), 56, 64);". >
Agreed.
> > + width = arch_counter_get_width(); > > + clocksource_counter.mask = CLOCKSOURCE_MASK(width); > > + cyclecounter.mask = CLOCKSOURCE_MASK(width); > > Since you move this to be computed at runtime, how about dropping the > static initialisation of the mask fields? >
Yep, already made the change locally after mailing out :)
> > + > > if (!arch_counter_suspend_stop) > > clocksource_counter.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP; > > start_count = arch_timer_read_counter(); > > @@ -1040,8 +1065,7 @@ static void __init arch_counter_register(unsigned type) > > timecounter_init(&arch_timer_kvm_info.timecounter, > > &cyclecounter, start_count); > > > > - /* 56 bits minimum, so we assume worst case rollover */ > > - sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate); > > + sched_clock_register(arch_timer_read_counter, width, arch_timer_rate); > > } > > > > static void arch_timer_stop(struct clock_event_device *clk) > > Another thing that needs addressing for high frequency counter support > is to move away from TVAL programming and switch to CVAL, as the > maximum deadline we can currently program is 4.2s at 1GHz. > > Fun fact: it has the interesting consequence of breaking XGene-1, > which implemented CVAL in terms of TVAL instead of the other way > around (what were these guys thinking?)
Yikes!
> , though I don't think anyone > will notice in practice. I have a preliminary patch on a branch > somewhere that I'll try to dust up and post in the coming days.
Sounds good, I'll clean this one up and send it out too.
-- Thanks, Oliver
| |