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

[Jan Beulich <jbeulich@xxxxxxxx>]

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