UART hjson should explicitly document CTRL_REGWEN software check requirement
Nobody has claimed this yet.
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
Description
Summary
The CTRL_REGWEN register in hw/ip/uart/data/uart.hjson describes the locking mechanism but does not explicitly state that software MUST check this register before writing to CTRL. This implicit constraint has led to both the OpenTitan DIF and the Zephyr driver independently omitting the check, resulting in potential silent failures when CTRL is locked.
Component
- File:
hw/ip/uart/data/uart.hjson - Register:
CTRL_REGWEN - Affected registers:
CTRL(controlled byCTRL_REGWEN)
Current hjson Description (Implicit Constraint)
{ name: "CTRL_REGWEN",
desc: "Controls write enable for CTRL register. If 0, CTRL is locked",
swaccess: "rw0c",
hwaccess: "none",
fields: [
{ bits: "0",
name: "regwen",
desc: "Once cleared, the CTRL register will be locked until the next reset.",
resval: 1 }
]
}
Problem: The description explains what the register does ("If 0, CTRL is locked") but does not state what software must do ("Software MUST check this register before writing CTRL; if 0, the write will be silently ignored").
Cross-Ecosystem Impact
This issue was discovered through a cross-ecosystem audit comparing the OpenTitan DIF with the Zephyr uart_opentitan.c driver. Both implementations independently omitted the CTRL_REGWEN check, which strongly suggests the constraint is not explicit enough in the specification.
| Implementation | File | Status |
|---|---|---|
| OpenTitan DIF | sw/device/lib/dif/dif_uart.c |
❌ Missing CTRL_REGWEN check before writing CTRL |
| Zephyr driver | drivers/serial/uart_opentitan.c |
❌ Missing CTRL_REGWEN check before writing CTRL |
Why This Matters
Silent Failure Scenario
1. Boot ROM configures UART and locks CTRL (CTRL_REGWEN = 0)
2. Zephyr/OpenTitan DIF calls uart_init() / dif_uart_configure()
3. Code writes to CTRL register
4. Hardware silently ignores the write (CTRL is locked)
5. Driver returns success (kDifOk / 0)
6. UART remains in ROM's configuration, not the driver's intended configuration
7. **Silent failure**: Software thinks UART is configured correctly, but it is not
Security Implications
If the boot ROM sets a specific baud rate for security reasons (e.g., to match a secure peripheral) and locks CTRL, a subsequent driver that attempts to reconfigure the UART will:
- Believe it succeeded
- Actually leave the UART in the ROM's configuration
- Potentially cause protocol mismatches or security boundary violations
Proposed Fix
Option A: Update hjson Description (Recommended)
Update the CTRL_REGWEN register description to explicitly state the software requirement:
{ name: "CTRL_REGWEN",
desc: '''
Controls write enable for the CTRL register.
When this bit is 1, the CTRL register can be modified by software.
When this bit is 0, the CTRL register is locked and writes are silently ignored.
**Software Requirement**: Before writing to the CTRL register, software MUST
read CTRL_REGWEN and verify it is 1. If CTRL_REGWEN is 0, software MUST NOT
attempt to write CTRL and SHOULD return an error to the caller.
''',
swaccess: "rw0c",
hwaccess: "none",
fields: [
{ bits: "0",
name: "regwen",
desc: "Once cleared, the CTRL register will be locked until the next reset.",
resval: 1 }
]
}
Option B: Add sw_check_required Annotation (Future Enhancement)
Consider adding a machine-readable annotation to hjson that can be consumed by code generation and static analysis tools:
{ name: "CTRL_REGWEN",
desc: "...",
swaccess: "rw0c",
hwaccess: "none",
// New annotation for tooling
sw_check_required: {
target_register: "CTRL",
check: "read_before_write",
failure_action: "return_error"
},
...
}
This would enable:
- Auto-generation of lock-check code in DIFs
- Static analysis tools to verify driver compliance
- Cross-ecosystem audits to detect missing checks
Option C: Fix DIF (Immediate Action)
Regardless of hjson changes, the OpenTitan DIF should be updated to include the CTRL_REGWEN check:
// In dif_uart_configure() or dif_uart_init()
dif_result_t dif_uart_configure(const dif_uart_t *uart, ...) {
// Check if CTRL is locked
uint32_t regwen = mmio_region_read32(uart->base_addr, UART_CTRL_REGWEN_REG_OFFSET);
if (!bitfield_bit32_read(regwen, UART_CTRL_REGWEN_REGWEN_BIT)) {
return kDifLocked; // or kDifError
}
// ... continue with configuration
}
Additional Context
This pattern may exist in other OpenTitan IP blocks. A systematic audit of all *_REGWEN registers across the hjson files would be valuable to ensure consistent documentation of software requirements.
References
- Current hjson:
hw/ip/uart/data/uart.hjson(CTRL_REGWEN register) - OpenTitan DIF:
sw/device/lib/dif/dif_uart.c(missing check) - Zephyr driver:
drivers/serial/uart_opentitan.c(missing check) - Cross-ecosystem audit methodology: "Dual-Source Ground Truth" framework
Request for Community Input
- Is the proposed hjson description change acceptable?
- Should we also update the DIF to include the check immediately?
- Should we audit other IP blocks for similar
*_REGWENdocumentation gaps?
We are happy to submit a PR for the hjson change and/or DIF fix once the community confirms the direction.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Open hw/ip/uart/data/uart.hjson and inspect CTRL_REGWEN alongside the CTRL register. Confirm whether maintainers want the documentation change alone or also the proposed DIF and annotation work, since the issue presents several options. Done means the agreed scope is explicit and the hjson remains valid.
Written by the indexing model from the issue text.
Assessment
- Domain
- documentation, embedded-iot
- Issue type
- Documentation
- Difficulty
- 1/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100