hisi: read the V5 die ID from the OTP shadow - #188
Conversation
`ipcinfo -i` has covered only V4, so every HISI_OT part -- 3516CV608,
3516CV610, 3516CV613, 3516DV500, 3519DV500 -- answers with nothing. On
OpenIPC that is not cosmetic: rcS derives the camera's MAC address from
this value, so a V5 board with no vendor-provisioned ethaddr comes up on
the shared placeholder and a second one collides with it.
V5 has no counterpart to the V4 die-ID block at 0x12020400. The vendor
reads its die ID through a bootrom call, but the OTP rows are shadowed
into a register window at the offset they occupy in OTP, which puts
OTP_DIE_ID's 16 bytes at 0x101E00F0. Offsets are from the CV610 SDK and
cited in the comment; the window is reachable from the non-secure side,
as the OEM's own hwconf.ko ioremaps 0x101E010C from a kernel module.
An unfused or unreadable row reads all-zeroes or all-ones. Reject both:
a caller that turns the string into a MAC would otherwise give every
board in a fleet the same address.
`if (!serial)` in print_serial() tested the address of a stack array,
which is never null, so a chip with no reader printed whatever the stack
held rather than failing. Honour the reader's return value instead.
Measured on a Hi3516CV608 (H4-52POX-S). The shadow window reads:
0x101E00F0 = 0x97090A14 0x101E00F4 = 0x20E41210
0x101E00F8 = 0x39182919 0x101E00FC = 0x04921EAC
0x101E010C = 0x00240180
That last one is the window identifying itself: 0x240180 is exactly
OTP_608_VERSION_ID from the SDK's svb.h, so this is the OTP shadow and
not some unrelated block that happens to hold entropy. `ipcinfo -i` went
from printing nothing to 140a09971012e42019291839ac1e9204.
One part cannot prove a value is per-die rather than per-model. This is
the field the vendor documents as the die ID, which is a far stronger
prior than a register found by sweeping, but a second V5 part is what
would settle it.
Claude-Session: https://claude.ai/code/session_011qHvUfNDKptXf41shaU1vE
PR Summary by Qodohisi: read V5 die IDs from the OTP shadow
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships |
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Thanks for this. The investigation is thorough and the CV608 evidence is convincing: the +0x10C word self-identifying the window as the OTP shadow, and the derived MAC matching mac_from_uid. Requesting changes for one gap and two small checks that belong next to it.
- The OTP base is verified on one CV608 and applied to all five
HISI_OTparts, and the read is now unconditional on the plainipctoolpath viahisi_chip_properties(). If0x101E0000is not the shadow on 3516DV500/3519DV500 the outcome is either a bus error that kills the whole report, or a stable foreign value that gives every board of that model the same MAC. Suggest gating on the parts the CV610 SDK actually covers until a DV500 has been measured (details inline). - The all-zero/all-ones rejection only covers the V5 arm. The V4 arm of
hisi_get_die_id()returns true on a zero block and prints 48 zeros. - The filter combines all four words, so a mix of
0xFFFFFFFFand0words passes as an identity.
Non-blocking: ipcinfo -i now exits 1 on chips without a reader and aborts any later flags (worth a line in the description); ipcinfo never calls hal_cleanup(), so the printk and sensor-clock side effects of setup_hal_hisi() persist after the boot-time call; a few nits inline.
I could not verify on hardware. Both the lab 3519DV500 and CV608 were unreachable today. This read-only sequence is the whole test on either part:
for a in 0x101E00F0 0x101E00F4 0x101E00F8 0x101E00FC 0x101E010C; do devmem $a 32; done0x101E010C should come back as a 0x24xxxx-style OTP version id, and the four die words should differ from the CV608 values in the description. A second CV608 reading different words would also settle per-die versus per-model.
| uint32_t any_bit_set = 0; | ||
| uint32_t all_bits_set = 0xFFFFFFFF; | ||
| for (int i = 0; i < V5_DIE_ID_WORDS; i++) { | ||
| if (!mem_reg(V5_OTP_SHADOW_BASE + V5_OTP_DIE_ID + i * 4, &id[i], |
There was a problem hiding this comment.
Blocking. This base is verified on one CV608, and the SDK it comes from covers CV608/CV610/CV613, but it runs on every HISI_OT part including 3516DV500 and 3519DV500, and hisi_chip_properties() now performs the read unconditionally on the plain ipctool path.
Two failure modes if 0x101E0000 is not the OTP shadow on one of those:
- Unbacked window:
mem_reg()only guards themmap; the dereference attools.cis unguarded and there is no SIGBUS handler, so the whole YAML report dies. See 81ccfa4 (the 3536CV100 bus-error fix) for the in-tree precedent. - Backed by some other stable register: it passes the all-zero/all-ones filter below and every board of that model reports the same id, which is the fleet-wide MAC collision this PR sets out to prevent.
Suggest gating the reader on the chip IDs the CV610 SDK covers until a DV500 or 3519DV500 has been measured. The read is harmless, so this is the whole test:
for a in 0x101E00F0 0x101E00F4 0x101E00F8 0x101E00FC 0x101E010C; do devmem $a 32; done+0x10C should be a 0x24xxxx-style version id and the four die words should differ from the CV608 values in the description. I tried to run it on the lab 3519DV500 and CV608 today but both were unreachable.
|
|
||
| bool hisi_get_die_id(char *buf, ssize_t len) { | ||
| switch (chip_generation) { | ||
| case HISI_V4: |
There was a problem hiding this comment.
The V4 arm skips the all-zero/all-ones rejection that the V5 arm gets at line 757. hisi_ev300_get_die_id() returns true on a block of six zero words (the trailing-'0' strip never finds a non-zero digit, so it never breaks), and ipcinfo -i then prints 48 zeros with exit 0. Same for all-ones.
Since the invariant this PR introduces is "callers derive a MAC from this", the check belongs here in the dispatcher so both arms honour it.
| // An unfused or unreadable row reads all-zeroes or all-ones. Neither is an | ||
| // identity, and callers turn this string into a MAC address -- handing one | ||
| // out would give every board in a fleet the same address, so fail instead. | ||
| if (!any_bit_set || all_bits_set == 0xFFFFFFFF) |
There was a problem hiding this comment.
This combines all four words, so a shadow that reads e.g. {0xFFFFFFFF, 0xFFFFFFFF, 0, 0} (partially fused or partially locked) passes both tests and is emitted as an identity.
Rejecting a 16-byte value whose bytes are all 0x00 or 0xFF closes that without refusing a legitimate id that happens to contain a zero word. A per-word reject would be too strict for that reason.
| char *ptr = buf; | ||
| for (int i = 0; i < V5_DIE_ID_WORDS; i++) | ||
| for (int b = 0; b < 4; b++) | ||
| ptr += snprintf(ptr, buf + len - ptr, "%02x", |
There was a problem hiding this comment.
Nit: sixteen snprintf calls with running pointer arithmetic lean on the * 8 + 1 precheck above to keep buf + len - ptr non-negative, and a reader has to re-derive that. Copying the words into a uint8_t[16] and hex-encoding in one loop, as bootrom.c does, keeps the endian independence and lets the precheck become 2 * sizeof id + 1.
|
|
||
|
|
||
| bool hisi_ev300_get_die_id(char *buf, ssize_t len); | ||
| bool hisi_get_die_id(char *buf, ssize_t len); |
There was a problem hiding this comment.
Two small things now that there is a single entry point:
hisi_ev300_get_die_id()keeps its ownchip_generation != HISI_V4guard and stays exported, but this dispatcher is now its only caller. Making itstaticand dropping the inner guard leaves one place encoding the routing. As it stands, a futurecase HISI_V4A:that reuses it would be silently dead.- A new API is the moment to take
size_t, likesstar_get_die_id(), which is called side by side with it inprint_serial().
| // is never null -- a chip with no reader printed whatever the stack held. | ||
| // Provisioning scripts derive a MAC from this, so a miss has to be silent | ||
| // on stdout and non-zero on exit. | ||
| if (!found || !*serial) |
There was a problem hiding this comment.
Worth a line in the description: on master if (!serial) was never true, so -i always fell through with exit 0. Now a chip with no reader (V4A, INFINITY6C, anything non-HiSi/non-SStar) exits 1, and any flags after -i on the command line never run.
That is what ethaddr_provision wants and it matches -c/-f/-t, but it is a contract change for scripts that chain flags or test the exit code.
|
|
||
| static void print_serial() { | ||
| char serial[512]; | ||
| char serial[512] = {0}; |
There was a problem hiding this comment.
Nit: the zero-fill and the !*serial test are redundant with found. Every reader writes at least 12 chars before returning true and returns false on every failure path, so if (!found) with a plain char serial[512] is behaviourally identical, and it makes clear which signal is authoritative.
The comment at 111-114 narrates the removed bug. The one sentence worth keeping in code is the last one (provisioning derives a MAC, so a miss must be silent on stdout and non-zero on exit); the rest belongs in the commit message.
| char serial[512] = {0}; | ||
| bool found = false; | ||
|
|
||
| const char *vendor = getchipvendor(); |
There was a problem hiding this comment.
Pre-existing, but this PR puts ipcinfo -i on every V5 boot's rcS path, so flagging it: getchipvendor() runs setup_hal_hisi(), which writes 0 0 0 0 to /proc/sys/kernel/printk and on HISI_OT force-enables the sensor clock (CRG8464/CRG8472). ipcinfo never calls hal_cleanup() on any exit path (only sensors.c and i2cspi.c do), so the console stays silent and the CRG stays modified for the rest of boot.
One hal_cleanup() before each exit, or an atexit() registered after getchipname(), fixes it.
2 participants
ipcinfo -icovers HiSilicon V4 and SigmaStar. EveryHISI_OTpart — 3516CV608, 3516CV610, 3516CV613, 3516DV500, 3519DV500 — answers with nothing, becausehisi_ev300_get_die_id()returns early on any generation butHISI_V4.On OpenIPC that is not cosmetic.
ethaddr_provision()in the firmware'srcSderives the camera's MAC from this value, so a V5 board with no vendor-provisionedethaddrcomes up on the shared placeholder, and a second one collides with it.Where the V5 die ID lives
V5 has no counterpart to the V4 die-ID block at
0x12020400. The vendor reads its die ID through a bootrom call (otp_get_die_id(),gsl/drivers/share_drivers/share_drivers.c), but every OTP row is also shadowed into a register window at the offset it occupies in OTP, so the 16 bytes ofOTP_DIE_IDare readable with an ordinary/dev/memmap. Both halves are in the Hi3516CV610 SDK:Despite the
SCPU_(secure CPU) name, the window is reachable from the non-secure side: the OEM's ownhwconf.kois an ordinary kernel module and itioremapsOTP_SHADOW_BASE + 0x10C(OTP_VERSION_ID_REG), andsvb.creads+0x20,+0x118and+0x124with plainreadl.Measured
On a Hi3516CV608 (H4-52POX-S), through
/dev/mem:That last read is the window identifying itself —
0x240180is exactlyOTP_608_VERSION_IDfrom the SDK'ssvb.h, so this is the OTP shadow and not an unrelated block that happens to hold entropy.Flashed into an OpenIPC image, the camera provisioned itself on the first boot:
eth0and the savedethaddrboth became02:77:8e:1c:92:05, the value the firmware's ownmac_from_uidderives from that uid.What the change does
hisi_ot_get_die_id()reads the four words and emits 32 hex characters in OTP byte order — the shadow words are little-endian, so byte i of whatotp_get_die_id()would return isword[i / 4] >> (8 * (i % 4)).hisi_get_die_id()dispatches onchip_generation, V4 to the existing reader andHISI_OTto the new one.hisi_chip_properties()uses it too, soipctool's JSON gets the id on V5 as well.print_serial()'sif (!serial)tested the address of a stack array, which is never null, so a chip with no reader printed whatever the stack held instead of failing. It now honours the reader's return value.What one part cannot prove
That the value is per-die rather than per-model. This is the field the vendor documents as the die ID, but a second V5 part reading something different is what would settle it —
devmem 0x101E00F0 32is the whole test, and the read is harmless. Only the CV608 was available here; CV610, CV613, DV500 and 3519DV500 take the same path untested.https://claude.ai/code/session_011qHvUfNDKptXf41shaU1vE