| Message ID | 20230105025855.16936-4-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | eficonfig: add vertical scroll support and refactoring | expand |
On 1/5/23 03:58, Masahisa Kojima wrote: > The current eficonfig menu does not support vertical scroll, > so it can not display the menu entries greater than > the console row size. > > This commit add the vertial scroll support. > The console size is retrieved by > SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then > calculates the row size for menu entry by subtracting > menu header and description row size from the console row size. > "start" and "end" are added in the efimenu structure. > "start" keeps the menu entry index at the top, "end" keeps > the bottom menu entry index. item_data_print() menu function > only draws the menu entry between "start" and "end". > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> Hello Masahisa, unfortunately this does not work. I create a boot.scr script to create 100 boot options: #!/bin/bash a=0 while [ $a -lt 100 ]; do b="0000$a" c=${b: -4} echo "efidebug boot add -b $c label$c host 0:1 dtbdump.efi" >> boot.txt a=$(( $a + 1 )) done mkimage -T script -n foo -d boot.txt boot.scr rm boot.txt In sandbox_defconfig: host bind 0 ../sandbox.img host load 0:1 $kernel_addr_r boot.scr source $kernel_addr_r efidebug boot dump Now I have 100 boot options. I start 'eficonfig'. Whenever I push the up or down key this immediately leads to exiting the command and I am back at the console prompt. Best regards Heinrich > --- > No update since v1 > > cmd/eficonfig.c | 79 ++++++++++++++++++++++++++++++++++++-------- > include/efi_config.h | 4 +++ > include/efi_loader.h | 1 + > 3 files changed, 70 insertions(+), 14 deletions(-) > > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c > index 6d5ebc4055..bf765a239b 100644 > --- a/cmd/eficonfig.c > +++ b/cmd/eficonfig.c > @@ -29,8 +29,13 @@ static const char *eficonfig_change_boot_order_desc = > " Press SPACE to activate or deactivate the entry\n" > " Select [Save] to complete, ESC/CTRL+C to quit"; > > +static struct efi_simple_text_output_protocol *cout; > +static int avail_row; > + > #define EFICONFIG_DESCRIPTION_MAX 32 > #define EFICONFIG_OPTIONAL_DATA_MAX 64 > +#define EFICONFIG_MENU_HEADER_ROW_NUM 3 > +#define EFICONFIG_MENU_DESC_ROW_NUM 5 > > /** > * struct eficonfig_filepath_info - structure to be used to store file path > @@ -156,18 +161,16 @@ void eficonfig_print_entry(void *data) > struct eficonfig_entry *entry = data; > bool reverse = (entry->efi_menu->active == entry->num); > > - /* TODO: support scroll or page for many entries */ > + if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num) > + return; > > - /* > - * Move cursor to line where the entry will be drawn (entry->num) > - * First 3 lines(menu header) + 1 empty line > - */ > - printf(ANSI_CURSOR_POSITION, entry->num + 4, 7); > + printf(ANSI_CURSOR_POSITION, (entry->num - entry->efi_menu->start) + > + EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7); > > if (reverse) > puts(ANSI_COLOR_REVERSE); > > - printf("%s", entry->title); > + printf(ANSI_CLEAR_LINE "%s", entry->title); > > if (reverse) > puts(ANSI_COLOR_RESET); > @@ -190,8 +193,8 @@ void eficonfig_display_statusline(struct menu *m) > ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION > "%s" > ANSI_CLEAR_LINE_TO_END, > - 1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1, > - entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc); > + 1, 1, entry->efi_menu->menu_header, avail_row + 4, 1, > + avail_row + 5, 1, entry->efi_menu->menu_desc); > } > > /** > @@ -213,13 +216,23 @@ char *eficonfig_choice_entry(void *data) > > switch (key) { > case KEY_UP: > - if (efi_menu->active > 0) > + if (efi_menu->active > 0) { > --efi_menu->active; > + if (efi_menu->start > efi_menu->active) { > + efi_menu->start--; > + efi_menu->end--; > + } > + } > /* no menu key selected, regenerate menu */ > return NULL; > case KEY_DOWN: > - if (efi_menu->active < efi_menu->count - 1) > + if (efi_menu->active < efi_menu->count - 1) { > ++efi_menu->active; > + if (efi_menu->end < efi_menu->active) { > + efi_menu->start++; > + efi_menu->end++; > + } > + } > /* no menu key selected, regenerate menu */ > return NULL; > case KEY_SELECT: > @@ -399,6 +412,8 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu, > > efi_menu->delay = -1; > efi_menu->active = 0; > + efi_menu->start = 0; > + efi_menu->end = avail_row - 1; > > if (menu_header) { > efi_menu->menu_header = strdup(menu_header); > @@ -1865,7 +1880,11 @@ static void eficonfig_print_change_boot_order_entry(void *data) > struct eficonfig_entry *entry = data; > bool reverse = (entry->efi_menu->active == entry->num); > > - printf(ANSI_CURSOR_POSITION, entry->num + 4, 7); > + if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num) > + return; > + > + printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE, > + (entry->num - entry->efi_menu->start) + EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7); > > if (reverse) > puts(ANSI_COLOR_REVERSE); > @@ -1916,8 +1935,13 @@ char *eficonfig_choice_change_boot_order(void *data) > } > fallthrough; > case KEY_UP: > - if (efi_menu->active > 0) > + if (efi_menu->active > 0) { > --efi_menu->active; > + if (efi_menu->start > efi_menu->active) { > + efi_menu->start--; > + efi_menu->end--; > + } > + } > return NULL; > case KEY_MINUS: > if (efi_menu->active < efi_menu->count - 3) { > @@ -1933,11 +1957,20 @@ char *eficonfig_choice_change_boot_order(void *data) > list_add(&entry->list, &tmp->list); > > ++efi_menu->active; > + if (efi_menu->end < efi_menu->active) { > + efi_menu->start++; > + efi_menu->end++; > + } > } > return NULL; > case KEY_DOWN: > - if (efi_menu->active < efi_menu->count - 1) > + if (efi_menu->active < efi_menu->count - 1) { > ++efi_menu->active; > + if (efi_menu->end < efi_menu->active) { > + efi_menu->start++; > + efi_menu->end++; > + } > + } > return NULL; > case KEY_SELECT: > /* "Save" */ > @@ -2585,6 +2618,7 @@ static efi_status_t eficonfig_init(void) > efi_status_t ret = EFI_SUCCESS; > static bool init; > struct efi_handler *handler; > + unsigned long columns, rows; > > if (!init) { > ret = efi_search_protocol(efi_root, &efi_guid_text_input_protocol, &handler); > @@ -2595,6 +2629,23 @@ static efi_status_t eficonfig_init(void) > EFI_OPEN_PROTOCOL_GET_PROTOCOL); > if (ret != EFI_SUCCESS) > return ret; > + ret = efi_search_protocol(efi_root, &efi_guid_text_output_protocol, &handler); > + if (ret != EFI_SUCCESS) > + return ret; > + > + ret = efi_protocol_open(handler, (void **)&cout, efi_root, NULL, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL); > + if (ret != EFI_SUCCESS) > + return ret; > + > + cout->query_mode(cout, cout->mode->mode, &columns, &rows); > + avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM + > + EFICONFIG_MENU_DESC_ROW_NUM); > + if (avail_row <= 0) { > + eficonfig_print_msg("Console size is too small!"); > + return EFI_INVALID_PARAMETER; > + } > + /* TODO: Should we check the minimum column size? */ > } > > init = true; > diff --git a/include/efi_config.h b/include/efi_config.h > index cec5715f84..6a104e4b1d 100644 > --- a/include/efi_config.h > +++ b/include/efi_config.h > @@ -49,6 +49,8 @@ struct eficonfig_entry { > * @menu_header: menu header string > * @menu_desc: menu description string > * @list: menu entry list structure > + * @start: top menu index to draw > + * @end: bottom menu index to draw > */ > struct efimenu { > int delay; > @@ -57,6 +59,8 @@ struct efimenu { > char *menu_header; > const char *menu_desc; > struct list_head list; > + int start; > + int end; > }; > > /** > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 699176872d..91d8a5ef0c 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -328,6 +328,7 @@ extern const efi_guid_t efi_esrt_guid; > extern const efi_guid_t smbios_guid; > /*GUID of console */ > extern const efi_guid_t efi_guid_text_input_protocol; > +extern const efi_guid_t efi_guid_text_output_protocol; > > extern char __efi_runtime_start[], __efi_runtime_stop[]; > extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
On 1/14/23 11:06, Heinrich Schuchardt wrote: > On 1/5/23 03:58, Masahisa Kojima wrote: >> The current eficonfig menu does not support vertical scroll, >> so it can not display the menu entries greater than >> the console row size. >> >> This commit add the vertial scroll support. >> The console size is retrieved by >> SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then >> calculates the row size for menu entry by subtracting >> menu header and description row size from the console row size. >> "start" and "end" are added in the efimenu structure. >> "start" keeps the menu entry index at the top, "end" keeps >> the bottom menu entry index. item_data_print() menu function >> only draws the menu entry between "start" and "end". >> >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > Hello Masahisa, > > unfortunately this does not work. > > I create a boot.scr script to create 100 boot options: > > #!/bin/bash > a=0 > while [ $a -lt 100 ]; do > b="0000$a" > c=${b: -4} > echo "efidebug boot add -b $c label$c host 0:1 dtbdump.efi" >> boot.txt > a=$(( $a + 1 )) > done > mkimage -T script -n foo -d boot.txt boot.scr > rm boot.txt > > In sandbox_defconfig: > > host bind 0 ../sandbox.img > host load 0:1 $kernel_addr_r boot.scr > source $kernel_addr_r > efidebug boot dump > > Now I have 100 boot options. > > I start 'eficonfig'. Whenever I push the up or down key this immediately > leads to exiting the command and I am back at the console prompt. > > Best regards > > Heinrich > The problem seems to be that the space for variables is exhausted. The user should see an error message in this case. With CONFIG_EFI_VAR_BUF_SIZE=65536 the navigation works but I only see entries label0000 - label0095. Where are the other entries? I have been adding CONFIG_LEGACY_IMAGE_FORMAT=y to read the boot.scr file. Best regards Heinrich
On Sat, 14 Jan 2023 at 19:26, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 1/14/23 11:06, Heinrich Schuchardt wrote: > > On 1/5/23 03:58, Masahisa Kojima wrote: > >> The current eficonfig menu does not support vertical scroll, > >> so it can not display the menu entries greater than > >> the console row size. > >> > >> This commit add the vertial scroll support. > >> The console size is retrieved by > >> SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then > >> calculates the row size for menu entry by subtracting > >> menu header and description row size from the console row size. > >> "start" and "end" are added in the efimenu structure. > >> "start" keeps the menu entry index at the top, "end" keeps > >> the bottom menu entry index. item_data_print() menu function > >> only draws the menu entry between "start" and "end". > >> > >> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > > Hello Masahisa, > > > > unfortunately this does not work. > > > > I create a boot.scr script to create 100 boot options: > > > > #!/bin/bash > > a=0 > > while [ $a -lt 100 ]; do > > b="0000$a" > > c=${b: -4} > > echo "efidebug boot add -b $c label$c host 0:1 dtbdump.efi" >> boot.txt > > a=$(( $a + 1 )) > > done > > mkimage -T script -n foo -d boot.txt boot.scr > > rm boot.txt > > > > In sandbox_defconfig: > > > > host bind 0 ../sandbox.img > > host load 0:1 $kernel_addr_r boot.scr > > source $kernel_addr_r > > efidebug boot dump > > > > Now I have 100 boot options. > > > > I start 'eficonfig'. Whenever I push the up or down key this immediately > > leads to exiting the command and I am back at the console prompt. > > > > Best regards > > > > Heinrich > > > > The problem seems to be that the space for variables is exhausted. > The user should see an error message in this case. I will check the error cases and add error messages. I will send a fix as a separate series. > > With CONFIG_EFI_VAR_BUF_SIZE=65536 the navigation works but I only see > entries label0000 - label0095. Where are the other entries? Current eficonfig menu can have at most 99 entries. I guess the following menu entries appears in your environment. - 96 entries (label0000 - label0095) - 2 entries ("host 0:1" and "host 0:2" - 1 entry ("Quit") The current maximum number of menu entries(99) is debatable, I think the menu should have a predefined maximum number. Thank you for your review and testing. Best Regards, Masahisa Kojima > > I have been adding CONFIG_LEGACY_IMAGE_FORMAT=y to read the boot.scr file. > > Best regards > > Heinrich
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 6d5ebc4055..bf765a239b 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -29,8 +29,13 @@ static const char *eficonfig_change_boot_order_desc = " Press SPACE to activate or deactivate the entry\n" " Select [Save] to complete, ESC/CTRL+C to quit"; +static struct efi_simple_text_output_protocol *cout; +static int avail_row; + #define EFICONFIG_DESCRIPTION_MAX 32 #define EFICONFIG_OPTIONAL_DATA_MAX 64 +#define EFICONFIG_MENU_HEADER_ROW_NUM 3 +#define EFICONFIG_MENU_DESC_ROW_NUM 5 /** * struct eficonfig_filepath_info - structure to be used to store file path @@ -156,18 +161,16 @@ void eficonfig_print_entry(void *data) struct eficonfig_entry *entry = data; bool reverse = (entry->efi_menu->active == entry->num); - /* TODO: support scroll or page for many entries */ + if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num) + return; - /* - * Move cursor to line where the entry will be drawn (entry->num) - * First 3 lines(menu header) + 1 empty line - */ - printf(ANSI_CURSOR_POSITION, entry->num + 4, 7); + printf(ANSI_CURSOR_POSITION, (entry->num - entry->efi_menu->start) + + EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7); if (reverse) puts(ANSI_COLOR_REVERSE); - printf("%s", entry->title); + printf(ANSI_CLEAR_LINE "%s", entry->title); if (reverse) puts(ANSI_COLOR_RESET); @@ -190,8 +193,8 @@ void eficonfig_display_statusline(struct menu *m) ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION "%s" ANSI_CLEAR_LINE_TO_END, - 1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1, - entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc); + 1, 1, entry->efi_menu->menu_header, avail_row + 4, 1, + avail_row + 5, 1, entry->efi_menu->menu_desc); } /** @@ -213,13 +216,23 @@ char *eficonfig_choice_entry(void *data) switch (key) { case KEY_UP: - if (efi_menu->active > 0) + if (efi_menu->active > 0) { --efi_menu->active; + if (efi_menu->start > efi_menu->active) { + efi_menu->start--; + efi_menu->end--; + } + } /* no menu key selected, regenerate menu */ return NULL; case KEY_DOWN: - if (efi_menu->active < efi_menu->count - 1) + if (efi_menu->active < efi_menu->count - 1) { ++efi_menu->active; + if (efi_menu->end < efi_menu->active) { + efi_menu->start++; + efi_menu->end++; + } + } /* no menu key selected, regenerate menu */ return NULL; case KEY_SELECT: @@ -399,6 +412,8 @@ efi_status_t eficonfig_process_common(struct efimenu *efi_menu, efi_menu->delay = -1; efi_menu->active = 0; + efi_menu->start = 0; + efi_menu->end = avail_row - 1; if (menu_header) { efi_menu->menu_header = strdup(menu_header); @@ -1865,7 +1880,11 @@ static void eficonfig_print_change_boot_order_entry(void *data) struct eficonfig_entry *entry = data; bool reverse = (entry->efi_menu->active == entry->num); - printf(ANSI_CURSOR_POSITION, entry->num + 4, 7); + if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num) + return; + + printf(ANSI_CURSOR_POSITION ANSI_CLEAR_LINE, + (entry->num - entry->efi_menu->start) + EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7); if (reverse) puts(ANSI_COLOR_REVERSE); @@ -1916,8 +1935,13 @@ char *eficonfig_choice_change_boot_order(void *data) } fallthrough; case KEY_UP: - if (efi_menu->active > 0) + if (efi_menu->active > 0) { --efi_menu->active; + if (efi_menu->start > efi_menu->active) { + efi_menu->start--; + efi_menu->end--; + } + } return NULL; case KEY_MINUS: if (efi_menu->active < efi_menu->count - 3) { @@ -1933,11 +1957,20 @@ char *eficonfig_choice_change_boot_order(void *data) list_add(&entry->list, &tmp->list); ++efi_menu->active; + if (efi_menu->end < efi_menu->active) { + efi_menu->start++; + efi_menu->end++; + } } return NULL; case KEY_DOWN: - if (efi_menu->active < efi_menu->count - 1) + if (efi_menu->active < efi_menu->count - 1) { ++efi_menu->active; + if (efi_menu->end < efi_menu->active) { + efi_menu->start++; + efi_menu->end++; + } + } return NULL; case KEY_SELECT: /* "Save" */ @@ -2585,6 +2618,7 @@ static efi_status_t eficonfig_init(void) efi_status_t ret = EFI_SUCCESS; static bool init; struct efi_handler *handler; + unsigned long columns, rows; if (!init) { ret = efi_search_protocol(efi_root, &efi_guid_text_input_protocol, &handler); @@ -2595,6 +2629,23 @@ static efi_status_t eficonfig_init(void) EFI_OPEN_PROTOCOL_GET_PROTOCOL); if (ret != EFI_SUCCESS) return ret; + ret = efi_search_protocol(efi_root, &efi_guid_text_output_protocol, &handler); + if (ret != EFI_SUCCESS) + return ret; + + ret = efi_protocol_open(handler, (void **)&cout, efi_root, NULL, + EFI_OPEN_PROTOCOL_GET_PROTOCOL); + if (ret != EFI_SUCCESS) + return ret; + + cout->query_mode(cout, cout->mode->mode, &columns, &rows); + avail_row = rows - (EFICONFIG_MENU_HEADER_ROW_NUM + + EFICONFIG_MENU_DESC_ROW_NUM); + if (avail_row <= 0) { + eficonfig_print_msg("Console size is too small!"); + return EFI_INVALID_PARAMETER; + } + /* TODO: Should we check the minimum column size? */ } init = true; diff --git a/include/efi_config.h b/include/efi_config.h index cec5715f84..6a104e4b1d 100644 --- a/include/efi_config.h +++ b/include/efi_config.h @@ -49,6 +49,8 @@ struct eficonfig_entry { * @menu_header: menu header string * @menu_desc: menu description string * @list: menu entry list structure + * @start: top menu index to draw + * @end: bottom menu index to draw */ struct efimenu { int delay; @@ -57,6 +59,8 @@ struct efimenu { char *menu_header; const char *menu_desc; struct list_head list; + int start; + int end; }; /** diff --git a/include/efi_loader.h b/include/efi_loader.h index 699176872d..91d8a5ef0c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -328,6 +328,7 @@ extern const efi_guid_t efi_esrt_guid; extern const efi_guid_t smbios_guid; /*GUID of console */ extern const efi_guid_t efi_guid_text_input_protocol; +extern const efi_guid_t efi_guid_text_output_protocol; extern char __efi_runtime_start[], __efi_runtime_stop[]; extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
The current eficonfig menu does not support vertical scroll, so it can not display the menu entries greater than the console row size. This commit add the vertial scroll support. The console size is retrieved by SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then calculates the row size for menu entry by subtracting menu header and description row size from the console row size. "start" and "end" are added in the efimenu structure. "start" keeps the menu entry index at the top, "end" keeps the bottom menu entry index. item_data_print() menu function only draws the menu entry between "start" and "end". Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- No update since v1 cmd/eficonfig.c | 79 ++++++++++++++++++++++++++++++++++++-------- include/efi_config.h | 4 +++ include/efi_loader.h | 1 + 3 files changed, 70 insertions(+), 14 deletions(-)