[fix]: volatile store the intr-critical regs in uart, now uart input …#33
[fix]: volatile store the intr-critical regs in uart, now uart input …#33chyyuu wants to merge 1 commit intorcore-os:devfrom
Conversation
…with intr in riscv64 can work. ----------------- `uart_16550` uses non-volatile ptr::write() which the compiler may optimise away. Re-write the interrupt-critical registers with volatile stores — in particular MCR.OUT2 (bit 3) gates the 16550 interrupt output; without it the UART never signals the PLIC.
There was a problem hiding this comment.
Pull request overview
This PR addresses missing UART interrupt signaling on RISC-V QEMU virt by ensuring interrupt-critical UART 16550 registers are written using volatile MMIO stores, avoiding compiler optimizations that could drop non-volatile writes.
Changes:
- Compute UART MMIO base once and reuse it for initialization.
- After
MmioSerialPort::init(), re-program FCR/MCR/IER usingcore::ptr::write_volatileto ensure UART interrupts can reach the PLIC (notably viaMCR.OUT2).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| core::ptr::write_volatile((base + 4) as *mut u8, 0x0B); // MCR: DTR+RTS+OUT2 | ||
| core::ptr::write_volatile((base + 1) as *mut u8, 0x01); // IER: RX data available | ||
| } |
There was a problem hiding this comment.
The volatile writes to MCR/IER enable UART interrupts unconditionally (MCR.OUT2 + IER=0x01). Since irq is a feature flag on this platform, consider gating the interrupt-enabling writes with #[cfg(feature = "irq")] (or otherwise ensuring they can’t fire when IRQ support/handlers aren’t enabled) to avoid behavior changes in non-IRQ builds.
| core::ptr::write_volatile((base + 4) as *mut u8, 0x0B); // MCR: DTR+RTS+OUT2 | |
| core::ptr::write_volatile((base + 1) as *mut u8, 0x01); // IER: RX data available | |
| } | |
| } | |
| #[cfg(feature = "irq")] | |
| unsafe { | |
| core::ptr::write_volatile((base + 4) as *mut u8, 0x0B); // MCR: DTR+RTS+OUT2 (enables UART IRQ output) | |
| core::ptr::write_volatile((base + 1) as *mut u8, 0x01); // IER: RX data available (UART IRQ enable) | |
| } |
| unsafe { | ||
| core::ptr::write_volatile((base + 2) as *mut u8, 0x01); // FCR: FIFO enable, 1-byte trigger | ||
| core::ptr::write_volatile((base + 4) as *mut u8, 0x0B); // MCR: DTR+RTS+OUT2 | ||
| core::ptr::write_volatile((base + 1) as *mut u8, 0x01); // IER: RX data available | ||
| } |
There was a problem hiding this comment.
This new unsafe MMIO block would benefit from an explicit // SAFETY: note explaining why (base + offset) as *mut u8 is valid and correctly mapped as UART device memory (and why 8-bit accesses are correct for this platform). This makes the unsafety audit-friendly and consistent with other parts of the repo that document unsafe invariants.
| core::ptr::write_volatile((base + 2) as *mut u8, 0x01); // FCR: FIFO enable, 1-byte trigger | ||
| core::ptr::write_volatile((base + 4) as *mut u8, 0x0B); // MCR: DTR+RTS+OUT2 | ||
| core::ptr::write_volatile((base + 1) as *mut u8, 0x01); // IER: RX data available |
There was a problem hiding this comment.
The register offsets and values here (e.g., base + 2, 0x0B, 0x01) are hardware-specific magic numbers. Consider introducing local const offsets/bitmasks (FCR/MCR/IER and OUT2) to make the intent clearer and reduce the chance of accidental drift if these settings change.
…with intr in riscv64 can work.
uart_16550uses non-volatile ptr::write() which the compiler may optimise away. Re-write the interrupt-critical registers with volatile stores — in particular MCR.OUT2 (bit 3) gates the 16550 interrupt output; without it the UART never signals the PLIC.