[3/4] ui/ncurses: Add system config checkbox to enable kexec_file load
diff mbox

Message ID 1490287580-21725-4-git-send-email-erichte@linux.vnet.ibm.com
State Superseded
Headers show

Commit Message

Eric Richter March 23, 2017, 4:46 p.m. UTC
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(-)

Comments

Samuel Mendoza-Jonas March 24, 2017, 2:22 a.m. UTC | #1
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);
>  	}
Eric Richter March 24, 2017, 6:46 p.m. UTC | #2
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);
>>  	}
>
Samuel Mendoza-Jonas March 29, 2017, 12:20 a.m. UTC | #3
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);
> > >  	}
> 
>
Jeremy Kerr May 5, 2017, 2:58 a.m. UTC | #4
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
Eric Richter May 5, 2017, 8:56 p.m. UTC | #5
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
>

Patch
diff mbox

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);
 	}