[XEN PATCH v2] Create a Kconfig option to set preferred reboot method

Per Bilse posted 1 patch 1 year, 2 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230125182706.1480160-1-per.bilse@citrix.com
xen/arch/x86/Kconfig    | 84 +++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/shutdown.c | 30 ++++++++++++++-
2 files changed, 112 insertions(+), 2 deletions(-)
[XEN PATCH v2] Create a Kconfig option to set preferred reboot method
Posted by Per Bilse 1 year, 2 months ago
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(-)

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;
 
-- 
2.31.1
Re: [XEN PATCH v2] Create a Kconfig option to set preferred reboot method
Posted by Jan Beulich 1 year, 1 month ago
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