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
