Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AArch64: graceful power-off from external #2047

Closed
wants to merge 2 commits into from

Conversation

Pennyzct
Copy link
Contributor

@Pennyzct Pennyzct commented Jul 28, 2020

Reason for This PR

#2046

Description of Changes

Add a new API action PressGPIOPowerOff to inject GPIO Pin 3 keypress to the microvm.
This can be used to trigger a graceful shutdown of the microVM, if the guest has support for gpio pl061 and gpio-keys.
Firecracker needs to add emulated gpio pl061 controller and gpio-keys node in microvm.
Related commit follows ARM PrimeCell General Purpose Input/Output(PL061) specification.
See https://static.docs.arm.com/ddi0190/b/DDI0190.pdf

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any API changes are reflected in firecracker/swagger.yaml.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Pennyzct added 2 commits July 28, 2020 09:54
In order to gracefully poweroff microvm from external, firecracker
needs to add gpio pl061 controller and gpio-keys node.
This commit implements ARM PrimeCell General Purpose Input/Output(PL061)
specification.
See https://static.docs.arm.com/ddi0190/b/DDI0190.pdf

Signed-off-by: Penny Zheng <[email protected]>
Signed-off-by: Wei Chen <[email protected]>
Added a new API action to inject GPIO Pin 3 keypress to the microvm.
This can be used to trigger a graceful shutdown of the microVM, if the
guest has support for gpio pl061 and gpio-keys.
This new action is only supported on AArch64.

Signed-off-by: Penny Zheng <[email protected]>
@Pennyzct Pennyzct changed the title Gpio emulation AArch64: graceful power-off from external Jul 28, 2020
@Pennyzct
Copy link
Contributor Author

@Weichen81

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution!

Overall PR looks good, but I want to dive-deep on the options available here. I'm afraid using such a specific GPIO mechanism isn't the best solution (although I don't have a better idea right now).
In the future we might want to add ACPI support and handle this consistently across archs and kernel configs.

We'll take a deeper look at the proposal and implementation and come back with more feedback.

Comment on lines +22 to +23
timer = ">=0.1.3"
chrono = ">=0.2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a different mechanism than relying on timer (which also brings in chrono) for multiple reasons:

  • timer crate has an incompatible MPL-2.0 license
  • timer is implemented using a multi-thread timer model: spawns a new thread to do the work after a certain time. This is not in line with our security model/posture where we want to keep strict control over process threads (our seccomp filters do not allow spawning new threads).
  • chrono is a bloated dependency with a long-ish trail of child dependencies. We'd like to avoid it since the use-case for it is simple enough to cover with std::time.

Possible alternative: use TimerFd - for example that's what we use in our Rate Limiter implementation: https://github.com/firecracker-microvm/firecracker/blob/master/src/rate_limiter/src/lib.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. I'll try to replace them with TimerFd. ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our device model uses a single emulation thread that asynchronously handles device events. TimerFd would be such a gpio_pl061 device event.

The device needs to use our EventManager Subscriber interface to register async events.
A simple example device with a single event can be seen in our Serial device implementation:

impl Subscriber for Serial {

Comment on lines +99 to +100
#[cfg(target_arch = "aarch64")]
pub gpio_pl061: Option<Arc<Mutex<devices::legacy::GPIO>>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to create a gpio bus (which only currently has this device)?

@Pennyzct
Copy link
Contributor Author

Hi @acatangiu @dianpopa thanks for the review~
The reason why I chose gpio-keys to power off VM lies here.
ACPI may be the consistent way across archs . ;). Although, right now, AArch64 needs extra UEFI support to boot from ACPI, and that may lead to extra boot-up time and memory footprint.

@dianpopa dianpopa added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jul 29, 2020
@Pennyzct
Copy link
Contributor Author

Hi @acatangiu @dianpopa
For issuing level-triggered interrupt, I'm now using KVM_IRQ_LINE ioctl, and using timer to emulate outbound irq line being raised for 100ms. Although, it seems to me that KVM_IRQ_LINE hasn't been included in syscall whitelist.
Or we could still use KVM_IRQFD ioctl to do level-triggered interrupt, but then we also need to implement a new func register_irqfd_resample in https://github.com/firecracker-microvm/kvm-ioctls/blob/master/src/ioctls/vm.rs to allow resample.

With KVM_IRQFD_FLAG_RESAMPLE, KVM_IRQFD supports a de-assert and notify
mechanism allowing emulation of level-triggered, irqfd-based interrupts.
When KVM_IRQFD_FLAG_RESAMPLE is set the user must pass an additional eventfd in the kvm_irqfd.resamplefd field.
When operating in resample mode, posting of an interrupt through kvm_irq.fd asserts
the specified gsi in the irqchip. When the irqchip is resampled, such as from an EOI,
the gsi is de-asserted and the user is notified via kvm_irqfd.resamplefd.

So Wdyt? ☺

@dianpopa
Copy link
Contributor

Hi ,

Sorry for the late late reply. We want to take some time and think about the design decisions implied by adding support for shutdown on aarch64. We have lots of other priorities so it might take a while until we come back to your PR. Thanks for your contribution and your patience @Pennyzct !

@dianpopa dianpopa added Status: Blocked Indicates that an issue or pull request cannot currently be worked on and removed Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Oct 23, 2020
@dianpopa
Copy link
Contributor

Hi @Pennyzct ,

Last year was a very busy one for us and I wanted to let you know that we are still trying to reach a conclusion regarding your PR. We are still interested in implementing shutdown functionality on arm64 too but we still need some time to decide on how this functionality is going to integrate with Firecracker. The reason for that is that we are currently reconsidering our decision to use FDT for describing the hardware components of an aarch64 microVM and until we have not reached a conclusion we would like to postpone merging this PR. We will get back to you when we have decided on a path forward! Thanks!

@dianpopa
Copy link
Contributor

Hi @Pennyzct,

We reached a conclusion regarding this functionality. Later this year we are planning on adding ACPI PM support in Firecracker, check this out. Having ACPI power management will allow for a more straightforward way of doing reboot on all platforms currently supported without the need to use a dedicated device. As a consequence, I am closing this PR for the moment. Thanks for your implication!

@dianpopa dianpopa closed this Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Blocked Indicates that an issue or pull request cannot currently be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants