* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-11 17:43 ` Marc Zyngier
@ 2014-09-11 23:55 ` Doug Anderson
2014-09-11 23:56 ` Stephen Boyd
2014-09-12 11:43 ` Christopher Covington
2 siblings, 0 replies; 29+ messages in thread
From: Doug Anderson @ 2014-09-11 23:55 UTC (permalink / raw)
To: Marc Zyngier
Cc: Will Deacon, olof, Sonny Rao, Catalin Marinas, Mark Rutland,
Stephen Boyd, Sudeep Holla, Christopher Covington,
Lorenzo Pieralisi, Thomas Gleixner, Daniel Lezcano, Nathan Lynch,
linux-arm-kernel, robh+dt, Pawel Moll, ijc+devicetree, galak,
devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-11 17:43 ` Marc Zyngier
2014-09-11 23:55 ` Doug Anderson
@ 2014-09-11 23:56 ` Stephen Boyd
2014-09-12 0:01 ` Doug Anderson
[not found] ` <CAPz6YkUTXU9_b2BU5QghKTHVTJ3ngVX9EOzsMWnjigtV9TioHw@mail.gmail.com>
2014-09-12 11:43 ` Christopher Covington
2 siblings, 2 replies; 29+ messages in thread
From: Stephen Boyd @ 2014-09-11 23:56 UTC (permalink / raw)
To: Marc Zyngier, Doug Anderson
Cc: Will Deacon, olof, Sonny Rao, Catalin Marinas, Mark Rutland,
Sudeep Holla, Christopher Covington, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-11 23:56 ` Stephen Boyd
@ 2014-09-12 0:01 ` Doug Anderson
2014-09-12 10:20 ` Marc Zyngier
[not found] ` <CAPz6YkUTXU9_b2BU5QghKTHVTJ3ngVX9EOzsMWnjigtV9TioHw@mail.gmail.com>
1 sibling, 1 reply; 29+ messages in thread
From: Doug Anderson @ 2014-09-12 0:01 UTC (permalink / raw)
To: Stephen Boyd
Cc: Marc Zyngier, Will Deacon, olof, Sonny Rao, Catalin Marinas,
Mark Rutland, Sudeep Holla, Christopher Covington,
Lorenzo Pieralisi, Thomas Gleixner, Daniel Lezcano, Nathan Lynch,
linux-arm-kernel, robh+dt, Pawel Moll, ijc+devicetree, galak,
devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-12 0:01 ` Doug Anderson
@ 2014-09-12 10:20 ` Marc Zyngier
0 siblings, 0 replies; 29+ messages in thread
From: Marc Zyngier @ 2014-09-12 10:20 UTC (permalink / raw)
To: Doug Anderson
Cc: Stephen Boyd, Will Deacon, olof, Sonny Rao, Catalin Marinas,
Mark Rutland, Sudeep Holla, Christopher Covington,
Lorenzo Pieralisi, Thomas Gleixner, Daniel Lezcano, Nathan Lynch,
linux-arm-kernel, robh+dt, Pawel Moll, ijc+devicetree, galak,
devicetree, linux-kernel
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...
^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <CAPz6YkUTXU9_b2BU5QghKTHVTJ3ngVX9EOzsMWnjigtV9TioHw@mail.gmail.com>]
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-11 17:43 ` Marc Zyngier
2014-09-11 23:55 ` Doug Anderson
2014-09-11 23:56 ` Stephen Boyd
@ 2014-09-12 11:43 ` Christopher Covington
2014-09-12 12:14 ` Marc Zyngier
2 siblings, 1 reply; 29+ messages in thread
From: Christopher Covington @ 2014-09-12 11:43 UTC (permalink / raw)
To: Marc Zyngier
Cc: Doug Anderson, Will Deacon, olof, Sonny Rao, Catalin Marinas,
Mark Rutland, Stephen Boyd, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel
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.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-12 11:43 ` Christopher Covington
@ 2014-09-12 12:14 ` Marc Zyngier
2014-09-12 18:59 ` Stephen Boyd
0 siblings, 1 reply; 29+ messages in thread
From: Marc Zyngier @ 2014-09-12 12:14 UTC (permalink / raw)
To: Christopher Covington
Cc: Doug Anderson, Will Deacon, olof, Sonny Rao, Catalin Marinas,
Mark Rutland, Stephen Boyd, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel
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...
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-12 12:14 ` Marc Zyngier
@ 2014-09-12 18:59 ` Stephen Boyd
2014-09-15 11:10 ` Catalin Marinas
0 siblings, 1 reply; 29+ messages in thread
From: Stephen Boyd @ 2014-09-12 18:59 UTC (permalink / raw)
To: Marc Zyngier, Christopher Covington
Cc: Doug Anderson, Will Deacon, olof, Sonny Rao, Catalin Marinas,
Mark Rutland, Sudeep Holla, Lorenzo Pieralisi, Thomas Gleixner,
Daniel Lezcano, Nathan Lynch, linux-arm-kernel, robh+dt,
Pawel Moll, ijc+devicetree, galak, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-12 18:59 ` Stephen Boyd
@ 2014-09-15 11:10 ` Catalin Marinas
2014-09-15 20:33 ` Stephen Boyd
0 siblings, 1 reply; 29+ messages in thread
From: Catalin Marinas @ 2014-09-15 11:10 UTC (permalink / raw)
To: Stephen Boyd
Cc: Marc Zyngier, Christopher Covington, Doug Anderson, Will Deacon,
olof, Sonny Rao, Mark Rutland, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 11:10 ` Catalin Marinas
@ 2014-09-15 20:33 ` Stephen Boyd
2014-09-15 21:47 ` Sonny Rao
2014-09-16 11:03 ` Catalin Marinas
0 siblings, 2 replies; 29+ messages in thread
From: Stephen Boyd @ 2014-09-15 20:33 UTC (permalink / raw)
To: Catalin Marinas
Cc: Marc Zyngier, Christopher Covington, Doug Anderson, Will Deacon,
olof, Sonny Rao, Mark Rutland, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 20:33 ` Stephen Boyd
@ 2014-09-15 21:47 ` Sonny Rao
2014-09-15 21:49 ` Stephen Boyd
2014-09-16 11:03 ` Catalin Marinas
1 sibling, 1 reply; 29+ messages in thread
From: Sonny Rao @ 2014-09-15 21:47 UTC (permalink / raw)
To: Stephen Boyd
Cc: Catalin Marinas, Marc Zyngier, Christopher Covington,
Doug Anderson, Will Deacon, olof, Mark Rutland, Sudeep Holla,
Lorenzo Pieralisi, Thomas Gleixner, Daniel Lezcano, Nathan Lynch,
linux-arm-kernel, robh+dt, Pawel Moll, ijc+devicetree, galak,
devicetree, linux-kernel, Heiko Stübner
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
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 21:47 ` Sonny Rao
@ 2014-09-15 21:49 ` Stephen Boyd
2014-09-15 21:52 ` Sonny Rao
0 siblings, 1 reply; 29+ messages in thread
From: Stephen Boyd @ 2014-09-15 21:49 UTC (permalink / raw)
To: Sonny Rao
Cc: Catalin Marinas, Marc Zyngier, Christopher Covington,
Doug Anderson, Will Deacon, olof, Mark Rutland, Sudeep Holla,
Lorenzo Pieralisi, Thomas Gleixner, Daniel Lezcano, Nathan Lynch,
linux-arm-kernel, robh+dt, Pawel Moll, ijc+devicetree, galak,
devicetree, linux-kernel, Heiko Stübner
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 21:49 ` Stephen Boyd
@ 2014-09-15 21:52 ` Sonny Rao
2014-09-15 22:04 ` Sonny Rao
0 siblings, 1 reply; 29+ messages in thread
From: Sonny Rao @ 2014-09-15 21:52 UTC (permalink / raw)
To: Stephen Boyd
Cc: Catalin Marinas, Marc Zyngier, Christopher Covington,
Doug Anderson, Will Deacon, olof, Mark Rutland, Sudeep Holla,
Lorenzo Pieralisi, Thomas Gleixner, Daniel Lezcano, Nathan Lynch,
linux-arm-kernel, robh+dt, Pawel Moll, ijc+devicetree, galak,
devicetree, linux-kernel, Heiko Stübner
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
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 21:52 ` Sonny Rao
@ 2014-09-15 22:04 ` Sonny Rao
2014-09-15 22:51 ` Christopher Covington
0 siblings, 1 reply; 29+ messages in thread
From: Sonny Rao @ 2014-09-15 22:04 UTC (permalink / raw)
To: Stephen Boyd
Cc: Catalin Marinas, Marc Zyngier, Christopher Covington,
Doug Anderson, Will Deacon, olof, Mark Rutland, Sudeep Holla,
Lorenzo Pieralisi, Thomas Gleixner, Daniel Lezcano, Nathan Lynch,
linux-arm-kernel, robh+dt, Pawel Moll, ijc+devicetree, galak,
devicetree, linux-kernel, Heiko Stübner
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
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 22:04 ` Sonny Rao
@ 2014-09-15 22:51 ` Christopher Covington
2014-09-16 0:24 ` Sonny Rao
2014-09-16 10:42 ` Catalin Marinas
0 siblings, 2 replies; 29+ messages in thread
From: Christopher Covington @ 2014-09-15 22:51 UTC (permalink / raw)
To: Sonny Rao
Cc: Stephen Boyd, Catalin Marinas, Marc Zyngier, Doug Anderson,
Will Deacon, olof, Mark Rutland, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel, Heiko Stübner
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 22:51 ` Christopher Covington
@ 2014-09-16 0:24 ` Sonny Rao
2014-09-16 10:42 ` Catalin Marinas
1 sibling, 0 replies; 29+ messages in thread
From: Sonny Rao @ 2014-09-16 0:24 UTC (permalink / raw)
To: Christopher Covington
Cc: Stephen Boyd, Catalin Marinas, Marc Zyngier, Doug Anderson,
Will Deacon, olof, Mark Rutland, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel, Heiko Stübner
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 22:51 ` Christopher Covington
2014-09-16 0:24 ` Sonny Rao
@ 2014-09-16 10:42 ` Catalin Marinas
2014-09-16 11:22 ` Christopher Covington
1 sibling, 1 reply; 29+ messages in thread
From: Catalin Marinas @ 2014-09-16 10:42 UTC (permalink / raw)
To: Christopher Covington
Cc: Sonny Rao, Stephen Boyd, Marc Zyngier, Doug Anderson,
Will Deacon, olof, Mark Rutland, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel, Heiko Stübner
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-16 10:42 ` Catalin Marinas
@ 2014-09-16 11:22 ` Christopher Covington
0 siblings, 0 replies; 29+ messages in thread
From: Christopher Covington @ 2014-09-16 11:22 UTC (permalink / raw)
To: Catalin Marinas
Cc: Sonny Rao, Stephen Boyd, Marc Zyngier, Doug Anderson,
Will Deacon, olof, Mark Rutland, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel, Heiko Stübner
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] clocksource: arch_timer: Allow the device tree to specify the physical timer
2014-09-15 20:33 ` Stephen Boyd
2014-09-15 21:47 ` Sonny Rao
@ 2014-09-16 11:03 ` Catalin Marinas
1 sibling, 0 replies; 29+ messages in thread
From: Catalin Marinas @ 2014-09-16 11:03 UTC (permalink / raw)
To: Stephen Boyd
Cc: Marc Zyngier, Christopher Covington, Doug Anderson, Will Deacon,
olof, Sonny Rao, Mark Rutland, Sudeep Holla, Lorenzo Pieralisi,
Thomas Gleixner, Daniel Lezcano, Nathan Lynch, linux-arm-kernel,
robh+dt, Pawel Moll, ijc+devicetree, galak, devicetree,
linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread