Skip to content

Commit ca4d600

Browse files
committed
Documented the CLZ search, and corrected the readme's AP claim
Two readme fixes that the previous commit made due. The image list in section 3 enumerates every example, so adding demo_clz.elf without adding it here would have left the readme telling a reader to build five images out of six. The ninja line and the list both name it now, and section 6 says what the optimisation is, what it is worth (2188 to 1316 bytes of the priority search), that it applies in the default 32-priority configuration rather than only above 32, and why a Thumb build deliberately keeps the portable loop. The second one is not mine and is worth calling out. Section 7 told anyone reusing this code that PRBAR.AP is encoded "the reverse of the widely-published Armv8-R AArch64 macro set" and to re-calibrate before trusting it. That is false, and mpu.h has said so since the shift bug was found: AP[2] selects read-only and AP[1] grants EL0 access, exactly as published, and the four-region calibration that suggested otherwise was measuring a program_region() that shifted every PRBAR field one bit too far left. So the readme was contradicting the header beside it on the encoding of a permissions register, which is the worst place to be wrong -- a reader who "re-calibrated" against that advice would have inverted their own access permissions. Replaced with what mpu.h establishes, including the part that is genuinely worth carrying: region coverage is enforced even when the permissions are not what you asked for, so an unmapped address faults while a "read-only" region quietly accepts writes, and only provoking a real fault tells the two apart. That is the transferable lesson the old text was reaching for. No code change; FVP ctest still 7/7. Assisted-by: Claude Code (Opus 5) <noreply@anthropic.com>
1 parent a7bf618 commit ca4d600

1 file changed

Lines changed: 34 additions & 8 deletions

File tree

ports/cortex_r52/gnu/readme_threadx.txt

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ baseline selects the soft ABI rather than removing the FPU.
4848
-DCMAKE_TOOLCHAIN_FILE=cmake/cortex_r52.cmake \
4949
-DTX_R52_BUILD_FVP_EXAMPLE=ON .
5050
ninja -C build_r52 boot_check.elf demo_m2.elf demo_m3.elf \
51-
demo_threadx.elf demo_mpu.elf
51+
demo_threadx.elf demo_mpu.elf demo_clz.elf
5252
ctest --test-dir build_r52
5353

5454
The images target the free Armv8-R AEM FVP (FVP_BaseR_AEMv8R):
@@ -58,6 +58,7 @@ The images target the free Armv8-R AEM FVP (FVP_BaseR_AEMv8R):
5858
demo_m3.elf generic timer tick, GICv3 and preemption
5959
demo_threadx.elf the standard eight-thread demo plus verification
6060
demo_mpu.elf PMSAv8-R protection and cache enable
61+
demo_clz.elf the CLZ lowest-set-bit priority search
6162
demo_m5.elf lazy VFP context save (needs TX_R52_ENABLE_VFP)
6263

6364
Each image reports its own result and terminates the model through the
@@ -132,6 +133,20 @@ and the thread stack pointer and run counter offsets, at compile time: a
132133
layout change becomes a build failure instead of silent corruption.
133134

134135

136+
Also worth knowing: this port replaces tx_thread.h's portable lowest-set-bit
137+
search with the CLZ instruction, which is what the scheduler uses to pick the
138+
next thread to run. Upstream gates that on __TARGET_ARCH_ARM, an Arm Compiler
139+
5 predefine that GCC does not define, so the optimisation had never once been
140+
compiled in under GCC; the guard here asks __ARM_FEATURE_CLZ instead. It is
141+
worth 40% of the priority search's code size (2188 to 1316 bytes in
142+
tx_thread_system_suspend.o) and it applies in the default 32-priority
143+
configuration, not only above 32. A Thumb build deliberately keeps the
144+
portable loop, because __ARM_FEATURE_CLZ describes the architecture rather
145+
than the instruction set and Thumb-1 has no CLZ. demo_clz.elf is the
146+
regression test, and it fails to build rather than silently testing the
147+
portable loop if the CLZ path is ever disabled again.
148+
149+
135150
7. Memory Protection
136151

137152
PMSAv8-R regions are described by a table rather than a sequence of
@@ -142,13 +157,24 @@ disables the regions it does not use.
142157

143158
Two cautions for anyone reusing this code on silicon:
144159

145-
- The PRBAR.AP encoding used here was calibrated against the hardware.
146-
The low bit is read-only and the high bit grants EL0 access, which is
147-
the reverse of the widely-published Armv8-R AArch64 macro set. Getting
148-
it wrong produces regions that report as read-only and accept writes,
149-
because region coverage is still enforced: an unmapped address faults
150-
while a "read-only" region does not. Re-calibrate before trusting
151-
these values on a different implementation.
160+
- The PRBAR.AP encoding is the standard Armv8-R one, as published:
161+
AP[2] selects read-only and AP[1] grants EL0 access. No calibration is
162+
needed, and an earlier revision of this file said otherwise -- it
163+
claimed the two bits were reversed, on the strength of a real
164+
measurement with a wrong cause. program_region() had been shifting
165+
every PRBAR field one bit too far left, so the AP value's low bit
166+
landed in the true AP[2] and writes faulted exactly when that bit was
167+
set, which looks precisely like a reversed encoding. The shift is
168+
fixed; see the comment on the MPU_AP_* macros in mpu.h for the full
169+
calibration table and what it actually proved.
170+
171+
The reason it took a silicon run to notice: region coverage is enforced
172+
even when permissions are not what you asked for, so an unmapped
173+
address faults while a "read-only" region quietly accepts writes. A
174+
configuration-only review cannot tell the two apart -- provoke a real
175+
fault instead. Verified on S32Z280 silicon: a write to an RO region
176+
faults and execution from an XN region takes a prefetch abort, the
177+
latter never having worked under the old shift.
152178

153179
- The code and data regions must not share a 64-byte granule. The
154180
linker script separates them with ".data ALIGN(64) :" on the output

0 commit comments

Comments
 (0)