[PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Doug Anderson]

Some 32-bit (ARMv7) systems are architected like this:

* The firmware doesn't know and doesn't care about hypervisor mode and
  we don't want to add the complexity of hypervisor there.

* The firmware isn't involved in SMP bringup or resume.

* The ARCH timer come up with an uninitialized offset between the
  virtual and physical counters.  Each core gets a different random
  offset.

On systems like the above, it doesn't make sense to use the virtual
counter.  There's nobody managing the offset and each time a core goes
down and comes back up it will get reinitialized to some other random
value.

Let's add a property to the device tree to say that we shouldn't use
the virtual timer.  Firmware could potentially remove this property
before passing the device tree to the kernel if it really wants the
kernel to use a virtual timer.

Note that it's been said that ARM64 (ARMv8) systems the firmware and
kernel really can't be architected as described above.  That means
using the physical timer like this really only makes sense for ARMv7
systems.

In order for this patch to do anything useful, we also need Sonny's
patch at <https://patchwork.kernel.org/patch/4790921/>

Signed-off-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
Changes in v2:
- Add "#ifdef CONFIG_ARM" as per Will Deacon

 Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++
 drivers/clocksource/arm_arch_timer.c                 | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
index 37b2caf..876d32b 100644
--- a/Documentation/devicetree/bindings/arm/arch_timer.txt
+++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
@@ -22,6 +22,12 @@ to deliver its interrupts via SPIs.
 - always-on : a boolean property. If present, the timer is powered through an
   always-on power domain, therefore it never loses context.
 
+** Optional properties:
+
+- arm,use-physical-timer : Don't ever use the virtual timer, just use the
+  physical one.  Not supported for ARM64.
+
+
 Example:
 
 	timer {
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5163ec1..e7aa256 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -649,6 +649,11 @@ static void __init arch_timer_init(struct device_node *np)
 		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
 	arch_timer_detect_rate(NULL, np);
 
+#ifdef CONFIG_ARM
+	if (of_property_read_bool(np, "arm,use-physical-timer"))
+		arch_timer_use_virtual = false;
+#endif
+
 	/*
 	 * If HYP mode is available, we know that the physical timer
 	 * has been configured to be accessible from PL1. Use it, so
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Will Deacon]

On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote:
> Some 32-bit (ARMv7) systems are architected like this:
> 
> * The firmware doesn't know and doesn't care about hypervisor mode and
>   we don't want to add the complexity of hypervisor there.
> 
> * The firmware isn't involved in SMP bringup or resume.
> 
> * The ARCH timer come up with an uninitialized offset between the
>   virtual and physical counters.  Each core gets a different random
>   offset.
> 
> On systems like the above, it doesn't make sense to use the virtual
> counter.  There's nobody managing the offset and each time a core goes
> down and comes back up it will get reinitialized to some other random
> value.

You probably need to rephrase this slightly, as there *is* still a
requirement on the hypervisor/firmware (actually, two!). See below.

> Let's add a property to the device tree to say that we shouldn't use
> the virtual timer.  Firmware could potentially remove this property
> before passing the device tree to the kernel if it really wants the
> kernel to use a virtual timer.
> 
> Note that it's been said that ARM64 (ARMv8) systems the firmware and
> kernel really can't be architected as described above.  That means
> using the physical timer like this really only makes sense for ARMv7
> systems.

I'd go further: this only makes sense if you're booting in secure SVC
mode.

> In order for this patch to do anything useful, we also need Sonny's
> patch at <https://patchwork.kernel.org/patch/4790921/>
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
> Changes in v2:
> - Add "#ifdef CONFIG_ARM" as per Will Deacon
> 
>  Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++
>  drivers/clocksource/arm_arch_timer.c                 | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
> index 37b2caf..876d32b 100644
> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
> @@ -22,6 +22,12 @@ to deliver its interrupts via SPIs.
>  - always-on : a boolean property. If present, the timer is powered through an
>    always-on power domain, therefore it never loses context.
>  
> +** Optional properties:
> +
> +- arm,use-physical-timer : Don't ever use the virtual timer, just use the
> +  physical one.  Not supported for ARM64.

I'd say `Only supported for ARM' to better match what we've done. Probably
also worth mentioning that this relies on the hypervisor/firmware having set
CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF
;) if you want to boot on the non-secure side (e.g. as a guest).

Will

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Doug Anderson]

Will,

On Thu, Sep 11, 2014 at 9:47 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote:
>> Some 32-bit (ARMv7) systems are architected like this:
>>
>> * The firmware doesn't know and doesn't care about hypervisor mode and
>>   we don't want to add the complexity of hypervisor there.
>>
>> * The firmware isn't involved in SMP bringup or resume.
>>
>> * The ARCH timer come up with an uninitialized offset between the
>>   virtual and physical counters.  Each core gets a different random
>>   offset.
>>
>> On systems like the above, it doesn't make sense to use the virtual
>> counter.  There's nobody managing the offset and each time a core goes
>> down and comes back up it will get reinitialized to some other random
>> value.
>
> You probably need to rephrase this slightly, as there *is* still a
> requirement on the hypervisor/firmware (actually, two!). See below.

Sure.  I added two more bullet points.


>> Let's add a property to the device tree to say that we shouldn't use
>> the virtual timer.  Firmware could potentially remove this property
>> before passing the device tree to the kernel if it really wants the
>> kernel to use a virtual timer.
>>
>> Note that it's been said that ARM64 (ARMv8) systems the firmware and
>> kernel really can't be architected as described above.  That means
>> using the physical timer like this really only makes sense for ARMv7
>> systems.
>
> I'd go further: this only makes sense if you're booting in secure SVC
> mode.

OK.


>> In order for this patch to do anything useful, we also need Sonny's
>> patch at <https://patchwork.kernel.org/patch/4790921/>
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> ---
>> Changes in v2:
>> - Add "#ifdef CONFIG_ARM" as per Will Deacon
>>
>>  Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++
>>  drivers/clocksource/arm_arch_timer.c                 | 5 +++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> index 37b2caf..876d32b 100644
>> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
>> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> @@ -22,6 +22,12 @@ to deliver its interrupts via SPIs.
>>  - always-on : a boolean property. If present, the timer is powered through an
>>    always-on power domain, therefore it never loses context.
>>
>> +** Optional properties:
>> +
>> +- arm,use-physical-timer : Don't ever use the virtual timer, just use the
>> +  physical one.  Not supported for ARM64.
>
> I'd say `Only supported for ARM' to better match what we've done. Probably
> also worth mentioning that this relies on the hypervisor/firmware having set
> CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF
> ;) if you want to boot on the non-secure side (e.g. as a guest).

Note that the reset value of CNTHCTL.PL1PCEN and CNTHCTL.PL1PCTEN are
both 1 in my version of the ARM ARM.  On the other hand CNTVOFF is
documented to have an UNKNOWN reset value.  If only ARM had guaranteed
that CNTVOFF started out as 0 (which seems like it would have been
sensible) we wouldn't be in this mess.  :-/

I've adjusted the wording.  Hopefully it looks good to you.

-Doug

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Marc Zyngier]

On 11/09/14 17:47, Will Deacon wrote:
> On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote:
>> Some 32-bit (ARMv7) systems are architected like this:
>>
>> * The firmware doesn't know and doesn't care about hypervisor mode and
>>   we don't want to add the complexity of hypervisor there.
>>
>> * The firmware isn't involved in SMP bringup or resume.
>>
>> * The ARCH timer come up with an uninitialized offset between the
>>   virtual and physical counters.  Each core gets a different random
>>   offset.
>>
>> On systems like the above, it doesn't make sense to use the virtual
>> counter.  There's nobody managing the offset and each time a core goes
>> down and comes back up it will get reinitialized to some other random
>> value.
> 
> You probably need to rephrase this slightly, as there *is* still a
> requirement on the hypervisor/firmware (actually, two!). See below.
> 
>> Let's add a property to the device tree to say that we shouldn't use
>> the virtual timer.  Firmware could potentially remove this property
>> before passing the device tree to the kernel if it really wants the
>> kernel to use a virtual timer.
>>
>> Note that it's been said that ARM64 (ARMv8) systems the firmware and
>> kernel really can't be architected as described above.  That means
>> using the physical timer like this really only makes sense for ARMv7
>> systems.
> 
> I'd go further: this only makes sense if you're booting in secure SVC
> mode.

If that's the case, what's the problem? Enter monitor mode, set SCR.NS
to one, nuke CNTVOFF, revert, job done.

What am I missing?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Will Deacon]

On Thu, Sep 11, 2014 at 05:59:53PM +0100, Doug Anderson wrote:
> On Thu, Sep 11, 2014 at 9:47 AM, Will Deacon <will.deacon@arm.com> wrote:
> > I'd say `Only supported for ARM' to better match what we've done. Probably
> > also worth mentioning that this relies on the hypervisor/firmware having set
> > CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF
> > ;) if you want to boot on the non-secure side (e.g. as a guest).
> 
> Note that the reset value of CNTHCTL.PL1PCEN and CNTHCTL.PL1PCTEN are
> both 1 in my version of the ARM ARM.  On the other hand CNTVOFF is
> documented to have an UNKNOWN reset value.  If only ARM had guaranteed
> that CNTVOFF started out as 0 (which seems like it would have been
> sensible) we wouldn't be in this mess.  :-/

I'm afraid we went the opposite way -- in ARMv8 there are a tiny handful
of EL3 registers that are well-defined out of reset, then the rest of the
system is UNKNOWN. The hardware guys prefer that and it can also be useful
for very low-level debugging (system crashes, do a reset, read out the
state).

Will

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Doug Anderson]

Hi,

On Thu, Sep 11, 2014 at 10:00 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/09/14 17:47, Will Deacon wrote:
>> On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote:
>>> Some 32-bit (ARMv7) systems are architected like this:
>>>
>>> * The firmware doesn't know and doesn't care about hypervisor mode and
>>>   we don't want to add the complexity of hypervisor there.
>>>
>>> * The firmware isn't involved in SMP bringup or resume.
>>>
>>> * The ARCH timer come up with an uninitialized offset between the
>>>   virtual and physical counters.  Each core gets a different random
>>>   offset.
>>>
>>> On systems like the above, it doesn't make sense to use the virtual
>>> counter.  There's nobody managing the offset and each time a core goes
>>> down and comes back up it will get reinitialized to some other random
>>> value.
>>
>> You probably need to rephrase this slightly, as there *is* still a
>> requirement on the hypervisor/firmware (actually, two!). See below.
>>
>>> Let's add a property to the device tree to say that we shouldn't use
>>> the virtual timer.  Firmware could potentially remove this property
>>> before passing the device tree to the kernel if it really wants the
>>> kernel to use a virtual timer.
>>>
>>> Note that it's been said that ARM64 (ARMv8) systems the firmware and
>>> kernel really can't be architected as described above.  That means
>>> using the physical timer like this really only makes sense for ARMv7
>>> systems.
>>
>> I'd go further: this only makes sense if you're booting in secure SVC
>> mode.
>
> If that's the case, what's the problem? Enter monitor mode, set SCR.NS
> to one, nuke CNTVOFF, revert, job done.
>
> What am I missing?

Stuff like this was talked about in the thread about Sonny's patch at
<https://patchwork.kernel.org/patch/4790921/>

...in that case we were always talking about HYP mode, though.  I
don't think anyone has explicitly talked about just switching to
monitor mode and then leaving ourselves in Secure SVC after we're
done.  It would be nice (especially for the VDSO guys) if we could
just init the virtual offset...

We would need to run this code potentially at processor bringup and
after suspend/resume, but that seems possible too.

Is the transition to monitor mode and back simple?  Where would you
suggest putting this code?  It would definitely need to be pretty
early.  We'd also need to be able to detect that we're in Secure SVC
and not mess up anyone else who happened to boot in Non Secure SVC.

-Doug

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Doug Anderson]

Will,

On Thu, Sep 11, 2014 at 10:07 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Sep 11, 2014 at 05:59:53PM +0100, Doug Anderson wrote:
>> On Thu, Sep 11, 2014 at 9:47 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > I'd say `Only supported for ARM' to better match what we've done. Probably
>> > also worth mentioning that this relies on the hypervisor/firmware having set
>> > CNTHCTL.PL1PCEN and CNTHCTL.EL1PCTEN (but assumedly made a mess of CNTVOFF
>> > ;) if you want to boot on the non-secure side (e.g. as a guest).
>>
>> Note that the reset value of CNTHCTL.PL1PCEN and CNTHCTL.PL1PCTEN are
>> both 1 in my version of the ARM ARM.  On the other hand CNTVOFF is
>> documented to have an UNKNOWN reset value.  If only ARM had guaranteed
>> that CNTVOFF started out as 0 (which seems like it would have been
>> sensible) we wouldn't be in this mess.  :-/
>
> I'm afraid we went the opposite way -- in ARMv8 there are a tiny handful
> of EL3 registers that are well-defined out of reset, then the rest of the
> system is UNKNOWN. The hardware guys prefer that and it can also be useful
> for very low-level debugging (system crashes, do a reset, read out the
> state).

Well, I guess if that's the way it is then software will have to deal
with it.  It sure seems like having things default to some state (even
if it's zero) at reset is really nice.  Otherwise you get things where
one 1 of every 10 times you boot the system it behaves differently and
that's hard to track down.  ...of course, then it maybe forces you to
think about really initting all bits in software and that's not
terrible.


-Doug

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Marc Zyngier]

On 11/09/14 18:11, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 11, 2014 at 10:00 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 11/09/14 17:47, Will Deacon wrote:
>>> On Thu, Sep 11, 2014 at 05:16:44PM +0100, Doug Anderson wrote:
>>>> Some 32-bit (ARMv7) systems are architected like this:
>>>>
>>>> * The firmware doesn't know and doesn't care about hypervisor mode and
>>>>   we don't want to add the complexity of hypervisor there.
>>>>
>>>> * The firmware isn't involved in SMP bringup or resume.
>>>>
>>>> * The ARCH timer come up with an uninitialized offset between the
>>>>   virtual and physical counters.  Each core gets a different random
>>>>   offset.
>>>>
>>>> On systems like the above, it doesn't make sense to use the virtual
>>>> counter.  There's nobody managing the offset and each time a core goes
>>>> down and comes back up it will get reinitialized to some other random
>>>> value.
>>>
>>> You probably need to rephrase this slightly, as there *is* still a
>>> requirement on the hypervisor/firmware (actually, two!). See below.
>>>
>>>> Let's add a property to the device tree to say that we shouldn't use
>>>> the virtual timer.  Firmware could potentially remove this property
>>>> before passing the device tree to the kernel if it really wants the
>>>> kernel to use a virtual timer.
>>>>
>>>> Note that it's been said that ARM64 (ARMv8) systems the firmware and
>>>> kernel really can't be architected as described above.  That means
>>>> using the physical timer like this really only makes sense for ARMv7
>>>> systems.
>>>
>>> I'd go further: this only makes sense if you're booting in secure SVC
>>> mode.
>>
>> If that's the case, what's the problem? Enter monitor mode, set SCR.NS
>> to one, nuke CNTVOFF, revert, job done.
>>
>> What am I missing?
> 
> Stuff like this was talked about in the thread about Sonny's patch at
> <https://patchwork.kernel.org/patch/4790921/>
> 
> ...in that case we were always talking about HYP mode, though.  I

That's because I always assumed that you'd be running non-secure,
dropped there by some idiotic firmware without any way to go back up.

> don't think anyone has explicitly talked about just switching to
> monitor mode and then leaving ourselves in Secure SVC after we're
> done.  It would be nice (especially for the VDSO guys) if we could
> just init the virtual offset...
> 
> We would need to run this code potentially at processor bringup and
> after suspend/resume, but that seems possible too.

Note that this would be an ARMv7 only thing (you can't do that on ARMv8,
at all).

> Is the transition to monitor mode and back simple?  Where would you
> suggest putting this code?  It would definitely need to be pretty
> early.  We'd also need to be able to detect that we're in Secure SVC
> and not mess up anyone else who happened to boot in Non Secure SVC.

This would have to live in some very early platform-specific code. The
ugly part is that you cannot find out what world you're in (accessing
SCR is going to send you to UNDEF-land if accessed from NS).

If I was suicidal, I'd suggest you could pass a parameter to the command
line, interpreted by the timer code... But I since I'm not, let's
pretend I haven't said anything... ;-)

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Doug Anderson]

Marc,

On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> We would need to run this code potentially at processor bringup and
>> after suspend/resume, but that seems possible too.
>
> Note that this would be an ARMv7 only thing (you can't do that on ARMv8,
> at all).

Yes, of course.


>> Is the transition to monitor mode and back simple?  Where would you
>> suggest putting this code?  It would definitely need to be pretty
>> early.  We'd also need to be able to detect that we're in Secure SVC
>> and not mess up anyone else who happened to boot in Non Secure SVC.
>
> This would have to live in some very early platform-specific code. The
> ugly part is that you cannot find out what world you're in (accessing
> SCR is going to send you to UNDEF-land if accessed from NS).

Yup, so the question is: would such code be accepted upstream, or are
we going to embark on a big job for someone to figure out how to do
this only to get NAKed?

If there was some indication that folks would take this, I think we
might be able to get it coded up.  If someone else wanted to volunteer
to code it that would make me even happier, but maybe that's pushing
my luck.  ;)


> If I was suicidal, I'd suggest you could pass a parameter to the command
> line, interpreted by the timer code... But I since I'm not, let's
> pretend I haven't said anything... ;-)

I did this in the past (again, see Sonny's thread), but didn't
consider myself knowledgeable to know if that was truly a good test:

       asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
       pr_info("DOUG: val is %#010x", val);
       val |= (1 << 2);
       asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
       val = 0xffffffff;
       asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
       pr_info("DOUG: val is %#010x", val);

The idea being that if you can make modifications to the SCR register
(and see your changes take effect) then you must be in secure mode.
In my case the first printout was 0x0 and the second was 0x4.

-Doug

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Marc Zyngier]

On 11/09/14 18:29, Doug Anderson wrote:
> Marc,
> 
> On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> We would need to run this code potentially at processor bringup and
>>> after suspend/resume, but that seems possible too.
>>
>> Note that this would be an ARMv7 only thing (you can't do that on ARMv8,
>> at all).
> 
> Yes, of course.
> 
> 
>>> Is the transition to monitor mode and back simple?  Where would you
>>> suggest putting this code?  It would definitely need to be pretty
>>> early.  We'd also need to be able to detect that we're in Secure SVC
>>> and not mess up anyone else who happened to boot in Non Secure SVC.
>>
>> This would have to live in some very early platform-specific code. The
>> ugly part is that you cannot find out what world you're in (accessing
>> SCR is going to send you to UNDEF-land if accessed from NS).
> 
> Yup, so the question is: would such code be accepted upstream, or are
> we going to embark on a big job for someone to figure out how to do
> this only to get NAKed?
> 
> If there was some indication that folks would take this, I think we
> might be able to get it coded up.  If someone else wanted to volunteer
> to code it that would make me even happier, but maybe that's pushing
> my luck.  ;)

Writing the code is a 5 minute job. Getting it accepted is another
story, and I'm not sure everyone would agree on that.

>> If I was suicidal, I'd suggest you could pass a parameter to the command
>> line, interpreted by the timer code... But I since I'm not, let's
>> pretend I haven't said anything... ;-)
> 
> I did this in the past (again, see Sonny's thread), but didn't
> consider myself knowledgeable to know if that was truly a good test:
> 
>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>        pr_info("DOUG: val is %#010x", val);
>        val |= (1 << 2);
>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>        val = 0xffffffff;
>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>        pr_info("DOUG: val is %#010x", val);
> 
> The idea being that if you can make modifications to the SCR register
> (and see your changes take effect) then you must be in secure mode.
> In my case the first printout was 0x0 and the second was 0x4.

The main issue is when you're *not* in secure mode. It is likely that
this will explode badly. This is why I suggested something that is set
by the bootloader (after all. it knows which mode it is booted in), and
that the timer driver can use when the CPU comes up.

Still, very ugly...

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Doug Anderson]

Marc,

On Thu, Sep 11, 2014 at 10:43 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/09/14 18:29, Doug Anderson wrote:
>> Marc,
>>
>> On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> We would need to run this code potentially at processor bringup and
>>>> after suspend/resume, but that seems possible too.
>>>
>>> Note that this would be an ARMv7 only thing (you can't do that on ARMv8,
>>> at all).
>>
>> Yes, of course.
>>
>>
>>>> Is the transition to monitor mode and back simple?  Where would you
>>>> suggest putting this code?  It would definitely need to be pretty
>>>> early.  We'd also need to be able to detect that we're in Secure SVC
>>>> and not mess up anyone else who happened to boot in Non Secure SVC.
>>>
>>> This would have to live in some very early platform-specific code. The
>>> ugly part is that you cannot find out what world you're in (accessing
>>> SCR is going to send you to UNDEF-land if accessed from NS).
>>
>> Yup, so the question is: would such code be accepted upstream, or are
>> we going to embark on a big job for someone to figure out how to do
>> this only to get NAKed?
>>
>> If there was some indication that folks would take this, I think we
>> might be able to get it coded up.  If someone else wanted to volunteer
>> to code it that would make me even happier, but maybe that's pushing
>> my luck.  ;)
>
> Writing the code is a 5 minute job. Getting it accepted is another
> story, and I'm not sure everyone would agree on that.
>
>>> If I was suicidal, I'd suggest you could pass a parameter to the command
>>> line, interpreted by the timer code... But I since I'm not, let's
>>> pretend I haven't said anything... ;-)
>>
>> I did this in the past (again, see Sonny's thread), but didn't
>> consider myself knowledgeable to know if that was truly a good test:
>>
>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>        pr_info("DOUG: val is %#010x", val);
>>        val |= (1 << 2);
>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>>        val = 0xffffffff;
>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>        pr_info("DOUG: val is %#010x", val);
>>
>> The idea being that if you can make modifications to the SCR register
>> (and see your changes take effect) then you must be in secure mode.
>> In my case the first printout was 0x0 and the second was 0x4.
>
> The main issue is when you're *not* in secure mode. It is likely that
> this will explode badly. This is why I suggested something that is set
> by the bootloader (after all. it knows which mode it is booted in), and
> that the timer driver can use when the CPU comes up.
>
> Still, very ugly...

Ah, got it.  Well, unless someone can suggest a clean way to do this,
then I guess we'll keep what we've got...

-Doug

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Stephen Boyd]

On 09/11/14 10:43, Marc Zyngier wrote:
>>> If I was suicidal, I'd suggest you could pass a parameter to the command
>>> line, interpreted by the timer code... But I since I'm not, let's
>>> pretend I haven't said anything... ;-)
>> I did this in the past (again, see Sonny's thread), but didn't
>> consider myself knowledgeable to know if that was truly a good test:
>>
>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>        pr_info("DOUG: val is %#010x", val);
>>        val |= (1 << 2);
>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>>        val = 0xffffffff;
>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>        pr_info("DOUG: val is %#010x", val);
>>
>> The idea being that if you can make modifications to the SCR register
>> (and see your changes take effect) then you must be in secure mode.
>> In my case the first printout was 0x0 and the second was 0x4.
> The main issue is when you're *not* in secure mode. It is likely that
> this will explode badly. This is why I suggested something that is set
> by the bootloader (after all. it knows which mode it is booted in), and
> that the timer driver can use when the CPU comes up.

Where does this platform jump to when a CPU comes up? Is it
rockchip_secondary_startup()? I wonder if that path could have this
little bit of assembly to poke the cntvoff in monitor mode and then jump
to secondary_startup()? Before we boot any secondary CPUs we could also
read the cntvoff for CPU0 in the platform specific layer (where we know
we're running in secure mode) and then use that value as the "reset"
value for the secondaries. Or does this platform boot up in secure mode
some times and non-secure mode other times?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Doug Anderson]

Stephen,

On Thu, Sep 11, 2014 at 4:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/11/14 10:43, Marc Zyngier wrote:
>>>> If I was suicidal, I'd suggest you could pass a parameter to the command
>>>> line, interpreted by the timer code... But I since I'm not, let's
>>>> pretend I haven't said anything... ;-)
>>> I did this in the past (again, see Sonny's thread), but didn't
>>> consider myself knowledgeable to know if that was truly a good test:
>>>
>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>        pr_info("DOUG: val is %#010x", val);
>>>        val |= (1 << 2);
>>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>>>        val = 0xffffffff;
>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>        pr_info("DOUG: val is %#010x", val);
>>>
>>> The idea being that if you can make modifications to the SCR register
>>> (and see your changes take effect) then you must be in secure mode.
>>> In my case the first printout was 0x0 and the second was 0x4.
>> The main issue is when you're *not* in secure mode. It is likely that
>> this will explode badly. This is why I suggested something that is set
>> by the bootloader (after all. it knows which mode it is booted in), and
>> that the timer driver can use when the CPU comes up.
>
> Where does this platform jump to when a CPU comes up? Is it
> rockchip_secondary_startup()? I wonder if that path could have this
> little bit of assembly to poke the cntvoff in monitor mode and then jump
> to secondary_startup()? Before we boot any secondary CPUs we could also
> read the cntvoff for CPU0 in the platform specific layer (where we know
> we're running in secure mode) and then use that value as the "reset"
> value for the secondaries. Or does this platform boot up in secure mode
> some times and non-secure mode other times?

I guess it would depend a whole lot on the bootloader, wouldn't it?

With our current "get out of the way" bootloader, Linux always sees
"Secure SVC".  ...but if someone decided to put a new bootloader on
the system that wanted to do something different (implement security
and boot the kernel in nonsecure HYP or implement a hypervisor and
boot the kernel in nonsecure SVC) then everything would be different.

If someone were to write a bootloader like that (or perhaps if we're
running in a VM?) then I'd imagine that the whole world would be
different.  Somehow this secure bootloader and/or hypervisor would
_have_ to be involved in processor bringup and suspend/resume.  Since
I've never looked at code implementing either of these I'm just making
assumptions, though.

-Doug

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Sonny Rao]

On Thu, Sep 11, 2014 at 6:17 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/11/14 17:14, Sonny Rao wrote:
>
> On Thu, Sep 11, 2014 at 4:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>>
>> Where does this platform jump to when a CPU comes up? Is it
>> rockchip_secondary_startup()? I wonder if that path could have this
>> little bit of assembly to poke the cntvoff in monitor mode and then jump
>> to secondary_startup()? Before we boot any secondary CPUs we could also
>> read the cntvoff for CPU0 in the platform specific layer (where we know
>> we're running in secure mode) and then use that value as the "reset"
>> value for the secondaries. Or does this platform boot up in secure mode
>> some times and non-secure mode other times?
>
>
> Yes, In our case, with our firmware, we will go through some internal Rom
> code and then jump to rockchip_secondary_startup, but I don't think it's
> correct to force all users of this SoC to do it that way.
>
>
> What's being forced? The way internal rom jumps to sram? Is there any other
> way that secondary CPUs come out of reset on this SoC? From looking at the
> code it seems like the only path is internal rom jumps to sram (where
> rockchip_secondary_trampoline lives) which jumps to
> rockchip_secondary_startup() which then does an invalidate and jump to
> secondary_startup(). Linux controls everything besides the internal rom. Is
> something different in your case?


There are other ways it can be done, and I don't know all of the
possibilities, but there seems to be some protocol with the iROM that
tells it where to go, which the current SMP patches are using by
putting a magic number and an address in SRAM.  I think it's true that
in our case, it really is pretty simple and we have secure SVC mode
and not much else runs (besides the iROM).

Since I don't know all of the possibilities, I didn't want to preclude
the possibility that someone else handled things differently and
entered the kernel in non-secure mode, and have some code there that
broke in that instance, that's all I meant by "forced".

>
>  If there were a reasonable way to determine for sure that we are in secure
> mode, then yes we could do what you're suggesting, and I'd be happy to code
> that up.
>
>
> I think the problem is that there isn't a great way to determine whether
> we're in secure mode or not, and this is maybe by design?  I don't
> particularly understand that design choice.  It would be nice to hear some
> rationale from ARM folks.
>
>
> I'm thinking we would have a different boot-method for secure vs. non-secure
> and then we would know to configure cntvoff or not based on the boot method.
> Isn't that a reasonable way of knowing what should be done? It seems like we
> can at least modify the DT for this SoC.

Putting something into the device-tree is in fact the point of this
patch, so it is sort of doing what you're suggesting, although this
patch is about being able use to physical counters and doesn't
indicate anything about secure vs non-secure.  What else do you think
could be used to differentiate between the two cases, besides putting
it into the DT?

>
> I still wonder if there is such a bootloader/hypervisor/rom that's putting
> this SoC into non-secure mode and not configuring cntvoff. Doug's comments
> seem to suggest that the whole world would be different if this were true.
> Maybe Heiko knows?

As far as I'm aware, there's no bootloader/firmware that's ever
putting the CPU into non-secure mode for our case.

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Marc Zyngier]

On 12/09/14 01:01, Doug Anderson wrote:
> Stephen,
> 
> On Thu, Sep 11, 2014 at 4:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 09/11/14 10:43, Marc Zyngier wrote:
>>>>> If I was suicidal, I'd suggest you could pass a parameter to the command
>>>>> line, interpreted by the timer code... But I since I'm not, let's
>>>>> pretend I haven't said anything... ;-)
>>>> I did this in the past (again, see Sonny's thread), but didn't
>>>> consider myself knowledgeable to know if that was truly a good test:
>>>>
>>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>>        pr_info("DOUG: val is %#010x", val);
>>>>        val |= (1 << 2);
>>>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>>>>        val = 0xffffffff;
>>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>>        pr_info("DOUG: val is %#010x", val);
>>>>
>>>> The idea being that if you can make modifications to the SCR register
>>>> (and see your changes take effect) then you must be in secure mode.
>>>> In my case the first printout was 0x0 and the second was 0x4.
>>> The main issue is when you're *not* in secure mode. It is likely that
>>> this will explode badly. This is why I suggested something that is set
>>> by the bootloader (after all. it knows which mode it is booted in), and
>>> that the timer driver can use when the CPU comes up.
>>
>> Where does this platform jump to when a CPU comes up? Is it
>> rockchip_secondary_startup()? I wonder if that path could have this
>> little bit of assembly to poke the cntvoff in monitor mode and then jump
>> to secondary_startup()? Before we boot any secondary CPUs we could also
>> read the cntvoff for CPU0 in the platform specific layer (where we know
>> we're running in secure mode) and then use that value as the "reset"
>> value for the secondaries. Or does this platform boot up in secure mode
>> some times and non-secure mode other times?
> 
> I guess it would depend a whole lot on the bootloader, wouldn't it?

Yes, hence my suggestion of hooking such a thing from the timer code,
where we could have a clue what to do (and what not to).

> With our current "get out of the way" bootloader, Linux always sees
> "Secure SVC".  ...but if someone decided to put a new bootloader on
> the system that wanted to do something different (implement security
> and boot the kernel in nonsecure HYP or implement a hypervisor and
> boot the kernel in nonsecure SVC) then everything would be different.
> 
> If someone were to write a bootloader like that (or perhaps if we're
> running in a VM?) then I'd imagine that the whole world would be
> different.  Somehow this secure bootloader and/or hypervisor would
> _have_ to be involved in processor bringup and suspend/resume.  Since
> I've never looked at code implementing either of these I'm just making
> assumptions, though.

Exactly. That's why we're so hell bent on PSCI, because it solves all
these issues (and that's why I've added some rudimentary support for it
in u-boot).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Christopher Covington]

Hi Marc,

On 09/11/2014 01:43 PM, Marc Zyngier wrote:
> On 11/09/14 18:29, Doug Anderson wrote:
>> Marc,
>>
>> On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> We would need to run this code potentially at processor bringup and
>>>> after suspend/resume, but that seems possible too.
>>>
>>> Note that this would be an ARMv7 only thing (you can't do that on ARMv8,
>>> at all).
>>
>> Yes, of course.
>>
>>
>>>> Is the transition to monitor mode and back simple?  Where would you
>>>> suggest putting this code?  It would definitely need to be pretty
>>>> early.  We'd also need to be able to detect that we're in Secure SVC
>>>> and not mess up anyone else who happened to boot in Non Secure SVC.
>>>
>>> This would have to live in some very early platform-specific code. The
>>> ugly part is that you cannot find out what world you're in (accessing
>>> SCR is going to send you to UNDEF-land if accessed from NS).
>>
>> Yup, so the question is: would such code be accepted upstream, or are
>> we going to embark on a big job for someone to figure out how to do
>> this only to get NAKed?
>>
>> If there was some indication that folks would take this, I think we
>> might be able to get it coded up.  If someone else wanted to volunteer
>> to code it that would make me even happier, but maybe that's pushing
>> my luck.  ;)
> 
> Writing the code is a 5 minute job. Getting it accepted is another
> story, and I'm not sure everyone would agree on that.
> 
>>> If I was suicidal, I'd suggest you could pass a parameter to the command
>>> line, interpreted by the timer code... But I since I'm not, let's
>>> pretend I haven't said anything... ;-)
>>
>> I did this in the past (again, see Sonny's thread), but didn't
>> consider myself knowledgeable to know if that was truly a good test:
>>
>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>        pr_info("DOUG: val is %#010x", val);
>>        val |= (1 << 2);
>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>>        val = 0xffffffff;
>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>        pr_info("DOUG: val is %#010x", val);
>>
>> The idea being that if you can make modifications to the SCR register
>> (and see your changes take effect) then you must be in secure mode.
>> In my case the first printout was 0x0 and the second was 0x4.
> 
> The main issue is when you're *not* in secure mode. It is likely that
> this will explode badly. This is why I suggested something that is set
> by the bootloader (after all. it knows which mode it is booted in), and
> that the timer driver can use when the CPU comes up.

What exactly does "exploding badly" look like? Causing and undefined
instruction exception? That's just a branch with a mode switch. Any reason the
code couldn't deal with that or even use that to its advantage?

Thanks,
Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Marc Zyngier]

Hi Christopher,

On 12/09/14 12:43, Christopher Covington wrote:
> Hi Marc,
> 
> On 09/11/2014 01:43 PM, Marc Zyngier wrote:
>> On 11/09/14 18:29, Doug Anderson wrote:
>>> Marc,
>>>
>>> On Thu, Sep 11, 2014 at 10:22 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> We would need to run this code potentially at processor bringup and
>>>>> after suspend/resume, but that seems possible too.
>>>>
>>>> Note that this would be an ARMv7 only thing (you can't do that on ARMv8,
>>>> at all).
>>>
>>> Yes, of course.
>>>
>>>
>>>>> Is the transition to monitor mode and back simple?  Where would you
>>>>> suggest putting this code?  It would definitely need to be pretty
>>>>> early.  We'd also need to be able to detect that we're in Secure SVC
>>>>> and not mess up anyone else who happened to boot in Non Secure SVC.
>>>>
>>>> This would have to live in some very early platform-specific code. The
>>>> ugly part is that you cannot find out what world you're in (accessing
>>>> SCR is going to send you to UNDEF-land if accessed from NS).
>>>
>>> Yup, so the question is: would such code be accepted upstream, or are
>>> we going to embark on a big job for someone to figure out how to do
>>> this only to get NAKed?
>>>
>>> If there was some indication that folks would take this, I think we
>>> might be able to get it coded up.  If someone else wanted to volunteer
>>> to code it that would make me even happier, but maybe that's pushing
>>> my luck.  ;)
>>
>> Writing the code is a 5 minute job. Getting it accepted is another
>> story, and I'm not sure everyone would agree on that.
>>
>>>> If I was suicidal, I'd suggest you could pass a parameter to the command
>>>> line, interpreted by the timer code... But I since I'm not, let's
>>>> pretend I haven't said anything... ;-)
>>>
>>> I did this in the past (again, see Sonny's thread), but didn't
>>> consider myself knowledgeable to know if that was truly a good test:
>>>
>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>        pr_info("DOUG: val is %#010x", val);
>>>        val |= (1 << 2);
>>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>>>        val = 0xffffffff;
>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>        pr_info("DOUG: val is %#010x", val);
>>>
>>> The idea being that if you can make modifications to the SCR register
>>> (and see your changes take effect) then you must be in secure mode.
>>> In my case the first printout was 0x0 and the second was 0x4.
>>
>> The main issue is when you're *not* in secure mode. It is likely that
>> this will explode badly. This is why I suggested something that is set
>> by the bootloader (after all. it knows which mode it is booted in), and
>> that the timer driver can use when the CPU comes up.
> 
> What exactly does "exploding badly" look like? Causing and undefined
> instruction exception? That's just a branch with a mode switch. Any reason the
> code couldn't deal with that or even use that to its advantage?

We surely can handle the UNDEF and do something there. We just can't do
it the way Doug described it above.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Stephen Boyd]

On 09/12/14 05:14, Marc Zyngier wrote:
> Hi Christopher,
>
> On 12/09/14 12:43, Christopher Covington wrote:
>> Hi Marc,
>>
>> On 09/11/2014 01:43 PM, Marc Zyngier wrote:
>>> On 11/09/14 18:29, Doug Anderson wrote:
>>>
>>>> I did this in the past (again, see Sonny's thread), but didn't
>>>> consider myself knowledgeable to know if that was truly a good test:
>>>>
>>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>>        pr_info("DOUG: val is %#010x", val);
>>>>        val |= (1 << 2);
>>>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
>>>>        val = 0xffffffff;
>>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
>>>>        pr_info("DOUG: val is %#010x", val);
>>>>
>>>> The idea being that if you can make modifications to the SCR register
>>>> (and see your changes take effect) then you must be in secure mode.
>>>> In my case the first printout was 0x0 and the second was 0x4.
>>> The main issue is when you're *not* in secure mode. It is likely that
>>> this will explode badly. This is why I suggested something that is set
>>> by the bootloader (after all. it knows which mode it is booted in), and
>>> that the timer driver can use when the CPU comes up.
>> What exactly does "exploding badly" look like? Causing and undefined
>> instruction exception? That's just a branch with a mode switch. Any reason the
>> code couldn't deal with that or even use that to its advantage?
> We surely can handle the UNDEF and do something there. We just can't do
> it the way Doug described it above.
>
>

I suggested doing that for something else a while ago and Will and Dave
we're not thrilled[1]. The suggestion back then was to use DT to
indicate what mode the kernel is running in.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Catalin Marinas]

On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
> On 09/12/14 05:14, Marc Zyngier wrote:
> > On 12/09/14 12:43, Christopher Covington wrote:
> >> On 09/11/2014 01:43 PM, Marc Zyngier wrote:
> >>> On 11/09/14 18:29, Doug Anderson wrote:
> >>>
> >>>> I did this in the past (again, see Sonny's thread), but didn't
> >>>> consider myself knowledgeable to know if that was truly a good test:
> >>>>
> >>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
> >>>>        pr_info("DOUG: val is %#010x", val);
> >>>>        val |= (1 << 2);
> >>>>        asm volatile("mcr p15, 0, %0, c1, c1, 0" : : "r" (val));
> >>>>        val = 0xffffffff;
> >>>>        asm volatile("mrc p15, 0, %0, c1, c1, 0" : "=r" (val));
> >>>>        pr_info("DOUG: val is %#010x", val);
> >>>>
> >>>> The idea being that if you can make modifications to the SCR register
> >>>> (and see your changes take effect) then you must be in secure mode.
> >>>> In my case the first printout was 0x0 and the second was 0x4.

BTW, if you want to change the SCR.NS bit (and CNTVOFF), the kernel must
run in Monitor mode (by setting the CPSR mode bits, 32-bit only).

> >>> The main issue is when you're *not* in secure mode. It is likely that
> >>> this will explode badly. This is why I suggested something that is set
> >>> by the bootloader (after all. it knows which mode it is booted in), and
> >>> that the timer driver can use when the CPU comes up.
> >>
> >> What exactly does "exploding badly" look like? Causing and undefined
> >> instruction exception? That's just a branch with a mode switch. Any reason the
> >> code couldn't deal with that or even use that to its advantage?
> >
> > We surely can handle the UNDEF and do something there. We just can't do
> > it the way Doug described it above.
> 
> I suggested doing that for something else a while ago and Will and Dave
> we're not thrilled[1]. The suggestion back then was to use DT to
> indicate what mode the kernel is running in.
> 
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html

I think the context was slightly different. As I re-read the thread, it
seems that the discussion was around whether to use some SMC interface
or not based on whether the kernel is running secure or non-secure. The
argument made by Will was to actually specify the type of the firmware
SMC interface in the DT and use it in the kernel (and probably assume
the kernel is running in secure mode if no smc interface is specified in
the DT; you could have both though, running in secure mode and also
having firmware).

In this arch timer case, we need to work around a firmware bug (or
feature as 32-bit ARM kernels never required CNTVOFF initialisation by
firmware, no matter how small such firmware is). We don't expect a
specific SMC call to initialise CNTVOFF, so we can't describe it in the
DT.

One problem with undef for detecting whether the core is in secure or
non-secure mode is that sometimes the initialisation code needs to run
very early when the kernel hooks may not be fully initialised. We could
only detect this mode on the booting CPU and save it in a global
variable (of course, assuming the other CPUs boot in the same mode).
Other code could make use of such information as appropriate. Of course,
there is always a risk that it will be abused.

-- 
Catalin

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Stephen Boyd]

On 09/15/14 04:10, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
>> On 09/12/14 05:14, Marc Zyngier wrote:
>>> We surely can handle the UNDEF and do something there. We just can't do
>>> it the way Doug described it above.
>> I suggested doing that for something else a while ago and Will and Dave
>> we're not thrilled[1]. The suggestion back then was to use DT to
>> indicate what mode the kernel is running in.
>>
>> [1]
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
> I think the context was slightly different. As I re-read the thread, it
> seems that the discussion was around whether to use some SMC interface
> or not based on whether the kernel is running secure or non-secure. The
> argument made by Will was to actually specify the type of the firmware
> SMC interface in the DT and use it in the kernel (and probably assume
> the kernel is running in secure mode if no smc interface is specified in
> the DT; you could have both though, running in secure mode and also
> having firmware).
>
> In this arch timer case, we need to work around a firmware bug (or
> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
> firmware, no matter how small such firmware is). We don't expect a
> specific SMC call to initialise CNTVOFF, so we can't describe it in the
> DT.

Agreed, we can't described SMC calls that don't exist. From my
perspective it's just another part of the cpu boot sequence that needs
to be handled in the kernel, so describing the requirement via the
cpu-boot method seems appropriate. It seems like we're making it harder
than it should be by handling the undef when we could have slightly
different SMP boot code (and suspend/resume code) depending on the boot
method property.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Sonny Rao]

On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> On 09/15/14 04:10, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
> >> On 09/12/14 05:14, Marc Zyngier wrote:
> >>> We surely can handle the UNDEF and do something there. We just can't do
> >>> it the way Doug described it above.
> >> I suggested doing that for something else a while ago and Will and Dave
> >> we're not thrilled[1]. The suggestion back then was to use DT to
> >> indicate what mode the kernel is running in.
> >>
> >> [1]
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
> > I think the context was slightly different. As I re-read the thread, it
> > seems that the discussion was around whether to use some SMC interface
> > or not based on whether the kernel is running secure or non-secure. The
> > argument made by Will was to actually specify the type of the firmware
> > SMC interface in the DT and use it in the kernel (and probably assume
> > the kernel is running in secure mode if no smc interface is specified in
> > the DT; you could have both though, running in secure mode and also
> > having firmware).
> >
> > In this arch timer case, we need to work around a firmware bug (or
> > feature as 32-bit ARM kernels never required CNTVOFF initialisation by
> > firmware, no matter how small such firmware is). We don't expect a
> > specific SMC call to initialise CNTVOFF, so we can't describe it in the
> > DT.
>
> Agreed, we can't described SMC calls that don't exist. From my
> perspective it's just another part of the cpu boot sequence that needs
> to be handled in the kernel, so describing the requirement via the
> cpu-boot method seems appropriate. It seems like we're making it harder
> than it should be by handling the undef when we could have slightly
> different SMP boot code (and suspend/resume code) depending on the boot
> method property.


+heiko

So, for the case of rk3288, based on this discussion what I'm going to
propose is to add code to rockchip.c which looks for a particular SMP
enable method -- say something like "rockchip,rk3288-smp-secure-svc"
which will then assume we have been booted in secure SVC mode and do
the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
as well, so I think it will need to scan the DT fairly early on the
boot CPU and also perform the function there.

I'll look into implementing this and post code.  Comments and
suggestions appreciated, thanks.


>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Stephen Boyd]

On 09/15/14 14:47, Sonny Rao wrote:
> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 09/15/14 04:10, Catalin Marinas wrote:
>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
>>>> On 09/12/14 05:14, Marc Zyngier wrote:
>>>>> We surely can handle the UNDEF and do something there. We just can't do
>>>>> it the way Doug described it above.
>>>> I suggested doing that for something else a while ago and Will and Dave
>>>> we're not thrilled[1]. The suggestion back then was to use DT to
>>>> indicate what mode the kernel is running in.
>>>>
>>>> [1]
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
>>> I think the context was slightly different. As I re-read the thread, it
>>> seems that the discussion was around whether to use some SMC interface
>>> or not based on whether the kernel is running secure or non-secure. The
>>> argument made by Will was to actually specify the type of the firmware
>>> SMC interface in the DT and use it in the kernel (and probably assume
>>> the kernel is running in secure mode if no smc interface is specified in
>>> the DT; you could have both though, running in secure mode and also
>>> having firmware).
>>>
>>> In this arch timer case, we need to work around a firmware bug (or
>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
>>> firmware, no matter how small such firmware is). We don't expect a
>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the
>>> DT.
>> Agreed, we can't described SMC calls that don't exist. From my
>> perspective it's just another part of the cpu boot sequence that needs
>> to be handled in the kernel, so describing the requirement via the
>> cpu-boot method seems appropriate. It seems like we're making it harder
>> than it should be by handling the undef when we could have slightly
>> different SMP boot code (and suspend/resume code) depending on the boot
>> method property.
>
> +heiko
>
> So, for the case of rk3288, based on this discussion what I'm going to
> propose is to add code to rockchip.c which looks for a particular SMP
> enable method -- say something like "rockchip,rk3288-smp-secure-svc"
> which will then assume we have been booted in secure SVC mode and do
> the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
> as well, so I think it will need to scan the DT fairly early on the
> boot CPU and also perform the function there.
>
> I'll look into implementing this and post code.  Comments and
> suggestions appreciated, thanks.

What goes wrong if we read the cntvoff from the boot CPU during
smp_prepare_cpus() phase and use that to set the cntvoff on the other
CPUs? That avoids needing to do anything very early by making the value
the same. It does mean that cntvoff is some random out of reset value
for CPU0, but at least it's consistent.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Sonny Rao]

On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/15/14 14:47, Sonny Rao wrote:
>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>> On 09/15/14 04:10, Catalin Marinas wrote:
>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
>>>>> On 09/12/14 05:14, Marc Zyngier wrote:
>>>>>> We surely can handle the UNDEF and do something there. We just can't do
>>>>>> it the way Doug described it above.
>>>>> I suggested doing that for something else a while ago and Will and Dave
>>>>> we're not thrilled[1]. The suggestion back then was to use DT to
>>>>> indicate what mode the kernel is running in.
>>>>>
>>>>> [1]
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
>>>> I think the context was slightly different. As I re-read the thread, it
>>>> seems that the discussion was around whether to use some SMC interface
>>>> or not based on whether the kernel is running secure or non-secure. The
>>>> argument made by Will was to actually specify the type of the firmware
>>>> SMC interface in the DT and use it in the kernel (and probably assume
>>>> the kernel is running in secure mode if no smc interface is specified in
>>>> the DT; you could have both though, running in secure mode and also
>>>> having firmware).
>>>>
>>>> In this arch timer case, we need to work around a firmware bug (or
>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
>>>> firmware, no matter how small such firmware is). We don't expect a
>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the
>>>> DT.
>>> Agreed, we can't described SMC calls that don't exist. From my
>>> perspective it's just another part of the cpu boot sequence that needs
>>> to be handled in the kernel, so describing the requirement via the
>>> cpu-boot method seems appropriate. It seems like we're making it harder
>>> than it should be by handling the undef when we could have slightly
>>> different SMP boot code (and suspend/resume code) depending on the boot
>>> method property.
>>
>> +heiko
>>
>> So, for the case of rk3288, based on this discussion what I'm going to
>> propose is to add code to rockchip.c which looks for a particular SMP
>> enable method -- say something like "rockchip,rk3288-smp-secure-svc"
>> which will then assume we have been booted in secure SVC mode and do
>> the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
>> as well, so I think it will need to scan the DT fairly early on the
>> boot CPU and also perform the function there.
>>
>> I'll look into implementing this and post code.  Comments and
>> suggestions appreciated, thanks.
>
> What goes wrong if we read the cntvoff from the boot CPU during
> smp_prepare_cpus() phase and use that to set the cntvoff on the other
> CPUs? That avoids needing to do anything very early by making the value
> the same. It does mean that cntvoff is some random out of reset value
> for CPU0, but at least it's consistent.

I think we cannot read the value if we're not in hyp mode.


>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
>

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Sonny Rao]

On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 09/15/14 14:47, Sonny Rao wrote:
>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>> On 09/15/14 04:10, Catalin Marinas wrote:
>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
>>>>>> On 09/12/14 05:14, Marc Zyngier wrote:
>>>>>>> We surely can handle the UNDEF and do something there. We just can't do
>>>>>>> it the way Doug described it above.
>>>>>> I suggested doing that for something else a while ago and Will and Dave
>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to
>>>>>> indicate what mode the kernel is running in.
>>>>>>
>>>>>> [1]
>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
>>>>> I think the context was slightly different. As I re-read the thread, it
>>>>> seems that the discussion was around whether to use some SMC interface
>>>>> or not based on whether the kernel is running secure or non-secure. The
>>>>> argument made by Will was to actually specify the type of the firmware
>>>>> SMC interface in the DT and use it in the kernel (and probably assume
>>>>> the kernel is running in secure mode if no smc interface is specified in
>>>>> the DT; you could have both though, running in secure mode and also
>>>>> having firmware).
>>>>>
>>>>> In this arch timer case, we need to work around a firmware bug (or
>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
>>>>> firmware, no matter how small such firmware is). We don't expect a
>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the
>>>>> DT.
>>>> Agreed, we can't described SMC calls that don't exist. From my
>>>> perspective it's just another part of the cpu boot sequence that needs
>>>> to be handled in the kernel, so describing the requirement via the
>>>> cpu-boot method seems appropriate. It seems like we're making it harder
>>>> than it should be by handling the undef when we could have slightly
>>>> different SMP boot code (and suspend/resume code) depending on the boot
>>>> method property.
>>>
>>> +heiko
>>>
>>> So, for the case of rk3288, based on this discussion what I'm going to
>>> propose is to add code to rockchip.c which looks for a particular SMP
>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc"
>>> which will then assume we have been booted in secure SVC mode and do
>>> the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
>>> as well, so I think it will need to scan the DT fairly early on the
>>> boot CPU and also perform the function there.
>>>
>>> I'll look into implementing this and post code.  Comments and
>>> suggestions appreciated, thanks.
>>
>> What goes wrong if we read the cntvoff from the boot CPU during
>> smp_prepare_cpus() phase and use that to set the cntvoff on the other
>> CPUs? That avoids needing to do anything very early by making the value
>> the same. It does mean that cntvoff is some random out of reset value
>> for CPU0, but at least it's consistent.
>
> I think we cannot read the value if we're not in hyp mode.

Well, thinking about it a little more, I think you still have a good point.

We don't need to do this early on, as long as we haven't started using
the arch timers yet.  If we are still able to do this at the point
where we're executing the code in arch/arm/mach-rockchip/platsmp.c
that finds the enable method then we can just handle it there.

>
>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> hosted by The Linux Foundation
>>

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Christopher Covington]

Hi Sonny,

On 09/15/2014 06:04 PM, Sonny Rao wrote:
> On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>> On 09/15/14 14:47, Sonny Rao wrote:
>>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>>> On 09/15/14 04:10, Catalin Marinas wrote:
>>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
>>>>>>> On 09/12/14 05:14, Marc Zyngier wrote:
>>>>>>>> We surely can handle the UNDEF and do something there. We just can't do
>>>>>>>> it the way Doug described it above.
>>>>>>> I suggested doing that for something else a while ago and Will and Dave
>>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to
>>>>>>> indicate what mode the kernel is running in.
>>>>>>>
>>>>>>> [1]
>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
>>>>>> I think the context was slightly different. As I re-read the thread, it
>>>>>> seems that the discussion was around whether to use some SMC interface
>>>>>> or not based on whether the kernel is running secure or non-secure. The
>>>>>> argument made by Will was to actually specify the type of the firmware
>>>>>> SMC interface in the DT and use it in the kernel (and probably assume
>>>>>> the kernel is running in secure mode if no smc interface is specified in
>>>>>> the DT; you could have both though, running in secure mode and also
>>>>>> having firmware).
>>>>>>
>>>>>> In this arch timer case, we need to work around a firmware bug (or
>>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
>>>>>> firmware, no matter how small such firmware is). We don't expect a
>>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the
>>>>>> DT.
>>>>> Agreed, we can't described SMC calls that don't exist. From my
>>>>> perspective it's just another part of the cpu boot sequence that needs
>>>>> to be handled in the kernel, so describing the requirement via the
>>>>> cpu-boot method seems appropriate. It seems like we're making it harder
>>>>> than it should be by handling the undef when we could have slightly
>>>>> different SMP boot code (and suspend/resume code) depending on the boot
>>>>> method property.
>>>>
>>>> +heiko
>>>>
>>>> So, for the case of rk3288, based on this discussion what I'm going to
>>>> propose is to add code to rockchip.c which looks for a particular SMP
>>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc"
>>>> which will then assume we have been booted in secure SVC mode and do
>>>> the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
>>>> as well, so I think it will need to scan the DT fairly early on the
>>>> boot CPU and also perform the function there.
>>>>
>>>> I'll look into implementing this and post code.  Comments and
>>>> suggestions appreciated, thanks.
>>>
>>> What goes wrong if we read the cntvoff from the boot CPU during
>>> smp_prepare_cpus() phase and use that to set the cntvoff on the other
>>> CPUs? That avoids needing to do anything very early by making the value
>>> the same. It does mean that cntvoff is some random out of reset value
>>> for CPU0, but at least it's consistent.
>>
>> I think we cannot read the value if we're not in hyp mode.
> 
> Well, thinking about it a little more, I think you still have a good point.
> 
> We don't need to do this early on, as long as we haven't started using
> the arch timers yet.  If we are still able to do this at the point
> where we're executing the code in arch/arm/mach-rockchip/platsmp.c
> that finds the enable method then we can just handle it there.

I've been playing around with the probe-based approach and while I need to do
a lot more testing, it seems to be working for the first tens of instructions.
I hope to be able to share a draft of that soon. Basically, I just read the
current NSACR value and write it back (although maybe in the long term we
would want to make sure a few of those bits are set or cleared). If that
succeeds, we know we're in secure SVC and can proceed to set up MON and HYP.

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Sonny Rao]

On Mon, Sep 15, 2014 at 3:51 PM, Christopher Covington
<cov@codeaurora.org> wrote:
> Hi Sonny,
>
> On 09/15/2014 06:04 PM, Sonny Rao wrote:
>> On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>>> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>> On 09/15/14 14:47, Sonny Rao wrote:
>>>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>>>> On 09/15/14 04:10, Catalin Marinas wrote:
>>>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
>>>>>>>> On 09/12/14 05:14, Marc Zyngier wrote:
>>>>>>>>> We surely can handle the UNDEF and do something there. We just can't do
>>>>>>>>> it the way Doug described it above.
>>>>>>>> I suggested doing that for something else a while ago and Will and Dave
>>>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to
>>>>>>>> indicate what mode the kernel is running in.
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
>>>>>>> I think the context was slightly different. As I re-read the thread, it
>>>>>>> seems that the discussion was around whether to use some SMC interface
>>>>>>> or not based on whether the kernel is running secure or non-secure. The
>>>>>>> argument made by Will was to actually specify the type of the firmware
>>>>>>> SMC interface in the DT and use it in the kernel (and probably assume
>>>>>>> the kernel is running in secure mode if no smc interface is specified in
>>>>>>> the DT; you could have both though, running in secure mode and also
>>>>>>> having firmware).
>>>>>>>
>>>>>>> In this arch timer case, we need to work around a firmware bug (or
>>>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
>>>>>>> firmware, no matter how small such firmware is). We don't expect a
>>>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the
>>>>>>> DT.
>>>>>> Agreed, we can't described SMC calls that don't exist. From my
>>>>>> perspective it's just another part of the cpu boot sequence that needs
>>>>>> to be handled in the kernel, so describing the requirement via the
>>>>>> cpu-boot method seems appropriate. It seems like we're making it harder
>>>>>> than it should be by handling the undef when we could have slightly
>>>>>> different SMP boot code (and suspend/resume code) depending on the boot
>>>>>> method property.
>>>>>
>>>>> +heiko
>>>>>
>>>>> So, for the case of rk3288, based on this discussion what I'm going to
>>>>> propose is to add code to rockchip.c which looks for a particular SMP
>>>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc"
>>>>> which will then assume we have been booted in secure SVC mode and do
>>>>> the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
>>>>> as well, so I think it will need to scan the DT fairly early on the
>>>>> boot CPU and also perform the function there.
>>>>>
>>>>> I'll look into implementing this and post code.  Comments and
>>>>> suggestions appreciated, thanks.
>>>>
>>>> What goes wrong if we read the cntvoff from the boot CPU during
>>>> smp_prepare_cpus() phase and use that to set the cntvoff on the other
>>>> CPUs? That avoids needing to do anything very early by making the value
>>>> the same. It does mean that cntvoff is some random out of reset value
>>>> for CPU0, but at least it's consistent.
>>>
>>> I think we cannot read the value if we're not in hyp mode.
>>
>> Well, thinking about it a little more, I think you still have a good point.
>>
>> We don't need to do this early on, as long as we haven't started using
>> the arch timers yet.  If we are still able to do this at the point
>> where we're executing the code in arch/arm/mach-rockchip/platsmp.c
>> that finds the enable method then we can just handle it there.
>
> I've been playing around with the probe-based approach and while I need to do
> a lot more testing, it seems to be working for the first tens of instructions.
> I hope to be able to share a draft of that soon. Basically, I just read the
> current NSACR value and write it back (although maybe in the long term we
> would want to make sure a few of those bits are set or cleared). If that
> succeeds, we know we're in secure SVC and can proceed to set up MON and HYP.

Christopher, sounds promising, please do share, thanks!

Marc or Will, what do you guys think about this approach?


>
> Christopher
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by the Linux Foundation.

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Catalin Marinas]

On Mon, Sep 15, 2014 at 11:51:14PM +0100, Christopher Covington wrote:
> Hi Sonny,
> 
> On 09/15/2014 06:04 PM, Sonny Rao wrote:
> > On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> >> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >>> On 09/15/14 14:47, Sonny Rao wrote:
> >>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >>>>> On 09/15/14 04:10, Catalin Marinas wrote:
> >>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
> >>>>>>> On 09/12/14 05:14, Marc Zyngier wrote:
> >>>>>>>> We surely can handle the UNDEF and do something there. We just can't do
> >>>>>>>> it the way Doug described it above.
> >>>>>>> I suggested doing that for something else a while ago and Will and Dave
> >>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to
> >>>>>>> indicate what mode the kernel is running in.
> >>>>>>>
> >>>>>>> [1]
> >>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
> >>>>>> I think the context was slightly different. As I re-read the thread, it
> >>>>>> seems that the discussion was around whether to use some SMC interface
> >>>>>> or not based on whether the kernel is running secure or non-secure. The
> >>>>>> argument made by Will was to actually specify the type of the firmware
> >>>>>> SMC interface in the DT and use it in the kernel (and probably assume
> >>>>>> the kernel is running in secure mode if no smc interface is specified in
> >>>>>> the DT; you could have both though, running in secure mode and also
> >>>>>> having firmware).
> >>>>>>
> >>>>>> In this arch timer case, we need to work around a firmware bug (or
> >>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
> >>>>>> firmware, no matter how small such firmware is). We don't expect a
> >>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the
> >>>>>> DT.
> >>>>> Agreed, we can't described SMC calls that don't exist. From my
> >>>>> perspective it's just another part of the cpu boot sequence that needs
> >>>>> to be handled in the kernel, so describing the requirement via the
> >>>>> cpu-boot method seems appropriate. It seems like we're making it harder
> >>>>> than it should be by handling the undef when we could have slightly
> >>>>> different SMP boot code (and suspend/resume code) depending on the boot
> >>>>> method property.
> >>>>
> >>>> +heiko
> >>>>
> >>>> So, for the case of rk3288, based on this discussion what I'm going to
> >>>> propose is to add code to rockchip.c which looks for a particular SMP
> >>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc"
> >>>> which will then assume we have been booted in secure SVC mode and do
> >>>> the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
> >>>> as well, so I think it will need to scan the DT fairly early on the
> >>>> boot CPU and also perform the function there.
> >>>>
> >>>> I'll look into implementing this and post code.  Comments and
> >>>> suggestions appreciated, thanks.
> >>>
> >>> What goes wrong if we read the cntvoff from the boot CPU during
> >>> smp_prepare_cpus() phase and use that to set the cntvoff on the other
> >>> CPUs? That avoids needing to do anything very early by making the value
> >>> the same. It does mean that cntvoff is some random out of reset value
> >>> for CPU0, but at least it's consistent.
> >>
> >> I think we cannot read the value if we're not in hyp mode.
> > 
> > Well, thinking about it a little more, I think you still have a good point.
> > 
> > We don't need to do this early on, as long as we haven't started using
> > the arch timers yet.  If we are still able to do this at the point
> > where we're executing the code in arch/arm/mach-rockchip/platsmp.c
> > that finds the enable method then we can just handle it there.
> 
> I've been playing around with the probe-based approach and while I need to do
> a lot more testing, it seems to be working for the first tens of instructions.
> I hope to be able to share a draft of that soon. Basically, I just read the
> current NSACR value and write it back (although maybe in the long term we
> would want to make sure a few of those bits are set or cleared). If that
> succeeds, we know we're in secure SVC and can proceed to set up MON and HYP.

But when it doesn't succeed, you get an undefined instruction fault
(since NSACR is only writable in secure mode).

-- 
Catalin

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Catalin Marinas]

On Mon, Sep 15, 2014 at 09:33:03PM +0100, Stephen Boyd wrote:
> On 09/15/14 04:10, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
> >> On 09/12/14 05:14, Marc Zyngier wrote:
> >>> We surely can handle the UNDEF and do something there. We just can't do
> >>> it the way Doug described it above.
> >> I suggested doing that for something else a while ago and Will and Dave
> >> we're not thrilled[1]. The suggestion back then was to use DT to
> >> indicate what mode the kernel is running in.
> >>
> >> [1]
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
> > I think the context was slightly different. As I re-read the thread, it
> > seems that the discussion was around whether to use some SMC interface
> > or not based on whether the kernel is running secure or non-secure. The
> > argument made by Will was to actually specify the type of the firmware
> > SMC interface in the DT and use it in the kernel (and probably assume
> > the kernel is running in secure mode if no smc interface is specified in
> > the DT; you could have both though, running in secure mode and also
> > having firmware).
> >
> > In this arch timer case, we need to work around a firmware bug (or
> > feature as 32-bit ARM kernels never required CNTVOFF initialisation by
> > firmware, no matter how small such firmware is). We don't expect a
> > specific SMC call to initialise CNTVOFF, so we can't describe it in the
> > DT.
> 
> Agreed, we can't described SMC calls that don't exist. From my
> perspective it's just another part of the cpu boot sequence that needs
> to be handled in the kernel, so describing the requirement via the
> cpu-boot method seems appropriate. It seems like we're making it harder
> than it should be by handling the undef when we could have slightly
> different SMP boot code (and suspend/resume code) depending on the boot
> method property.

For 32-bit ARM SoCs, I think you can describe this via some specific
enable-method property. What I don't like though is the multitude of
enable methods (trying to reduce them on arm64) and the fact that
registers like CNTVOFF are rather architecture than SoC specific.

-- 
Catalin

Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer

[Christopher Covington]

On 09/16/2014 06:42 AM, Catalin Marinas wrote:
> On Mon, Sep 15, 2014 at 11:51:14PM +0100, Christopher Covington wrote:
>> Hi Sonny,
>>
>> On 09/15/2014 06:04 PM, Sonny Rao wrote:
>>> On Mon, Sep 15, 2014 at 2:52 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>>>> On Mon, Sep 15, 2014 at 2:49 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>>> On 09/15/14 14:47, Sonny Rao wrote:
>>>>>> On Mon, Sep 15, 2014 at 1:33 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>>>>> On 09/15/14 04:10, Catalin Marinas wrote:
>>>>>>>> On Fri, Sep 12, 2014 at 07:59:29PM +0100, Stephen Boyd wrote:
>>>>>>>>> On 09/12/14 05:14, Marc Zyngier wrote:
>>>>>>>>>> We surely can handle the UNDEF and do something there. We just can't do
>>>>>>>>>> it the way Doug described it above.
>>>>>>>>> I suggested doing that for something else a while ago and Will and Dave
>>>>>>>>> we're not thrilled[1]. The suggestion back then was to use DT to
>>>>>>>>> indicate what mode the kernel is running in.
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/105321.html
>>>>>>>> I think the context was slightly different. As I re-read the thread, it
>>>>>>>> seems that the discussion was around whether to use some SMC interface
>>>>>>>> or not based on whether the kernel is running secure or non-secure. The
>>>>>>>> argument made by Will was to actually specify the type of the firmware
>>>>>>>> SMC interface in the DT and use it in the kernel (and probably assume
>>>>>>>> the kernel is running in secure mode if no smc interface is specified in
>>>>>>>> the DT; you could have both though, running in secure mode and also
>>>>>>>> having firmware).
>>>>>>>>
>>>>>>>> In this arch timer case, we need to work around a firmware bug (or
>>>>>>>> feature as 32-bit ARM kernels never required CNTVOFF initialisation by
>>>>>>>> firmware, no matter how small such firmware is). We don't expect a
>>>>>>>> specific SMC call to initialise CNTVOFF, so we can't describe it in the
>>>>>>>> DT.
>>>>>>> Agreed, we can't described SMC calls that don't exist. From my
>>>>>>> perspective it's just another part of the cpu boot sequence that needs
>>>>>>> to be handled in the kernel, so describing the requirement via the
>>>>>>> cpu-boot method seems appropriate. It seems like we're making it harder
>>>>>>> than it should be by handling the undef when we could have slightly
>>>>>>> different SMP boot code (and suspend/resume code) depending on the boot
>>>>>>> method property.
>>>>>>
>>>>>> +heiko
>>>>>>
>>>>>> So, for the case of rk3288, based on this discussion what I'm going to
>>>>>> propose is to add code to rockchip.c which looks for a particular SMP
>>>>>> enable method -- say something like "rockchip,rk3288-smp-secure-svc"
>>>>>> which will then assume we have been booted in secure SVC mode and do
>>>>>> the CNTVOFF fixup.  I believe, it will need to do this on the boot CPU
>>>>>> as well, so I think it will need to scan the DT fairly early on the
>>>>>> boot CPU and also perform the function there.
>>>>>>
>>>>>> I'll look into implementing this and post code.  Comments and
>>>>>> suggestions appreciated, thanks.
>>>>>
>>>>> What goes wrong if we read the cntvoff from the boot CPU during
>>>>> smp_prepare_cpus() phase and use that to set the cntvoff on the other
>>>>> CPUs? That avoids needing to do anything very early by making the value
>>>>> the same. It does mean that cntvoff is some random out of reset value
>>>>> for CPU0, but at least it's consistent.
>>>>
>>>> I think we cannot read the value if we're not in hyp mode.
>>>
>>> Well, thinking about it a little more, I think you still have a good point.
>>>
>>> We don't need to do this early on, as long as we haven't started using
>>> the arch timers yet.  If we are still able to do this at the point
>>> where we're executing the code in arch/arm/mach-rockchip/platsmp.c
>>> that finds the enable method then we can just handle it there.
>>
>> I've been playing around with the probe-based approach and while I need to do
>> a lot more testing, it seems to be working for the first tens of instructions.
>> I hope to be able to share a draft of that soon. Basically, I just read the
>> current NSACR value and write it back (although maybe in the long term we
>> would want to make sure a few of those bits are set or cleared). If that
>> succeeds, we know we're in secure SVC and can proceed to set up MON and HYP.
> 
> But when it doesn't succeed, you get an undefined instruction fault
> (since NSACR is only writable in secure mode).

Yes. I see it as a conditional branch to VBAR+4 with a mode switch side effect.

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.