feat(aarch64): support external graceful shutdown via SendCtrlAltDel#5984
feat(aarch64): support external graceful shutdown via SendCtrlAltDel#598414sea wants to merge 5 commits into
Conversation
ShadowCurse
left a comment
There was a problem hiding this comment.
Overall this seems pretty good. I'll let you know when guest kernels are rebuilt for testing.
| raw_interrupt_status: u8, | ||
| /// Alternate function select (GPIOAFSEL). | ||
| alternate_function_select: u8, | ||
| metrics: &'static PL061DeviceMetrics, |
There was a problem hiding this comment.
There will be only one gpio device, so no need to store a ref to it. Just access METRICS directly.
| data: u8, | ||
| /// Pin directions (GPIODIR): 0 = input, 1 = output. | ||
| direction: u8, | ||
| /// Interrupt sense (GPIOIS): 0 = edge, 1 = level. | ||
| interrupt_sense: u8, | ||
| /// Interrupt both-edges select (GPIOIBE). | ||
| interrupt_both_edges: u8, | ||
| /// Interrupt event/polarity (GPIOIEV). | ||
| interrupt_event: u8, | ||
| /// Interrupt mask/enable (GPIOIE). | ||
| interrupt_mask: u8, | ||
| /// Raw (pre-mask) interrupt status (GPIORIS). | ||
| raw_interrupt_status: u8, | ||
| /// Alternate function select (GPIOAFSEL). | ||
| alternate_function_select: u8, |
There was a problem hiding this comment.
Since this is the exact state as in PL061State, just use it here. Would be easier to store and set it later.
| pub fn set_input_level(&mut self, line: u8, high: bool) -> Result<(), PL061Error> { | ||
| if line >= GPIO_PIN_COUNT { | ||
| return Err(PL061Error::InvalidGpioLine(line)); | ||
| } |
There was a problem hiding this comment.
As I can see, this is never accessed outside this file, so it can be made private. This also allows you to remove the line check (and the InvalidGpioLine error variant with it). You can also replace the check with assert!(line < GPIO_PIN_COUNT).
| 0..PL061_DATA_REG_END => Some(u32::from(self.data & data_mask(offset))), | ||
| PL061_DIR => Some(u32::from(self.direction)), |
There was a problem hiding this comment.
I think:
let result = match offset {
0..PL061_DATA_REG_END => self.data & data_mask(offset),
PL061_DIR => self.direction,
...
_ => return None,
};
Some(u32::from(result))would be more readable with less syntax noise.
| PL061_MIS => Some(u32::from(self.masked_interrupt_status())), | ||
| PL061_AFSEL => Some(u32::from(self.alternate_function_select)), | ||
| PL061_ID_REG_START..PL061_ID_REG_END => { | ||
| let index = usize::try_from((offset - PL061_ID_REG_START) >> 2).unwrap(); |
There was a problem hiding this comment.
this is a general comment about usage of try_from: here and in many other place you already guaranteed that these conversions cannot fail (in this example you already did a check offset >= PL061_REGISTER_SPACE_SIZE in the bus_read, in others you mask off the last byte of the value before converting it to u8). Because of this, there is no reason to use try_from. For u64 -> usize you can use vmm::utils::u64_to_usize. For u8 just use as u8 with #[allow(clippy::cast_possible_truncation)].
| let value = match data.len() { | ||
| 1 => u32::from(data[0]), | ||
| 4 => u32::from_le_bytes(data.try_into().unwrap()), | ||
| _ => unreachable!(), | ||
| }; |
There was a problem hiding this comment.
the write_reg converts the value: u32 into u8 all the time, so there is no reason to even try to construct u32. Just pass u8 directly. This will also remove needed conversion to u8 in the write_reg.
| write_u32(&mut device, 0x004, 0b1111_1111); | ||
|
|
||
| assert_eq!(read_u32(&mut device, PL061_DIR), 0b0000_0011); | ||
| assert_eq!(read_u32(&mut device, 0x004), 0b0000_0001); |
There was a problem hiding this comment.
what are these specific 0x004 and 0x010 registers?
There was a problem hiding this comment.
These aren't separate registers — they're masked accesses into the GPIODATA aperture (0x000..0x3FF). The PL061 encodes a per-line bitmask in address bits [9:2] of the access, so an access at offset line_mask << 2 reads/writes only the selected lines (the PrimeCell "masked GPIO data" mechanism, which is also how the Linux gpio-pl061 driver does single-line I/O).
0x004=0b0000_0001 << 2→ mask for line 00x010=0b0000_0100 << 2→ mask for line 2
I've reworked the test to be self-documenting via a data_aperture(line_mask) helper instead of bare offsets, and the aperture is documented at the PL061_DATA_REG_END / data_mask() definitions.
|
|
||
| ### Added | ||
|
|
||
| - [#2046](https://github.com/firecracker-microvm/firecracker/issues/2046): The |
There was a problem hiding this comment.
Let's split this commit into several smaller ones so it is easier to review. Something like:
- add PL061 device (just the impl)
- wire the device inside vmm
- add documentation
- add changelog entry
There was a problem hiding this comment.
Done — split into 5 commits as suggested: (1) add the PL061 device impl, (2) wire it into the VMM, (3) docs, (4) CHANGELOG entry. The test_api.py change is its own commit (5) so it can be dropped/updated independently once the CI kernels are rebuilt with the gpio-keys config.
| // New variants must be appended at the end: `DeviceType` is serialized into snapshots with | ||
| // `bitcode`, which encodes enum variants positionally, so reordering would break older | ||
| // snapshots that encoded later variants. |
| @pytest.mark.skipif( | ||
| platform.machine() != "x86_64", reason="not yet implemented on aarch64" | ||
| platform.machine() != "x86_64", | ||
| reason="On x86 CTRL+ALT+DEL triggers a kernel-level reboot via the i8042 device. The " |
There was a problem hiding this comment.
This can be in a separate commit to simply drop/update it when guest kernels will be rebuilt with new configs
Add a minimal PL061 GPIO controller modelled as an aarch64 MMIO device. It implements the core register bank (data with the address-masked aperture, direction, the interrupt sense/both-edges/event/mask/status/clear registers and the PrimeCell identification registers) plus host-side input injection so higher layers can drive a virtual GPIO line. The interrupt line is modelled as edge-triggered to match Firecracker's plain KVM irqfd injection (no resample fd): a level-high line would never be de-asserted and the GIC would re-fire it after the guest EOIs, storming the host. The register state is serializable so it can be carried across snapshots. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: 14sea <wanhuaning@gmail.com>
Until now `SendCtrlAltDel` was rejected with a 400 on aarch64 and there was no way to request a graceful shutdown of a microVM from the host. Fixes firecracker-microvm#2046. Attach the PL061 GPIO controller as an aarch64 MMIO device and describe a `gpio-keys` power button (KEY_POWER) in the FDT. `SendCtrlAltDel` is reused: on x86_64 it still injects CTRL+ALT+DEL through the i8042 device; on aarch64 it drives a short press/release pulse on the virtual power button. A guest with the standard gpio-keys driver and a power-key consumer (e.g. systemd-logind, which defaults to HandlePowerKey=poweroff) then shuts down cleanly and Firecracker exits on the resulting KVM_SYSTEM_EVENT_SHUTDOWN. Details: - The PL061 register state is saved and restored across snapshots, so the power button keeps working on a restored microVM. The snapshot version is bumped to 11.0.0 accordingly. - The press/release pulse runs synchronously so the two edges stay atomic with respect to other API actions; a concurrent snapshot can never capture the button half-pressed. - The gpio-keys kernel symbols are added to the shared CI kernel config so the guest can advertise the power key once the CI kernels are rebuilt. Validated on aarch64 + KVM hardware: KEY_POWER is delivered to the guest with no interrupt storm, systemd-logind powers the VM off automatically, and the path also works after a snapshot/restore cycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: 14sea <wanhuaning@gmail.com>
Describe the aarch64 behaviour of the SendCtrlAltDel action (virtual power button via the PL061 gpio-keys device) in the actions and device-api docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: 14sea <wanhuaning@gmail.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: 14sea <wanhuaning@gmail.com>
Update the API test now that SendCtrlAltDel is accepted on aarch64. Kept as a separate commit so it can be easily dropped or updated once the CI guest kernels are rebuilt with the gpio-keys config. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: 14sea <wanhuaning@gmail.com>
e41324e to
5ab0bc7
Compare
|
Thanks for the review! Force-pushed addressing all comments:
Each commit builds standalone and the |
Changes
Add external graceful shutdown support on aarch64 by reusing the existing
SendCtrlAltDelaction.describe a
gpio-keyspower button (KEY_POWER) in the FDT.SendCtrlAltDel: on x86_64 it still injects CTRL+ALT+DEL through thei8042 device (unchanged); on aarch64 it drives a short, synchronous
press/release pulse on the virtual power button. The API no longer rejects the
action with a
400on aarch64.irqfd injection (no resample fd). A level-high line would never be de-asserted
and the GIC would re-fire it after the guest EOIs, storming the host.
working on a restored microVM. The snapshot version is bumped
10.0.0→11.0.0.(
CONFIG_GPIOLIB,CONFIG_GPIO_PL061,CONFIG_INPUT_KEYBOARD,CONFIG_KEYBOARD_GPIO) toresources/guest_configs/ci.config, and updatedocs/api_requests/actions.md,docs/device-api.mdandCHANGELOG.md.This follows the direction confirmed by @ShadowCurse in
#2046:
no feature gating (always enabled on aarch64), reuse
SendCtrlAltDel, and relyon the
systemd-logindalready present in the CI rootfs.A couple of points worth a maintainer's eye:
ci.configratherthan a separate
debug.config-style fragment, because the integration testsboot the non-debug CI kernels (which only concatenate
ci.config), and thefile already carries the x86 "CTRL+ALT+DEL support" section. Happy to move it
if you'd prefer a dedicated fragment.
test_send_ctrl_alt_delon aarch64. It is keptskipif-skipped on aarch64for now: the kernel config is included here, but a fresh PR runs against the
currently-published guest-kernel artifacts, which do not yet have gpio-keys.
Once the CI guest kernels are rebuilt with this
ci.configchange the skip canbe removed so the existing test exercises the aarch64 path. Would you like me
to drop the skip in this PR, or as a follow-up after the artifacts are
rebuilt?
Hardware validation
Validated on real aarch64 + KVM hardware (RK3568, Ubuntu guest), on this exact
rebased commit:
/proc/interruptsshows the line asEdge; the countincrements by exactly +2 per action (press + release) and host CPU stays
effectively idle (~0.7% over the window). A level storm would peg a CPU.
systemd-logindactive,SendCtrlAltDel→systemd-poweroff→reboot: Power down→KVM_SYSTEM_EVENT→Firecracker exiting successfully. exit_code=0.still takes the interrupt (
+2) and emits a byte-identicalKEY_POWERpress/release event stream on
/dev/input/event0, confirming the PL061interrupt-enable state survives restore.
Reason
Closes #2046. aarch64 previously rejected
SendCtrlAltDelwith a400and had nomechanism to request a graceful shutdown of a microVM from the host, unlike
x86_64. This brings aarch64 to parity using the standard guest gpio-keys path.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
clippy -D warningson bothx86_64 and aarch64 (
tools/devtool buildon aarch64 hardware); CI runs thefull
checkbuild --all.tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests (PL061 device unit tests added; the
test_send_ctrl_alt_delintegration test remains aarch64-skipped pending a gpio-keys CI kernel — see
above).
TODO.rust-vmm.