xen: improve shutdown hook

[mhorne]

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.

[markj]

This looks reasonable to me but should really be tested.

[royger]

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

[ehem_freebsd_m5p.com]

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.

[royger]

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.

[royger]

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.

[mhorne]

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

[mhorne]

sys/dev/xen/control/control.c

349–360

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

[ehem_freebsd_m5p.com]

In D42343#969231, @royger wrote:

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.

[royger]

In D42343#969529, @ehem_freebsd_m5p.com wrote:

In D42343#969231, @royger wrote:

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.

[royger]

Change LGTM, thanks :).