xen/arch/x86/Kconfig | 95 +++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/shutdown.c | 11 +++++ 2 files changed, 106 insertions(+)
This patch provides the option to compile in a preferred reboot method,
as an alternative to specifying it on the Xen command line. It uses the
same internals as the command line 'reboot' parameter, and will be
overridden by a choice on the command line.
I have referred to this as 'reboot method' rather than 'reboot type' as
used in the code. A 'type' suggests something to happen after the
reboot, akin to a UNIX run level, whereas 'method' clearly identifies
how the reboot will be achieved. I thought it best for this to be
clear in an outward facing utility.
Signed-off-by: Per Bilse <per.bilse@citrix.com>
---
xen/arch/x86/Kconfig | 95 +++++++++++++++++++++++++++++++++++++++++
xen/arch/x86/shutdown.c | 11 +++++
2 files changed, 106 insertions(+)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6a7825f4ba..d35b14aa17 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -306,6 +306,101 @@ config MEM_SHARING
bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED
depends on HVM
+config REBOOT_SYSTEM_DEFAULT
+ default y
+ bool "Xen-defined reboot method"
+ help
+ Xen will choose the most appropriate reboot method,
+ which will be EFI, ACPI, or by way of the keyboard
+ controller, depending on system features. Disabling
+ this will allow you to choose exactly how the system
+ will be rebooted.
+
+choice
+ bool "Choose 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 this choice.
+
+ warm Don't set the cold reboot flag
+ cold Set the cold reboot flag
+ none Suppress automatic reboot after panics or crashes
+ triple Force a triple fault (init)
+ kbd Use the keyboard controller, cold reset
+ 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_WARM
+ bool "warm"
+ help
+ Don't set the cold reboot flag.
+
+ config REBOOT_METHOD_COLD
+ bool "cold"
+ help
+ Set the cold reboot flag.
+
+ config REBOOT_METHOD_NONE
+ bool "none"
+ help
+ Suppress automatic reboot after panics or crashes.
+
+ config REBOOT_METHOD_TRIPLE
+ bool "triple"
+ help
+ Force a triple fault (init).
+
+ config REBOOT_METHOD_KBD
+ bool "kbd"
+ help
+ Use the keyboard controller, cold reset.
+
+ config REBOOT_METHOD_ACPI
+ bool "acpi"
+ help
+ Use the RESET_REG in the FADT.
+
+ config REBOOT_METHOD_PCI
+ bool "pci"
+ help
+ Use the so-called "PCI reset register", CF9.
+
+ config REBOOT_METHOD_POWER
+ bool "power"
+ help
+ Like 'pci' but for a full power-cyle reset.
+
+ config REBOOT_METHOD_EFI
+ bool "efi"
+ help
+ Use the EFI reboot (if running under EFI).
+
+ config REBOOT_METHOD_XEN
+ bool "xen"
+ help
+ Use Xen SCHEDOP hypercall (if running under Xen as a guest).
+
+endchoice
+
+config REBOOT_METHOD
+ string
+ default "w" if REBOOT_METHOD_WARM
+ default "c" if REBOOT_METHOD_COLD
+ default "n" if REBOOT_METHOD_NONE
+ default "t" if REBOOT_METHOD_TRIPLE
+ default "k" if REBOOT_METHOD_KBD
+ default "a" if REBOOT_METHOD_ACPI
+ default "p" if REBOOT_METHOD_PCI
+ default "P" if REBOOT_METHOD_POWER
+ default "e" if REBOOT_METHOD_EFI
+ default "x" if REBOOT_METHOD_XEN
+
endmenu
source "common/Kconfig"
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 7619544d14..f44a188e2a 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -28,6 +28,7 @@
#include <asm/apic.h>
#include <asm/guest.h>
+/* NOTE: these constants are duplicated in arch/x86/Kconfig; keep in synch */
enum reboot_type {
BOOT_INVALID,
BOOT_TRIPLE = 't',
@@ -143,6 +144,8 @@ void machine_halt(void)
__machine_halt(NULL);
}
+#ifdef CONFIG_REBOOT_SYSTEM_DEFAULT
+
static void default_reboot_type(void)
{
if ( reboot_type != BOOT_INVALID )
@@ -533,6 +536,8 @@ static const struct dmi_system_id __initconstrel reboot_dmi_table[] = {
{ }
};
+#endif /* CONFIG_REBOOT_SYSTEM_DEFAULT */
+
static int __init cf_check reboot_init(void)
{
/*
@@ -542,8 +547,12 @@ static int __init cf_check reboot_init(void)
if ( reboot_type != BOOT_INVALID )
return 0;
+#ifdef CONFIG_REBOOT_SYSTEM_DEFAULT
default_reboot_type();
dmi_check_system(reboot_dmi_table);
+#else
+ set_reboot_type(CONFIG_REBOOT_METHOD);
+#endif
return 0;
}
__initcall(reboot_init);
@@ -595,8 +604,10 @@ void machine_restart(unsigned int delay_millisecs)
tboot_shutdown(TB_SHUTDOWN_REBOOT);
}
+#ifdef CONFIG_REBOOT_SYSTEM_DEFAULT
/* Just in case reboot_init() didn't run yet. */
default_reboot_type();
+#endif
orig_reboot_type = reboot_type;
/* Rebooting needs to touch the page at absolute address 0. */
--
2.31.1On 16.01.2023 18:21, Per Bilse wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -306,6 +306,101 @@ config MEM_SHARING
> bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED
> depends on HVM
>
> +config REBOOT_SYSTEM_DEFAULT
> + default y
> + bool "Xen-defined reboot method"
May I ask that you swap the two lines? (Personally I consider kconfig
too forgiving here - it doesn't make a lot of sense to set a default
when the type isn't known yet.)
> + help
> + Xen will choose the most appropriate reboot method,
> + which will be EFI, ACPI, or by way of the keyboard
> + controller, depending on system features. Disabling
> + this will allow you to choose exactly how the system
> + will be rebooted.
Indentation: Help text is to be indented by a tab and two spaces. See
pre-existing examples.
> +choice
> + bool "Choose 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 this choice.
> +
> + warm Don't set the cold reboot flag
> + cold Set the cold reboot flag
These two are modifiers, not reboot methods on their own. They set a
field in the BDA to tell the BIOS how much initialization / checking
to do (in the legacy case). Therefore they shouldn't be selectable
right here. If you feel like it you could add another boolean to
control that default.
> + none Suppress automatic reboot after panics or crashes
> + triple Force a triple fault (init)
> + kbd Use the keyboard controller, cold reset
> + 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_WARM
> + bool "warm"
> + help
> + Don't set the cold reboot flag.
I don't think help text is very useful in sub-items of a choice. Plus
you also explain each item above.
> + config REBOOT_METHOD_COLD
> + bool "cold"
> + help
> + Set the cold reboot flag.
> +
> + config REBOOT_METHOD_NONE
> + bool "none"
> + help
> + Suppress automatic reboot after panics or crashes.
> +
> + config REBOOT_METHOD_TRIPLE
> + bool "triple"
> + help
> + Force a triple fault (init).
> +
> + config REBOOT_METHOD_KBD
> + bool "kbd"
> + help
> + Use the keyboard controller, cold reset.
> +
> + config REBOOT_METHOD_ACPI
> + bool "acpi"
> + help
> + Use the RESET_REG in the FADT.
> +
> + config REBOOT_METHOD_PCI
> + bool "pci"
> + help
> + Use the so-called "PCI reset register", CF9.
> +
> + config REBOOT_METHOD_POWER
> + bool "power"
> + help
> + Like 'pci' but for a full power-cyle reset.
> +
> + config REBOOT_METHOD_EFI
> + bool "efi"
> + help
> + Use the EFI reboot (if running under EFI).
> +
> + config REBOOT_METHOD_XEN
> + bool "xen"
> + help
> + Use Xen SCHEDOP hypercall (if running under Xen as a guest).
This wants to depend on XEN_GUEST, doesn't it?
> +endchoice
> +
> +config REBOOT_METHOD
> + string
> + default "w" if REBOOT_METHOD_WARM
> + default "c" if REBOOT_METHOD_COLD
> + default "n" if REBOOT_METHOD_NONE
> + default "t" if REBOOT_METHOD_TRIPLE
> + default "k" if REBOOT_METHOD_KBD
> + default "a" if REBOOT_METHOD_ACPI
> + default "p" if REBOOT_METHOD_PCI
> + default "P" if REBOOT_METHOD_POWER
> + default "e" if REBOOT_METHOD_EFI
> + default "x" if REBOOT_METHOD_XEN
I think it would be neater (and more forward compatible) if the strings
were actually spelled out here. We may, at any time, make set_reboot_type()
look at more than just the first character.
> @@ -143,6 +144,8 @@ void machine_halt(void)
> __machine_halt(NULL);
> }
>
> +#ifdef CONFIG_REBOOT_SYSTEM_DEFAULT
> +
> static void default_reboot_type(void)
> {
> if ( reboot_type != BOOT_INVALID )
> @@ -533,6 +536,8 @@ static const struct dmi_system_id __initconstrel reboot_dmi_table[] = {
> { }
> };
>
> +#endif /* CONFIG_REBOOT_SYSTEM_DEFAULT */
> +
> static int __init cf_check reboot_init(void)
> {
> /*
> @@ -542,8 +547,12 @@ static int __init cf_check reboot_init(void)
> if ( reboot_type != BOOT_INVALID )
> return 0;
>
> +#ifdef CONFIG_REBOOT_SYSTEM_DEFAULT
> default_reboot_type();
> dmi_check_system(reboot_dmi_table);
> +#else
> + set_reboot_type(CONFIG_REBOOT_METHOD);
> +#endif
I don't think you should eliminate the use of DMI - that's machine
specific information which should imo still overrule a build-time default.
See also the comment just out of context - it's a difference whether the
override came from the command line or was set at build time.
> @@ -595,8 +604,10 @@ void machine_restart(unsigned int delay_millisecs)
> tboot_shutdown(TB_SHUTDOWN_REBOOT);
> }
>
> +#ifdef CONFIG_REBOOT_SYSTEM_DEFAULT
> /* Just in case reboot_init() didn't run yet. */
> default_reboot_type();
> +#endif
> orig_reboot_type = reboot_type;
As the comment says, this is done here to cover the case of a very early
crash. You want to apply the build-time default then as well. Hence I
think you want to invoke set_reboot_type(CONFIG_REBOOT_METHOD) from
inside default_reboot_type(), rather than in place of it (perhaps while
#ifdef-ing out its alternative code). That'll then also make sure the
command line setting overrides the built-in default - it doesn't look
as if that would work with the current arrangements.
Furthermore a reboot type of "e" will result in reboot_type getting set
to BOOT_INVALID when not running on top of EFI. I think you want to fall
back to default_reboot_type() in that case.
So, taking everything together, maybe
static void default_reboot_type(void)
{
#ifndef CONFIG_REBOOT_SYSTEM_DEFAULT
if ( reboot_type == BOOT_INVALID )
set_reboot_type(CONFIG_REBOOT_METHOD);
#endif
if ( reboot_type != BOOT_INVALID )
return;
if ( xen_guest )
...
? Which of course would require (conditionally?) dropping __init from
set_reboot_type(). (And I wonder whether the #ifndef wouldn't better be
"#ifdef CONFIG_REBOOT_METHOD".)
Jan
On 17/01/2023 15:55, Jan Beulich wrote: > On 16.01.2023 18:21, Per Bilse wrote: >> +config REBOOT_SYSTEM_DEFAULT >> + default y >> + bool "Xen-defined reboot method" > > May I ask that you swap the two lines? (Personally I consider kconfig > too forgiving here - it doesn't make a lot of sense to set a default > when the type isn't known yet.) Certainly, I spotted it myself after sending, and was kicking myself for not seeing it earlier. >> +choice >> + bool "Choose 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 this choice. >> + >> + warm Don't set the cold reboot flag >> + cold Set the cold reboot flag > > These two are modifiers, not reboot methods on their own. They set a > field in the BDA to tell the BIOS how much initialization / checking > to do (in the legacy case). Therefore they shouldn't be selectable > right here. If you feel like it you could add another boolean to > control that default. My understanding is that it was desired to provide a compiled-in equivalent of the command line 'reboot=' option (which includes cold and warm), but this may be a misunderstanding. I can separate these two out. >> + config REBOOT_METHOD_WARM >> + bool "warm" >> + help >> + Don't set the cold reboot flag. > > I don't think help text is very useful in sub-items of a choice. Plus > you also explain each item above. I thought it better to err on the side of caution. The help texts will appear at two different menu levels, but I can remove it. >> + config REBOOT_METHOD_XEN >> + bool "xen" >> + help >> + Use Xen SCHEDOP hypercall (if running under Xen as a guest). > > This wants to depend on XEN_GUEST, doesn't it? Yes, depending on context. In providing a compiled-in equivalent of the command-line parameter, it should arguably provide and accept the same set of options, but I'll change it. >> + default "x" if REBOOT_METHOD_XEN > > I think it would be neater (and more forward compatible) if the strings > were actually spelled out here. We may, at any time, make set_reboot_type() > look at more than just the first character. OK. >> +#ifdef CONFIG_REBOOT_SYSTEM_DEFAULT >> default_reboot_type(); >> dmi_check_system(reboot_dmi_table); >> +#else >> + set_reboot_type(CONFIG_REBOOT_METHOD); >> +#endif > > I don't think you should eliminate the use of DMI - that's machine > specific information which should imo still overrule a build-time default. > See also the comment just out of context - it's a difference whether the > override came from the command line or was set at build time. OK; again, it's a slightly different take on the purpose than I had, but I can change it. Also for the rest. Best, -- Per
On 17.01.2023 20:28, Per Bilse wrote: > On 17/01/2023 15:55, Jan Beulich wrote: >> On 16.01.2023 18:21, Per Bilse wrote: >>> + config REBOOT_METHOD_XEN >>> + bool "xen" >>> + help >>> + Use Xen SCHEDOP hypercall (if running under Xen as a guest). >> >> This wants to depend on XEN_GUEST, doesn't it? > > Yes, depending on context. In providing a compiled-in equivalent > of the command-line parameter, it should arguably provide and accept > the same set of options, but I'll change it. If consistency between the two cases is the goal, then why not adjust command line handling (in a separate patch) to "not know" about "x" when !XEN_GUEST? Jan
On 18/01/2023 07:15, Jan Beulich wrote: > On 17.01.2023 20:28, Per Bilse wrote: >> On 17/01/2023 15:55, Jan Beulich wrote: >>> On 16.01.2023 18:21, Per Bilse wrote: >>>> + config REBOOT_METHOD_XEN >>>> + bool "xen" >>>> + help >>>> + Use Xen SCHEDOP hypercall (if running under Xen as a guest). >>> >>> This wants to depend on XEN_GUEST, doesn't it? >> >> Yes, depending on context. In providing a compiled-in equivalent >> of the command-line parameter, it should arguably provide and accept >> the same set of options, but I'll change it. > > If consistency between the two cases is the goal, then why not adjust > command line handling (in a separate patch) to "not know" about "x" > when !XEN_GUEST? Because that would be a different ticket, and we have enough tickets. :-) But no worries, your suggestions make perfect sense in a widened context, I just went with a minimalist interpretation in order to keep changes minimal. Best, -- Per
© 2016 - 2024 Red Hat, Inc.