On Mon, 2016-06-27 at 14:13 +0100, Marc Zyngier wrote:
> On 22/06/16 02:45, Scott Wood wrote:
> >
> > On Fri, 2016-05-13 at 11:24 +0100, Marc Zyngier wrote:
> > >
> > > On Thu, 12 May 2016 23:37:39 -0500
> > > Scott Wood <oss@xxxxxxxxxxxx> wrote:
> > >
> > > > +{
> > > > + u64 cval_old, cval_new;
> > > > + int timeout = 200;
> > > Can we have a comment on how this value has been chosen?
> > It's an arbitrary value well beyond the point at which we've seen it fail.
> So can we please have a comment *in the code* that explains how this
> value has been picked?
Sure, if you want. I just wasn't sure there was much value in a comment that
essentially says that there's no special meaning to this particular value.
> > > > int
> > > > access, unsigned long evt,
> > > > arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> > > > }
> > > >
> > > > +#ifdef CONFIG_ARM64
> > > > +static __always_inline void rewrite_tval(const int access,
> > > > + unsigned long evt, struct clock_event_device *clk)
> > > > +{
> > > > + u64 cval_old, cval_new;
> > > > + int timeout = 200;
> > > > +
> > > > + do {
> > > > + cval_old = __arch_counter_get_cntvct();
> > > > + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL,
> > > > evt,
> > > > clk);
> > > > + cval_new = __arch_counter_get_cntvct();
> > > Don't you need to guarantee the order of accesses here?
> > I'm not 100% sure. The erratum workaround sample code doesn't show any
> > barriers, and adding more barriers could make it harder for the loop to
> > successfully complete. There's already a barrier after the write, so the
> > only
> > concern should be whether the timer read could be reordered after the
> > timer
> > write, which could cause the loop to exit even if the write was bad. Do
> > you
> > know if A53 or A57 will reorder a counter read relative to a tval write?
> I can't see any absolute guarantee that they wouldn't be reordered (but
> I have no insight on the micro-architecture either). I'd rather err on
> the side of caution here.
OK, I'll see how well it works with the added barrier.
> > > > clk->set_state_shutdown =
> > > > arch_timer_shutdown_virt;
> > > > clk->set_state_oneshot_stopped =
> > > > arch_timer_shutdown_virt;
> > > > clk->set_next_event =
> > > > arch_timer_set_next_event_virt;
> > > > +
> > > > +#ifdef CONFIG_ARM64
> > > > + if
> > > > (static_branch_unlikely(&arch_timer_read_ool_enabled))
> > > > + clk->set_next_event =
> > > > + arch_timer_set_next_event_vir
> > > > t_er
> > > > rata;
> > > On the same line, please.
> > I was trying to avoid going beyond 80 columns.
> Please ignore what checkpatch says. Readability is more important (and
> I've given up using a vintage vt100...).
I'm not using a vintage vt100 either but I still have (approximately) 80-
column terminals, because I like having two terminals side-by-side with a
reasonable font size... and since different people have different terminal
setups, CodingStyle specifies a standard limit.
I think I can make it moot with the suggestion to have a helper function,
though.
> > > } else {
> > > > arch_timer_read_counter =
> > > > arch_counter_get_cntvct_mem;
> > > >
> > > > @@ -763,6 +870,9 @@ static void __init arch_timer_of_init(struct
> > > > device_node *np)
> > > >
> > > > arch_timer_c3stop = !of_property_read_bool(np, "always-on");
> > > >
> > > > + if (of_property_read_bool(np, "fsl,erratum-a008585"))
> > > > + static_branch_enable(&arch_timer_read_ool_enabled);
> > > > +
> > > > /*
> > > > * If we cannot rely on firmware initializing the timer
> > > > registers
> > > > then
> > > > * we should use the physical timers instead.
> > > An outstanding question is how we're going to deal with this in KVM,
> > > because a guest absolutely needs to know about it (I can definitely see
> > > time jumping in guests running on a LS2080).
> > The property will need to be in the guest's device tree. I'm not too
> > familiar
> > with how KVM handles device trees on arm... From looking at the QEMU
> > source
> > it seems that the dtb is passed in by the user. So either that dtb will
> > need
> > the erratum property in it, or QEMU (and KVM tool?) would need to patch it
> > into the guest dtb based on seeing the property in /proc/device-tree.
> There is no guarantee that the host device tree is always accessible to
> userspace. So we're probably looking at requiring a new KVM device API
> that would expose the timer properties, one of them being this erratum.
> That's certainly going to be fun to handle.
KVM on PPC relies on /proc/device-tree -- when would it not be available?
Where is the dtb passed to QEMU expected to come from?
In any case, let's get the kernel sorted out first.
-Scott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
