Message ID | 20230116172102.43469-1-per.bilse@citrix.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [XEN] Create a Kconfig option to set preferred reboot method | expand |
On 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
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. */
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(+)