[PATCH] clocksource: arch_timer: Fix code to use physical timers when requested

[Sonny Rao]

On Fri, Aug 29, 2014 at 3:04 AM, Mark Rutland <mark.rutland at arm.com> wrote:
> On Fri, Aug 29, 2014 at 01:10:49AM +0100, Sonny Rao wrote:
>> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland at arm.com> wrote:
>> > On Thu, Aug 28, 2014 at 04:33:31AM +0100, Doug Anderson wrote:
>> >> Hi,
>> >>
>> >> On Wed, Aug 27, 2014 at 7:58 PM, Olof Johansson <olof at lixom.net> wrote:
>> >> > On Wed, Aug 27, 2014 at 5:56 PM, Stephen Boyd <sboyd at codeaurora.org> wrote:
>> >> >> On 08/27/14 15:33, Olof Johansson wrote:
>> >> >>> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd at codeaurora.org> wrote:
>> >> >>>
>> >> >>>> Is there any reason why the virtual counter can't be read? Maybe we're
>> >> >>>> the hyp and we need to make sure we don't use the virtual timer so that
>> >> >>>> the guest can use it, but that doesn't have any effect on the usage of
>> >> >>>> the virtual counter for the clocksource.
>> >> >>> There are several cases where virtual is unusable -- in particular it
>> >> >>> might not have been configured properly (i.e. the phys/virt offset is
>> >> >>> at a bad value).
>> >> >>>
>> >> >>
>> >> >> Any specifics? It would be nice to say so in the commit text so that
>> >> >> others using such devices know they need this patch. I'm guessing the
>> >> >> firmware can't be fixed?
>> >>
>> >> Even if we could change things to use a virtual timer in some cases,
>> >> Sonny's patch still fixes a bug.  The code as written right now makes
>> >> pretenses about supporting the physical timer, but it doesn't work.
>> >> That should be fixed.
>> >
>> > The code does support the physical timer. It does not support the
>> > physical counter (and makes no pretenses that it does).
>> >
>>
>> Is there some reason that it should not support it?  It seems like the
>> two things are highly related.
>
> While the two are related, in sane systems the use of the physical
> counters is rendered unnecessary by the ability to write to CNTVOFF at
> PL2.

By sane, do you mean a system which starts the kernel in PL2?  Or one
that has CNTVOFF initialized to the same value on all CPUs?

>
> If an OS is booted at PL1 the physical timers aren't guaranteed to be
> accessible, so the OS must use the virtual timers. As the OS could be
> virtualized it must use the virtual counters.

I was curious to learn more about these modes and looked at the spec.
The spec I have seems to say that in a VMSA implementation without
virtualization, then CNTPCT is always available, but if it has
virtualization then a bit needs to be set, which I think is what
you're referring to.  I think the spec also says that virtualization
extensions are optional.  How do you deal with the case that they are
not implemented?  Or maybe that simply isn't supported?

> If an OS is booted at PL2 it can access the physical counters, and
> should do so in case something like KVM will be used later. The OS can
> write to CNTVOFF at PL2, and if it sets CNTVOFF to zero the physical and
> virtual counters are equivalent. Thus it can use the virtual counters
> and doesn't need to have additional code in several places (including
> the VDSO) where it needs to choose to read which counters to read.
>
> The problem only exists where PL2 exists and the firmware/bootloader
> skipped PL2 without initialising the necessary PL2 state. This is in
> general a stupid thing to do; it introduces a problem that need not
> exist and throws away the option of using the features PL2 provides.
> This is a firmware/bootloader bug.

Well it's not quite that simple, this is actually an issue with the
hardware that the CNTVOFF comes up with different values on different
cores.  This happens not only at boot, but any time the core is
powered on, which could include deep sleep or CPU hotplug and suspend
to ram.  The firmware may not be involved in all these cases, so we
cannot rely on it to fix this problem.

>
>> > I had hoped we wouldn't encounter cases where CNTVOFF was hopelessly
>> > ill-configured on a platform, but evidently we have. So we need some
>> > workaround for that.
>> >
>> >> > Yeah, there are a few. The big.LITTLE on the Chromebook 2 models have
>> >> > this issue, due to the A7 cluster having an incorrect offset
>> >> > programmed. However, arch timers aren't supported on that SoC in the
>> >> > first place, so it's not a problem in reality.
>> >> >
>> >> > The other known platform is rk3288. It has products out in the wild
>> >> > where firmware updates are unlikely.
>> >>
>> >> One other reason is that (I'm told) that the virtual offset is lost in
>> >> certain power down conditions (powering down a core, going into S3,
>> >> etc).  When we power back up the offset is effectively reset to a
>> >> random value.  That means we need something to reprogram the virtual
>> >> timer offset whenever we power things back up.
>> >>
>> >> If we've got a hypervisor then the hypervisor will definitely be
>> >> involved in powering things back up and it can reset the virtual
>> >> offset.  ...but forcing systems to implement a hypervisor (or somehow
>> >> adding an interface for the kernel to call back into firmware) is a
>> >> huge effort and it means more hard-to-update code sitting in firmware.
>> >
>> > Not if you boot Linux at hyp, as we've recommended for this precise
>> > reason. That doesn't fix other things like CNTFRQ if the secure
>> > initialisation doesn't poke that, however.
>>
>> That's interesting, we could look into that.
>
> I would strongly recommend that you do. :)
>
> Linux has supported booting at Hyp for quite a while now, so I would
> hope the only issues would be the organisation of the firmware and/or
> bootloader.

I'm still investigating this for our firmware (but cannot guarantee
that anyone else will do this), but like I mentioned above, it
probably won't solve the problem for suspend to RAM or deep
sleep/power gating of cpu cores.

> Thanks,
> Mark.