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

[Per Bilse <Per.Bilse@xxxxxxxxxx>]

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