sunxi-fel: handle H616/A133 secure-FEL handoff - #236
jameshilliard wants to merge 2 commits into
Conversation
apritzel
left a comment
There was a problem hiding this comment.
This patch is massive (aka barely reviewable) and apparently combines code movement/refactoring and new features, which is not a good idea.
Please split this up to refactor the code first, then apply the changes required for the H616.
Also the detection doesn't work: on my non-secure boards, 0x03006240 also reads as zero, which triggers the secure code and hangs. Can you try to read the secure fuse word, and maybe filter for that bit?
And why does the secure workaround now only triggers for the SID read? And what does it have to do with the SPL? The smc #0 before was just triggered once, on the first sunxi-fel command, then the rest just fell in place. I am still hopeful something similar is possible on the newer SoCs, without having explicit entry and exit code.
At the very least this should be explained in detail in the commit message or in comments.
That's just what I found after 5 minutes into the patch, but as mentioned, it's very hard to comprehend. Please tell your LLM to make this understandable and reviewable by human beings.
ab40817 to
ebd5305
Compare
|
The patch is now a single commit: The generic SPL path is left alone. The H616-specific part is limited to the secure-FEL startup transition and
That avoids triggering the secure path on non-secure H616 boards where the The SMC workaround method is explicit in SoC data:
H616 still follows the global startup model: The difference from older SoCs is the transition mechanism. Older SoCs can The H616 thunk issues the SMC, switches from monitor mode to secure SVC with The new thunk header is added to the top-level Validation on the H616 board: That applied the secure-SVC return thunk once in the I also tested SPL loading with the separate TOC0 loader changes in my working That applied the secure-SVC return thunk before TOC0 parsing, wrapped the TOC0 |
7dae1f5 to
486caf2
Compare
ced88c1 to
681257a
Compare
681257a to
af8d769
Compare
| .rvbar_reg_alt= 0x08100040, | ||
| .ver_reg = 0x03000024, | ||
| .needs_smc_workaround_if_zero_word_at_addr = 0x03006240, | ||
| .secure_boot_fuse_addr = 0x030060a0, |
There was a problem hiding this comment.
This should be an offset from .sid_base (or even always sid_base + 0xa0).
There was a problem hiding this comment.
Fixed. This is now encoded as an offset from sid_base; H616 uses secure_boot_fuse_offset = 0xa0.
| return false; | ||
|
|
||
| if (!soc_info->needs_smc_workaround_if_zero_word_at_addr) | ||
| return true; |
There was a problem hiding this comment.
I don't think we want to assume the workaround is needed if we don't have a way of distinguishing secure SVC from NS SVC, as we would apply the workaround on every sunxi-fel invocation.
There was a problem hiding this comment.
Fixed. The secure boot fuse is now only a guard. The runtime zero-word probe still has to match before the workaround is applied, so a secure fuse alone cannot make every sunxi-fel invocation run the thunk.
| aw_fel_read(dev, soc_info->secure_boot_fuse_addr, | ||
| &val, sizeof(val)); | ||
| if (!(le32toh(val) & soc_info->secure_boot_fuse_mask)) | ||
| return false; |
There was a problem hiding this comment.
Have you ever seen a value other than 0 or 1 in 0x030060a0? I don't think this needs a mask; any nonzero value is secure.
There was a problem hiding this comment.
Fixed. The mask is gone; any nonzero value in the secure boot status word is treated as secure boot.
|
A133 puts the monitor vector table at 0x300c0 (in SRAM C), and then wipes SRAM A1 and SRAM C at the beginning of NBROM, so you have to copy the monitor mode code back to SRAM before you can do |
14eeace to
e2f9995
Compare
|
The updated version uses that smarter-handler approach for H616. Instead of copying the stock SBROM SMC handler, the thunk installs a So this avoids replacing the whole monitor vector table on H616. The handler |
|
Thanks for the updates! I can confirm this works on A133 as well, with the same settings, so the following change: diff --git a/soc_info.c b/soc_info.c
index fa7b7e2..03b8dd6 100644
--- a/soc_info.c
+++ b/soc_info.c
@@ -766,8 +766,10 @@ soc_info_t soc_info_table[] = {
.brom_hook_shadow_addr = 0x0003c000,
.dma_max_len = FEL_RX_DMA_MAX_LEN,
},
- .needs_smc_workaround_if_zero_word_at_addr = 0x100004,
- .smc_workaround = SMC_WORKAROUND_DIRECT_SMC,
+ .needs_smc_workaround_if_zero_word_at_addr = 0x03006240,
+ .secure_boot_fuse_offset = 0xa0,
+ .smc_workaround = SMC_WORKAROUND_SECURE_SVC_SMC_THUNK,
+ .monitor_smc_handler = &h616_monitor_smc_handler,
.watchdog = &wd_h6_compat,
},{
.swap_buffers = NULL /* End of the table */ |
e2f9995 to
33cc609
Compare
I re-based an applied this change. |
"Installs a temporary SMC handler" is imprecise wording. The thunk does not write When
So the resulting transition is the sequence you describe: issue the SMC, fix the CPU and GIC state, and return to the BROM in secure SVC. The temporary vector redirect only controls where execution begins after the SMC. On H616 alone, the fixups could follow |
33cc609 to
4d91982
Compare
4d91982 to
3c13af1
Compare
Yes, that is sufficient and substantially simpler. The vector patch does not require privileged access: I replaced the branch-to-copied-handler path with this sequence. It removes the runtime branch construction and separate monitor handler entirely. I tested it from a cold H616 FEL boot: the secure SID read completed and FEL remained usable afterward. With the TOC0 loader series applied on top, a full SPL/FIT transfer also reached U-Boot proper successfully. |
3c13af1 to
ea275a7
Compare
|
@smaeul can you check if these latest changes also work on your A133 board? |
ea275a7 to
c5f15de
Compare
|
The register name and purpose come from Allwinner's H616 BSP source code. In #define SID_PRCTL (SUNXI_SID_BASE + 0x40)
#define SID_EFUSE (SUNXI_SID_BASE + 0x200)
#define SID_SECURE_MODE (SUNXI_SID_BASE + 0xA0)In int sid_probe_security_mode(void)
{
return readl(SID_SECURE_MODE) & 1;
}The same file's Our check serves the same purpose as This status check is only a prerequisite for applying the workaround. It identifies secure-boot configuration, not the CPU's current Both checks are needed because |
c5f15de to
dd0619a
Compare
|
I switched the restricted-memory probe to DetectionThe probe now reads The separate status check at AssemblyThe thunk retains the single-instruction SP, LR and the original CPSR are kept in unbanked, caller-clobbered registers. The thunk restores the original vector word, clears SCR and MVBAR, configures the secure GIC view, then switches to SVC with IRQ/FIQ still masked. It restores SP/LR before restoring the interrupt masks and returning to the BROM. This removes the temporary stack, SRAM swaps, dummy SPL entry point, banked-register instruction encodings, and ARMv5 detection. The assembly uses instruction mnemonics with an explicit architecture declaration. The thunk image is now 124 bytes instead of 304 bytes, excluding its appended parameters. The host uses a fixed-size buffer containing the thunk and three address parameters, without allocation or swap-table construction. It rejects enabled MMU/caches instead of silently skipping the transition, removes the inherited 250 ms delay from this workaround, and rechecks the runtime probe after execution. The normal SID and SPL paths remain unchanged. The header was regenerated using the existing Podman toolchain and formatting; H616 Validation
A133 has not been hardware-tested with this revision. |
f9c2e5c to
a95fdd4
Compare
|
I've simplified this further since the previous update. The refactoring and H616/A133 feature remain separate commits, but the transition now uses an annotated The transition is now:
Unlike the previous version, this does not manually save/restore CPSR or copy SP/LR: SVC SP/LR are never modified. MVBAR and the GIC priority-mask/distributor settings are also left alone. The BROM already initialized the GIC; only AckCtl needs adjusting. The two DSBs remain to complete writes before SMC and the final return. The sequence is now 68 bytes plus two address parameters, or 76 bytes uploaded, compared with 124 bytes plus three parameters previously. Detection is unchanged: the secure-boot status check excludes normal-boot devices, and the protected LCJS read prevents repeating the transition once secure state is accessible. This works across separate FEL invocations, not just within one process. The normal SID and SPL paths remain unchanged. The instruction sequence passed repeated H616 cold boots, full SID reads, and an SPL/U-Boot/FIT boot with successful hash verification. The final host-only refactor was checked byte-for-byte against that tested payload. The earlier A133 confirmation covered the older implementation; this reduced version still needs an A133 hardware retest. |
|
Mostly a cosmetic review here, I'm not really sure I understand how the offsets to static data are calculated in the arm code, but it looks strange. Other than that it would be good to add more comments (more or less one for each instruction) to the code to clarify what it's doing. |
|
|
||
| if (!soc_info->needs_smc_workaround_if_zero_word_at_addr) | ||
| return false; | ||
| aw_fel_read(dev, soc_info->needs_smc_workaround_if_zero_word_at_addr, |
There was a problem hiding this comment.
You can add a newline before this and preserve the original comment before the call.
There was a problem hiding this comment.
Restored both.
| uint32_t arm_code[] = { | ||
| /* Keep the FEL return address in unbanked r12. */ | ||
| htole32(0xe1a0c00e), /* mov r12, lr */ | ||
| /* Patch the SMC vector to return in monitor mode. */ |
There was a problem hiding this comment.
Maybe some more details about each instruction would be good, what is being loaded/stored, etc.
There was a problem hiding this comment.
Added per-instruction explanations.
| /* Keep the FEL return address in unbanked r12. */ | ||
| htole32(0xe1a0c00e), /* mov r12, lr */ | ||
| /* Patch the SMC vector to return in monitor mode. */ | ||
| htole32(0xe59f0038), /* ldr r0, [pc, #56] */ |
There was a problem hiding this comment.
Here pc is at offset 0x4 of arm_code so pc + 56 should be 0x3c. I guess we are trying to load 0x44 here. Maybe I'm missing something?
There was a problem hiding this comment.
A32 PC reads as instruction address + 8: 0x04 + 8 + 56 = 0x44. Now documented.
| /* Patch the SMC vector to return in monitor mode. */ | ||
| htole32(0xe59f0038), /* ldr r0, [pc, #56] */ | ||
| htole32(0xe5901008), /* ldr r1, [r0, #8] */ | ||
| htole32(0xe59f202c), /* ldr r2, [pc, #44] */ |
There was a problem hiding this comment.
And I guess we are loading offset 0x40 here.
There was a problem hiding this comment.
Correct: 0x0c + 8 + 44 = 0x40, holding the mov pc, lr encoding.
| soc_info_t *soc_info = dev->soc_info; | ||
| uint32_t val; | ||
|
|
||
| if (!soc_info->needs_smc_workaround_if_zero_word_at_addr) |
There was a problem hiding this comment.
Maybe we can take the occasion to rename this smc_workaround_check_reg or something else that is shorter?
There was a problem hiding this comment.
Renamed to smc_workaround_probe_addr, since it also probes SRAM.
|
a95fdd4 works for me on A133 |
Separate the runtime SMC-workaround probe from the code that executes the workaround so later SoCs can choose a different implementation without mixing that change into the existing direct-SMC path. Rename the restricted-memory probe field to smc_workaround_probe_addr and document its zero-address and zero-read semantics. Describe the workaround method in SoC data and make every existing secure-FEL user select the direct-SMC method explicitly. This preserves the current behaviour because direct SMC remains the only implementation in this patch. Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
On secure-boot H616 and A133 devices, FEL starts in non-secure state. A direct SMC alone does not leave the BROM command loop in secure SVC, and A133 NBROM also clears the monitor vector table. Temporarily patch the writable SRAM SMC vector at MVBAR + 8 with "mov pc, lr". After SMC, this branches back without restoring SPSR_mon, leaving execution in monitor mode. Encode the sequence directly in fel.c using the existing direct-SMC instruction-array style. Keep the FEL return address in r12 and let SMC save CPSR in SPSR_mon. Restore the original vector word and clear SCR to select secure state. Set GICC_CTLR.AckCtl so the unchanged BROM IRQ handler can acknowledge Group 1 interrupts through secure GICC_IAR. Preserve the other GIC settings: the BROM already enables the distributor and CPU interface and sets the priority mask. Leave MVBAR unchanged, since IRQs use VBAR with SCR.IRQ clear. Return directly to FEL with "movs pc, r12", restoring CPSR from SPSR_mon, including the original A/I/F masks and endianness. SVC SP/LR remain untouched. Exception entry and return provide context synchronization. DSBs complete the vector store before SMC and the GIC update and vector restoration before returning. The sequence assumes the BROM's ARM-state, MMU/cache-disabled FEL entry. Gate the workaround on the secure boot status word at SID base + 0xa0. Use EFUSE_LCJS at efuse offset 0x48 as the restricted-memory probe: bit 11 is the secure-boot fuse, so the secure view is non-zero when secure boot is enabled. Both probes are read-only. Apply the workaround at startup, leaving the existing SID and SPL paths unchanged. Once LCJS becomes readable, its non-zero value suppresses repeat application, including across separate sunxi-fel invocations. Normal-boot devices are excluded by the secure boot status check. Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
a95fdd4 to
e483171
Compare
Comments added. |
H616 and A133 enter FEL in non-secure SVC when secure boot is enabled. Secure register access and the later AArch64 handoff require a transition to secure SVC that leaves the BROM FEL command loop usable.
This series contains two commits:
smc_workaround_probe_addr. Existing direct-SMC behavior is unchanged.arm_code[]array infel.c.Detection
The workaround first checks the SID controller's secure-boot status word at
sid_base + 0xa0. A zero value skips the workaround. This register describes secure-boot configuration, not the CPU's current security state.The runtime state probe reads
EFUSE_LCJSat0x03006248, corresponding to efuse offset0x48. Bit 11 is the secure-boot fuse, so this word is nonzero in the secure view on a secure-boot device. The restricted non-secure read returns zero. Once the transition succeeds, subsequent FEL invocations see the nonzero value and skip the workaround. Both checks are read-only; no fuses are programmed.Secure-SVC Transition
NBROM can clear the monitor vectors while leaving MVBAR pointing to SRAM. On H616/A133, the known vector table address is
0x300c0, and that SRAM is writable from non-secure FEL. The code uses the address supplied in SoC data; it does not need to read or write MVBAR from non-secure state.r12, save the original SMC vector word, and temporarily replace the word atMVBAR + 8withmov pc, lr.smc #0. The patched vector branches to the instruction after the SMC without restoring CPSR fromSPSR_mon, leaving execution in Monitor mode.GICC_CTLR.AckCtlso the unchanged BROM IRQ handler can acknowledge Group 1 interrupts through secureGICC_IAR.movs pc, r12, restoring CPSR fromSPSR_monand entering secure SVC.SVC SP/LR, MVBAR, the GIC distributor, and the priority mask remain untouched. The two DSBs complete the vector write before SMC and the vector restoration/GIC update before returning. The sequence assumes the BROM's ARM-state, MMU/cache-disabled FEL entry.
The uploaded payload is 76 bytes, including its two address parameters. Comments explain each operation and the A32
PC + 8literal-load calculations. No separate assembly source, generated header, SRAM swapping, or build-system changes are required.The workaround runs at startup, not specifically during SID reads or SPL loading. Those paths remain unchanged, and the secure state persists across subsequent FEL commands and separate host invocations. TOC0 loading and USB transfer optimizations are outside this series.
Validation
a95fdd4works. The subsequent review changes only rename the probe field and improve comments; the 76-byte payload is byte-for-byte unchanged.