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
