arm: init CNTVOFF #2052
arm: init CNTVOFF #2052
Conversation
There was a problem hiding this comment.
@igoropaniuk, is this init fine to you (applies to armv7 only) ?
core/arch/arm/sm/sm_a32.S
Outdated
| * So test VIRT and GENTIMER Mask both here. | ||
| */ | ||
| read_idpfr1 r2 | ||
| ands r2, r2, #(IDPFR1_GENTIMER_MASK | IDPFR1_VIRT_MASK) |
There was a problem hiding this comment.
You need both ID_PFR1[generic timer] != 0 and ID_PFR1[virtualization extensions] != 0 to access the register, no?
The proposed test requires only 1 of them to be non null to init the register.
| * If the implementation includes the Virtualization Extensions | ||
| * this is a RW register, accessible from Hyp mode, and | ||
| * from Monitor mode when SCR.NS is set to 1. | ||
| * If the implementation includes the Security Extensions |
There was a problem hiding this comment.
here replace Security Extensions with Generic Timers?
There was a problem hiding this comment.
This comments are directly from ARM architecture reference mannual. So keep Security extensions.
core/arch/arm/sm/sm_a32.S
Outdated
| ands r2, r2, #(IDPFR1_GENTIMER_MASK | IDPFR1_VIRT_MASK) | ||
| movne r2, #0 | ||
| /* Reset CNTVOFF to 0 */ | ||
| mcrrne p15, 4, r2, r2, c14 |
There was a problem hiding this comment.
Can be define a macro write_cntvoff from arm32_macros.S ? (comment a previous line would become redundant).
There was a problem hiding this comment.
mcrr could be used in write_cntvoff, but I am afraid mcrrne could not.
There was a problem hiding this comment.
On the other hand, this isn't optimized code so we could perhaps bear the cost of a bne for the benefit of more readable code.
MrVan
force-pushed
the
cntvoff
branch
2 times, most recently
from
cc93963
to
d6d18d1
Compare
|
@etienne-lms @jenswi-linaro Updated with adding write_cntvoff macro, only test generic timer mask and use |
|
|
core/arch/arm/sm/sm_a32.S
Outdated
| * mode, regardless of the value of SCR.NS. | ||
| */ | ||
| read_idpfr1 r2 | ||
| ands r2, r2, #IDPFR1_GENTIMER_MASK |
There was a problem hiding this comment.
Shouldn't you test also IDPFR1[virtualiation] ?
read_idpfr1 r2
ands r2, r2, #IDPFR1_GENTIMER_MASK
+ andsne r2, r2, #IDPFR1_VIRT_MASK
beq .no_gentimer
mov r2, #0
write_cntvoff r2, r2
There was a problem hiding this comment.
use this ands r2, r2, #(IDPFR1_GENTIMER_MASK | IDPFR1_VIRT_MASK)?
There was a problem hiding this comment.
I think you need (r2 & (gentimer_flag | virt_flag)) == (gentimer_flag | virt_flag).
Is my proposal wrong? It should do:
if (idpfr1[gentimer_flag] != 0 &&
idpfr1[virt_flag] != 0)
write_cntvoff(0);There was a problem hiding this comment.
My mistake. You are right. Thanks.
| #ifdef CFG_INIT_CNTVOFF | ||
| read_scr r0 | ||
| orr r0, r0, #SCR_NS /* Set NS bit in SCR */ | ||
| write_scr r0 |
There was a problem hiding this comment.
Need an ISB after setting (and clearing) SCR[NS]?
Also, maybe use a sequence where we switch in non secure only if CNTVOFF is accessed. Yet, this is not a big issue: we're booting, no perf penalty...
There was a problem hiding this comment.
ISB is needed. I'll add it.
|
@etienne-lms Updated with isb added after write_scr. Fix the IDPFR1_GENTIMER_MASK and IDPFR1_VIRT_MASK check with such logic: Add @jenswi-linaro 's ack tag. |
|
Please make the subject prefix: "arm32: sm: ...", since this applies to secure monitor only. |
There is an property "arm,cpu-registers-not-fw-configured" in Linux side, that could workaround the issue that firmare initialize CNTVOFF. But if use that property, virtualization support will be break in linux. Also without CNTVOFF or that property no defined, kernel could not boot up on i.MX7D with two cores. So we init CNTVOFF in OP-TEE to make kernel work well. Signed-off-by: Peng Fan <peng.fan@nxp.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
|
Updated with subject prefix "arm32: sm: xx" |
There is an property "arm,cpu-registers-not-fw-configured" in Linux side,
that could workaround the issue that firmare initialize CNTVOFF.
But if use that property, virtualization support will be break in linux.
Also without CNTVOFF or that property no defined, kernel could not
boot up on i.MX7D with two cores.
So we init CNTVOFF in OP-TEE to make kernel work well.
I did not find a better place to do this, so add it in sm_init and configure SCR.NS.
Signed-off-by: Peng Fan peng.fan@nxp.com