Page MenuHomeFreeBSD

xen: improve shutdown hook
ClosedPublic

Authored by mhorne on Oct 23 2023, 8:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 9, 12:31 PM
Unknown Object (File)
Thu, May 9, 12:30 PM
Unknown Object (File)
Thu, May 9, 1:38 AM
Unknown Object (File)
Mon, Apr 22, 12:31 PM
Unknown Object (File)
Dec 28 2023, 8:36 AM
Unknown Object (File)
Dec 23 2023, 2:31 AM
Unknown Object (File)
Dec 17 2023, 4:08 PM
Unknown Object (File)
Dec 14 2023, 7:06 PM

Details

Summary

Make better use of the shutdown flags. In particular this now handles
standard reboot where RB_POWERCYCLE is not set. For RB_HALT, perform a
shutdown. Typically this flag would halt/busy-loop the machine, but for
Xen guests I think a poweroff is more sensible.

Give the function a prefix while here.

Test Plan

Unfortunately, I have no way to test this.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 54304
Build 51194: arc lint + arc unit

Event Timeline

This looks reasonable to me but should really be tested.

This revision is now accepted and ready to land.Nov 2 2023, 3:33 PM

There's also SHUTDOWN_crash which should likely be used if the kernel panicked, likely by checking KERNEL_PANICKED()?

I mostly concur with D42343, I've noticed the behavior and found it suboptimal. Only trick is after a fair bit of time not looking at this I found a small issue which needs to be addressed.

sys/dev/xen/control/control.c
349–360

Took me quite a while of not looking at this and simply doing other things and then coming back to spot the issue. This does seem pretty reasonable for FreeBSD as a Xen DomU, this is not appropriate for FreeBSD as a Xen Dom0. Mainly if FreeBSD is domain 0, then halt should likely do the normal halt action.

My opinion is that halt should behave equally on bare metal than when virtualized, otherwise it's just going to confuse users. If we believe that halt should behave differently (iow: do power off) it should be changed uniformly.

sys/dev/xen/control/control.c
349–360

Are you sure that howto cannot contain any extra flags that would make the switch no longer match?

It's not obvious to me which flags can be concurrently set in howto, and we have no guarantee that new flags won't be added that could be set together with the existing ones. Overall using a switch here looks fragile, even if currently a single flags gets set in howto for reboot.

Specify SHUTDOWN_crash when the system has panicked. Don't handle RB_HALT, consistent with other shutdown_final handlers.

This revision now requires review to proceed.Nov 6 2023, 5:08 PM
sys/dev/xen/control/control.c
349–360

Indeed, RB_NOSYNC may also be present in any of these shutdown cases.

My opinion is that halt should behave equally on bare metal than when virtualized, otherwise it's just going to confuse users. If we believe that halt should behave differently (iow: do power off) it should be changed uniformly.

First, my current opinion is FreeBSD on Xen as Domain 0, should behave similarly to FreeBSD on bare metal.

Second, my current opinion is spinning in a halt-loop is a poor fit for FreeBSD on Xen as a user Domain. The reason is the domain ends up sitting there doing nothing useful.

As such I think instead if ((howto & RB_HALT) == 0 && (hvm_start_flags & (SIF_INITDOMAIN | SIF_PRIVILEGED))) should lead to HYPERVISOR_shutdown(SHUTDOWN_poweroff);. In-between the original and what is here now.

sys/dev/xen/control/control.c
349–360

I was thinking this looked like it wanted to turn into a switch, but that might not work out. Feel free to ignore the original comment.

My opinion is that halt should behave equally on bare metal than when virtualized, otherwise it's just going to confuse users. If we believe that halt should behave differently (iow: do power off) it should be changed uniformly.

First, my current opinion is FreeBSD on Xen as Domain 0, should behave similarly to FreeBSD on bare metal.

Second, my current opinion is spinning in a halt-loop is a poor fit for FreeBSD on Xen as a user Domain. The reason is the domain ends up sitting there doing nothing useful.

You could say the same about a physical system sitting there doing nothing. IMO if the behavior has to change, it has to change globally, and not just for Xen guests. Otherwise it just adds confusion for end users.

Change LGTM, thanks :).

This revision is now accepted and ready to land.Nov 7 2023, 8:40 AM
This revision was automatically updated to reflect the committed changes.