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

[Jan Beulich <jbeulich@xxxxxxxx>]

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