| Message ID | 20230125182706.1480160-1-per.bilse@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v2] Create a Kconfig option to set preferred reboot method | expand |
On 25.01.2023 19:27, Per Bilse wrote: > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -306,6 +306,90 @@ config MEM_SHARING > bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED > depends on HVM > > +config REBOOT_SYSTEM_DEFAULT > + bool "Xen-defined reboot method" > + default y > + help > + Xen will choose the most appropriate reboot method, > + which will be a Xen SCHEDOP hypercall if running as > + a Xen guest, otherwise EFI, ACPI, or by way of the > + keyboard controller, depending on system features. > + Disabling this will allow you to specify how the > + system will be rebooted. > + > +choice > + bool "Reboot method" > + depends on !REBOOT_SYSTEM_DEFAULT > + default REBOOT_METHOD_ACPI > + help > + This is a compiled-in alternative to specifying the > + reboot method on the Xen command line. Specifying a > + method on the command line will override both this > + configuration and the warm boot option below. The way it's constructed right now, I don't think this is true. E.g. "reboot=warm" on the command line isn't going to override "PCI" selected here. It wouldn't even override REBOOT_WARM=n if I'm not mistaken, as the new call to set_reboot_type() would replace what was parsed from the command line. > + none Suppress automatic reboot after panics or crashes > + triple Force a triple fault (init) > + kbd Use the keyboard controller > + acpi Use the RESET_REG in the FADT > + pci Use the so-called "PCI reset register", CF9 > + power Like 'pci' but for a full power-cyle reset > + efi Use the EFI reboot (if running under EFI) > + xen Use Xen SCHEDOP hypercall (if running under Xen as a guest) > + > + config REBOOT_METHOD_NONE > + bool "none" To be honest I don't consider this a reboot "method". The parsing really should be treating this as a boolean (i.e. also permit "0", "false", or "off"). See also "noreboot" as a further (redundant) way of expressing that intention. What is important here is that this control doesn't affect the normal reboot process; it merely suppresses rebooting in case of a crash. While I wouldn't outright nack it, I think this aspect of behavior shouldn't be Kconfig-controlled. The more that it can only be overridden by "no-noreboot" (or similarly off equivalents like "noreboot=off") on the command line, which I guess you agree would be an awkward option to use. (Personally I think we should phase out "noreboot", as "reboot=no" has been around for long enough.) > + config REBOOT_METHOD_TRIPLE > + bool "triple" > + > + config REBOOT_METHOD_KBD > + bool "kbd" > + > + config REBOOT_METHOD_ACPI > + bool "acpi" > + > + config REBOOT_METHOD_PCI > + bool "pci" > + > + config REBOOT_METHOD_POWER > + bool "power" > + > + config REBOOT_METHOD_EFI > + bool "efi" For these prompts: They want to be meaningful to people seeing them. Imo acronyms want to be upper-case (when they're in common use) or be avoided (e.g. "kbd" -> "keyboard controller" or some such). My eye was particularly caught by "power", where I was wondering: What does that mean? > + config REBOOT_METHOD_XEN > + bool "xen" > + depends on !XEN_GUEST Why the "!" here? > +endchoice > + > +config REBOOT_METHOD > + string > + default "none" if REBOOT_METHOD_NONE > + default "triple" if REBOOT_METHOD_TRIPLE > + default "kbd" if REBOOT_METHOD_KBD > + default "acpi" if REBOOT_METHOD_ACPI > + default "pci" if REBOOT_METHOD_PCI > + default "Power" if REBOOT_METHOD_POWER > + default "efi" if REBOOT_METHOD_EFI > + default "xen" if REBOOT_METHOD_XEN > + > +config REBOOT_WARM > + bool "Warm reboot" > + default n > + help > + By default the system will perform a cold reboot. > + Enable this to carry out a warm reboot. This > + configuration will have no effect if a "reboot=" > + string is supplied on the Xen command line; in this > + case the reboot string must include "warm" if a warm > + reboot is desired. > + > +config REBOOT_TEMPERATURE > + string > + default "warm" if REBOOT_WARM > + default "cold" if !REBOOT_WARM && !REBOOT_SYSTEM_DEFAULT Instead of the dependency here, I think REBOOT_WARM should have a "depends on !REBOOT_SYSTEM_DEFAULT". But I view this second control as unnecessary anyway. All you need to do is ... > --- a/xen/arch/x86/shutdown.c > +++ b/xen/arch/x86/shutdown.c > @@ -28,6 +28,19 @@ > #include <asm/apic.h> > #include <asm/guest.h> > > +/* > + * We don't define a compiled-in reboot string if both method and > + * temperature are defaults, in which case we can compile better code. > + */ > +#ifdef CONFIG_REBOOT_METHOD > +#define REBOOT_STR CONFIG_REBOOT_METHOD "," CONFIG_REBOOT_TEMPERATURE > +#else > +#ifdef CONFIG_REBOOT_TEMPERATURE > +#define REBOOT_STR CONFIG_REBOOT_TEMPERATURE > +#endif > +#endif ... construct this accordingly based on REBOOT_WARM. > @@ -42,10 +55,13 @@ enum reboot_type { > static int reboot_mode; > > /* > - * reboot=t[riple] | k[bd] | a[cpi] | p[ci] | n[o] | [e]fi [, [w]arm | [c]old] > + * These constants are duplicated in full in arch/x86/Kconfig, keep in synch. > + * > + * reboot=t[riple] | k[bd] | a[cpi] | p[ci] | P[ower] | n[one] | [e]fi > + * [, [w]arm | [c]old] > * warm Don't set the cold reboot flag > * cold Set the cold reboot flag > - * no Suppress automatic reboot after panics or crashes > + * none Suppress automatic reboot after panics or crashes Why the change from "no" to "none"? Jan
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 6a7825f4ba..b881a118f1 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -306,6 +306,90 @@ config MEM_SHARING bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED depends on HVM +config REBOOT_SYSTEM_DEFAULT + bool "Xen-defined reboot method" + default y + help + Xen will choose the most appropriate reboot method, + which will be a Xen SCHEDOP hypercall if running as + a Xen guest, otherwise EFI, ACPI, or by way of the + keyboard controller, depending on system features. + Disabling this will allow you to specify how the + system will be rebooted. + +choice + bool "Reboot method" + depends on !REBOOT_SYSTEM_DEFAULT + default REBOOT_METHOD_ACPI + help + This is a compiled-in alternative to specifying the + reboot method on the Xen command line. Specifying a + method on the command line will override both this + configuration and the warm boot option below. + + none Suppress automatic reboot after panics or crashes + triple Force a triple fault (init) + kbd Use the keyboard controller + acpi Use the RESET_REG in the FADT + pci Use the so-called "PCI reset register", CF9 + power Like 'pci' but for a full power-cyle reset + efi Use the EFI reboot (if running under EFI) + xen Use Xen SCHEDOP hypercall (if running under Xen as a guest) + + config REBOOT_METHOD_NONE + bool "none" + + config REBOOT_METHOD_TRIPLE + bool "triple" + + config REBOOT_METHOD_KBD + bool "kbd" + + config REBOOT_METHOD_ACPI + bool "acpi" + + config REBOOT_METHOD_PCI + bool "pci" + + config REBOOT_METHOD_POWER + bool "power" + + config REBOOT_METHOD_EFI + bool "efi" + + config REBOOT_METHOD_XEN + bool "xen" + depends on !XEN_GUEST + +endchoice + +config REBOOT_METHOD + string + default "none" if REBOOT_METHOD_NONE + default "triple" if REBOOT_METHOD_TRIPLE + default "kbd" if REBOOT_METHOD_KBD + default "acpi" if REBOOT_METHOD_ACPI + default "pci" if REBOOT_METHOD_PCI + default "Power" if REBOOT_METHOD_POWER + default "efi" if REBOOT_METHOD_EFI + default "xen" if REBOOT_METHOD_XEN + +config REBOOT_WARM + bool "Warm reboot" + default n + help + By default the system will perform a cold reboot. + Enable this to carry out a warm reboot. This + configuration will have no effect if a "reboot=" + string is supplied on the Xen command line; in this + case the reboot string must include "warm" if a warm + reboot is desired. + +config REBOOT_TEMPERATURE + string + default "warm" if REBOOT_WARM + default "cold" if !REBOOT_WARM && !REBOOT_SYSTEM_DEFAULT + endmenu source "common/Kconfig" diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c index 7619544d14..4969af1316 100644 --- a/xen/arch/x86/shutdown.c +++ b/xen/arch/x86/shutdown.c @@ -28,6 +28,19 @@ #include <asm/apic.h> #include <asm/guest.h> +/* + * We don't define a compiled-in reboot string if both method and + * temperature are defaults, in which case we can compile better code. + */ +#ifdef CONFIG_REBOOT_METHOD +#define REBOOT_STR CONFIG_REBOOT_METHOD "," CONFIG_REBOOT_TEMPERATURE +#else +#ifdef CONFIG_REBOOT_TEMPERATURE +#define REBOOT_STR CONFIG_REBOOT_TEMPERATURE +#endif +#endif + +/* Do not modify without updating arch/x86/Kconfig, see below. */ enum reboot_type { BOOT_INVALID, BOOT_TRIPLE = 't', @@ -42,10 +55,13 @@ enum reboot_type { static int reboot_mode; /* - * reboot=t[riple] | k[bd] | a[cpi] | p[ci] | n[o] | [e]fi [, [w]arm | [c]old] + * These constants are duplicated in full in arch/x86/Kconfig, keep in synch. + * + * reboot=t[riple] | k[bd] | a[cpi] | p[ci] | P[ower] | n[one] | [e]fi + * [, [w]arm | [c]old] * warm Don't set the cold reboot flag * cold Set the cold reboot flag - * no Suppress automatic reboot after panics or crashes + * none Suppress automatic reboot after panics or crashes * triple Force a triple fault (init) * kbd Use the keyboard controller. cold reset (default) * acpi Use the RESET_REG in the FADT @@ -56,7 +72,12 @@ static int reboot_mode; */ static enum reboot_type reboot_type = BOOT_INVALID; +/* If we don't have a compiled-in boot string, we won't call after start-up. */ +#ifndef REBOOT_STR static int __init cf_check set_reboot_type(const char *str) +#else +static int cf_check set_reboot_type(const char *str) +#endif { int rc = 0; @@ -145,6 +166,11 @@ void machine_halt(void) static void default_reboot_type(void) { +#ifdef REBOOT_STR + if ( reboot_type == BOOT_INVALID ) + set_reboot_type(REBOOT_STR); +#endif + if ( reboot_type != BOOT_INVALID ) return;
Provide a user-friendly option to specify preferred reboot details at compile time. It uses the same internals as the command line 'reboot' parameter, and will be overridden by a choice on the command line. Signed-off-by: Per Bilse <per.bilse@citrix.com> --- v2: Incorporate feedback from initial patch. Separating out warm reboot as a separate boolean led to a proliferation of code changes, so we now use the details from Kconfig to assemble a reboot string identical to what would be specified on the command line. This leads to minimal changes and additions to the code. --- xen/arch/x86/Kconfig | 84 +++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/shutdown.c | 30 ++++++++++++++- 2 files changed, 112 insertions(+), 2 deletions(-)