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
