riscv64: install legacy serial irqchip via set_intc#591
Conversation
yzewei
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc,
slp and
tylerfanelli
as code owners
|
PTAL |
|
It would be nice to put the PR description to the commit message. |
done. PLZ |
mz-pdm
left a comment
mz-pdm
left a comment
There was a problem hiding this comment.
Thank you for adding the commit message. It would be even better if the markup, namely the backslashes and the double quote at the end, were removed, but other than that the change looks good to me.
got it. |
slp
left a comment
slp
left a comment
There was a problem hiding this comment.
NAK. RISCV does have its own IrqChip abstraction, KvmAia. The serial device implementation should be updated to install it via set_intc and make use of it for triggering interrupts.
I updated the RISC-V serial path, installed the irqchip via set_intc() during MMIO registration, and routed interrupt signals via IrqChip::set_irq(). |
yzewei
changed the title
RISC-V already has a dedicated irqchip abstraction in KvmAia, but the legacy MMIO serial device was still signaling its interrupt EventFd directly. That bypasses the installed interrupt controller instead of using the same set_intc()/set_irq() path as the rest of the MMIO interrupt plumbing. Install the irqchip on the legacy RISC-V serial device during MMIO registration, record the assigned IRQ line, and route interrupt delivery through IrqChip::set_irq(). Keep the existing EventFd as the backing interrupt source so the KVM irqfd registration remains unchanged. This aligns the serial device with the existing RISC-V irqchip abstraction and makes it use the interrupt controller configured by the VMM. Signed-off-by: Zewei Yang <yangzewei@loongson.cn>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates an IrqChip (Interrupt Controller Chip) for serial devices in the RISC-V64 legacy emulation, centralizing interrupt handling. The Serial struct now includes optional IrqChip and irq_line fields, with new methods set_intc and set_irq_line for configuration. The trigger_interrupt method has been updated to utilize the IrqChip if present. Corresponding changes in build_microvm and MMIODeviceManager ensure the IrqChip is properly passed and configured for serial devices. The review comments point out that using unwrap() on Mutex locks in both serial.rs and mmio.rs could lead to panics if the mutex becomes poisoned, suggesting a more robust error handling approach like expect() or explicit PoisonError handling.
4 participants
Background
In the RISCv64 boot path, the legacy serial initialization chain is not synchronized, causing
cargo check -p vmm --target riscv64gc-unknown-linux-gnuto fail to compile.The
attach_legacy_devices()method inbuilder.rscalls a non-existentserial_device.The KVM MMIO serial registration logic for RISCv64 calls a non-existent
serial.set_intc(intc). The current RISCv64 legacy serial does not provide this interface; its interrupt triggering path depends on the registered IRQFD, rather than actively injecting interrupts through the interrupt controller held by the serial device as in Aarch64.