Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570986 > unrolled thread

Re: [PATCH v20 08/17] clocksource/drivers/arm_arch_timer: Rework counter frequency detection.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v20 08/17] clocksource/drivers/arm_arch_timer: Rework  counter frequency detection. Fu Wei <fu.wei@linaro.org> - 2017-01-31 19:50 +0100
    Re: [PATCH v20 08/17] clocksource/drivers/arm_arch_timer: Rework  counter frequency detection. Mark Rutland <mark.rutland@arm.com> - 2017-01-31 20:00 +0100
      Re: [PATCH v20 08/17] clocksource/drivers/arm_arch_timer: Rework  counter frequency detection. Fu Wei <fu.wei@linaro.org> - 2017-01-31 20:10 +0100

#1570986 — Re: [PATCH v20 08/17] clocksource/drivers/arm_arch_timer: Rework counter frequency detection.

Hi Mark,

On 31 January 2017 at 01:49, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 26, 2017 at 01:49:03PM +0800, Fu Wei wrote:
>> On 26 January 2017 at 01:25, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Wed, Jan 25, 2017 at 02:46:12PM +0800, Fu Wei wrote:
>> >> On 25 January 2017 at 01:24, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> > On Wed, Jan 18, 2017 at 09:25:32PM +0800, fu.wei@linaro.org wrote:
>> >> >> From: Fu Wei <fu.wei@linaro.org>
>
>> > For CNT{,EL0}BaseN.CNTFRQ, I am very concerned by the wording in the
>> > current ARMv8 ARM ARM. This does not match my understanding, nor does it
>> > match the description in the ARMv7 ARM. I believe this may be a
>> > documentation error, and I'm chasing that up internally.
>> >
>> > Either the currently logic in the driver which attempts to read
>> > CNT{,EL0}BaseN.CNTFRQ is flawed, or the description in the ARM ARM is
>> > erroneous.
>>
>> Yes, those description did confuse me. :-(
>>
>> But according to another document(ARMv8-A Foundation Platform User
>> Guide  ARM DUI0677K),
>> Table 3-2 ARMv8-A Foundation Platform memory map (continued)
>>
>> AP_REFCLK CNTBase0, Generic Timer 64KB   S
>> AP_REFCLK CNTBase1, Generic Timer 64KB   S/NS
>>
>> Dose it means the timer frame 0 can be accessed in SECURE status  only,
>> and the timer frame 1 can be accessed in both status?
>
> That does appear to be what it says.
>
> I assume in this case CNTCTLBase.CNTSAR<0> is RES0.
>
>> And because Linux kernel is running on Non-secure EL1, so should we
>> skip "SECURE" timer in Linux?
>
> I guess you mean by checking the GTx Common flags, to see if the timer
> is secure? Yes, we must skip those.

Yes, exactly.

I think we can check the  GTx Common flags, if the timer is set as
SECURE, this driver should just skip this timer.

Reason:
1, IF the timer is designed to be a secure timer which is only can be
accessed in secure status, the ACPI table should label this as SECURE,
then driver should skip it.
2, IF the timer is accessible from both status, but the firmware want
to use this driver for secure OS,  the ACPI table should also label
this as SECURE(meanwhile firmware should configure CNTSAR too), then
driver should skip it, too.

Actually I have added this into my next patchset v21. If you don't
have other suggestion I can post it tomorrow.

Can I? any thought?

>
> Looking further at this, the ACPI spec is sorely lacking any statement
> as to the configuration of CNTCTLBase.{CNTSAR,CNTTIDR,CNTACR}, so it's
> not clear if we can access anything in a frame, even if it is listed as
> being a non-secure timer.
>
> I think we need a stronger statement here. Otherwise, we will encounter
> problems. Linux currently assumes that CNTCTLBase.CNTACR<N> is
> writeable, given a non-secure frame N. This is only the case if
> CNTCTLBase.CNTSAR.NS<N> == 1.

the original driver has checked these registers, but the problem is:
What if the timer frame is designed to be a secure timer, all the
register in this frame is only can be accessed in secure status, just
like foundation model?
Note: for foundation model, Please check Table 3-1 Access permissions
of 3.1 ARMv8-A Foundation Platform memory map in ARMv8-A Foundation
Platform User Guide

So I think we should check the GTDT first, if it's not a secure timer,
then we can go on checking CNTSAR. :-)

Please correct me, If I miss something. :-)

Great thanks for your info :-)

>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

[toc] | [next] | [standalone]

#1571003

On Wed, Feb 01, 2017 at 02:43:02AM +0800, Fu Wei wrote:
> On 31 January 2017 at 01:49, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jan 26, 2017 at 01:49:03PM +0800, Fu Wei wrote:
> >> On 26 January 2017 at 01:25, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Wed, Jan 25, 2017 at 02:46:12PM +0800, Fu Wei wrote:
> >> >> On 25 January 2017 at 01:24, Mark Rutland <mark.rutland@arm.com> wrote:
> >> >> > On Wed, Jan 18, 2017 at 09:25:32PM +0800, fu.wei@linaro.org wrote:
> >> >> >> From: Fu Wei <fu.wei@linaro.org>
> >
> >> But according to another document(ARMv8-A Foundation Platform User
> >> Guide  ARM DUI0677K),
> >> Table 3-2 ARMv8-A Foundation Platform memory map (continued)
> >>
> >> AP_REFCLK CNTBase0, Generic Timer 64KB   S
> >> AP_REFCLK CNTBase1, Generic Timer 64KB   S/NS
> >>
> >> Dose it means the timer frame 0 can be accessed in SECURE status  only,
> >> and the timer frame 1 can be accessed in both status?
> >
> > That does appear to be what it says.
> >
> > I assume in this case CNTCTLBase.CNTSAR<0> is RES0.
> >
> >> And because Linux kernel is running on Non-secure EL1, so should we
> >> skip "SECURE" timer in Linux?
> >
> > I guess you mean by checking the GTx Common flags, to see if the timer
> > is secure? Yes, we must skip those.
> 
> Yes, exactly.
> 
> I think we can check the  GTx Common flags, if the timer is set as
> SECURE, this driver should just skip this timer.

I completely agree that we must skip these.

> > Looking further at this, the ACPI spec is sorely lacking any statement
> > as to the configuration of CNTCTLBase.{CNTSAR,CNTTIDR,CNTACR}, so it's
> > not clear if we can access anything in a frame, even if it is listed as
> > being a non-secure timer.
> >
> > I think we need a stronger statement here. Otherwise, we will encounter
> > problems. Linux currently assumes that CNTCTLBase.CNTACR<N> is
> > writeable, given a non-secure frame N. This is only the case if
> > CNTCTLBase.CNTSAR.NS<N> == 1.
> 
> the original driver has checked these registers, but the problem is:
> What if the timer frame is designed to be a secure timer, all the
> register in this frame is only can be accessed in secure status, just
> like foundation model?
> Note: for foundation model, Please check Table 3-1 Access permissions
> of 3.1 ARMv8-A Foundation Platform memory map in ARMv8-A Foundation
> Platform User Guide
> 
> So I think we should check the GTDT first, if it's not a secure timer,
> then we can go on checking CNTSAR. :-)

I've clearly confused matters here. I completely agree that we must skip
timers the GTDT descrbies as secure.

My complaint here is that the spec does not explicitly state that
CNTCTLBase.CNTSAR.NS<N> must be set for timers *not* marked as secure
(though I believe that is the intent). That is a spec issue, not a code
issue.

We unfortunately can't check CNTNSAR, as it is secure-only. :(

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1571012

Hi Mark,

On 1 February 2017 at 02:49, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Feb 01, 2017 at 02:43:02AM +0800, Fu Wei wrote:
>> On 31 January 2017 at 01:49, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Jan 26, 2017 at 01:49:03PM +0800, Fu Wei wrote:
>> >> On 26 January 2017 at 01:25, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> > On Wed, Jan 25, 2017 at 02:46:12PM +0800, Fu Wei wrote:
>> >> >> On 25 January 2017 at 01:24, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> >> > On Wed, Jan 18, 2017 at 09:25:32PM +0800, fu.wei@linaro.org wrote:
>> >> >> >> From: Fu Wei <fu.wei@linaro.org>
>> >
>> >> But according to another document(ARMv8-A Foundation Platform User
>> >> Guide  ARM DUI0677K),
>> >> Table 3-2 ARMv8-A Foundation Platform memory map (continued)
>> >>
>> >> AP_REFCLK CNTBase0, Generic Timer 64KB   S
>> >> AP_REFCLK CNTBase1, Generic Timer 64KB   S/NS
>> >>
>> >> Dose it means the timer frame 0 can be accessed in SECURE status  only,
>> >> and the timer frame 1 can be accessed in both status?
>> >
>> > That does appear to be what it says.
>> >
>> > I assume in this case CNTCTLBase.CNTSAR<0> is RES0.
>> >
>> >> And because Linux kernel is running on Non-secure EL1, so should we
>> >> skip "SECURE" timer in Linux?
>> >
>> > I guess you mean by checking the GTx Common flags, to see if the timer
>> > is secure? Yes, we must skip those.
>>
>> Yes, exactly.
>>
>> I think we can check the  GTx Common flags, if the timer is set as
>> SECURE, this driver should just skip this timer.
>
> I completely agree that we must skip these.
>
>> > Looking further at this, the ACPI spec is sorely lacking any statement
>> > as to the configuration of CNTCTLBase.{CNTSAR,CNTTIDR,CNTACR}, so it's
>> > not clear if we can access anything in a frame, even if it is listed as
>> > being a non-secure timer.
>> >
>> > I think we need a stronger statement here. Otherwise, we will encounter
>> > problems. Linux currently assumes that CNTCTLBase.CNTACR<N> is
>> > writeable, given a non-secure frame N. This is only the case if
>> > CNTCTLBase.CNTSAR.NS<N> == 1.
>>
>> the original driver has checked these registers, but the problem is:
>> What if the timer frame is designed to be a secure timer, all the
>> register in this frame is only can be accessed in secure status, just
>> like foundation model?
>> Note: for foundation model, Please check Table 3-1 Access permissions
>> of 3.1 ARMv8-A Foundation Platform memory map in ARMv8-A Foundation
>> Platform User Guide
>>
>> So I think we should check the GTDT first, if it's not a secure timer,
>> then we can go on checking CNTSAR. :-)
>
> I've clearly confused matters here. I completely agree that we must skip
> timers the GTDT descrbies as secure.

Yes, got it :-)

>
> My complaint here is that the spec does not explicitly state that
> CNTCTLBase.CNTSAR.NS<N> must be set for timers *not* marked as secure
> (though I believe that is the intent). That is a spec issue, not a code
> issue.

agree :-)

>
> We unfortunately can't check CNTNSAR, as it is secure-only. :(

yes, the spec says:
In a system that implements both Secure and Non-secure states, this
register is only accessible by Secure accesses.

So I think the firmware(from vendor) can decide which timer frame
should be marked as secure according to the GTDT, then kernel just get
this info from GTDT instead of checking CNTNSAR.


>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web