| Message ID | 1490287580-21725-4-git-send-email-erichte@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2017-03-23 at 11:46 -0500, Eric Richter wrote: > This patch adds an option to the system configuration menu that if > checked, enables the use of kexec_file_load. > > Signed-off-by: Eric Richter <erichte@linux.vnet.ibm.com> > --- > ui/ncurses/nc-config.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/ui/ncurses/nc-config.c b/ui/ncurses/nc-config.c > index 8349629..1e9524b 100644 > --- a/ui/ncurses/nc-config.c > +++ b/ui/ncurses/nc-config.c > @@ -33,7 +33,7 @@ > #include "nc-config.h" > #include "nc-widgets.h" > > -#define N_FIELDS 48 > +#define N_FIELDS 50 > > extern struct help_text config_help_text; > > @@ -68,6 +68,7 @@ struct config_screen { > bool autoboot_enabled; > bool ipmi_override; > bool net_override; > + bool kexec_method; > > struct { > struct nc_widget_label *autoboot_l; > @@ -82,6 +83,9 @@ struct config_screen { > struct nc_widget_label *timeout_l; > struct nc_widget_label *timeout_help_l; > > + struct nc_widget_label *kexec_method_l; > + struct nc_widget_checkbox *kexec_method_cb; > + > struct nc_widget_label *ipmi_type_l; > struct nc_widget_label *ipmi_clear_l; > struct nc_widget_checkbox *ipmi_clear_cb; > @@ -256,6 +260,8 @@ static int screen_process_form(struct config_screen *screen) > config->autoboot_timeout_sec = x; > } > > + config->kexec_method = widget_checkbox_get_value(screen->widgets.kexec_method_cb); > + > if (screen->ipmi_override) > if (widget_checkbox_get_value(screen->widgets.ipmi_clear_cb)) > config->ipmi_bootdev = IPMI_BOOTDEV_INVALID; > @@ -490,6 +496,16 @@ static void config_screen_layout_widgets(struct config_screen *screen) > } else > y += 1; > > + > + wl = widget_label_base(screen->widgets.kexec_method_l); > + widget_set_visible(wl, true); > + widget_move(wl, y, screen->label_x); > + > + wf = widget_checkbox_base(screen->widgets.kexec_method_cb); > + widget_set_visible(wf, true); > + widget_move(wf, y, screen->field_x); > + y += 1; > + > if (screen->ipmi_override) { > wl = widget_label_base(screen->widgets.ipmi_type_l); > widget_set_visible(wl, true); > @@ -927,6 +943,11 @@ static void config_screen_setup_widgets(struct config_screen *screen, > widget_textbox_set_fixed_size(screen->widgets.timeout_f); > widget_textbox_set_validator_integer(screen->widgets.timeout_f, 0, 999); > > + screen->widgets.kexec_method_l = widget_new_label(set, 0, 0, > + _("Use file_load:")); This probably needs a more descriptive label, I don't think it's clear what "file load" means if you don't already have kexec on you mind. Maybe "Use secure kexec", or something similar? > + screen->widgets.kexec_method_cb = widget_new_checkbox(set, 0, 0, > + screen->kexec_method); I think you can just use config->kexec_method directly instead of adding the screen->kexec_method field. > + > if (config->ipmi_bootdev) { > label = talloc_asprintf(screen, > _("%s IPMI boot option: %s"), > @@ -1170,6 +1191,7 @@ static void config_screen_draw(struct config_screen *screen, > config_screen_setup_empty(screen); > } else { > screen->net_conf_type = find_net_conf_type(config); > + screen->kexec_method = config->kexec_method; > config_screen_setup_widgets(screen, config, sysinfo); > config_screen_layout_widgets(screen); > }
On 03/23/2017 09:22 PM, Samuel Mendoza-Jonas wrote: > On Thu, 2017-03-23 at 11:46 -0500, Eric Richter wrote: >> This patch adds an option to the system configuration menu that if >> checked, enables the use of kexec_file_load. >> >> Signed-off-by: Eric Richter <erichte@linux.vnet.ibm.com> >> --- >> ui/ncurses/nc-config.c | 24 +++++++++++++++++++++++- >> 1 file changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/ui/ncurses/nc-config.c b/ui/ncurses/nc-config.c >> index 8349629..1e9524b 100644 >> --- a/ui/ncurses/nc-config.c >> +++ b/ui/ncurses/nc-config.c >> @@ -33,7 +33,7 @@ >> #include "nc-config.h" >> #include "nc-widgets.h" >> >> -#define N_FIELDS 48 >> +#define N_FIELDS 50 >> >> extern struct help_text config_help_text; >> >> @@ -68,6 +68,7 @@ struct config_screen { >> bool autoboot_enabled; >> bool ipmi_override; >> bool net_override; >> + bool kexec_method; >> >> struct { >> struct nc_widget_label *autoboot_l; >> @@ -82,6 +83,9 @@ struct config_screen { >> struct nc_widget_label *timeout_l; >> struct nc_widget_label *timeout_help_l; >> >> + struct nc_widget_label *kexec_method_l; >> + struct nc_widget_checkbox *kexec_method_cb; >> + >> struct nc_widget_label *ipmi_type_l; >> struct nc_widget_label *ipmi_clear_l; >> struct nc_widget_checkbox *ipmi_clear_cb; >> @@ -256,6 +260,8 @@ static int screen_process_form(struct config_screen *screen) >> config->autoboot_timeout_sec = x; >> } >> >> + config->kexec_method = widget_checkbox_get_value(screen->widgets.kexec_method_cb); >> + >> if (screen->ipmi_override) >> if (widget_checkbox_get_value(screen->widgets.ipmi_clear_cb)) >> config->ipmi_bootdev = IPMI_BOOTDEV_INVALID; >> @@ -490,6 +496,16 @@ static void config_screen_layout_widgets(struct config_screen *screen) >> } else >> y += 1; >> >> + >> + wl = widget_label_base(screen->widgets.kexec_method_l); >> + widget_set_visible(wl, true); >> + widget_move(wl, y, screen->label_x); >> + >> + wf = widget_checkbox_base(screen->widgets.kexec_method_cb); >> + widget_set_visible(wf, true); >> + widget_move(wf, y, screen->field_x); >> + y += 1; >> + >> if (screen->ipmi_override) { >> wl = widget_label_base(screen->widgets.ipmi_type_l); >> widget_set_visible(wl, true); >> @@ -927,6 +943,11 @@ static void config_screen_setup_widgets(struct config_screen *screen, >> widget_textbox_set_fixed_size(screen->widgets.timeout_f); >> widget_textbox_set_validator_integer(screen->widgets.timeout_f, 0, 999); >> >> + screen->widgets.kexec_method_l = widget_new_label(set, 0, 0, >> + _("Use file_load:")); > > This probably needs a more descriptive label, I don't think it's clear > what "file load" means if you don't already have kexec on you mind. > Maybe "Use secure kexec", or something similar? > The string "Use file_load:" while unspecific, is exactly the length of space before the widgets are on the right. "Use secure kexec:" gets cut off by the widget, if that stays aligned with the others above/below. Would it be preferable to push the checkbox widget out a bit to accommodate a clearer label? >> + screen->widgets.kexec_method_cb = widget_new_checkbox(set, 0, 0, >> + screen->kexec_method); > > I think you can just use config->kexec_method directly instead of adding > the screen->kexec_method field. > >> + >> if (config->ipmi_bootdev) { >> label = talloc_asprintf(screen, >> _("%s IPMI boot option: %s"), >> @@ -1170,6 +1191,7 @@ static void config_screen_draw(struct config_screen *screen, >> config_screen_setup_empty(screen); >> } else { >> screen->net_conf_type = find_net_conf_type(config); >> + screen->kexec_method = config->kexec_method; >> config_screen_setup_widgets(screen, config, sysinfo); >> config_screen_layout_widgets(screen); >> } >
On Fri, 2017-03-24 at 13:46 -0500, Eric Richter wrote: > > On 03/23/2017 09:22 PM, Samuel Mendoza-Jonas wrote: > > On Thu, 2017-03-23 at 11:46 -0500, Eric Richter wrote: > > > This patch adds an option to the system configuration menu that if > > > checked, enables the use of kexec_file_load. > > > > > > Signed-off-by: Eric Richter <erichte@linux.vnet.ibm.com> > > > --- > > > ui/ncurses/nc-config.c | 24 +++++++++++++++++++++++- > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/ui/ncurses/nc-config.c b/ui/ncurses/nc-config.c > > > index 8349629..1e9524b 100644 > > > --- a/ui/ncurses/nc-config.c > > > +++ b/ui/ncurses/nc-config.c > > > @@ -33,7 +33,7 @@ > > > #include "nc-config.h" > > > #include "nc-widgets.h" > > > > > > -#define N_FIELDS 48 > > > +#define N_FIELDS 50 > > > > > > extern struct help_text config_help_text; > > > > > > @@ -68,6 +68,7 @@ struct config_screen { > > > bool autoboot_enabled; > > > bool ipmi_override; > > > bool net_override; > > > + bool kexec_method; > > > > > > struct { > > > struct nc_widget_label *autoboot_l; > > > @@ -82,6 +83,9 @@ struct config_screen { > > > struct nc_widget_label *timeout_l; > > > struct nc_widget_label *timeout_help_l; > > > > > > + struct nc_widget_label *kexec_method_l; > > > + struct nc_widget_checkbox *kexec_method_cb; > > > + > > > struct nc_widget_label *ipmi_type_l; > > > struct nc_widget_label *ipmi_clear_l; > > > struct nc_widget_checkbox *ipmi_clear_cb; > > > @@ -256,6 +260,8 @@ static int screen_process_form(struct config_screen *screen) > > > config->autoboot_timeout_sec = x; > > > } > > > > > > + config->kexec_method = widget_checkbox_get_value(screen->widgets.kexec_method_cb); > > > + > > > if (screen->ipmi_override) > > > if (widget_checkbox_get_value(screen->widgets.ipmi_clear_cb)) > > > config->ipmi_bootdev = IPMI_BOOTDEV_INVALID; > > > @@ -490,6 +496,16 @@ static void config_screen_layout_widgets(struct config_screen *screen) > > > } else > > > y += 1; > > > > > > + > > > + wl = widget_label_base(screen->widgets.kexec_method_l); > > > + widget_set_visible(wl, true); > > > + widget_move(wl, y, screen->label_x); > > > + > > > + wf = widget_checkbox_base(screen->widgets.kexec_method_cb); > > > + widget_set_visible(wf, true); > > > + widget_move(wf, y, screen->field_x); > > > + y += 1; > > > + > > > if (screen->ipmi_override) { > > > wl = widget_label_base(screen->widgets.ipmi_type_l); > > > widget_set_visible(wl, true); > > > @@ -927,6 +943,11 @@ static void config_screen_setup_widgets(struct config_screen *screen, > > > widget_textbox_set_fixed_size(screen->widgets.timeout_f); > > > widget_textbox_set_validator_integer(screen->widgets.timeout_f, 0, 999); > > > > > > + screen->widgets.kexec_method_l = widget_new_label(set, 0, 0, > > > + _("Use file_load:")); > > > > This probably needs a more descriptive label, I don't think it's clear > > what "file load" means if you don't already have kexec on you mind. > > Maybe "Use secure kexec", or something similar? > > > > The string "Use file_load:" while unspecific, is exactly the length of > space before the widgets are on the right. "Use secure kexec:" gets cut > off by the widget, if that stays aligned with the others above/below. > > Would it be preferable to push the checkbox widget out a bit to > accommodate a clearer label? Ah yes, I wasn't thinking about the width limit. Perhaps just "Secure kexec:" would be sufficient to convey what the option enables without making the label too wide. > > > > + screen->widgets.kexec_method_cb = widget_new_checkbox(set, 0, 0, > > > + screen->kexec_method); > > > > I think you can just use config->kexec_method directly instead of adding > > the screen->kexec_method field. > > > > > + > > > if (config->ipmi_bootdev) { > > > label = talloc_asprintf(screen, > > > _("%s IPMI boot option: %s"), > > > @@ -1170,6 +1191,7 @@ static void config_screen_draw(struct config_screen *screen, > > > config_screen_setup_empty(screen); > > > } else { > > > screen->net_conf_type = find_net_conf_type(config); > > > + screen->kexec_method = config->kexec_method; > > > config_screen_setup_widgets(screen, config, sysinfo); > > > config_screen_layout_widgets(screen); > > > } > >
Hi Eric, > This patch adds an option to the system configuration menu that if > checked, enables the use of kexec_file_load. If possible, I'd like to avoid adding a config option for this. It's pretty opaque to the user, and should really be specified by the system integrator, not the end user. That is, unless I'm missing something about why one would be chosen over the other. Shouldn't we always use kexec_file_load if the kernel (and kexec) has support for it? Cheers, Jeremy
On 05/04/2017 09:58 PM, Jeremy Kerr wrote: > Hi Eric, > >> This patch adds an option to the system configuration menu that if >> checked, enables the use of kexec_file_load. > > If possible, I'd like to avoid adding a config option for this. It's > pretty opaque to the user, and should really be specified by the system > integrator, not the end user. I think it is fair to avoid having the user worry about this. In most cases, this option should never be touched anyway. > That is, unless I'm missing something about why one would be chosen over > the other. Shouldn't we always use kexec_file_load if the kernel (and > kexec) has support for it? There are a few differences between the two I can think of, where having both might be useful: 1. kexec_file_load does not support passing a device tree as an argument 2. Currently on x86, kexec_file_load always verifies the signature for kernels if the option is enabled in kconfig, regardless of secure boot mode. In other words, if secure mode is disabled (for debug, development, etc), the target kernel will still need to pass signature checks. Note: Signature verification is currently unimplemented on Power, so this could change. Now that I got the devil's advocate argument out of the way, I don't think either case is actually a problem. I am not sure what #1 is used for, and if whatever uses it couldn't be implemented elsewhere. Secondly, #2 is fairly edge case, where the user would probably need to reflash anyway. So, I think it makes sense for it to be a compile-time option. I'll write up a patch for that shortly. Thanks, Eric Richter > Cheers, > > > Jeremy >
diff --git a/ui/ncurses/nc-config.c b/ui/ncurses/nc-config.c index 8349629..1e9524b 100644 --- a/ui/ncurses/nc-config.c +++ b/ui/ncurses/nc-config.c @@ -33,7 +33,7 @@ #include "nc-config.h" #include "nc-widgets.h" -#define N_FIELDS 48 +#define N_FIELDS 50 extern struct help_text config_help_text; @@ -68,6 +68,7 @@ struct config_screen { bool autoboot_enabled; bool ipmi_override; bool net_override; + bool kexec_method; struct { struct nc_widget_label *autoboot_l; @@ -82,6 +83,9 @@ struct config_screen { struct nc_widget_label *timeout_l; struct nc_widget_label *timeout_help_l; + struct nc_widget_label *kexec_method_l; + struct nc_widget_checkbox *kexec_method_cb; + struct nc_widget_label *ipmi_type_l; struct nc_widget_label *ipmi_clear_l; struct nc_widget_checkbox *ipmi_clear_cb; @@ -256,6 +260,8 @@ static int screen_process_form(struct config_screen *screen) config->autoboot_timeout_sec = x; } + config->kexec_method = widget_checkbox_get_value(screen->widgets.kexec_method_cb); + if (screen->ipmi_override) if (widget_checkbox_get_value(screen->widgets.ipmi_clear_cb)) config->ipmi_bootdev = IPMI_BOOTDEV_INVALID; @@ -490,6 +496,16 @@ static void config_screen_layout_widgets(struct config_screen *screen) } else y += 1; + + wl = widget_label_base(screen->widgets.kexec_method_l); + widget_set_visible(wl, true); + widget_move(wl, y, screen->label_x); + + wf = widget_checkbox_base(screen->widgets.kexec_method_cb); + widget_set_visible(wf, true); + widget_move(wf, y, screen->field_x); + y += 1; + if (screen->ipmi_override) { wl = widget_label_base(screen->widgets.ipmi_type_l); widget_set_visible(wl, true); @@ -927,6 +943,11 @@ static void config_screen_setup_widgets(struct config_screen *screen, widget_textbox_set_fixed_size(screen->widgets.timeout_f); widget_textbox_set_validator_integer(screen->widgets.timeout_f, 0, 999); + screen->widgets.kexec_method_l = widget_new_label(set, 0, 0, + _("Use file_load:")); + screen->widgets.kexec_method_cb = widget_new_checkbox(set, 0, 0, + screen->kexec_method); + if (config->ipmi_bootdev) { label = talloc_asprintf(screen, _("%s IPMI boot option: %s"), @@ -1170,6 +1191,7 @@ static void config_screen_draw(struct config_screen *screen, config_screen_setup_empty(screen); } else { screen->net_conf_type = find_net_conf_type(config); + screen->kexec_method = config->kexec_method; config_screen_setup_widgets(screen, config, sysinfo); config_screen_layout_widgets(screen); }
This patch adds an option to the system configuration menu that if checked, enables the use of kexec_file_load. Signed-off-by: Eric Richter <erichte@linux.vnet.ibm.com> --- ui/ncurses/nc-config.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)