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

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

[Sonny Rao]

This is a bug fix for using physical arch timers when
the arch_timer_use_virtual boolean is false.  It restores the
arch_counter_get_cntpct() function after removal in

0d651e4e "clocksource: arch_timer: use virtual counters"

and completes the implementation of memory mapped access for physical
timers, so if a system is trying to use physical timers, it will
function properly.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
 arch/arm/include/asm/arch_timer.h    |  9 +++++++++
 arch/arm64/include/asm/arch_timer.h  | 10 ++++++++++
 drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
 3 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 0704e0c..e72aa4d 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
 	return val;
 }
 
+static inline u64 arch_counter_get_cntpct(void)
+{
+	u64 cval;
+
+	isb();
+	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
+}
+
 static inline u64 arch_counter_get_cntvct(void)
 {
 	u64 cval;
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 9400596..58657c4 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
 #endif
 }
 
+static inline u64 arch_counter_get_cntpct(void)
+{
+	u64 cval;
+
+	isb();
+	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
+
+	return cval;
+}
+
 static inline u64 arch_counter_get_cntvct(void)
 {
 	u64 cval;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5163ec1..ad723cb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -30,6 +30,8 @@
 #define CNTTIDR		0x08
 #define CNTTIDR_VIRT(n)	(BIT(1) << ((n) * 4))
 
+#define CNTPCT_LO	0x00
+#define CNTPCT_HI	0x04
 #define CNTVCT_LO	0x08
 #define CNTVCT_HI	0x0c
 #define CNTFRQ		0x10
@@ -386,6 +388,19 @@ static u64 arch_counter_get_cntvct_mem(void)
 	return ((u64) vct_hi << 32) | vct_lo;
 }
 
+static u64 arch_counter_get_cntpct_mem(void)
+{
+	u32 pct_lo, pct_hi, tmp_hi;
+
+	do {
+		pct_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
+		pct_lo = readl_relaxed(arch_counter_base + CNTPCT_LO);
+		tmp_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
+	} while (pct_hi != tmp_hi);
+
+	return ((u64) pct_hi << 32) | pct_lo;
+}
+
 /*
  * Default to cp15 based access because arm64 uses this function for
  * sched_clock() before DT is probed and the cp15 method is guaranteed
@@ -429,10 +444,17 @@ static void __init arch_counter_register(unsigned type)
 	u64 start_count;
 
 	/* Register the CP15 based counter if we have one */
-	if (type & ARCH_CP15_TIMER)
-		arch_timer_read_counter = arch_counter_get_cntvct;
-	else
-		arch_timer_read_counter = arch_counter_get_cntvct_mem;
+	if (type & ARCH_CP15_TIMER) {
+		if (arch_timer_use_virtual)
+			arch_timer_read_counter = arch_counter_get_cntvct;
+		else
+			arch_timer_read_counter = arch_counter_get_cntpct;
+	} else {
+		if (arch_timer_use_virtual)
+			arch_timer_read_counter = arch_counter_get_cntvct_mem;
+		else
+			arch_timer_read_counter = arch_counter_get_cntpct_mem;
+	}
 
 	start_count = arch_timer_read_counter();
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
-- 
1.8.3.2

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

[Sonny Rao]

This is a bug fix for using physical arch timers when
the arch_timer_use_virtual boolean is false.  It restores the
arch_counter_get_cntpct() function after removal in

0d651e4e "clocksource: arch_timer: use virtual counters"

and completes the implementation of memory mapped access for physical
timers, so if a system is trying to use physical timers, it will
function properly.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
 arch/arm/include/asm/arch_timer.h    |  9 +++++++++
 arch/arm64/include/asm/arch_timer.h  | 10 ++++++++++
 drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
 3 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 0704e0c..e72aa4d 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
 	return val;
 }
 
+static inline u64 arch_counter_get_cntpct(void)
+{
+	u64 cval;
+
+	isb();
+	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
+}
+
 static inline u64 arch_counter_get_cntvct(void)
 {
 	u64 cval;
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 9400596..58657c4 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
 #endif
 }
 
+static inline u64 arch_counter_get_cntpct(void)
+{
+	u64 cval;
+
+	isb();
+	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
+
+	return cval;
+}
+
 static inline u64 arch_counter_get_cntvct(void)
 {
 	u64 cval;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5163ec1..ad723cb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -30,6 +30,8 @@
 #define CNTTIDR		0x08
 #define CNTTIDR_VIRT(n)	(BIT(1) << ((n) * 4))
 
+#define CNTPCT_LO	0x00
+#define CNTPCT_HI	0x04
 #define CNTVCT_LO	0x08
 #define CNTVCT_HI	0x0c
 #define CNTFRQ		0x10
@@ -386,6 +388,19 @@ static u64 arch_counter_get_cntvct_mem(void)
 	return ((u64) vct_hi << 32) | vct_lo;
 }
 
+static u64 arch_counter_get_cntpct_mem(void)
+{
+	u32 pct_lo, pct_hi, tmp_hi;
+
+	do {
+		pct_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
+		pct_lo = readl_relaxed(arch_counter_base + CNTPCT_LO);
+		tmp_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
+	} while (pct_hi != tmp_hi);
+
+	return ((u64) pct_hi << 32) | pct_lo;
+}
+
 /*
  * Default to cp15 based access because arm64 uses this function for
  * sched_clock() before DT is probed and the cp15 method is guaranteed
@@ -429,10 +444,17 @@ static void __init arch_counter_register(unsigned type)
 	u64 start_count;
 
 	/* Register the CP15 based counter if we have one */
-	if (type & ARCH_CP15_TIMER)
-		arch_timer_read_counter = arch_counter_get_cntvct;
-	else
-		arch_timer_read_counter = arch_counter_get_cntvct_mem;
+	if (type & ARCH_CP15_TIMER) {
+		if (arch_timer_use_virtual)
+			arch_timer_read_counter = arch_counter_get_cntvct;
+		else
+			arch_timer_read_counter = arch_counter_get_cntpct;
+	} else {
+		if (arch_timer_use_virtual)
+			arch_timer_read_counter = arch_counter_get_cntvct_mem;
+		else
+			arch_timer_read_counter = arch_counter_get_cntpct_mem;
+	}
 
 	start_count = arch_timer_read_counter();
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
-- 
1.8.3.2

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

[Olof Johansson]

On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false.  It restores the
> arch_counter_get_cntpct() function after removal in
>
> 0d651e4e "clocksource: arch_timer: use virtual counters"
>
> and completes the implementation of memory mapped access for physical
> timers, so if a system is trying to use physical timers, it will
> function properly.
>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>

Acked-by: Olof Johansson <olof@lixom.net>

This should have a:

Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")

tag too, and possibly cc stable?


-Olof

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

[Olof Johansson]

On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false.  It restores the
> arch_counter_get_cntpct() function after removal in
>
> 0d651e4e "clocksource: arch_timer: use virtual counters"
>
> and completes the implementation of memory mapped access for physical
> timers, so if a system is trying to use physical timers, it will
> function properly.
>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>

Acked-by: Olof Johansson <olof@lixom.net>

This should have a:

Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")

tag too, and possibly cc stable?


-Olof

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

[Sonny Rao]

On Wed, Aug 27, 2014 at 2:19 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>> This is a bug fix for using physical arch timers when
>> the arch_timer_use_virtual boolean is false.  It restores the
>> arch_counter_get_cntpct() function after removal in
>>
>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>
>> and completes the implementation of memory mapped access for physical
>> timers, so if a system is trying to use physical timers, it will
>> function properly.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>
> Acked-by: Olof Johansson <olof@lixom.net>
>
> This should have a:
>
> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>
> tag too, and possibly cc stable?

Ok, as far as stable goes, this patch wouldn't apply cleanly going all
the way back to  0d651e4e65e9
As-is, it would need to go after 220069945b29 "clocksource:
arch_timer: Add support for memory mapped timers" and there would need
to be another, simpler, version that went between those two commits.

So, I'm not sure what to do in this situation regarding stable?

>
>
> -Olof

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

[Sonny Rao]

On Wed, Aug 27, 2014 at 2:19 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>> This is a bug fix for using physical arch timers when
>> the arch_timer_use_virtual boolean is false.  It restores the
>> arch_counter_get_cntpct() function after removal in
>>
>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>
>> and completes the implementation of memory mapped access for physical
>> timers, so if a system is trying to use physical timers, it will
>> function properly.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>
> Acked-by: Olof Johansson <olof@lixom.net>
>
> This should have a:
>
> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>
> tag too, and possibly cc stable?

Ok, as far as stable goes, this patch wouldn't apply cleanly going all
the way back to  0d651e4e65e9
As-is, it would need to go after 220069945b29 "clocksource:
arch_timer: Add support for memory mapped timers" and there would need
to be another, simpler, version that went between those two commits.

So, I'm not sure what to do in this situation regarding stable?

>
>
> -Olof

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

[Stephen Boyd]

+Mark (author of change in question)

On 08/27/14 14:27, Sonny Rao wrote:
> On Wed, Aug 27, 2014 at 2:19 PM, Olof Johansson <olof@lixom.net> wrote:
>> On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>>> This is a bug fix for using physical arch timers when
>>> the arch_timer_use_virtual boolean is false.  It restores the
>>> arch_counter_get_cntpct() function after removal in
>>>
>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>
>>> and completes the implementation of memory mapped access for physical
>>> timers, so if a system is trying to use physical timers, it will
>>> function properly.
>>>
>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> Acked-by: Olof Johansson <olof@lixom.net>
>>
>> This should have a:
>>
>> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>>
>> tag too, and possibly cc stable?
> Ok, as far as stable goes, this patch wouldn't apply cleanly going all
> the way back to  0d651e4e65e9
> As-is, it would need to go after 220069945b29 "clocksource:
> arch_timer: Add support for memory mapped timers" and there would need
> to be another, simpler, version that went between those two commits.
>
> So, I'm not sure what to do in this situation regarding stable?

Is there any reason why the virtual counter can't be read? Maybe we're
the hyp and we need to make sure we don't use the virtual timer so that
the guest can use it, but that doesn't have any effect on the usage of
the virtual counter for the clocksource.

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

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

[Stephen Boyd]

+Mark (author of change in question)

On 08/27/14 14:27, Sonny Rao wrote:
> On Wed, Aug 27, 2014 at 2:19 PM, Olof Johansson <olof@lixom.net> wrote:
>> On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>>> This is a bug fix for using physical arch timers when
>>> the arch_timer_use_virtual boolean is false.  It restores the
>>> arch_counter_get_cntpct() function after removal in
>>>
>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>
>>> and completes the implementation of memory mapped access for physical
>>> timers, so if a system is trying to use physical timers, it will
>>> function properly.
>>>
>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> Acked-by: Olof Johansson <olof@lixom.net>
>>
>> This should have a:
>>
>> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>>
>> tag too, and possibly cc stable?
> Ok, as far as stable goes, this patch wouldn't apply cleanly going all
> the way back to  0d651e4e65e9
> As-is, it would need to go after 220069945b29 "clocksource:
> arch_timer: Add support for memory mapped timers" and there would need
> to be another, simpler, version that went between those two commits.
>
> So, I'm not sure what to do in this situation regarding stable?

Is there any reason why the virtual counter can't be read? Maybe we're
the hyp and we need to make sure we don't use the virtual timer so that
the guest can use it, but that doesn't have any effect on the usage of
the virtual counter for the clocksource.

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

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

[Olof Johansson]

On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> +Mark (author of change in question)
>
> On 08/27/14 14:27, Sonny Rao wrote:
>> On Wed, Aug 27, 2014 at 2:19 PM, Olof Johansson <olof@lixom.net> wrote:
>>> On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>>>> This is a bug fix for using physical arch timers when
>>>> the arch_timer_use_virtual boolean is false.  It restores the
>>>> arch_counter_get_cntpct() function after removal in
>>>>
>>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>>
>>>> and completes the implementation of memory mapped access for physical
>>>> timers, so if a system is trying to use physical timers, it will
>>>> function properly.
>>>>
>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>> Acked-by: Olof Johansson <olof@lixom.net>
>>>
>>> This should have a:
>>>
>>> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>>>
>>> tag too, and possibly cc stable?
>> Ok, as far as stable goes, this patch wouldn't apply cleanly going all
>> the way back to  0d651e4e65e9
>> As-is, it would need to go after 220069945b29 "clocksource:
>> arch_timer: Add support for memory mapped timers" and there would need
>> to be another, simpler, version that went between those two commits.
>>
>> So, I'm not sure what to do in this situation regarding stable?

Greg tends to make a best-effort, you'll find out when he looks at
backporting and can reply with a "punt" or "here's the right patch"
then.

> Is there any reason why the virtual counter can't be read? Maybe we're
> the hyp and we need to make sure we don't use the virtual timer so that
> the guest can use it, but that doesn't have any effect on the usage of
> the virtual counter for the clocksource.

There are several cases where virtual is unusable -- in particular it
might not have been configured properly (i.e. the phys/virt offset is
at a bad value).


-Olof

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

[Olof Johansson]

On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> +Mark (author of change in question)
>
> On 08/27/14 14:27, Sonny Rao wrote:
>> On Wed, Aug 27, 2014 at 2:19 PM, Olof Johansson <olof@lixom.net> wrote:
>>> On Wed, Aug 27, 2014 at 2:03 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
>>>> This is a bug fix for using physical arch timers when
>>>> the arch_timer_use_virtual boolean is false.  It restores the
>>>> arch_counter_get_cntpct() function after removal in
>>>>
>>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>>
>>>> and completes the implementation of memory mapped access for physical
>>>> timers, so if a system is trying to use physical timers, it will
>>>> function properly.
>>>>
>>>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>> Acked-by: Olof Johansson <olof@lixom.net>
>>>
>>> This should have a:
>>>
>>> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>>>
>>> tag too, and possibly cc stable?
>> Ok, as far as stable goes, this patch wouldn't apply cleanly going all
>> the way back to  0d651e4e65e9
>> As-is, it would need to go after 220069945b29 "clocksource:
>> arch_timer: Add support for memory mapped timers" and there would need
>> to be another, simpler, version that went between those two commits.
>>
>> So, I'm not sure what to do in this situation regarding stable?

Greg tends to make a best-effort, you'll find out when he looks at
backporting and can reply with a "punt" or "here's the right patch"
then.

> Is there any reason why the virtual counter can't be read? Maybe we're
> the hyp and we need to make sure we don't use the virtual timer so that
> the guest can use it, but that doesn't have any effect on the usage of
> the virtual counter for the clocksource.

There are several cases where virtual is unusable -- in particular it
might not have been configured properly (i.e. the phys/virt offset is
at a bad value).


-Olof

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

[Stephen Boyd]

On 08/27/14 15:33, Olof Johansson wrote:
> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
>> Is there any reason why the virtual counter can't be read? Maybe we're
>> the hyp and we need to make sure we don't use the virtual timer so that
>> the guest can use it, but that doesn't have any effect on the usage of
>> the virtual counter for the clocksource.
> There are several cases where virtual is unusable -- in particular it
> might not have been configured properly (i.e. the phys/virt offset is
> at a bad value).
>

Any specifics? It would be nice to say so in the commit text so that
others using such devices know they need this patch. I'm guessing the
firmware can't be fixed?

In this particular case is there actually a virtual interrupt but we've
explicitly removed it from the DT so that the driver can be forced into
using the physical counter? Or are we getting saved by the hyp check?

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

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

[Stephen Boyd]

On 08/27/14 15:33, Olof Johansson wrote:
> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
>> Is there any reason why the virtual counter can't be read? Maybe we're
>> the hyp and we need to make sure we don't use the virtual timer so that
>> the guest can use it, but that doesn't have any effect on the usage of
>> the virtual counter for the clocksource.
> There are several cases where virtual is unusable -- in particular it
> might not have been configured properly (i.e. the phys/virt offset is
> at a bad value).
>

Any specifics? It would be nice to say so in the commit text so that
others using such devices know they need this patch. I'm guessing the
firmware can't be fixed?

In this particular case is there actually a virtual interrupt but we've
explicitly removed it from the DT so that the driver can be forced into
using the physical counter? Or are we getting saved by the hyp check?

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

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

[Olof Johansson]

On Wed, Aug 27, 2014 at 5:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 08/27/14 15:33, Olof Johansson wrote:
>> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>>> Is there any reason why the virtual counter can't be read? Maybe we're
>>> the hyp and we need to make sure we don't use the virtual timer so that
>>> the guest can use it, but that doesn't have any effect on the usage of
>>> the virtual counter for the clocksource.
>> There are several cases where virtual is unusable -- in particular it
>> might not have been configured properly (i.e. the phys/virt offset is
>> at a bad value).
>>
>
> Any specifics? It would be nice to say so in the commit text so that
> others using such devices know they need this patch. I'm guessing the
> firmware can't be fixed?

Yeah, there are a few. The big.LITTLE on the Chromebook 2 models have
this issue, due to the A7 cluster having an incorrect offset
programmed. However, arch timers aren't supported on that SoC in the
first place, so it's not a problem in reality.

The other known platform is rk3288. It has products out in the wild
where firmware updates are unlikely.

Essentially, I expect many vendors who use BSP kernels by default to
have firmware that forgets to setup the offset, since hardware doesn't
come up with a default one, and their older BSP kernels doesn't access
the virtual one.

> In this particular case is there actually a virtual interrupt but we've
> explicitly removed it from the DT so that the driver can be forced into
> using the physical counter? Or are we getting saved by the hyp check?

The SoC has the virtual timer, and if it has firmware that supports it
there's a good reason to still have it there. After all, DT describes
hardware.

I have a patch I should post that adds a property to make the driver
pick the physical timer instead, since right now it'll always use
virtual if it's available. I'll try to get that posted later tonight.


-Olof

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

[Olof Johansson]

On Wed, Aug 27, 2014 at 5:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 08/27/14 15:33, Olof Johansson wrote:
>> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>>> Is there any reason why the virtual counter can't be read? Maybe we're
>>> the hyp and we need to make sure we don't use the virtual timer so that
>>> the guest can use it, but that doesn't have any effect on the usage of
>>> the virtual counter for the clocksource.
>> There are several cases where virtual is unusable -- in particular it
>> might not have been configured properly (i.e. the phys/virt offset is
>> at a bad value).
>>
>
> Any specifics? It would be nice to say so in the commit text so that
> others using such devices know they need this patch. I'm guessing the
> firmware can't be fixed?

Yeah, there are a few. The big.LITTLE on the Chromebook 2 models have
this issue, due to the A7 cluster having an incorrect offset
programmed. However, arch timers aren't supported on that SoC in the
first place, so it's not a problem in reality.

The other known platform is rk3288. It has products out in the wild
where firmware updates are unlikely.

Essentially, I expect many vendors who use BSP kernels by default to
have firmware that forgets to setup the offset, since hardware doesn't
come up with a default one, and their older BSP kernels doesn't access
the virtual one.

> In this particular case is there actually a virtual interrupt but we've
> explicitly removed it from the DT so that the driver can be forced into
> using the physical counter? Or are we getting saved by the hyp check?

The SoC has the virtual timer, and if it has firmware that supports it
there's a good reason to still have it there. After all, DT describes
hardware.

I have a patch I should post that adds a property to make the driver
pick the physical timer instead, since right now it'll always use
virtual if it's available. I'll try to get that posted later tonight.


-Olof

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

[Doug Anderson]

Hi,

On Wed, Aug 27, 2014 at 7:58 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, Aug 27, 2014 at 5:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 08/27/14 15:33, Olof Johansson wrote:
>>> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>
>>>> Is there any reason why the virtual counter can't be read? Maybe we're
>>>> the hyp and we need to make sure we don't use the virtual timer so that
>>>> the guest can use it, but that doesn't have any effect on the usage of
>>>> the virtual counter for the clocksource.
>>> There are several cases where virtual is unusable -- in particular it
>>> might not have been configured properly (i.e. the phys/virt offset is
>>> at a bad value).
>>>
>>
>> Any specifics? It would be nice to say so in the commit text so that
>> others using such devices know they need this patch. I'm guessing the
>> firmware can't be fixed?

Even if we could change things to use a virtual timer in some cases,
Sonny's patch still fixes a bug.  The code as written right now makes
pretenses about supporting the physical timer, but it doesn't work.
That should be fixed.


> Yeah, there are a few. The big.LITTLE on the Chromebook 2 models have
> this issue, due to the A7 cluster having an incorrect offset
> programmed. However, arch timers aren't supported on that SoC in the
> first place, so it's not a problem in reality.
>
> The other known platform is rk3288. It has products out in the wild
> where firmware updates are unlikely.

One other reason is that (I'm told) that the virtual offset is lost in
certain power down conditions (powering down a core, going into S3,
etc).  When we power back up the offset is effectively reset to a
random value.  That means we need something to reprogram the virtual
timer offset whenever we power things back up.

If we've got a hypervisor then the hypervisor will definitely be
involved in powering things back up and it can reset the virtual
offset.  ...but forcing systems to implement a hypervisor (or somehow
adding an interface for the kernel to call back into firmware) is a
huge effort and it means more hard-to-update code sitting in firmware.

Note: having the virtual offset initted to a random value seems like
an unfortunate design choice for the virtual timer offset
(guaranteeing it was initted to 0 would have avoided the problem), but
that's what we seem to have.


-Doug

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

[Doug Anderson]

Hi,

On Wed, Aug 27, 2014 at 7:58 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, Aug 27, 2014 at 5:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 08/27/14 15:33, Olof Johansson wrote:
>>> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>
>>>> Is there any reason why the virtual counter can't be read? Maybe we're
>>>> the hyp and we need to make sure we don't use the virtual timer so that
>>>> the guest can use it, but that doesn't have any effect on the usage of
>>>> the virtual counter for the clocksource.
>>> There are several cases where virtual is unusable -- in particular it
>>> might not have been configured properly (i.e. the phys/virt offset is
>>> at a bad value).
>>>
>>
>> Any specifics? It would be nice to say so in the commit text so that
>> others using such devices know they need this patch. I'm guessing the
>> firmware can't be fixed?

Even if we could change things to use a virtual timer in some cases,
Sonny's patch still fixes a bug.  The code as written right now makes
pretenses about supporting the physical timer, but it doesn't work.
That should be fixed.


> Yeah, there are a few. The big.LITTLE on the Chromebook 2 models have
> this issue, due to the A7 cluster having an incorrect offset
> programmed. However, arch timers aren't supported on that SoC in the
> first place, so it's not a problem in reality.
>
> The other known platform is rk3288. It has products out in the wild
> where firmware updates are unlikely.

One other reason is that (I'm told) that the virtual offset is lost in
certain power down conditions (powering down a core, going into S3,
etc).  When we power back up the offset is effectively reset to a
random value.  That means we need something to reprogram the virtual
timer offset whenever we power things back up.

If we've got a hypervisor then the hypervisor will definitely be
involved in powering things back up and it can reset the virtual
offset.  ...but forcing systems to implement a hypervisor (or somehow
adding an interface for the kernel to call back into firmware) is a
huge effort and it means more hard-to-update code sitting in firmware.

Note: having the virtual offset initted to a random value seems like
an unfortunate design choice for the virtual timer offset
(guaranteeing it was initted to 0 would have avoided the problem), but
that's what we seem to have.


-Doug

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

[Mark Rutland]

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

The code does support the physical timer. It does not support the
physical counter (and makes no pretenses that it does).

I had hoped we wouldn't encounter cases where CNTVOFF was hopelessly
ill-configured on a platform, but evidently we have. So we need some
workaround for that.

> > Yeah, there are a few. The big.LITTLE on the Chromebook 2 models have
> > this issue, due to the A7 cluster having an incorrect offset
> > programmed. However, arch timers aren't supported on that SoC in the
> > first place, so it's not a problem in reality.
> >
> > The other known platform is rk3288. It has products out in the wild
> > where firmware updates are unlikely.
> 
> One other reason is that (I'm told) that the virtual offset is lost in
> certain power down conditions (powering down a core, going into S3,
> etc).  When we power back up the offset is effectively reset to a
> random value.  That means we need something to reprogram the virtual
> timer offset whenever we power things back up.
> 
> If we've got a hypervisor then the hypervisor will definitely be
> involved in powering things back up and it can reset the virtual
> offset.  ...but forcing systems to implement a hypervisor (or somehow
> adding an interface for the kernel to call back into firmware) is a
> huge effort and it means more hard-to-update code sitting in firmware.

Not if you boot Linux at hyp, as we've recommended for this precise
reason. That doesn't fix other things like CNTFRQ if the secure
initialisation doesn't poke that, however.

Thanks,
Mark.

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

[Mark Rutland]

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

The code does support the physical timer. It does not support the
physical counter (and makes no pretenses that it does).

I had hoped we wouldn't encounter cases where CNTVOFF was hopelessly
ill-configured on a platform, but evidently we have. So we need some
workaround for that.

> > Yeah, there are a few. The big.LITTLE on the Chromebook 2 models have
> > this issue, due to the A7 cluster having an incorrect offset
> > programmed. However, arch timers aren't supported on that SoC in the
> > first place, so it's not a problem in reality.
> >
> > The other known platform is rk3288. It has products out in the wild
> > where firmware updates are unlikely.
> 
> One other reason is that (I'm told) that the virtual offset is lost in
> certain power down conditions (powering down a core, going into S3,
> etc).  When we power back up the offset is effectively reset to a
> random value.  That means we need something to reprogram the virtual
> timer offset whenever we power things back up.
> 
> If we've got a hypervisor then the hypervisor will definitely be
> involved in powering things back up and it can reset the virtual
> offset.  ...but forcing systems to implement a hypervisor (or somehow
> adding an interface for the kernel to call back into firmware) is a
> huge effort and it means more hard-to-update code sitting in firmware.

Not if you boot Linux at hyp, as we've recommended for this precise
reason. That doesn't fix other things like CNTFRQ if the secure
initialisation doesn't poke that, however.

Thanks,
Mark.

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

[Christopher Covington]

Hi Mark,

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

I think if you could please explain the following code, that may help clear up
some of the confusion.

		if (arch_timer_use_virtual) {
			clk->irq = arch_timer_ppi[VIRT_PPI];
			clk->set_mode = arch_timer_set_mode_virt;
			clk->set_next_event = arch_timer_set_next_event_virt;
		} else {
			clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
			clk->set_mode = arch_timer_set_mode_phys;
			clk->set_next_event = arch_timer_set_next_event_phys;
		}

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c#n272

Perhaps you mean to say the code does not support *non-secure access* to the
physical timer?

Thanks,
Christopher

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

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

[Christopher Covington]

Hi Mark,

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

I think if you could please explain the following code, that may help clear up
some of the confusion.

		if (arch_timer_use_virtual) {
			clk->irq = arch_timer_ppi[VIRT_PPI];
			clk->set_mode = arch_timer_set_mode_virt;
			clk->set_next_event = arch_timer_set_next_event_virt;
		} else {
			clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
			clk->set_mode = arch_timer_set_mode_phys;
			clk->set_next_event = arch_timer_set_next_event_phys;
		}

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c#n272

Perhaps you mean to say the code does not support *non-secure access* to the
physical timer?

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] clocksource: arch_timer: Fix code to use physical timers when requested

[Mark Rutland]

On Thu, Aug 28, 2014 at 06:09:32PM +0100, Christopher Covington wrote:
> Hi Mark,

Hi Christopher,

> On 08/28/2014 05:35 AM, Mark Rutland wrote:
> > On Thu, Aug 28, 2014 at 04:33:31AM +0100, Doug Anderson wrote:
> >> Hi,
> >>
> >> On Wed, Aug 27, 2014 at 7:58 PM, Olof Johansson <olof@lixom.net> wrote:
> >>> On Wed, Aug 27, 2014 at 5:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >>>> On 08/27/14 15:33, Olof Johansson wrote:
> >>>>> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >>>>>
> >>>>>> Is there any reason why the virtual counter can't be read? Maybe we're
> >>>>>> the hyp and we need to make sure we don't use the virtual timer so that
> >>>>>> the guest can use it, but that doesn't have any effect on the usage of
> >>>>>> the virtual counter for the clocksource.
> >>>>>
> >>>>> There are several cases where virtual is unusable -- in particular it
> >>>>> might not have been configured properly (i.e. the phys/virt offset is
> >>>>> at a bad value).
> >>>>
> >>>> Any specifics? It would be nice to say so in the commit text so that
> >>>> others using such devices know they need this patch. I'm guessing the
> >>>> firmware can't be fixed?
> >>
> >> Even if we could change things to use a virtual timer in some cases,
> >> Sonny's patch still fixes a bug.  The code as written right now makes
> >> pretenses about supporting the physical timer, but it doesn't work.
> >> That should be fixed.
> > 
> > The code does support the physical timer. It does not support the
> > physical counter (and makes no pretenses that it does).
> 
> I think if you could please explain the following code, that may help clear up
> some of the confusion.
> 
> 		if (arch_timer_use_virtual) {
> 			clk->irq = arch_timer_ppi[VIRT_PPI];
> 			clk->set_mode = arch_timer_set_mode_virt;
> 			clk->set_next_event = arch_timer_set_next_event_virt;
> 		} else {
> 			clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
> 			clk->set_mode = arch_timer_set_mode_phys;
> 			clk->set_next_event = arch_timer_set_next_event_phys;
> 		}
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c#n272
> 
> Perhaps you mean to say the code does not support *non-secure access* to the
> physical timer?

Not quite, and one issue here is conflating the timer and the counter.

We use the physical timer iff we know it is accessible and correct to
use (e.g. when we have been booted at Hyp). If we don't know that,
arch_timer_use_virtual is set and we use the virtual timer. In that
sense, we support the use of the physical timer.

If the system is sane (with uniform CNTVOFF), that is fine. The fact
that we have a broken system to work around does not mean that the
driver is broken nor that it is making false pretenses. The driver is
perfectly functional given a sane environment.

The use of virtual/physical is not a feature that falls to personal
preference; it is a requirement for correctness that we only use the
physical timer when we have an absolute guarantee that it is possible
and correct to do so. So saying that we "support" the physical timer is
also somewhat misleading; we use what we must.

We _always_ use the virtual counter rather than the physical counter as
this saved on having different paths for host and guest (which is
important for the 64-bit VDSO, for example). In that sense we do not
support the use of the physical counter. We don't need to use it in a
sane system.

Thanks,
Mark.

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

[Mark Rutland]

On Thu, Aug 28, 2014 at 06:09:32PM +0100, Christopher Covington wrote:
> Hi Mark,

Hi Christopher,

> On 08/28/2014 05:35 AM, Mark Rutland wrote:
> > On Thu, Aug 28, 2014 at 04:33:31AM +0100, Doug Anderson wrote:
> >> Hi,
> >>
> >> On Wed, Aug 27, 2014 at 7:58 PM, Olof Johansson <olof@lixom.net> wrote:
> >>> On Wed, Aug 27, 2014 at 5:56 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >>>> On 08/27/14 15:33, Olof Johansson wrote:
> >>>>> On Wed, Aug 27, 2014 at 3:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >>>>>
> >>>>>> Is there any reason why the virtual counter can't be read? Maybe we're
> >>>>>> the hyp and we need to make sure we don't use the virtual timer so that
> >>>>>> the guest can use it, but that doesn't have any effect on the usage of
> >>>>>> the virtual counter for the clocksource.
> >>>>>
> >>>>> There are several cases where virtual is unusable -- in particular it
> >>>>> might not have been configured properly (i.e. the phys/virt offset is
> >>>>> at a bad value).
> >>>>
> >>>> Any specifics? It would be nice to say so in the commit text so that
> >>>> others using such devices know they need this patch. I'm guessing the
> >>>> firmware can't be fixed?
> >>
> >> Even if we could change things to use a virtual timer in some cases,
> >> Sonny's patch still fixes a bug.  The code as written right now makes
> >> pretenses about supporting the physical timer, but it doesn't work.
> >> That should be fixed.
> > 
> > The code does support the physical timer. It does not support the
> > physical counter (and makes no pretenses that it does).
> 
> I think if you could please explain the following code, that may help clear up
> some of the confusion.
> 
> 		if (arch_timer_use_virtual) {
> 			clk->irq = arch_timer_ppi[VIRT_PPI];
> 			clk->set_mode = arch_timer_set_mode_virt;
> 			clk->set_next_event = arch_timer_set_next_event_virt;
> 		} else {
> 			clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
> 			clk->set_mode = arch_timer_set_mode_phys;
> 			clk->set_next_event = arch_timer_set_next_event_phys;
> 		}
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c#n272
> 
> Perhaps you mean to say the code does not support *non-secure access* to the
> physical timer?

Not quite, and one issue here is conflating the timer and the counter.

We use the physical timer iff we know it is accessible and correct to
use (e.g. when we have been booted at Hyp). If we don't know that,
arch_timer_use_virtual is set and we use the virtual timer. In that
sense, we support the use of the physical timer.

If the system is sane (with uniform CNTVOFF), that is fine. The fact
that we have a broken system to work around does not mean that the
driver is broken nor that it is making false pretenses. The driver is
perfectly functional given a sane environment.

The use of virtual/physical is not a feature that falls to personal
preference; it is a requirement for correctness that we only use the
physical timer when we have an absolute guarantee that it is possible
and correct to do so. So saying that we "support" the physical timer is
also somewhat misleading; we use what we must.

We _always_ use the virtual counter rather than the physical counter as
this saved on having different paths for host and guest (which is
important for the 64-bit VDSO, for example). In that sense we do not
support the use of the physical counter. We don't need to use it in a
sane system.

Thanks,
Mark.

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

[Sonny Rao]

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

Is there some reason that it should not support it?  It seems like the
two things are highly related.

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

That's interesting, we could look into that.

> Thanks,
> Mark.

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

[Sonny Rao]

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

Is there some reason that it should not support it?  It seems like the
two things are highly related.

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

That's interesting, we could look into that.

> Thanks,
> Mark.

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

[Mark Rutland]

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

While the two are related, in sane systems the use of the physical
counters is rendered unnecessary by the ability to write to CNTVOFF at
PL2.

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

If an OS is booted at PL2 it can access the physical counters, and
should do so in case something like KVM will be used later. The OS can
write to CNTVOFF at PL2, and if it sets CNTVOFF to zero the physical and
virtual counters are equivalent. Thus it can use the virtual counters
and doesn't need to have additional code in several places (including
the VDSO) where it needs to choose to read which counters to read.

The problem only exists where PL2 exists and the firmware/bootloader
skipped PL2 without initialising the necessary PL2 state. This is in
general a stupid thing to do; it introduces a problem that need not
exist and throws away the option of using the features PL2 provides.
This is a firmware/bootloader bug.

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

I would strongly recommend that you do. :)

Linux has supported booting at Hyp for quite a while now, so I would
hope the only issues would be the organisation of the firmware and/or
bootloader.

Thanks,
Mark.

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

[Mark Rutland]

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

While the two are related, in sane systems the use of the physical
counters is rendered unnecessary by the ability to write to CNTVOFF at
PL2.

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

If an OS is booted at PL2 it can access the physical counters, and
should do so in case something like KVM will be used later. The OS can
write to CNTVOFF at PL2, and if it sets CNTVOFF to zero the physical and
virtual counters are equivalent. Thus it can use the virtual counters
and doesn't need to have additional code in several places (including
the VDSO) where it needs to choose to read which counters to read.

The problem only exists where PL2 exists and the firmware/bootloader
skipped PL2 without initialising the necessary PL2 state. This is in
general a stupid thing to do; it introduces a problem that need not
exist and throws away the option of using the features PL2 provides.
This is a firmware/bootloader bug.

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

I would strongly recommend that you do. :)

Linux has supported booting at Hyp for quite a while now, so I would
hope the only issues would be the organisation of the firmware and/or
bootloader.

Thanks,
Mark.

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

[Sonny Rao]

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

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

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

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

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

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

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

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

> Thanks,
> Mark.

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

[Sonny Rao]

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

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

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

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

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

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

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

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

> Thanks,
> Mark.

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

[Mark Rutland]

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

So long as all CPUs have a uniform view of time (with the same CNTFRQ
and no observable offset), things are sane.

Fundamentally, everything that can only be configured at a higher
privilege level must have been configured.

So if booted at PL2, it's not necessary for CNTVOFF to be initialised
while for PL1 it's critical that it is.

> > If an OS is booted at PL1 the physical timers aren't guaranteed to be
> > accessible, so the OS must use the virtual timers. As the OS could be
> > virtualized it must use the virtual counters.
> 
> I was curious to learn more about these modes and looked at the spec.
> The spec I have seems to say that in a VMSA implementation without
> virtualization, then CNTPCT is always available, but if it has
> virtualization then a bit needs to be set, which I think is what
> you're referring to.  I think the spec also says that virtualization
> extensions are optional.  How do you deal with the case that they are
> not implemented?  Or maybe that simply isn't supported?

Do you mean where the virtualization extensions are not implemented, but
the timers are? If so:

 * CNTVOFF doesn't exist, and there is no distinction between virtual
   and physical time.
 
 * There is no PL2.

 * The virtual counters and timers exist, and are accessible to PL1.

So using the virtual timers and counters in this case (as we do) works.

> > If an OS is booted at PL2 it can access the physical counters, and
> > should do so in case something like KVM will be used later. The OS can
> > write to CNTVOFF at PL2, and if it sets CNTVOFF to zero the physical and
> > virtual counters are equivalent. Thus it can use the virtual counters
> > and doesn't need to have additional code in several places (including
> > the VDSO) where it needs to choose to read which counters to read.
> >
> > The problem only exists where PL2 exists and the firmware/bootloader
> > skipped PL2 without initialising the necessary PL2 state. This is in
> > general a stupid thing to do; it introduces a problem that need not
> > exist and throws away the option of using the features PL2 provides.
> > This is a firmware/bootloader bug.
> 
> Well it's not quite that simple, this is actually an issue with the
> hardware that the CNTVOFF comes up with different values on different
> cores.  This happens not only at boot, but any time the core is
> powered on, which could include deep sleep or CPU hotplug and suspend
> to ram.  The firmware may not be involved in all these cases, so we
> cannot rely on it to fix this problem.

If the only thing that can restore that state is firmware, then I would
take that as an indication that firmware _must_ be involved in those
cases.

Surely there is other secure or PL2 state that you need to restore in
those cases anyhow?

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

That doesn't sound right to me. I assume from those cases you'd return
back to the OS at PL2. If state is lost in those cases which an OS
cannot restore with PL2 privilege, then you have bigger problems.

Thanks,
Mark.

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

[Mark Rutland]

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

So long as all CPUs have a uniform view of time (with the same CNTFRQ
and no observable offset), things are sane.

Fundamentally, everything that can only be configured at a higher
privilege level must have been configured.

So if booted at PL2, it's not necessary for CNTVOFF to be initialised
while for PL1 it's critical that it is.

> > If an OS is booted at PL1 the physical timers aren't guaranteed to be
> > accessible, so the OS must use the virtual timers. As the OS could be
> > virtualized it must use the virtual counters.
> 
> I was curious to learn more about these modes and looked at the spec.
> The spec I have seems to say that in a VMSA implementation without
> virtualization, then CNTPCT is always available, but if it has
> virtualization then a bit needs to be set, which I think is what
> you're referring to.  I think the spec also says that virtualization
> extensions are optional.  How do you deal with the case that they are
> not implemented?  Or maybe that simply isn't supported?

Do you mean where the virtualization extensions are not implemented, but
the timers are? If so:

 * CNTVOFF doesn't exist, and there is no distinction between virtual
   and physical time.
 
 * There is no PL2.

 * The virtual counters and timers exist, and are accessible to PL1.

So using the virtual timers and counters in this case (as we do) works.

> > If an OS is booted at PL2 it can access the physical counters, and
> > should do so in case something like KVM will be used later. The OS can
> > write to CNTVOFF at PL2, and if it sets CNTVOFF to zero the physical and
> > virtual counters are equivalent. Thus it can use the virtual counters
> > and doesn't need to have additional code in several places (including
> > the VDSO) where it needs to choose to read which counters to read.
> >
> > The problem only exists where PL2 exists and the firmware/bootloader
> > skipped PL2 without initialising the necessary PL2 state. This is in
> > general a stupid thing to do; it introduces a problem that need not
> > exist and throws away the option of using the features PL2 provides.
> > This is a firmware/bootloader bug.
> 
> Well it's not quite that simple, this is actually an issue with the
> hardware that the CNTVOFF comes up with different values on different
> cores.  This happens not only at boot, but any time the core is
> powered on, which could include deep sleep or CPU hotplug and suspend
> to ram.  The firmware may not be involved in all these cases, so we
> cannot rely on it to fix this problem.

If the only thing that can restore that state is firmware, then I would
take that as an indication that firmware _must_ be involved in those
cases.

Surely there is other secure or PL2 state that you need to restore in
those cases anyhow?

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

That doesn't sound right to me. I assume from those cases you'd return
back to the OS at PL2. If state is lost in those cases which an OS
cannot restore with PL2 privilege, then you have bigger problems.

Thanks,
Mark.

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

[Lorenzo Pieralisi]

On Thu, Sep 04, 2014 at 06:01:27PM +0100, Sonny Rao wrote:

[...]

> > If an OS is booted at PL2 it can access the physical counters, and
> > should do so in case something like KVM will be used later. The OS can
> > write to CNTVOFF at PL2, and if it sets CNTVOFF to zero the physical and
> > virtual counters are equivalent. Thus it can use the virtual counters
> > and doesn't need to have additional code in several places (including
> > the VDSO) where it needs to choose to read which counters to read.
> >
> > The problem only exists where PL2 exists and the firmware/bootloader
> > skipped PL2 without initialising the necessary PL2 state. This is in
> > general a stupid thing to do; it introduces a problem that need not
> > exist and throws away the option of using the features PL2 provides.
> > This is a firmware/bootloader bug.
> 
> Well it's not quite that simple, this is actually an issue with the
> hardware that the CNTVOFF comes up with different values on different
> cores.  This happens not only at boot, but any time the core is
> powered on, which could include deep sleep or CPU hotplug and suspend
> to ram.  The firmware may not be involved in all these cases, so we
> cannot rely on it to fix this problem.

And why isn't firmware involved in those cases if it _is_ involved in
cold boot ? Resuming from low-power means resuming the machine/core as it
was when it was running before power down, anything that deviates from that
behaviour is a programming bug.

Lorenzo

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

[Lorenzo Pieralisi]

On Thu, Sep 04, 2014 at 06:01:27PM +0100, Sonny Rao wrote:

[...]

> > If an OS is booted at PL2 it can access the physical counters, and
> > should do so in case something like KVM will be used later. The OS can
> > write to CNTVOFF at PL2, and if it sets CNTVOFF to zero the physical and
> > virtual counters are equivalent. Thus it can use the virtual counters
> > and doesn't need to have additional code in several places (including
> > the VDSO) where it needs to choose to read which counters to read.
> >
> > The problem only exists where PL2 exists and the firmware/bootloader
> > skipped PL2 without initialising the necessary PL2 state. This is in
> > general a stupid thing to do; it introduces a problem that need not
> > exist and throws away the option of using the features PL2 provides.
> > This is a firmware/bootloader bug.
> 
> Well it's not quite that simple, this is actually an issue with the
> hardware that the CNTVOFF comes up with different values on different
> cores.  This happens not only at boot, but any time the core is
> powered on, which could include deep sleep or CPU hotplug and suspend
> to ram.  The firmware may not be involved in all these cases, so we
> cannot rely on it to fix this problem.

And why isn't firmware involved in those cases if it _is_ involved in
cold boot ? Resuming from low-power means resuming the machine/core as it
was when it was running before power down, anything that deviates from that
behaviour is a programming bug.

Lorenzo

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

[Doug Anderson]

Mark,

On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Not if you boot Linux at hyp, as we've recommended for this precise
> reason. That doesn't fix other things like CNTFRQ if the secure
> initialisation doesn't poke that, however.

I'll freely admit that I'm out of my league and out of my comfort zone
here, but...

In the theory that firmware ought to be as minimal as possible
(because it's hard to update and hard to keep in sync with kernel
versions), it seems like firmware ought to start the kernel out in as
permissive mode as it's willing to provide, right?

If the kernel is started out as permissive as possible then it can do
anything it needs to.  Future versions of the kernel can be
implemented to do any way-cool things that they want to do without an
update to firmware, right?  ...and current versions of the kernel can
just shed permissions if they don't want them.

...so if I understand correctly, "Secure SVC" mode is more permissive
than "Non Secure HYP" mode, right?  It looks to me as if we currently
start the kernel in "Secure SVC" mode.  What do you think about the
kernel detecting Secure SVC and then dropping down permission levels
(to Non Secure HYP).  Once it did this, it could update things like
the virtual offset and then transition down further into non-secure
SVC mode.


...or maybe this has been discussed millions of times already and I'm
just clueless.  ...or maybe this is just too hard for the kernel to do
in a generic way?


-Doug

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

[Doug Anderson]

Mark,

On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Not if you boot Linux at hyp, as we've recommended for this precise
> reason. That doesn't fix other things like CNTFRQ if the secure
> initialisation doesn't poke that, however.

I'll freely admit that I'm out of my league and out of my comfort zone
here, but...

In the theory that firmware ought to be as minimal as possible
(because it's hard to update and hard to keep in sync with kernel
versions), it seems like firmware ought to start the kernel out in as
permissive mode as it's willing to provide, right?

If the kernel is started out as permissive as possible then it can do
anything it needs to.  Future versions of the kernel can be
implemented to do any way-cool things that they want to do without an
update to firmware, right?  ...and current versions of the kernel can
just shed permissions if they don't want them.

...so if I understand correctly, "Secure SVC" mode is more permissive
than "Non Secure HYP" mode, right?  It looks to me as if we currently
start the kernel in "Secure SVC" mode.  What do you think about the
kernel detecting Secure SVC and then dropping down permission levels
(to Non Secure HYP).  Once it did this, it could update things like
the virtual offset and then transition down further into non-secure
SVC mode.


...or maybe this has been discussed millions of times already and I'm
just clueless.  ...or maybe this is just too hard for the kernel to do
in a generic way?


-Doug

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

[Catalin Marinas]

On Fri, Sep 05, 2014 at 11:11:47PM +0100, Doug Anderson wrote:
> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Not if you boot Linux at hyp, as we've recommended for this precise
> > reason. That doesn't fix other things like CNTFRQ if the secure
> > initialisation doesn't poke that, however.
> 
> I'll freely admit that I'm out of my league and out of my comfort zone
> here, but...
> 
> In the theory that firmware ought to be as minimal as possible
> (because it's hard to update and hard to keep in sync with kernel
> versions), it seems like firmware ought to start the kernel out in as
> permissive mode as it's willing to provide, right?

Not necessarily (and definitely not for arm64). And we've seen in
practice that the actual deployed kernel may run in a different security
mode than what's in mainline and used for initial development (you may
just not see all the patches upstream). For development, that's indeed
simpler, but once you go into production, a customer requesting some
secure OS for payments etc. and Linux is moved to the non-secure side
(and you end up putting hacks in the kernel because they were not
spotted during initial development with Linux running in secure mode).

> If the kernel is started out as permissive as possible then it can do
> anything it needs to.  Future versions of the kernel can be
> implemented to do any way-cool things that they want to do without an
> update to firmware, right?  ...and current versions of the kernel can
> just shed permissions if they don't want them.
> 
> ...so if I understand correctly, "Secure SVC" mode is more permissive
> than "Non Secure HYP" mode, right?  It looks to me as if we currently
> start the kernel in "Secure SVC" mode.  What do you think about the
> kernel detecting Secure SVC and then dropping down permission levels
> (to Non Secure HYP).  Once it did this, it could update things like
> the virtual offset and then transition down further into non-secure
> SVC mode.

If we talk about ARMv8/AArch64, Secure SVC (aka secure EL1) is not more
permissive than Non-secure Hyp (aka non-secure EL2). The only way to go
from secure EL1 to non-secure EL2 is via EL3 (and SMC call) which means
firmware code. Certain registers like CNTFRQ are only writable in EL3,
CNTVOFF in EL2 or EL3.

-- 
Catalin

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

[Catalin Marinas]

On Fri, Sep 05, 2014 at 11:11:47PM +0100, Doug Anderson wrote:
> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > Not if you boot Linux at hyp, as we've recommended for this precise
> > reason. That doesn't fix other things like CNTFRQ if the secure
> > initialisation doesn't poke that, however.
> 
> I'll freely admit that I'm out of my league and out of my comfort zone
> here, but...
> 
> In the theory that firmware ought to be as minimal as possible
> (because it's hard to update and hard to keep in sync with kernel
> versions), it seems like firmware ought to start the kernel out in as
> permissive mode as it's willing to provide, right?

Not necessarily (and definitely not for arm64). And we've seen in
practice that the actual deployed kernel may run in a different security
mode than what's in mainline and used for initial development (you may
just not see all the patches upstream). For development, that's indeed
simpler, but once you go into production, a customer requesting some
secure OS for payments etc. and Linux is moved to the non-secure side
(and you end up putting hacks in the kernel because they were not
spotted during initial development with Linux running in secure mode).

> If the kernel is started out as permissive as possible then it can do
> anything it needs to.  Future versions of the kernel can be
> implemented to do any way-cool things that they want to do without an
> update to firmware, right?  ...and current versions of the kernel can
> just shed permissions if they don't want them.
> 
> ...so if I understand correctly, "Secure SVC" mode is more permissive
> than "Non Secure HYP" mode, right?  It looks to me as if we currently
> start the kernel in "Secure SVC" mode.  What do you think about the
> kernel detecting Secure SVC and then dropping down permission levels
> (to Non Secure HYP).  Once it did this, it could update things like
> the virtual offset and then transition down further into non-secure
> SVC mode.

If we talk about ARMv8/AArch64, Secure SVC (aka secure EL1) is not more
permissive than Non-secure Hyp (aka non-secure EL2). The only way to go
from secure EL1 to non-secure EL2 is via EL3 (and SMC call) which means
firmware code. Certain registers like CNTFRQ are only writable in EL3,
CNTVOFF in EL2 or EL3.

-- 
Catalin

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

[Doug Anderson]

Catalin,

On Mon, Sep 8, 2014 at 6:54 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Sep 05, 2014 at 11:11:47PM +0100, Doug Anderson wrote:
>> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Not if you boot Linux at hyp, as we've recommended for this precise
>> > reason. That doesn't fix other things like CNTFRQ if the secure
>> > initialisation doesn't poke that, however.
>>
>> I'll freely admit that I'm out of my league and out of my comfort zone
>> here, but...
>>
>> In the theory that firmware ought to be as minimal as possible
>> (because it's hard to update and hard to keep in sync with kernel
>> versions), it seems like firmware ought to start the kernel out in as
>> permissive mode as it's willing to provide, right?
>
> Not necessarily (and definitely not for arm64). And we've seen in
> practice that the actual deployed kernel may run in a different security
> mode than what's in mainline and used for initial development (you may
> just not see all the patches upstream). For development, that's indeed
> simpler, but once you go into production, a customer requesting some
> secure OS for payments etc. and Linux is moved to the non-secure side
> (and you end up putting hacks in the kernel because they were not
> spotted during initial development with Linux running in secure mode).

I guess the important part of my statement is "as it's willing to
provide".  In your case your firmware isn't willing to provide "secure
SVC".  In our case it is.

We've actually shipped products where the firmware boots the kernel in
"secure" mode.  These products have a very different security model
than you're envisioning, I guess.  In these products, less firmware is
better and adding firmware code to do the whole transition to "non
secure hyp mode" just isn't worth it.  It's not just a one-time blob
of code in the firmware before booting the kernel.  (I'm told) it
means somehow keeping firmware code around to help out with turning
processors off/on and to help with resume from S3.  That's
infrastructure that we don't want to add.

To put it another way: if you're already architecting your system to
have firmware manage the secure state then everything you have said
makes sense.  You've got to solve all the problems (processor bringup,
suspend/resume, etc) anyway, so being consistent about having the
kernel in nonsecure HYP makes sense.  ...but if you have no need to
ever limit access to "secure mode" then adding a whole lot of code to
the firmware doesn't make sense to me.


>> If the kernel is started out as permissive as possible then it can do
>> anything it needs to.  Future versions of the kernel can be
>> implemented to do any way-cool things that they want to do without an
>> update to firmware, right?  ...and current versions of the kernel can
>> just shed permissions if they don't want them.
>>
>> ...so if I understand correctly, "Secure SVC" mode is more permissive
>> than "Non Secure HYP" mode, right?  It looks to me as if we currently
>> start the kernel in "Secure SVC" mode.  What do you think about the
>> kernel detecting Secure SVC and then dropping down permission levels
>> (to Non Secure HYP).  Once it did this, it could update things like
>> the virtual offset and then transition down further into non-secure
>> SVC mode.
>
> If we talk about ARMv8/AArch64, Secure SVC (aka secure EL1) is not more
> permissive than Non-secure Hyp (aka non-secure EL2). The only way to go
> from secure EL1 to non-secure EL2 is via EL3 (and SMC call) which means
> firmware code. Certain registers like CNTFRQ are only writable in EL3,
> CNTVOFF in EL2 or EL3.

Hmmm, I guess I was under the impression that if you were in Secure
SVC mode then you are guaranteed to have enough access that you could
control where the SMC call would go.  Thus you could install your own
SMC handler and effectively transition yourself from Secure SVC to
Monitor mode, then to Hypervisor mode.

I was also under the impression that if you were in Nonsecure HYP mode
that you couldn't guarantee that you could control the transition back
to Secure SVC mode.

If the above is incorrect then I guess my statement is incorrect.  Can
you confirm?


...however, even if "Secure SVC" isn't guaranteed to be more
permissive than "Nonsecure HYP", it is often the case that you can
transition from "Secure SVC" to "Nonsecure HYP" (by setting up
handlers, etc) because the system hasn't been locked down.  Given that
if we "do nothing" in firmware we seem to boot the kernel in Secure
SVC w/ the ability to transition to Nonsecure HYP, that seems like a
good reason for the kernel to be able to handle that switching.


-Doug

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

[Doug Anderson]

Catalin,

On Mon, Sep 8, 2014 at 6:54 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Sep 05, 2014 at 11:11:47PM +0100, Doug Anderson wrote:
>> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Not if you boot Linux at hyp, as we've recommended for this precise
>> > reason. That doesn't fix other things like CNTFRQ if the secure
>> > initialisation doesn't poke that, however.
>>
>> I'll freely admit that I'm out of my league and out of my comfort zone
>> here, but...
>>
>> In the theory that firmware ought to be as minimal as possible
>> (because it's hard to update and hard to keep in sync with kernel
>> versions), it seems like firmware ought to start the kernel out in as
>> permissive mode as it's willing to provide, right?
>
> Not necessarily (and definitely not for arm64). And we've seen in
> practice that the actual deployed kernel may run in a different security
> mode than what's in mainline and used for initial development (you may
> just not see all the patches upstream). For development, that's indeed
> simpler, but once you go into production, a customer requesting some
> secure OS for payments etc. and Linux is moved to the non-secure side
> (and you end up putting hacks in the kernel because they were not
> spotted during initial development with Linux running in secure mode).

I guess the important part of my statement is "as it's willing to
provide".  In your case your firmware isn't willing to provide "secure
SVC".  In our case it is.

We've actually shipped products where the firmware boots the kernel in
"secure" mode.  These products have a very different security model
than you're envisioning, I guess.  In these products, less firmware is
better and adding firmware code to do the whole transition to "non
secure hyp mode" just isn't worth it.  It's not just a one-time blob
of code in the firmware before booting the kernel.  (I'm told) it
means somehow keeping firmware code around to help out with turning
processors off/on and to help with resume from S3.  That's
infrastructure that we don't want to add.

To put it another way: if you're already architecting your system to
have firmware manage the secure state then everything you have said
makes sense.  You've got to solve all the problems (processor bringup,
suspend/resume, etc) anyway, so being consistent about having the
kernel in nonsecure HYP makes sense.  ...but if you have no need to
ever limit access to "secure mode" then adding a whole lot of code to
the firmware doesn't make sense to me.


>> If the kernel is started out as permissive as possible then it can do
>> anything it needs to.  Future versions of the kernel can be
>> implemented to do any way-cool things that they want to do without an
>> update to firmware, right?  ...and current versions of the kernel can
>> just shed permissions if they don't want them.
>>
>> ...so if I understand correctly, "Secure SVC" mode is more permissive
>> than "Non Secure HYP" mode, right?  It looks to me as if we currently
>> start the kernel in "Secure SVC" mode.  What do you think about the
>> kernel detecting Secure SVC and then dropping down permission levels
>> (to Non Secure HYP).  Once it did this, it could update things like
>> the virtual offset and then transition down further into non-secure
>> SVC mode.
>
> If we talk about ARMv8/AArch64, Secure SVC (aka secure EL1) is not more
> permissive than Non-secure Hyp (aka non-secure EL2). The only way to go
> from secure EL1 to non-secure EL2 is via EL3 (and SMC call) which means
> firmware code. Certain registers like CNTFRQ are only writable in EL3,
> CNTVOFF in EL2 or EL3.

Hmmm, I guess I was under the impression that if you were in Secure
SVC mode then you are guaranteed to have enough access that you could
control where the SMC call would go.  Thus you could install your own
SMC handler and effectively transition yourself from Secure SVC to
Monitor mode, then to Hypervisor mode.

I was also under the impression that if you were in Nonsecure HYP mode
that you couldn't guarantee that you could control the transition back
to Secure SVC mode.

If the above is incorrect then I guess my statement is incorrect.  Can
you confirm?


...however, even if "Secure SVC" isn't guaranteed to be more
permissive than "Nonsecure HYP", it is often the case that you can
transition from "Secure SVC" to "Nonsecure HYP" (by setting up
handlers, etc) because the system hasn't been locked down.  Given that
if we "do nothing" in firmware we seem to boot the kernel in Secure
SVC w/ the ability to transition to Nonsecure HYP, that seems like a
good reason for the kernel to be able to handle that switching.


-Doug

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

[Will Deacon]

On Wed, Sep 10, 2014 at 06:17:23PM +0100, Doug Anderson wrote:
> On Mon, Sep 8, 2014 at 6:54 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Fri, Sep 05, 2014 at 11:11:47PM +0100, Doug Anderson wrote:
> >> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > Not if you boot Linux at hyp, as we've recommended for this precise
> >> > reason. That doesn't fix other things like CNTFRQ if the secure
> >> > initialisation doesn't poke that, however.
> >>
> >> I'll freely admit that I'm out of my league and out of my comfort zone
> >> here, but...
> >>
> >> In the theory that firmware ought to be as minimal as possible
> >> (because it's hard to update and hard to keep in sync with kernel
> >> versions), it seems like firmware ought to start the kernel out in as
> >> permissive mode as it's willing to provide, right?
> >
> > Not necessarily (and definitely not for arm64). And we've seen in
> > practice that the actual deployed kernel may run in a different security
> > mode than what's in mainline and used for initial development (you may
> > just not see all the patches upstream). For development, that's indeed
> > simpler, but once you go into production, a customer requesting some
> > secure OS for payments etc. and Linux is moved to the non-secure side
> > (and you end up putting hacks in the kernel because they were not
> > spotted during initial development with Linux running in secure mode).
> 
> I guess the important part of my statement is "as it's willing to
> provide".  In your case your firmware isn't willing to provide "secure
> SVC".  In our case it is.
> 
> We've actually shipped products where the firmware boots the kernel in
> "secure" mode.  These products have a very different security model
> than you're envisioning, I guess.  In these products, less firmware is
> better and adding firmware code to do the whole transition to "non
> secure hyp mode" just isn't worth it.  It's not just a one-time blob
> of code in the firmware before booting the kernel.  (I'm told) it
> means somehow keeping firmware code around to help out with turning
> processors off/on and to help with resume from S3.  That's
> infrastructure that we don't want to add.

Setting aside the security model, booting in secure mode can also have a
significant impact on the programming model of system IP, not just the CPU.
For example, the SMMU register file suddenly looks different and the way in
which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
have drivers that know about this so, whilst not impossible, there's a
non-trivial amount of work (and then maintenance overhead) if you want to
support booting Linux on the secure side.

The secure world may be more permissive in some regards, but it's actually
a burden in others. It's not a simple superset of non-secure, and this is
more evident than ever in ARMv8.

As I understand it, this conversation started because we're booting at
non-secure EL1 with a badly configured EL2. That sounds like something
we may have to fix/work around in Linux (Olof points out that it used to
work), but we shouldn't conflate that with booting on the secure side to
get away from broken firmware.

Will

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

[Will Deacon]

On Wed, Sep 10, 2014 at 06:17:23PM +0100, Doug Anderson wrote:
> On Mon, Sep 8, 2014 at 6:54 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Fri, Sep 05, 2014 at 11:11:47PM +0100, Doug Anderson wrote:
> >> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > Not if you boot Linux at hyp, as we've recommended for this precise
> >> > reason. That doesn't fix other things like CNTFRQ if the secure
> >> > initialisation doesn't poke that, however.
> >>
> >> I'll freely admit that I'm out of my league and out of my comfort zone
> >> here, but...
> >>
> >> In the theory that firmware ought to be as minimal as possible
> >> (because it's hard to update and hard to keep in sync with kernel
> >> versions), it seems like firmware ought to start the kernel out in as
> >> permissive mode as it's willing to provide, right?
> >
> > Not necessarily (and definitely not for arm64). And we've seen in
> > practice that the actual deployed kernel may run in a different security
> > mode than what's in mainline and used for initial development (you may
> > just not see all the patches upstream). For development, that's indeed
> > simpler, but once you go into production, a customer requesting some
> > secure OS for payments etc. and Linux is moved to the non-secure side
> > (and you end up putting hacks in the kernel because they were not
> > spotted during initial development with Linux running in secure mode).
> 
> I guess the important part of my statement is "as it's willing to
> provide".  In your case your firmware isn't willing to provide "secure
> SVC".  In our case it is.
> 
> We've actually shipped products where the firmware boots the kernel in
> "secure" mode.  These products have a very different security model
> than you're envisioning, I guess.  In these products, less firmware is
> better and adding firmware code to do the whole transition to "non
> secure hyp mode" just isn't worth it.  It's not just a one-time blob
> of code in the firmware before booting the kernel.  (I'm told) it
> means somehow keeping firmware code around to help out with turning
> processors off/on and to help with resume from S3.  That's
> infrastructure that we don't want to add.

Setting aside the security model, booting in secure mode can also have a
significant impact on the programming model of system IP, not just the CPU.
For example, the SMMU register file suddenly looks different and the way in
which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
have drivers that know about this so, whilst not impossible, there's a
non-trivial amount of work (and then maintenance overhead) if you want to
support booting Linux on the secure side.

The secure world may be more permissive in some regards, but it's actually
a burden in others. It's not a simple superset of non-secure, and this is
more evident than ever in ARMv8.

As I understand it, this conversation started because we're booting at
non-secure EL1 with a badly configured EL2. That sounds like something
we may have to fix/work around in Linux (Olof points out that it used to
work), but we shouldn't conflate that with booting on the secure side to
get away from broken firmware.

Will

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

[Doug Anderson]

Will,

On Wed, Sep 10, 2014 at 10:34 AM, Will Deacon <will.deacon@arm.com> wrote:
> Setting aside the security model, booting in secure mode can also have a
> significant impact on the programming model of system IP, not just the CPU.
> For example, the SMMU register file suddenly looks different and the way in
> which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
> have drivers that know about this so, whilst not impossible, there's a
> non-trivial amount of work (and then maintenance overhead) if you want to
> support booting Linux on the secure side.

Hrm.  I could have sworn someone told me that exynos5250-snow is
booted in Secure SVC mode and has been that way forever.  I could
certainly be wrong.  Stupid question, but should any of your
statements apply to an A15 or an A12?  Maybe things change with newer
CPUs?


On the rk3288 I have in front of me I tried running the following at
(semi early) boot to help confirm we were in Secure SVC.  Standard "I
don't know what I'm doing" caveats apply:

       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);

When I did that I found that I was able to read back 0x00000004.  OK,
and I just replicated that same behavior on exynos5250-snow.

Since I think the SCR register is only accessible from "Secure PL1
modes" and I showed that I could change it, to me that meant that I
must be in "Secure PL1".  Please yell if I'm just being an idiot and I
can look for some other way to test for secure mode.


Also: I wasn't actually suggesting running Linux in secure mode, I was
only suggesting _booting_ Linux in secure mode.  The idea was that
Linux would realize this and immediately switch to nonsecure HYP mode.
Linux doesn't normally run at HYP mode, so it would eventually drop
down into nonsecure SVC mode, too.

One (admittedly weak) justification was that if someone later wanted
to do some whizbang feature in the Linux kernel that required "secure
svc" mode then it would be possible on this hardware.  Obviously this
person would need to implement a whole crapload of code to handle
this, but they wouldn't need to add custom firmware.

The other (important) justification was to keep bootloaders simple.
All of the "I've never even thought about secure mode" bootloaders
that I've seen just happen to boot the kernel in Secure SVC.  That
makes me think that must be the easiest thing for them to do.  We
should support them.


> The secure world may be more permissive in some regards, but it's actually
> a burden in others. It's not a simple superset of non-secure, and this is
> more evident than ever in ARMv8.
>
> As I understand it, this conversation started because we're booting at
> non-secure EL1 with a badly configured EL2. That sounds like something
> we may have to fix/work around in Linux (Olof points out that it used to
> work), but we shouldn't conflate that with booting on the secure side to
> get away from broken firmware.

See above and also my summary to Mark Rutland.  I don't think your
statement above is quite where we're at, but hopefully we're on the
same page now.  :)

-Doug

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

[Doug Anderson]

Will,

On Wed, Sep 10, 2014 at 10:34 AM, Will Deacon <will.deacon@arm.com> wrote:
> Setting aside the security model, booting in secure mode can also have a
> significant impact on the programming model of system IP, not just the CPU.
> For example, the SMMU register file suddenly looks different and the way in
> which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
> have drivers that know about this so, whilst not impossible, there's a
> non-trivial amount of work (and then maintenance overhead) if you want to
> support booting Linux on the secure side.

Hrm.  I could have sworn someone told me that exynos5250-snow is
booted in Secure SVC mode and has been that way forever.  I could
certainly be wrong.  Stupid question, but should any of your
statements apply to an A15 or an A12?  Maybe things change with newer
CPUs?


On the rk3288 I have in front of me I tried running the following at
(semi early) boot to help confirm we were in Secure SVC.  Standard "I
don't know what I'm doing" caveats apply:

       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);

When I did that I found that I was able to read back 0x00000004.  OK,
and I just replicated that same behavior on exynos5250-snow.

Since I think the SCR register is only accessible from "Secure PL1
modes" and I showed that I could change it, to me that meant that I
must be in "Secure PL1".  Please yell if I'm just being an idiot and I
can look for some other way to test for secure mode.


Also: I wasn't actually suggesting running Linux in secure mode, I was
only suggesting _booting_ Linux in secure mode.  The idea was that
Linux would realize this and immediately switch to nonsecure HYP mode.
Linux doesn't normally run@HYP mode, so it would eventually drop
down into nonsecure SVC mode, too.

One (admittedly weak) justification was that if someone later wanted
to do some whizbang feature in the Linux kernel that required "secure
svc" mode then it would be possible on this hardware.  Obviously this
person would need to implement a whole crapload of code to handle
this, but they wouldn't need to add custom firmware.

The other (important) justification was to keep bootloaders simple.
All of the "I've never even thought about secure mode" bootloaders
that I've seen just happen to boot the kernel in Secure SVC.  That
makes me think that must be the easiest thing for them to do.  We
should support them.


> The secure world may be more permissive in some regards, but it's actually
> a burden in others. It's not a simple superset of non-secure, and this is
> more evident than ever in ARMv8.
>
> As I understand it, this conversation started because we're booting at
> non-secure EL1 with a badly configured EL2. That sounds like something
> we may have to fix/work around in Linux (Olof points out that it used to
> work), but we shouldn't conflate that with booting on the secure side to
> get away from broken firmware.

See above and also my summary to Mark Rutland.  I don't think your
statement above is quite where we're at, but hopefully we're on the
same page now.  :)

-Doug

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

[Will Deacon]

On Wed, Sep 10, 2014 at 07:09:59PM +0100, Doug Anderson wrote:
> On Wed, Sep 10, 2014 at 10:34 AM, Will Deacon <will.deacon@arm.com> wrote:
> > Setting aside the security model, booting in secure mode can also have a
> > significant impact on the programming model of system IP, not just the CPU.
> > For example, the SMMU register file suddenly looks different and the way in
> > which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
> > have drivers that know about this so, whilst not impossible, there's a
> > non-trivial amount of work (and then maintenance overhead) if you want to
> > support booting Linux on the secure side.
> 
> Hrm.  I could have sworn someone told me that exynos5250-snow is
> booted in Secure SVC mode and has been that way forever.  I could
> certainly be wrong.  Stupid question, but should any of your
> statements apply to an A15 or an A12?  Maybe things change with newer
> CPUs?

Snow does boot secure, which is why you don't get KVM without hacking the
bootloader. However, snow also doesn't use:

  - An ARMv8 CPU
  - GICv3
  - ARM SMMU
  - Virtualisation (in the supported software image)
  - Secure services (by the architectural definition)

> On the rk3288 I have in front of me I tried running the following at
> (semi early) boot to help confirm we were in Secure SVC.  Standard "I
> don't know what I'm doing" caveats apply:
> 
>        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);
> 
> When I did that I found that I was able to read back 0x00000004.  OK,
> and I just replicated that same behavior on exynos5250-snow.
> 
> Since I think the SCR register is only accessible from "Secure PL1
> modes" and I showed that I could change it, to me that meant that I
> must be in "Secure PL1".  Please yell if I'm just being an idiot and I
> can look for some other way to test for secure mode.

Again, this isn't the kind of platform I was referring to. Until recently,
(32-bit) Linux has booted in secure svc just fine and there's no reason to
break platforms relying on that.

> Also: I wasn't actually suggesting running Linux in secure mode, I was
> only suggesting _booting_ Linux in secure mode.  The idea was that
> Linux would realize this and immediately switch to nonsecure HYP mode.
> Linux doesn't normally run at HYP mode, so it would eventually drop
> down into nonsecure SVC mode, too.

For ARMv7, fill your boots. For ARMv8, this doesn't work unless you enter at
EL3, in which case the kernel is going to need to grow an awful lot of
knowledge to initialise arbitrary systems.

> The other (important) justification was to keep bootloaders simple.
> All of the "I've never even thought about secure mode" bootloaders
> that I've seen just happen to boot the kernel in Secure SVC.  That
> makes me think that must be the easiest thing for them to do.  We
> should support them.

Just to reiterate, you're talking ARMv7 and I'm talking ARMv8. For ARMv7,
there is a separate argument about whether we should make bootloaders
simple and the OS more complex, which has been had on the mailing list
before (I don't think there was a good conclusion though -- Nico and Catalin
were discussing about MCPM and PSCI, which is very much related to this
topic).

> > The secure world may be more permissive in some regards, but it's actually
> > a burden in others. It's not a simple superset of non-secure, and this is
> > more evident than ever in ARMv8.
> >
> > As I understand it, this conversation started because we're booting at
> > non-secure EL1 with a badly configured EL2. That sounds like something
> > we may have to fix/work around in Linux (Olof points out that it used to
> > work), but we shouldn't conflate that with booting on the secure side to
> > get away from broken firmware.
> 
> See above and also my summary to Mark Rutland.  I don't think your
> statement above is quite where we're at, but hopefully we're on the
> same page now.  :)

If `where we're at' is trying to boot an ARMv7 product, then you can boot in
secure svc and lose virtualisation support. Looking forward to ARMv8, this
isn't going to work, so I'd encourage you to start thinking about getting
a working bootloader as a requirement.

Will

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

[Will Deacon]

On Wed, Sep 10, 2014 at 07:09:59PM +0100, Doug Anderson wrote:
> On Wed, Sep 10, 2014 at 10:34 AM, Will Deacon <will.deacon@arm.com> wrote:
> > Setting aside the security model, booting in secure mode can also have a
> > significant impact on the programming model of system IP, not just the CPU.
> > For example, the SMMU register file suddenly looks different and the way in
> > which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
> > have drivers that know about this so, whilst not impossible, there's a
> > non-trivial amount of work (and then maintenance overhead) if you want to
> > support booting Linux on the secure side.
> 
> Hrm.  I could have sworn someone told me that exynos5250-snow is
> booted in Secure SVC mode and has been that way forever.  I could
> certainly be wrong.  Stupid question, but should any of your
> statements apply to an A15 or an A12?  Maybe things change with newer
> CPUs?

Snow does boot secure, which is why you don't get KVM without hacking the
bootloader. However, snow also doesn't use:

  - An ARMv8 CPU
  - GICv3
  - ARM SMMU
  - Virtualisation (in the supported software image)
  - Secure services (by the architectural definition)

> On the rk3288 I have in front of me I tried running the following at
> (semi early) boot to help confirm we were in Secure SVC.  Standard "I
> don't know what I'm doing" caveats apply:
> 
>        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);
> 
> When I did that I found that I was able to read back 0x00000004.  OK,
> and I just replicated that same behavior on exynos5250-snow.
> 
> Since I think the SCR register is only accessible from "Secure PL1
> modes" and I showed that I could change it, to me that meant that I
> must be in "Secure PL1".  Please yell if I'm just being an idiot and I
> can look for some other way to test for secure mode.

Again, this isn't the kind of platform I was referring to. Until recently,
(32-bit) Linux has booted in secure svc just fine and there's no reason to
break platforms relying on that.

> Also: I wasn't actually suggesting running Linux in secure mode, I was
> only suggesting _booting_ Linux in secure mode.  The idea was that
> Linux would realize this and immediately switch to nonsecure HYP mode.
> Linux doesn't normally run at HYP mode, so it would eventually drop
> down into nonsecure SVC mode, too.

For ARMv7, fill your boots. For ARMv8, this doesn't work unless you enter at
EL3, in which case the kernel is going to need to grow an awful lot of
knowledge to initialise arbitrary systems.

> The other (important) justification was to keep bootloaders simple.
> All of the "I've never even thought about secure mode" bootloaders
> that I've seen just happen to boot the kernel in Secure SVC.  That
> makes me think that must be the easiest thing for them to do.  We
> should support them.

Just to reiterate, you're talking ARMv7 and I'm talking ARMv8. For ARMv7,
there is a separate argument about whether we should make bootloaders
simple and the OS more complex, which has been had on the mailing list
before (I don't think there was a good conclusion though -- Nico and Catalin
were discussing about MCPM and PSCI, which is very much related to this
topic).

> > The secure world may be more permissive in some regards, but it's actually
> > a burden in others. It's not a simple superset of non-secure, and this is
> > more evident than ever in ARMv8.
> >
> > As I understand it, this conversation started because we're booting at
> > non-secure EL1 with a badly configured EL2. That sounds like something
> > we may have to fix/work around in Linux (Olof points out that it used to
> > work), but we shouldn't conflate that with booting on the secure side to
> > get away from broken firmware.
> 
> See above and also my summary to Mark Rutland.  I don't think your
> statement above is quite where we're at, but hopefully we're on the
> same page now.  :)

If `where we're at' is trying to boot an ARMv7 product, then you can boot in
secure svc and lose virtualisation support. Looking forward to ARMv8, this
isn't going to work, so I'd encourage you to start thinking about getting
a working bootloader as a requirement.

Will

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

[Doug Anderson]

Will,

On Wed, Sep 10, 2014 at 11:46 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Sep 10, 2014 at 07:09:59PM +0100, Doug Anderson wrote:
>> On Wed, Sep 10, 2014 at 10:34 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > Setting aside the security model, booting in secure mode can also have a
>> > significant impact on the programming model of system IP, not just the CPU.
>> > For example, the SMMU register file suddenly looks different and the way in
>> > which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
>> > have drivers that know about this so, whilst not impossible, there's a
>> > non-trivial amount of work (and then maintenance overhead) if you want to
>> > support booting Linux on the secure side.
>>
>> Hrm.  I could have sworn someone told me that exynos5250-snow is
>> booted in Secure SVC mode and has been that way forever.  I could
>> certainly be wrong.  Stupid question, but should any of your
>> statements apply to an A15 or an A12?  Maybe things change with newer
>> CPUs?
>
> Snow does boot secure, which is why you don't get KVM without hacking the
> bootloader. However, snow also doesn't use:

Well, you could get KVM if the kernel knew to switch from "secure SVC"
to "nonsecure HYP", right?  ...so one bonus of implementing this (for
32-bit ARM only, of course) is that legacy systems (like snow) would
magically get KVM without replacing the bootloader...


> Again, this isn't the kind of platform I was referring to. Until recently,
> (32-bit) Linux has booted in secure svc just fine and there's no reason to
> break platforms relying on that.

Yup, exactly.  I'm definitely not on the cutting edge here.  Just
working on plain old boring 32-bit systems.  ;)  I have even less
knowledge about 64-bit systems than 32 bit if that can be believed.
;)


> Just to reiterate, you're talking ARMv7 and I'm talking ARMv8. For ARMv7,
> there is a separate argument about whether we should make bootloaders
> simple and the OS more complex, which has been had on the mailing list
> before (I don't think there was a good conclusion though -- Nico and Catalin
> were discussing about MCPM and PSCI, which is very much related to this
> topic).

Yup.  I'm super happy that folks are figuring this out. :)  I think my
viewpoint / position is adequately clear at this point, so I'll step
back and let the experts work something out and focus my attention on
kitchens that have fewer chefs.  If things need to be different on
ARMv8 then the folks working on those system will need to solve these
problems.  ...but it would be nice if we weren't forced to solve them
on 32-bit.



>> > The secure world may be more permissive in some regards, but it's actually
>> > a burden in others. It's not a simple superset of non-secure, and this is
>> > more evident than ever in ARMv8.
>> >
>> > As I understand it, this conversation started because we're booting at
>> > non-secure EL1 with a badly configured EL2. That sounds like something
>> > we may have to fix/work around in Linux (Olof points out that it used to
>> > work), but we shouldn't conflate that with booting on the secure side to
>> > get away from broken firmware.
>>
>> See above and also my summary to Mark Rutland.  I don't think your
>> statement above is quite where we're at, but hopefully we're on the
>> same page now.  :)
>
> If `where we're at' is trying to boot an ARMv7 product, then you can boot in
> secure svc and lose virtualisation support. Looking forward to ARMv8, this
> isn't going to work, so I'd encourage you to start thinking about getting
> a working bootloader as a requirement.

Yup, definitely on the same page now.  With everyone working on this
I'd imagine that there will be some nice standards worked out by the
time real ARMv8 is ready to ship?

...so would you say that you're in support of landing the patch to
allow physical counters?  I know Olof has Acked the patch above, but
it's nice if there's general agreement that it's OK.


-Doug

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

[Doug Anderson]

Will,

On Wed, Sep 10, 2014 at 11:46 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Sep 10, 2014 at 07:09:59PM +0100, Doug Anderson wrote:
>> On Wed, Sep 10, 2014 at 10:34 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > Setting aside the security model, booting in secure mode can also have a
>> > significant impact on the programming model of system IP, not just the CPU.
>> > For example, the SMMU register file suddenly looks different and the way in
>> > which it is programmed also changes. The GIC (v3+) is similar. Linux doesn't
>> > have drivers that know about this so, whilst not impossible, there's a
>> > non-trivial amount of work (and then maintenance overhead) if you want to
>> > support booting Linux on the secure side.
>>
>> Hrm.  I could have sworn someone told me that exynos5250-snow is
>> booted in Secure SVC mode and has been that way forever.  I could
>> certainly be wrong.  Stupid question, but should any of your
>> statements apply to an A15 or an A12?  Maybe things change with newer
>> CPUs?
>
> Snow does boot secure, which is why you don't get KVM without hacking the
> bootloader. However, snow also doesn't use:

Well, you could get KVM if the kernel knew to switch from "secure SVC"
to "nonsecure HYP", right?  ...so one bonus of implementing this (for
32-bit ARM only, of course) is that legacy systems (like snow) would
magically get KVM without replacing the bootloader...


> Again, this isn't the kind of platform I was referring to. Until recently,
> (32-bit) Linux has booted in secure svc just fine and there's no reason to
> break platforms relying on that.

Yup, exactly.  I'm definitely not on the cutting edge here.  Just
working on plain old boring 32-bit systems.  ;)  I have even less
knowledge about 64-bit systems than 32 bit if that can be believed.
;)


> Just to reiterate, you're talking ARMv7 and I'm talking ARMv8. For ARMv7,
> there is a separate argument about whether we should make bootloaders
> simple and the OS more complex, which has been had on the mailing list
> before (I don't think there was a good conclusion though -- Nico and Catalin
> were discussing about MCPM and PSCI, which is very much related to this
> topic).

Yup.  I'm super happy that folks are figuring this out. :)  I think my
viewpoint / position is adequately clear at this point, so I'll step
back and let the experts work something out and focus my attention on
kitchens that have fewer chefs.  If things need to be different on
ARMv8 then the folks working on those system will need to solve these
problems.  ...but it would be nice if we weren't forced to solve them
on 32-bit.



>> > The secure world may be more permissive in some regards, but it's actually
>> > a burden in others. It's not a simple superset of non-secure, and this is
>> > more evident than ever in ARMv8.
>> >
>> > As I understand it, this conversation started because we're booting at
>> > non-secure EL1 with a badly configured EL2. That sounds like something
>> > we may have to fix/work around in Linux (Olof points out that it used to
>> > work), but we shouldn't conflate that with booting on the secure side to
>> > get away from broken firmware.
>>
>> See above and also my summary to Mark Rutland.  I don't think your
>> statement above is quite where we're at, but hopefully we're on the
>> same page now.  :)
>
> If `where we're at' is trying to boot an ARMv7 product, then you can boot in
> secure svc and lose virtualisation support. Looking forward to ARMv8, this
> isn't going to work, so I'd encourage you to start thinking about getting
> a working bootloader as a requirement.

Yup, definitely on the same page now.  With everyone working on this
I'd imagine that there will be some nice standards worked out by the
time real ARMv8 is ready to ship?

...so would you say that you're in support of landing the patch to
allow physical counters?  I know Olof has Acked the patch above, but
it's nice if there's general agreement that it's OK.


-Doug

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

[Will Deacon]

On Wed, Sep 10, 2014 at 08:50:16PM +0100, Doug Anderson wrote:
> On Wed, Sep 10, 2014 at 11:46 AM, Will Deacon <will.deacon@arm.com> wrote:
> > If `where we're at' is trying to boot an ARMv7 product, then you can boot in
> > secure svc and lose virtualisation support. Looking forward to ARMv8, this
> > isn't going to work, so I'd encourage you to start thinking about getting
> > a working bootloader as a requirement.
> 
> Yup, definitely on the same page now.  With everyone working on this
> I'd imagine that there will be some nice standards worked out by the
> time real ARMv8 is ready to ship?
> 
> ...so would you say that you're in support of landing the patch to
> allow physical counters?  I know Olof has Acked the patch above, but
> it's nice if there's general agreement that it's OK.

I'm in favour of fixing the regression, yes. What I didn't understand from
the patch is where arch_timer_use_virtual is set to false for your machine,
as we need to be careful not to regress arm64 (the vdso uses the virtual
counter there).

Will

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

[Will Deacon]

On Wed, Sep 10, 2014 at 08:50:16PM +0100, Doug Anderson wrote:
> On Wed, Sep 10, 2014 at 11:46 AM, Will Deacon <will.deacon@arm.com> wrote:
> > If `where we're at' is trying to boot an ARMv7 product, then you can boot in
> > secure svc and lose virtualisation support. Looking forward to ARMv8, this
> > isn't going to work, so I'd encourage you to start thinking about getting
> > a working bootloader as a requirement.
> 
> Yup, definitely on the same page now.  With everyone working on this
> I'd imagine that there will be some nice standards worked out by the
> time real ARMv8 is ready to ship?
> 
> ...so would you say that you're in support of landing the patch to
> allow physical counters?  I know Olof has Acked the patch above, but
> it's nice if there's general agreement that it's OK.

I'm in favour of fixing the regression, yes. What I didn't understand from
the patch is where arch_timer_use_virtual is set to false for your machine,
as we need to be careful not to regress arm64 (the vdso uses the virtual
counter there).

Will

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

[Doug Anderson]

Will,

On Thu, Sep 11, 2014 at 2:57 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Sep 10, 2014 at 08:50:16PM +0100, Doug Anderson wrote:
>> On Wed, Sep 10, 2014 at 11:46 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > If `where we're at' is trying to boot an ARMv7 product, then you can boot in
>> > secure svc and lose virtualisation support. Looking forward to ARMv8, this
>> > isn't going to work, so I'd encourage you to start thinking about getting
>> > a working bootloader as a requirement.
>>
>> Yup, definitely on the same page now.  With everyone working on this
>> I'd imagine that there will be some nice standards worked out by the
>> time real ARMv8 is ready to ship?
>>
>> ...so would you say that you're in support of landing the patch to
>> allow physical counters?  I know Olof has Acked the patch above, but
>> it's nice if there's general agreement that it's OK.
>
> I'm in favour of fixing the regression, yes. What I didn't understand from
> the patch is where arch_timer_use_virtual is set to false for your machine,
> as we need to be careful not to regress arm64 (the vdso uses the virtual
> counter there).

See <https://patchwork.kernel.org/patch/4889311/>

-Doug

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

[Doug Anderson]

Will,

On Thu, Sep 11, 2014 at 2:57 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Sep 10, 2014 at 08:50:16PM +0100, Doug Anderson wrote:
>> On Wed, Sep 10, 2014 at 11:46 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > If `where we're at' is trying to boot an ARMv7 product, then you can boot in
>> > secure svc and lose virtualisation support. Looking forward to ARMv8, this
>> > isn't going to work, so I'd encourage you to start thinking about getting
>> > a working bootloader as a requirement.
>>
>> Yup, definitely on the same page now.  With everyone working on this
>> I'd imagine that there will be some nice standards worked out by the
>> time real ARMv8 is ready to ship?
>>
>> ...so would you say that you're in support of landing the patch to
>> allow physical counters?  I know Olof has Acked the patch above, but
>> it's nice if there's general agreement that it's OK.
>
> I'm in favour of fixing the regression, yes. What I didn't understand from
> the patch is where arch_timer_use_virtual is set to false for your machine,
> as we need to be careful not to regress arm64 (the vdso uses the virtual
> counter there).

See <https://patchwork.kernel.org/patch/4889311/>

-Doug

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

[Christopher Covington]

On 09/05/2014 06:11 PM, Doug Anderson wrote:
> Mark,
> 
> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Not if you boot Linux at hyp, as we've recommended for this precise
>> reason. That doesn't fix other things like CNTFRQ if the secure
>> initialisation doesn't poke that, however.
> 
> I'll freely admit that I'm out of my league and out of my comfort zone
> here, but...
> 
> In the theory that firmware ought to be as minimal as possible
> (because it's hard to update and hard to keep in sync with kernel
> versions), it seems like firmware ought to start the kernel out in as
> permissive mode as it's willing to provide, right?
> 
> If the kernel is started out as permissive as possible then it can do
> anything it needs to.  Future versions of the kernel can be
> implemented to do any way-cool things that they want to do without an
> update to firmware, right?  ...and current versions of the kernel can
> just shed permissions if they don't want them.
> 
> ...so if I understand correctly, "Secure SVC" mode is more permissive
> than "Non Secure HYP" mode, right?  It looks to me as if we currently
> start the kernel in "Secure SVC" mode.  What do you think about the
> kernel detecting Secure SVC and then dropping down permission levels
> (to Non Secure HYP).  Once it did this, it could update things like
> the virtual offset and then transition down further into non-secure
> SVC mode.
> 
> ...or maybe this has been discussed millions of times already and I'm
> just clueless.  ...or maybe this is just too hard for the kernel to do
> in a generic way?

I think this is a great idea. When running on simulators, it would make (the
non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.

Implementing it on AArch64 should be trivial as you can just read CurrentEL
and work from whatever EL/PL you're at. Is there an easy way to check whether
you're in secure or nonsecure mode in AArch32? I seem to recall discussion
about putting this information into the DTB, which makes me think there isn't.

Christopher

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

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

[Christopher Covington]

On 09/05/2014 06:11 PM, Doug Anderson wrote:
> Mark,
> 
> On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Not if you boot Linux at hyp, as we've recommended for this precise
>> reason. That doesn't fix other things like CNTFRQ if the secure
>> initialisation doesn't poke that, however.
> 
> I'll freely admit that I'm out of my league and out of my comfort zone
> here, but...
> 
> In the theory that firmware ought to be as minimal as possible
> (because it's hard to update and hard to keep in sync with kernel
> versions), it seems like firmware ought to start the kernel out in as
> permissive mode as it's willing to provide, right?
> 
> If the kernel is started out as permissive as possible then it can do
> anything it needs to.  Future versions of the kernel can be
> implemented to do any way-cool things that they want to do without an
> update to firmware, right?  ...and current versions of the kernel can
> just shed permissions if they don't want them.
> 
> ...so if I understand correctly, "Secure SVC" mode is more permissive
> than "Non Secure HYP" mode, right?  It looks to me as if we currently
> start the kernel in "Secure SVC" mode.  What do you think about the
> kernel detecting Secure SVC and then dropping down permission levels
> (to Non Secure HYP).  Once it did this, it could update things like
> the virtual offset and then transition down further into non-secure
> SVC mode.
> 
> ...or maybe this has been discussed millions of times already and I'm
> just clueless.  ...or maybe this is just too hard for the kernel to do
> in a generic way?

I think this is a great idea. When running on simulators, it would make (the
non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.

Implementing it on AArch64 should be trivial as you can just read CurrentEL
and work from whatever EL/PL you're at. Is there an easy way to check whether
you're in secure or nonsecure mode in AArch32? I seem to recall discussion
about putting this information into the DTB, which makes me think there isn't.

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] clocksource: arch_timer: Fix code to use physical timers when requested

[Catalin Marinas]

On Wed, Sep 10, 2014 at 03:58:15PM +0100, Christopher Covington wrote:
> On 09/05/2014 06:11 PM, Doug Anderson wrote:
> > On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> Not if you boot Linux at hyp, as we've recommended for this precise
> >> reason. That doesn't fix other things like CNTFRQ if the secure
> >> initialisation doesn't poke that, however.
> > 
> > I'll freely admit that I'm out of my league and out of my comfort zone
> > here, but...
> > 
> > In the theory that firmware ought to be as minimal as possible
> > (because it's hard to update and hard to keep in sync with kernel
> > versions), it seems like firmware ought to start the kernel out in as
> > permissive mode as it's willing to provide, right?
> > 
> > If the kernel is started out as permissive as possible then it can do
> > anything it needs to.  Future versions of the kernel can be
> > implemented to do any way-cool things that they want to do without an
> > update to firmware, right?  ...and current versions of the kernel can
> > just shed permissions if they don't want them.
> > 
> > ...so if I understand correctly, "Secure SVC" mode is more permissive
> > than "Non Secure HYP" mode, right?  It looks to me as if we currently
> > start the kernel in "Secure SVC" mode.  What do you think about the
> > kernel detecting Secure SVC and then dropping down permission levels
> > (to Non Secure HYP).  Once it did this, it could update things like
> > the virtual offset and then transition down further into non-secure
> > SVC mode.
> > 
> > ...or maybe this has been discussed millions of times already and I'm
> > just clueless.  ...or maybe this is just too hard for the kernel to do
> > in a generic way?
> 
> I think this is a great idea. When running on simulators, it would make (the
> non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.
> 
> Implementing it on AArch64 should be trivial as you can just read CurrentEL
> and work from whatever EL/PL you're at. Is there an easy way to check whether
> you're in secure or nonsecure mode in AArch32? I seem to recall discussion
> about putting this information into the DTB, which makes me think there isn't.

(I replied couple of days ago but the SMTP server I was using still
hasn't delivered it)

Anyway, for AArch64 you get the CurrentEL but does not tell you whether
it's secure or not (the same as the mode bits in CPSR basically). Secure
EL1 is not more permissive than non-secure EL2, you still have to go
through EL3 firmware using an SMC call to be able to switch to EL2. So
for AArch64, you need to rely on firmware to do at least the correct
initialisation (unless you start the kernel at EL3).

-- 
Catalin

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

[Catalin Marinas]

On Wed, Sep 10, 2014 at 03:58:15PM +0100, Christopher Covington wrote:
> On 09/05/2014 06:11 PM, Doug Anderson wrote:
> > On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> Not if you boot Linux at hyp, as we've recommended for this precise
> >> reason. That doesn't fix other things like CNTFRQ if the secure
> >> initialisation doesn't poke that, however.
> > 
> > I'll freely admit that I'm out of my league and out of my comfort zone
> > here, but...
> > 
> > In the theory that firmware ought to be as minimal as possible
> > (because it's hard to update and hard to keep in sync with kernel
> > versions), it seems like firmware ought to start the kernel out in as
> > permissive mode as it's willing to provide, right?
> > 
> > If the kernel is started out as permissive as possible then it can do
> > anything it needs to.  Future versions of the kernel can be
> > implemented to do any way-cool things that they want to do without an
> > update to firmware, right?  ...and current versions of the kernel can
> > just shed permissions if they don't want them.
> > 
> > ...so if I understand correctly, "Secure SVC" mode is more permissive
> > than "Non Secure HYP" mode, right?  It looks to me as if we currently
> > start the kernel in "Secure SVC" mode.  What do you think about the
> > kernel detecting Secure SVC and then dropping down permission levels
> > (to Non Secure HYP).  Once it did this, it could update things like
> > the virtual offset and then transition down further into non-secure
> > SVC mode.
> > 
> > ...or maybe this has been discussed millions of times already and I'm
> > just clueless.  ...or maybe this is just too hard for the kernel to do
> > in a generic way?
> 
> I think this is a great idea. When running on simulators, it would make (the
> non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.
> 
> Implementing it on AArch64 should be trivial as you can just read CurrentEL
> and work from whatever EL/PL you're at. Is there an easy way to check whether
> you're in secure or nonsecure mode in AArch32? I seem to recall discussion
> about putting this information into the DTB, which makes me think there isn't.

(I replied couple of days ago but the SMTP server I was using still
hasn't delivered it)

Anyway, for AArch64 you get the CurrentEL but does not tell you whether
it's secure or not (the same as the mode bits in CPSR basically). Secure
EL1 is not more permissive than non-secure EL2, you still have to go
through EL3 firmware using an SMC call to be able to switch to EL2. So
for AArch64, you need to rely on firmware to do at least the correct
initialisation (unless you start the kernel at EL3).

-- 
Catalin

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

[Mark Rutland]

On Wed, Sep 10, 2014 at 03:58:15PM +0100, Christopher Covington wrote:
> On 09/05/2014 06:11 PM, Doug Anderson wrote:
> > Mark,
> > 
> > On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> Not if you boot Linux at hyp, as we've recommended for this precise
> >> reason. That doesn't fix other things like CNTFRQ if the secure
> >> initialisation doesn't poke that, however.
> > 
> > I'll freely admit that I'm out of my league and out of my comfort zone
> > here, but...
> > 
> > In the theory that firmware ought to be as minimal as possible
> > (because it's hard to update and hard to keep in sync with kernel
> > versions), it seems like firmware ought to start the kernel out in as
> > permissive mode as it's willing to provide, right?
> > 
> > If the kernel is started out as permissive as possible then it can do
> > anything it needs to.  Future versions of the kernel can be
> > implemented to do any way-cool things that they want to do without an
> > update to firmware, right?  ...and current versions of the kernel can
> > just shed permissions if they don't want them.
> > 
> > ...so if I understand correctly, "Secure SVC" mode is more permissive
> > than "Non Secure HYP" mode, right?  It looks to me as if we currently
> > start the kernel in "Secure SVC" mode.  What do you think about the
> > kernel detecting Secure SVC and then dropping down permission levels
> > (to Non Secure HYP).  Once it did this, it could update things like
> > the virtual offset and then transition down further into non-secure
> > SVC mode.
> > 
> > ...or maybe this has been discussed millions of times already and I'm
> > just clueless.  ...or maybe this is just too hard for the kernel to do
> > in a generic way?
> 
> I think this is a great idea. When running on simulators, it would make (the
> non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.

That's not true in general as other secure initialization will still be
necessary, and the extent and character of that initialization is going
to be implementation specific. 

If you have the infrastructure necessary to load and boot an arbitrary
kernel at the most highly privileged exception level, then you
necessarily have all the plumbing in place for loading an updateable
firmware into that level. That latter option is preferable.

The issue at hand is not whether Linux should be in charge of secure
world state. The issue at hand is that firmware booted Linux at PL1
without taking ownership of all PL2 state (CNTVOFF included). Were
either the firmware or Linux to manage PL2 (configuring CNTVOFF with a
consistent value on all CPUs) that problem would not exist.

> Implementing it on AArch64 should be trivial as you can just read
> CurrentEL
> and work from whatever EL/PL you're at.

While it's trivial to identify which EL software is executing in, doing
the right thing based on that is going to be far harder, especially in a
general purpose OS.

A loadable piece of firmware can know what is best for a particular
platform (e.g. which errata apply which require poking implementation
defined registers with magic implementation defined values) as it is
tied to that piece of hardware.

Mark.

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

[Mark Rutland]

On Wed, Sep 10, 2014 at 03:58:15PM +0100, Christopher Covington wrote:
> On 09/05/2014 06:11 PM, Doug Anderson wrote:
> > Mark,
> > 
> > On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> Not if you boot Linux at hyp, as we've recommended for this precise
> >> reason. That doesn't fix other things like CNTFRQ if the secure
> >> initialisation doesn't poke that, however.
> > 
> > I'll freely admit that I'm out of my league and out of my comfort zone
> > here, but...
> > 
> > In the theory that firmware ought to be as minimal as possible
> > (because it's hard to update and hard to keep in sync with kernel
> > versions), it seems like firmware ought to start the kernel out in as
> > permissive mode as it's willing to provide, right?
> > 
> > If the kernel is started out as permissive as possible then it can do
> > anything it needs to.  Future versions of the kernel can be
> > implemented to do any way-cool things that they want to do without an
> > update to firmware, right?  ...and current versions of the kernel can
> > just shed permissions if they don't want them.
> > 
> > ...so if I understand correctly, "Secure SVC" mode is more permissive
> > than "Non Secure HYP" mode, right?  It looks to me as if we currently
> > start the kernel in "Secure SVC" mode.  What do you think about the
> > kernel detecting Secure SVC and then dropping down permission levels
> > (to Non Secure HYP).  Once it did this, it could update things like
> > the virtual offset and then transition down further into non-secure
> > SVC mode.
> > 
> > ...or maybe this has been discussed millions of times already and I'm
> > just clueless.  ...or maybe this is just too hard for the kernel to do
> > in a generic way?
> 
> I think this is a great idea. When running on simulators, it would make (the
> non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.

That's not true in general as other secure initialization will still be
necessary, and the extent and character of that initialization is going
to be implementation specific. 

If you have the infrastructure necessary to load and boot an arbitrary
kernel at the most highly privileged exception level, then you
necessarily have all the plumbing in place for loading an updateable
firmware into that level. That latter option is preferable.

The issue at hand is not whether Linux should be in charge of secure
world state. The issue at hand is that firmware booted Linux at PL1
without taking ownership of all PL2 state (CNTVOFF included). Were
either the firmware or Linux to manage PL2 (configuring CNTVOFF with a
consistent value on all CPUs) that problem would not exist.

> Implementing it on AArch64 should be trivial as you can just read
> CurrentEL
> and work from whatever EL/PL you're at.

While it's trivial to identify which EL software is executing in, doing
the right thing based on that is going to be far harder, especially in a
general purpose OS.

A loadable piece of firmware can know what is best for a particular
platform (e.g. which errata apply which require poking implementation
defined registers with magic implementation defined values) as it is
tied to that piece of hardware.

Mark.

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

[Olof Johansson]

On Wed, Sep 10, 2014 at 8:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Sep 10, 2014 at 03:58:15PM +0100, Christopher Covington wrote:
>> On 09/05/2014 06:11 PM, Doug Anderson wrote:
>> > Mark,
>> >
>> > On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> Not if you boot Linux at hyp, as we've recommended for this precise
>> >> reason. That doesn't fix other things like CNTFRQ if the secure
>> >> initialisation doesn't poke that, however.
>> >
>> > I'll freely admit that I'm out of my league and out of my comfort zone
>> > here, but...
>> >
>> > In the theory that firmware ought to be as minimal as possible
>> > (because it's hard to update and hard to keep in sync with kernel
>> > versions), it seems like firmware ought to start the kernel out in as
>> > permissive mode as it's willing to provide, right?
>> >
>> > If the kernel is started out as permissive as possible then it can do
>> > anything it needs to.  Future versions of the kernel can be
>> > implemented to do any way-cool things that they want to do without an
>> > update to firmware, right?  ...and current versions of the kernel can
>> > just shed permissions if they don't want them.
>> >
>> > ...so if I understand correctly, "Secure SVC" mode is more permissive
>> > than "Non Secure HYP" mode, right?  It looks to me as if we currently
>> > start the kernel in "Secure SVC" mode.  What do you think about the
>> > kernel detecting Secure SVC and then dropping down permission levels
>> > (to Non Secure HYP).  Once it did this, it could update things like
>> > the virtual offset and then transition down further into non-secure
>> > SVC mode.
>> >
>> > ...or maybe this has been discussed millions of times already and I'm
>> > just clueless.  ...or maybe this is just too hard for the kernel to do
>> > in a generic way?
>>
>> I think this is a great idea. When running on simulators, it would make (the
>> non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.
>
> That's not true in general as other secure initialization will still be
> necessary, and the extent and character of that initialization is going
> to be implementation specific.
>
> If you have the infrastructure necessary to load and boot an arbitrary
> kernel at the most highly privileged exception level, then you
> necessarily have all the plumbing in place for loading an updateable
> firmware into that level. That latter option is preferable.

It might be preferable by you since it makes the kernel simpler, but
for the rest of us who are busy shipping product, it means extra work.
Why would we have to write a brand new firmware layer for this? No
fricking way.

> The issue at hand is not whether Linux should be in charge of secure
> world state. The issue at hand is that firmware booted Linux at PL1
> without taking ownership of all PL2 state (CNTVOFF included). Were
> either the firmware or Linux to manage PL2 (configuring CNTVOFF with a
> consistent value on all CPUs) that problem would not exist.

No, the issue at hand is that Linux is now expecting PL2 resources to
always have been set up. That is a regression in the true sense. It
didn't use to be the case, and now it is.



-Olof

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

[Olof Johansson]

On Wed, Sep 10, 2014 at 8:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Sep 10, 2014 at 03:58:15PM +0100, Christopher Covington wrote:
>> On 09/05/2014 06:11 PM, Doug Anderson wrote:
>> > Mark,
>> >
>> > On Thu, Aug 28, 2014 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> Not if you boot Linux at hyp, as we've recommended for this precise
>> >> reason. That doesn't fix other things like CNTFRQ if the secure
>> >> initialisation doesn't poke that, however.
>> >
>> > I'll freely admit that I'm out of my league and out of my comfort zone
>> > here, but...
>> >
>> > In the theory that firmware ought to be as minimal as possible
>> > (because it's hard to update and hard to keep in sync with kernel
>> > versions), it seems like firmware ought to start the kernel out in as
>> > permissive mode as it's willing to provide, right?
>> >
>> > If the kernel is started out as permissive as possible then it can do
>> > anything it needs to.  Future versions of the kernel can be
>> > implemented to do any way-cool things that they want to do without an
>> > update to firmware, right?  ...and current versions of the kernel can
>> > just shed permissions if they don't want them.
>> >
>> > ...so if I understand correctly, "Secure SVC" mode is more permissive
>> > than "Non Secure HYP" mode, right?  It looks to me as if we currently
>> > start the kernel in "Secure SVC" mode.  What do you think about the
>> > kernel detecting Secure SVC and then dropping down permission levels
>> > (to Non Secure HYP).  Once it did this, it could update things like
>> > the virtual offset and then transition down further into non-secure
>> > SVC mode.
>> >
>> > ...or maybe this has been discussed millions of times already and I'm
>> > just clueless.  ...or maybe this is just too hard for the kernel to do
>> > in a generic way?
>>
>> I think this is a great idea. When running on simulators, it would make (the
>> non-DTB parts of) the bootwrapper and QEMU's built-in bootloader unnecessary.
>
> That's not true in general as other secure initialization will still be
> necessary, and the extent and character of that initialization is going
> to be implementation specific.
>
> If you have the infrastructure necessary to load and boot an arbitrary
> kernel at the most highly privileged exception level, then you
> necessarily have all the plumbing in place for loading an updateable
> firmware into that level. That latter option is preferable.

It might be preferable by you since it makes the kernel simpler, but
for the rest of us who are busy shipping product, it means extra work.
Why would we have to write a brand new firmware layer for this? No
fricking way.

> The issue at hand is not whether Linux should be in charge of secure
> world state. The issue at hand is that firmware booted Linux at PL1
> without taking ownership of all PL2 state (CNTVOFF included). Were
> either the firmware or Linux to manage PL2 (configuring CNTVOFF with a
> consistent value on all CPUs) that problem would not exist.

No, the issue at hand is that Linux is now expecting PL2 resources to
always have been set up. That is a regression in the true sense. It
didn't use to be the case, and now it is.



-Olof

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

[Doug Anderson]

Mark,

On Wed, Sep 10, 2014 at 8:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> That's not true in general as other secure initialization will still be
> necessary, and the extent and character of that initialization is going
> to be implementation specific.

Can you give more examples of what you mean by implementation
specific?  Does this mean that secure initialization will be different
for every SoC out there?  ...or is it just different for different ARM
cores?

-Doug

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

[Doug Anderson]

Mark,

On Wed, Sep 10, 2014 at 8:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> That's not true in general as other secure initialization will still be
> necessary, and the extent and character of that initialization is going
> to be implementation specific.

Can you give more examples of what you mean by implementation
specific?  Does this mean that secure initialization will be different
for every SoC out there?  ...or is it just different for different ARM
cores?

-Doug

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

[Marc Zyngier]

[cc-ing Will for the VDSO bits]

Hi Sonny,

On 27/08/14 22:03, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false.  It restores the
> arch_counter_get_cntpct() function after removal in
> 
> 0d651e4e "clocksource: arch_timer: use virtual counters"

This isn't a bug, but rather a feature. It allows the kernel to
consistently use the virtual counter everywhere (including userspace),
no matter if it behaves as a host or a guest, without worrying about the
accessibility of the physical counter (hint: you have no way of knowing
if it is accessible or not).

> and completes the implementation of memory mapped access for physical
> timers, so if a system is trying to use physical timers, it will
> function properly.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
>  arch/arm/include/asm/arch_timer.h    |  9 +++++++++
>  arch/arm64/include/asm/arch_timer.h  | 10 ++++++++++
>  drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
>  3 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0704e0c..e72aa4d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
>  	return val;
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 9400596..58657c4 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
>  #endif
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> +
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5163ec1..ad723cb 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -30,6 +30,8 @@
>  #define CNTTIDR		0x08
>  #define CNTTIDR_VIRT(n)	(BIT(1) << ((n) * 4))
>  
> +#define CNTPCT_LO	0x00
> +#define CNTPCT_HI	0x04
>  #define CNTVCT_LO	0x08
>  #define CNTVCT_HI	0x0c
>  #define CNTFRQ		0x10
> @@ -386,6 +388,19 @@ static u64 arch_counter_get_cntvct_mem(void)
>  	return ((u64) vct_hi << 32) | vct_lo;
>  }
>  
> +static u64 arch_counter_get_cntpct_mem(void)
> +{
> +	u32 pct_lo, pct_hi, tmp_hi;
> +
> +	do {
> +		pct_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +		pct_lo = readl_relaxed(arch_counter_base + CNTPCT_LO);
> +		tmp_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +	} while (pct_hi != tmp_hi);
> +
> +	return ((u64) pct_hi << 32) | pct_lo;
> +}
> +
>  /*
>   * Default to cp15 based access because arm64 uses this function for
>   * sched_clock() before DT is probed and the cp15 method is guaranteed
> @@ -429,10 +444,17 @@ static void __init arch_counter_register(unsigned type)
>  	u64 start_count;
>  
>  	/* Register the CP15 based counter if we have one */
> -	if (type & ARCH_CP15_TIMER)
> -		arch_timer_read_counter = arch_counter_get_cntvct;
> -	else
> -		arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +	if (type & ARCH_CP15_TIMER) {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct;
> +	} else {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct_mem;
> +	}
>  
>  	start_count = arch_timer_read_counter();
>  	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
> 

What will be the effect on arm64 where the VDSO uses the virtual counter
to implement gettimeofday?

Thanks,

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

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

[Marc Zyngier]

[cc-ing Will for the VDSO bits]

Hi Sonny,

On 27/08/14 22:03, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false.  It restores the
> arch_counter_get_cntpct() function after removal in
> 
> 0d651e4e "clocksource: arch_timer: use virtual counters"

This isn't a bug, but rather a feature. It allows the kernel to
consistently use the virtual counter everywhere (including userspace),
no matter if it behaves as a host or a guest, without worrying about the
accessibility of the physical counter (hint: you have no way of knowing
if it is accessible or not).

> and completes the implementation of memory mapped access for physical
> timers, so if a system is trying to use physical timers, it will
> function properly.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
>  arch/arm/include/asm/arch_timer.h    |  9 +++++++++
>  arch/arm64/include/asm/arch_timer.h  | 10 ++++++++++
>  drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
>  3 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0704e0c..e72aa4d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
>  	return val;
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 9400596..58657c4 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
>  #endif
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> +
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5163ec1..ad723cb 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -30,6 +30,8 @@
>  #define CNTTIDR		0x08
>  #define CNTTIDR_VIRT(n)	(BIT(1) << ((n) * 4))
>  
> +#define CNTPCT_LO	0x00
> +#define CNTPCT_HI	0x04
>  #define CNTVCT_LO	0x08
>  #define CNTVCT_HI	0x0c
>  #define CNTFRQ		0x10
> @@ -386,6 +388,19 @@ static u64 arch_counter_get_cntvct_mem(void)
>  	return ((u64) vct_hi << 32) | vct_lo;
>  }
>  
> +static u64 arch_counter_get_cntpct_mem(void)
> +{
> +	u32 pct_lo, pct_hi, tmp_hi;
> +
> +	do {
> +		pct_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +		pct_lo = readl_relaxed(arch_counter_base + CNTPCT_LO);
> +		tmp_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +	} while (pct_hi != tmp_hi);
> +
> +	return ((u64) pct_hi << 32) | pct_lo;
> +}
> +
>  /*
>   * Default to cp15 based access because arm64 uses this function for
>   * sched_clock() before DT is probed and the cp15 method is guaranteed
> @@ -429,10 +444,17 @@ static void __init arch_counter_register(unsigned type)
>  	u64 start_count;
>  
>  	/* Register the CP15 based counter if we have one */
> -	if (type & ARCH_CP15_TIMER)
> -		arch_timer_read_counter = arch_counter_get_cntvct;
> -	else
> -		arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +	if (type & ARCH_CP15_TIMER) {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct;
> +	} else {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct_mem;
> +	}
>  
>  	start_count = arch_timer_read_counter();
>  	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
> 

What will be the effect on arm64 where the VDSO uses the virtual counter
to implement gettimeofday?

Thanks,

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

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

[Mark Rutland]

Hi Sonny,

On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false.  It restores the
> arch_counter_get_cntpct() function after removal in
> 
> 0d651e4e "clocksource: arch_timer: use virtual counters"
> 
> and completes the implementation of memory mapped access for physical
> timers, so if a system is trying to use physical timers, it will
> function properly.

To get back to the topic at hand:

Which platform is this required by?

Why exactly is arch_timer_use_virtual false in this case?

Thanks,
Mark.

> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
>  arch/arm/include/asm/arch_timer.h    |  9 +++++++++
>  arch/arm64/include/asm/arch_timer.h  | 10 ++++++++++
>  drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
>  3 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0704e0c..e72aa4d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
>  	return val;
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 9400596..58657c4 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
>  #endif
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> +
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5163ec1..ad723cb 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -30,6 +30,8 @@
>  #define CNTTIDR		0x08
>  #define CNTTIDR_VIRT(n)	(BIT(1) << ((n) * 4))
>  
> +#define CNTPCT_LO	0x00
> +#define CNTPCT_HI	0x04
>  #define CNTVCT_LO	0x08
>  #define CNTVCT_HI	0x0c
>  #define CNTFRQ		0x10
> @@ -386,6 +388,19 @@ static u64 arch_counter_get_cntvct_mem(void)
>  	return ((u64) vct_hi << 32) | vct_lo;
>  }
>  
> +static u64 arch_counter_get_cntpct_mem(void)
> +{
> +	u32 pct_lo, pct_hi, tmp_hi;
> +
> +	do {
> +		pct_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +		pct_lo = readl_relaxed(arch_counter_base + CNTPCT_LO);
> +		tmp_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +	} while (pct_hi != tmp_hi);
> +
> +	return ((u64) pct_hi << 32) | pct_lo;
> +}
> +
>  /*
>   * Default to cp15 based access because arm64 uses this function for
>   * sched_clock() before DT is probed and the cp15 method is guaranteed
> @@ -429,10 +444,17 @@ static void __init arch_counter_register(unsigned type)
>  	u64 start_count;
>  
>  	/* Register the CP15 based counter if we have one */
> -	if (type & ARCH_CP15_TIMER)
> -		arch_timer_read_counter = arch_counter_get_cntvct;
> -	else
> -		arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +	if (type & ARCH_CP15_TIMER) {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct;
> +	} else {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct_mem;
> +	}
>  
>  	start_count = arch_timer_read_counter();
>  	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

[Mark Rutland]

Hi Sonny,

On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false.  It restores the
> arch_counter_get_cntpct() function after removal in
> 
> 0d651e4e "clocksource: arch_timer: use virtual counters"
> 
> and completes the implementation of memory mapped access for physical
> timers, so if a system is trying to use physical timers, it will
> function properly.

To get back to the topic at hand:

Which platform is this required by?

Why exactly is arch_timer_use_virtual false in this case?

Thanks,
Mark.

> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---
>  arch/arm/include/asm/arch_timer.h    |  9 +++++++++
>  arch/arm64/include/asm/arch_timer.h  | 10 ++++++++++
>  drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
>  3 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0704e0c..e72aa4d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
>  	return val;
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 9400596..58657c4 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
>  #endif
>  }
>  
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> +	u64 cval;
> +
> +	isb();
> +	asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> +
> +	return cval;
> +}
> +
>  static inline u64 arch_counter_get_cntvct(void)
>  {
>  	u64 cval;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5163ec1..ad723cb 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -30,6 +30,8 @@
>  #define CNTTIDR		0x08
>  #define CNTTIDR_VIRT(n)	(BIT(1) << ((n) * 4))
>  
> +#define CNTPCT_LO	0x00
> +#define CNTPCT_HI	0x04
>  #define CNTVCT_LO	0x08
>  #define CNTVCT_HI	0x0c
>  #define CNTFRQ		0x10
> @@ -386,6 +388,19 @@ static u64 arch_counter_get_cntvct_mem(void)
>  	return ((u64) vct_hi << 32) | vct_lo;
>  }
>  
> +static u64 arch_counter_get_cntpct_mem(void)
> +{
> +	u32 pct_lo, pct_hi, tmp_hi;
> +
> +	do {
> +		pct_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +		pct_lo = readl_relaxed(arch_counter_base + CNTPCT_LO);
> +		tmp_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
> +	} while (pct_hi != tmp_hi);
> +
> +	return ((u64) pct_hi << 32) | pct_lo;
> +}
> +
>  /*
>   * Default to cp15 based access because arm64 uses this function for
>   * sched_clock() before DT is probed and the cp15 method is guaranteed
> @@ -429,10 +444,17 @@ static void __init arch_counter_register(unsigned type)
>  	u64 start_count;
>  
>  	/* Register the CP15 based counter if we have one */
> -	if (type & ARCH_CP15_TIMER)
> -		arch_timer_read_counter = arch_counter_get_cntvct;
> -	else
> -		arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +	if (type & ARCH_CP15_TIMER) {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct;
> +	} else {
> +		if (arch_timer_use_virtual)
> +			arch_timer_read_counter = arch_counter_get_cntvct_mem;
> +		else
> +			arch_timer_read_counter = arch_counter_get_cntpct_mem;
> +	}
>  
>  	start_count = arch_timer_read_counter();
>  	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
> -- 
> 1.8.3.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

[Doug Anderson]

Mark,

On Wed, Sep 10, 2014 at 10:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Sonny,
>
> On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
>> This is a bug fix for using physical arch timers when
>> the arch_timer_use_virtual boolean is false.  It restores the
>> arch_counter_get_cntpct() function after removal in
>>
>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>
>> and completes the implementation of memory mapped access for physical
>> timers, so if a system is trying to use physical timers, it will
>> function properly.
>
> To get back to the topic at hand:
>
> Which platform is this required by?

I've seen similar problems on the A7s on exynos5420 / exynos5800 and
on rk3288.  I can't say what other platforms might be affected.  Note
that the arch timers on exyno5420/exynos5800 are not supported anyway,
so I guess that means we're just worrying about the rk3288.


> Why exactly is arch_timer_use_virtual false in this case?

To re-summarize my understanding of everything (many of the below is
secondhand knowledge, so correct if wrong):

1. The initial problem is that the virtual offset is not initialized
by anyone and is per core (each core gets a different, random offset).
That makes the virtual counter useless.  ...but the kernel only uses
virtual counters.

2. As far as I know, we don't have any particular need for HYP mode
nor for limiting access to secure mode.

3. You can only change the virtual offset from HYP mode.  That means
someone needs to transition to HYP mode if we want to use virtual
counters.

4. If the kernel happens to be in HYP mode it will init the virtual offset.

5. We could transition to HYP mode once in the firmware and boot the
kernel like that, but since firmware is gone after we've booted the
kernel, we run into the same problem when we power off a processor and
when we resume from S3.  The firmware is not involved in these cases.
In these cases the processors have an uninitialized virtual offset
again.  These processors don't appear to magically come up in HYP
mode.  Thus it would be up to the kernel to transition to HYP mode,
init the offset, and get out of HYP mode.  ...or just use the physical
counter.


If you can suggest something that doesn't require us to involve the
firmware in processor bringup and in resume, I'm all ears.  We have a
desire not to involve the firmware there because of all the issues of
keeping kernel/firmware in sync and because of the extra difficultly
of shipping firmware updates (understandably the QA needed to validate
a new firmware is much higher than the QA needed to validate a new
kernel).


-Doug

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

[Doug Anderson]

Mark,

On Wed, Sep 10, 2014 at 10:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Sonny,
>
> On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
>> This is a bug fix for using physical arch timers when
>> the arch_timer_use_virtual boolean is false.  It restores the
>> arch_counter_get_cntpct() function after removal in
>>
>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>
>> and completes the implementation of memory mapped access for physical
>> timers, so if a system is trying to use physical timers, it will
>> function properly.
>
> To get back to the topic at hand:
>
> Which platform is this required by?

I've seen similar problems on the A7s on exynos5420 / exynos5800 and
on rk3288.  I can't say what other platforms might be affected.  Note
that the arch timers on exyno5420/exynos5800 are not supported anyway,
so I guess that means we're just worrying about the rk3288.


> Why exactly is arch_timer_use_virtual false in this case?

To re-summarize my understanding of everything (many of the below is
secondhand knowledge, so correct if wrong):

1. The initial problem is that the virtual offset is not initialized
by anyone and is per core (each core gets a different, random offset).
That makes the virtual counter useless.  ...but the kernel only uses
virtual counters.

2. As far as I know, we don't have any particular need for HYP mode
nor for limiting access to secure mode.

3. You can only change the virtual offset from HYP mode.  That means
someone needs to transition to HYP mode if we want to use virtual
counters.

4. If the kernel happens to be in HYP mode it will init the virtual offset.

5. We could transition to HYP mode once in the firmware and boot the
kernel like that, but since firmware is gone after we've booted the
kernel, we run into the same problem when we power off a processor and
when we resume from S3.  The firmware is not involved in these cases.
In these cases the processors have an uninitialized virtual offset
again.  These processors don't appear to magically come up in HYP
mode.  Thus it would be up to the kernel to transition to HYP mode,
init the offset, and get out of HYP mode.  ...or just use the physical
counter.


If you can suggest something that doesn't require us to involve the
firmware in processor bringup and in resume, I'm all ears.  We have a
desire not to involve the firmware there because of all the issues of
keeping kernel/firmware in sync and because of the extra difficultly
of shipping firmware updates (understandably the QA needed to validate
a new firmware is much higher than the QA needed to validate a new
kernel).


-Doug

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

[Sonny Rao]

On Wed, Sep 10, 2014 at 10:52 AM, Doug Anderson <dianders@chromium.org> wrote:
> Mark,
>
> On Wed, Sep 10, 2014 at 10:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi Sonny,
>>
>> On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
>>> This is a bug fix for using physical arch timers when
>>> the arch_timer_use_virtual boolean is false.  It restores the
>>> arch_counter_get_cntpct() function after removal in
>>>
>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>
>>> and completes the implementation of memory mapped access for physical
>>> timers, so if a system is trying to use physical timers, it will
>>> function properly.
>>
>> To get back to the topic at hand:
>>
>> Which platform is this required by?
>
> I've seen similar problems on the A7s on exynos5420 / exynos5800 and
> on rk3288.  I can't say what other platforms might be affected.  Note
> that the arch timers on exyno5420/exynos5800 are not supported anyway,
> so I guess that means we're just worrying about the rk3288.

Yeah, given that this problem has manifest on at least two different
SoCs using ARM's cores, it would have been nice if the offset were
specified to start out as zero when in reset by the architecture (and
was implemented that way in ARM's core IP), but it looks like it
wasn't.

>
>
>> Why exactly is arch_timer_use_virtual false in this case?
>
> To re-summarize my understanding of everything (many of the below is
> secondhand knowledge, so correct if wrong):
>
> 1. The initial problem is that the virtual offset is not initialized
> by anyone and is per core (each core gets a different, random offset).
> That makes the virtual counter useless.  ...but the kernel only uses
> virtual counters.
>
> 2. As far as I know, we don't have any particular need for HYP mode
> nor for limiting access to secure mode.
>
> 3. You can only change the virtual offset from HYP mode.  That means
> someone needs to transition to HYP mode if we want to use virtual
> counters.
>
> 4. If the kernel happens to be in HYP mode it will init the virtual offset.
>
> 5. We could transition to HYP mode once in the firmware and boot the
> kernel like that, but since firmware is gone after we've booted the
> kernel, we run into the same problem when we power off a processor and
> when we resume from S3.  The firmware is not involved in these cases.
> In these cases the processors have an uninitialized virtual offset
> again.  These processors don't appear to magically come up in HYP
> mode.  Thus it would be up to the kernel to transition to HYP mode,
> init the offset, and get out of HYP mode.  ...or just use the physical
> counter.
>
>
> If you can suggest something that doesn't require us to involve the
> firmware in processor bringup and in resume, I'm all ears.  We have a
> desire not to involve the firmware there because of all the issues of
> keeping kernel/firmware in sync and because of the extra difficultly
> of shipping firmware updates (understandably the QA needed to validate
> a new firmware is much higher than the QA needed to validate a new
> kernel).

One thing that was used in the past was to have the kernel load a blob
from /lib/firmware/ which did some re-initialization when coming out
of suspend or deep sleep.  We could do something similar here and have
it either fix virtual offset or put it into hyp mode.  That would help
solve our issue of making it easier to update and avoid QA hassles.
Is such a solution acceptable to upstream?


>
>
> -Doug

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

[Sonny Rao]

On Wed, Sep 10, 2014 at 10:52 AM, Doug Anderson <dianders@chromium.org> wrote:
> Mark,
>
> On Wed, Sep 10, 2014 at 10:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi Sonny,
>>
>> On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
>>> This is a bug fix for using physical arch timers when
>>> the arch_timer_use_virtual boolean is false.  It restores the
>>> arch_counter_get_cntpct() function after removal in
>>>
>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>
>>> and completes the implementation of memory mapped access for physical
>>> timers, so if a system is trying to use physical timers, it will
>>> function properly.
>>
>> To get back to the topic at hand:
>>
>> Which platform is this required by?
>
> I've seen similar problems on the A7s on exynos5420 / exynos5800 and
> on rk3288.  I can't say what other platforms might be affected.  Note
> that the arch timers on exyno5420/exynos5800 are not supported anyway,
> so I guess that means we're just worrying about the rk3288.

Yeah, given that this problem has manifest on at least two different
SoCs using ARM's cores, it would have been nice if the offset were
specified to start out as zero when in reset by the architecture (and
was implemented that way in ARM's core IP), but it looks like it
wasn't.

>
>
>> Why exactly is arch_timer_use_virtual false in this case?
>
> To re-summarize my understanding of everything (many of the below is
> secondhand knowledge, so correct if wrong):
>
> 1. The initial problem is that the virtual offset is not initialized
> by anyone and is per core (each core gets a different, random offset).
> That makes the virtual counter useless.  ...but the kernel only uses
> virtual counters.
>
> 2. As far as I know, we don't have any particular need for HYP mode
> nor for limiting access to secure mode.
>
> 3. You can only change the virtual offset from HYP mode.  That means
> someone needs to transition to HYP mode if we want to use virtual
> counters.
>
> 4. If the kernel happens to be in HYP mode it will init the virtual offset.
>
> 5. We could transition to HYP mode once in the firmware and boot the
> kernel like that, but since firmware is gone after we've booted the
> kernel, we run into the same problem when we power off a processor and
> when we resume from S3.  The firmware is not involved in these cases.
> In these cases the processors have an uninitialized virtual offset
> again.  These processors don't appear to magically come up in HYP
> mode.  Thus it would be up to the kernel to transition to HYP mode,
> init the offset, and get out of HYP mode.  ...or just use the physical
> counter.
>
>
> If you can suggest something that doesn't require us to involve the
> firmware in processor bringup and in resume, I'm all ears.  We have a
> desire not to involve the firmware there because of all the issues of
> keeping kernel/firmware in sync and because of the extra difficultly
> of shipping firmware updates (understandably the QA needed to validate
> a new firmware is much higher than the QA needed to validate a new
> kernel).

One thing that was used in the past was to have the kernel load a blob
from /lib/firmware/ which did some re-initialization when coming out
of suspend or deep sleep.  We could do something similar here and have
it either fix virtual offset or put it into hyp mode.  That would help
solve our issue of making it easier to update and avoid QA hassles.
Is such a solution acceptable to upstream?


>
>
> -Doug

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

[Doug Anderson]

Hi,

On Wed, Sep 10, 2014 at 10:52 AM, Doug Anderson <dianders@chromium.org> wrote:
> Mark,
>
> On Wed, Sep 10, 2014 at 10:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi Sonny,
>>
>> On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
>>> This is a bug fix for using physical arch timers when
>>> the arch_timer_use_virtual boolean is false.  It restores the
>>> arch_counter_get_cntpct() function after removal in
>>>
>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>
>>> and completes the implementation of memory mapped access for physical
>>> timers, so if a system is trying to use physical timers, it will
>>> function properly.
>>
>> To get back to the topic at hand:
>>
>> Which platform is this required by?
>
> I've seen similar problems on the A7s on exynos5420 / exynos5800 and
> on rk3288.  I can't say what other platforms might be affected.  Note
> that the arch timers on exyno5420/exynos5800 are not supported anyway,
> so I guess that means we're just worrying about the rk3288.
>
>
>> Why exactly is arch_timer_use_virtual false in this case?
>
> To re-summarize my understanding of everything (many of the below is
> secondhand knowledge, so correct if wrong):
>
> 1. The initial problem is that the virtual offset is not initialized
> by anyone and is per core (each core gets a different, random offset).
> That makes the virtual counter useless.  ...but the kernel only uses
> virtual counters.
>
> 2. As far as I know, we don't have any particular need for HYP mode
> nor for limiting access to secure mode.
>
> 3. You can only change the virtual offset from HYP mode.  That means
> someone needs to transition to HYP mode if we want to use virtual
> counters.
>
> 4. If the kernel happens to be in HYP mode it will init the virtual offset.
>
> 5. We could transition to HYP mode once in the firmware and boot the
> kernel like that, but since firmware is gone after we've booted the
> kernel, we run into the same problem when we power off a processor and
> when we resume from S3.  The firmware is not involved in these cases.
> In these cases the processors have an uninitialized virtual offset
> again.  These processors don't appear to magically come up in HYP
> mode.  Thus it would be up to the kernel to transition to HYP mode,
> init the offset, and get out of HYP mode.  ...or just use the physical
> counter.
>
>
> If you can suggest something that doesn't require us to involve the
> firmware in processor bringup and in resume, I'm all ears.  We have a
> desire not to involve the firmware there because of all the issues of
> keeping kernel/firmware in sync and because of the extra difficultly
> of shipping firmware updates (understandably the QA needed to validate
> a new firmware is much higher than the QA needed to validate a new
> kernel).

Note: we can avoid all of this mess by just using physical counters
and otherwise leaving the system alone.  That would mean landing
Sonny's patch plus another one that allows us to manually chose
physical counters.

If everyone can agree that's not terrible then we could probably end
the conversation there.

-Doug

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

[Doug Anderson]

Hi,

On Wed, Sep 10, 2014 at 10:52 AM, Doug Anderson <dianders@chromium.org> wrote:
> Mark,
>
> On Wed, Sep 10, 2014 at 10:27 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi Sonny,
>>
>> On Wed, Aug 27, 2014 at 10:03:39PM +0100, Sonny Rao wrote:
>>> This is a bug fix for using physical arch timers when
>>> the arch_timer_use_virtual boolean is false.  It restores the
>>> arch_counter_get_cntpct() function after removal in
>>>
>>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>>
>>> and completes the implementation of memory mapped access for physical
>>> timers, so if a system is trying to use physical timers, it will
>>> function properly.
>>
>> To get back to the topic at hand:
>>
>> Which platform is this required by?
>
> I've seen similar problems on the A7s on exynos5420 / exynos5800 and
> on rk3288.  I can't say what other platforms might be affected.  Note
> that the arch timers on exyno5420/exynos5800 are not supported anyway,
> so I guess that means we're just worrying about the rk3288.
>
>
>> Why exactly is arch_timer_use_virtual false in this case?
>
> To re-summarize my understanding of everything (many of the below is
> secondhand knowledge, so correct if wrong):
>
> 1. The initial problem is that the virtual offset is not initialized
> by anyone and is per core (each core gets a different, random offset).
> That makes the virtual counter useless.  ...but the kernel only uses
> virtual counters.
>
> 2. As far as I know, we don't have any particular need for HYP mode
> nor for limiting access to secure mode.
>
> 3. You can only change the virtual offset from HYP mode.  That means
> someone needs to transition to HYP mode if we want to use virtual
> counters.
>
> 4. If the kernel happens to be in HYP mode it will init the virtual offset.
>
> 5. We could transition to HYP mode once in the firmware and boot the
> kernel like that, but since firmware is gone after we've booted the
> kernel, we run into the same problem when we power off a processor and
> when we resume from S3.  The firmware is not involved in these cases.
> In these cases the processors have an uninitialized virtual offset
> again.  These processors don't appear to magically come up in HYP
> mode.  Thus it would be up to the kernel to transition to HYP mode,
> init the offset, and get out of HYP mode.  ...or just use the physical
> counter.
>
>
> If you can suggest something that doesn't require us to involve the
> firmware in processor bringup and in resume, I'm all ears.  We have a
> desire not to involve the firmware there because of all the issues of
> keeping kernel/firmware in sync and because of the extra difficultly
> of shipping firmware updates (understandably the QA needed to validate
> a new firmware is much higher than the QA needed to validate a new
> kernel).

Note: we can avoid all of this mess by just using physical counters
and otherwise leaving the system alone.  That would mean landing
Sonny's patch plus another one that allows us to manually chose
physical counters.

If everyone can agree that's not terrible then we could probably end
the conversation there.

-Doug