From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0F06CC433EF for ; Wed, 9 Mar 2022 14:34:54 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 688CB8397F; Wed, 9 Mar 2022 15:34:52 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="TFQ8jkrd"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1F51983982; Wed, 9 Mar 2022 15:34:50 +0100 (CET) Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 1FF6E8393E for ; Wed, 9 Mar 2022 15:34:46 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ej1-x635.google.com with SMTP id d10so5426365eje.10 for ; Wed, 09 Mar 2022 06:34:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=Eolt30URTJ2b1uIpXacz5U/87PbslBn3TJaVPWIiQZA=; b=TFQ8jkrdtSK1X+RVl7V+oXIMKctkFkZgHIDtYgbRLvrWEx7W9PoTWx7teqmbswofot zKyCRAyUFIeTx9+xIoWwDk1jL9FYlz75jCkYA5b2GSvBpskU5AIuWleTI8tqcH4V0LRw zE9Nn+AOZPHh3XJCKCMqxshugNrhs5JmG/XFVl/0mVuMPWjBgz6fVbvwl6JXfg2MTgmV NLc3U/lRexqd2NpXmZdmYJ1+h2KvTyBrZQAK5fw2OXKFGzGvdpW62j/UvLmpCYvGsGtT jsSTFyB5eAniTyAHKNHPuqIIZMXsqb32aas7ogZKw50/ikY/xf4jg5HX12Tu3be9nV7J qCkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=Eolt30URTJ2b1uIpXacz5U/87PbslBn3TJaVPWIiQZA=; b=ySYuGSNCJ4G/wwAYpDGJu97qMo9tqP9wDDa4BDG3B1GAjgzYjXJsvoRX2jDvY1CEx8 R0QebS141rxvTdArdAvIVdT0uwSqBhhBSpuDkbXNGnZf85KWcJ/mf+a6M2pcH4C0v2bg Lu7XXlERBok+e1ZWTTLzCDmi4fhxL0Ydl5G/IlzBDSaQNA5Y7LXoEBbHMA/+t/abxMpG 8/Lssl7nT3N33S3FjyFzPp8MriZq8wkXpbtiCGVS6ocTe8v3aKgXy68h3gh89TCQU/80 e04rbLA8iOOtTeEZWwrOgE0puxIf4ifUu1XmMWplfa3FNhAl2RaikR2Dr27dVVZ3ZQIG w4RQ== X-Gm-Message-State: AOAM533lI7z2Ew+9c4oz99PjkZfeoWEKs6ubxMkiL0vxfqGsoo/cBfEK jtpg1uGMEi8HU9sM4qTQkBhWow== X-Google-Smtp-Source: ABdhPJz7nwg4X3wItpqcPVtKRxL4UNzIyuRfl88qfHCJavEFxJxXpuk7eh1L2pOXg+qRKKrnqAPWIQ== X-Received: by 2002:a17:906:7210:b0:6d6:7881:1483 with SMTP id m16-20020a170906721000b006d678811483mr1040ejk.227.1646836485262; Wed, 09 Mar 2022 06:34:45 -0800 (PST) Received: from hades (athedsl-4461682.home.otenet.gr. [94.71.4.98]) by smtp.gmail.com with ESMTPSA id i22-20020a170906251600b006d6d9081f46sm807280ejb.150.2022.03.09.06.34.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 09 Mar 2022 06:34:44 -0800 (PST) Date: Wed, 9 Mar 2022 16:34:42 +0200 From: Ilias Apalodimas To: Masahisa Kojima Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Simon Glass , Takahiro Akashi , Francois Ozog , Mark Kettenis Subject: Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries Message-ID: References: <20220308140745.26180-1-masahisa.kojima@linaro.org> <20220308140745.26180-3-masahisa.kojima@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220308140745.26180-3-masahisa.kojima@linaro.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hi Kojima-san On Tue, Mar 08, 2022 at 11:07:45PM +0900, Masahisa Kojima wrote: > This commit adds the UEFI related menu entries and > distro_boot entries into the bootmenu. > > For UEFI, user can select which UEFI "Boot####" option > to execute, call UEFI bootmgr and UEFI boot variable > maintenance menu. UEFI bootmgr entry is required to > correctly handle "BootNext" variable. Hmm why? (some more info further down) > > For distro_boot, user can select the boot device > included in "boot_targets" u-boot environment variable. > > The menu example is as follows. > > *** U-Boot Boot Menu *** > > Boot 1. kernel (bootmenu_0) > Boot 2. kernel (bootmenu_1) > Reset board (bootmenu_2) > debian (BOOT0000) > ubuntu (BOOT0001) > UEFI Boot Manager > usb0 > scsi0 > virtio0 > dhcp > UEFI Boot Manager Maintenance > U-Boot console > > Press UP/DOWN to move, ENTER to select, ESC/CTRL+C to quit > > Signed-off-by: Masahisa Kojima > --- > Changes in v3: > - newly created > > cmd/bootmenu.c | 268 +++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 259 insertions(+), 9 deletions(-) > > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c > index 409ef9a848..a8dc50dcaa 100644 > --- a/cmd/bootmenu.c > +++ b/cmd/bootmenu.c > @@ -3,9 +3,12 @@ > * (C) Copyright 2011-2013 Pali Rohár > */ > > +#include > #include > #include > #include > +#include > +#include > #include > #include > #include > @@ -24,11 +27,20 @@ > */ > #define MAX_ENV_SIZE (9 + 2 + 1) > > +enum boot_type { > + BOOT_TYPE_NONE = 0, > + BOOT_TYPE_BOOTMENU, > + BOOT_TYPE_UEFI, > + BOOT_TYPE_DISTRO_BOOT, > +}; > + > struct bootmenu_entry { > unsigned short int num; /* unique number 0 .. MAX_COUNT */ > char key[3]; /* key identifier of number */ > - char *title; /* title of entry */ > + u16 *title; /* title of entry */ > char *command; /* hush command of entry */ > + enum boot_type type; > + u16 bootorder; > struct bootmenu_data *menu; /* this bootmenu */ > struct bootmenu_entry *next; /* next menu entry (num+1) */ > }; > @@ -75,7 +87,12 @@ static void bootmenu_print_entry(void *data) > if (reverse) > puts(ANSI_COLOR_REVERSE); > > - puts(entry->title); > + if (entry->type == BOOT_TYPE_BOOTMENU) > + printf("%ls (bootmenu_%d)", entry->title, entry->bootorder); > + else if (entry->type == BOOT_TYPE_UEFI) > + printf("%ls (BOOT%04X)", entry->title, entry->bootorder); > + else > + printf("%ls", entry->title); > > if (reverse) > puts(ANSI_COLOR_RESET); > @@ -87,6 +104,10 @@ static void bootmenu_autoboot_loop(struct bootmenu_data *menu, > int i, c; > > if (menu->delay > 0) { > + /* flush input */ > + while (tstc()) > + getchar(); > + > printf(ANSI_CURSOR_POSITION, menu->count + 5, 1); > printf(" Hit any key to stop autoboot: %2d ", menu->delay); > } > @@ -300,6 +321,8 @@ static struct bootmenu_data *bootmenu_create(int delay) > menu->active = (int)simple_strtol(default_str, NULL, 10); > > while ((option = bootmenu_getoption(i))) { > + u16 *buf; > + > sep = strchr(option, '='); > if (!sep) { > printf("Invalid bootmenu entry: %s\n", option); > @@ -311,13 +334,13 @@ static struct bootmenu_data *bootmenu_create(int delay) > goto cleanup; > > len = sep-option; > - entry->title = malloc(len + 1); > + buf = calloc(1, (len + 1) * sizeof(u16)); > + entry->title = buf; > if (!entry->title) { > free(entry); > goto cleanup; > } > - memcpy(entry->title, option, len); > - entry->title[len] = 0; > + utf8_utf16_strncpy(&buf, option, len); > > len = strlen(sep + 1); > entry->command = malloc(len + 1); > @@ -333,6 +356,190 @@ static struct bootmenu_data *bootmenu_create(int delay) > > entry->num = i; > entry->menu = menu; > + entry->type = BOOT_TYPE_BOOTMENU; > + entry->bootorder = i; > + entry->next = NULL; > + > + if (!iter) > + menu->first = entry; > + else > + iter->next = entry; > + > + iter = entry; > + ++i; > + > + if (i == MAX_COUNT - 1) > + break; > + } > + > +{ > + u16 *bootorder; > + efi_status_t ret; > + unsigned short j; > + efi_uintn_t num, size; > + void *load_option; > + struct efi_load_option lo; > + u16 varname[] = u"Boot####"; > + > + /* Initialize EFI drivers */ > + ret = efi_init_obj_list(); > + if (ret != EFI_SUCCESS) { > + log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n", > + ret & ~EFI_ERROR_MASK); > + goto cleanup; > + } > + > + bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size); > + if (!bootorder) > + goto bootmgr; > + > + num = size / sizeof(u16); > + for (j = 0; j < num; j++) { > + entry = malloc(sizeof(struct bootmenu_entry)); > + if (!entry) > + goto cleanup; > + > + efi_create_indexed_name(varname, sizeof(varname), > + "Boot", bootorder[j]); > + load_option = efi_get_var(varname, &efi_global_variable_guid, &size); > + if (!load_option) > + continue; > + > + ret = efi_deserialize_load_option(&lo, load_option, &size); > + if (ret != EFI_SUCCESS) { > + log_warning("Invalid load option for %ls\n", varname); > + free(load_option); > + continue; > + } > + > + if (lo.attributes & LOAD_OPTION_ACTIVE) { > + char *command; > + int command_size; > + > + entry->title = u16_strdup(lo.label); > + if (!entry->title) { > + free(load_option); > + free(entry); > + goto cleanup; > + } > + command_size = strlen("bootefi bootindex XXXX") + 1; As we commented on patch 2/2 you can simply set BootNext variable here and fire up the efibootmgr > + command = calloc(1, command_size); > + if (!command) { > + free(entry->title); > + free(load_option); > + free(entry); > + goto cleanup; > + } > + snprintf(command, command_size, "bootefi bootindex %X", bootorder[j]); > + entry->command = command; > + sprintf(entry->key, "%d", i); > + entry->num = i; > + entry->menu = menu; > + entry->type = BOOT_TYPE_UEFI; > + entry->bootorder = bootorder[j]; > + entry->next = NULL; > + > + if (!iter) > + menu->first = entry; > + else > + iter->next = entry; > + > + iter = entry; > + ++i; > + } > + > + if (i == MAX_COUNT - 1) > + break; > + } > + free(bootorder); > +} > + > +bootmgr: Why do we need an entire menu entry if the bootorder is not defined? Currently there's no logic in the efibootmgr to look for the standard filenames in the ESP (eg bootarm.efi, bootaa64.efi etc) if no boot option is defined. Instead this is implement in distro_bootcmd. I was thinking of something along the lines of: 1. bootmenu comes up 2. We read all the Boot#### variables that are defined and add them on the menu 2a. If the user doesn't explicitly choose a boot option we run 'bootefi bootmgr' 2b. If the user does select a different option we set BootNext and still run 'bootefi bootmgr' 3. If there's not Boot#### option defined, we should in the future add the functionality of searching for bootaa64.efi etc in ESP and still just launch the efi bootmgr > + /* Add UEFI Boot Manager entry if available */ > + if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) { > + if (i <= MAX_COUNT - 1) { > + entry = malloc(sizeof(struct bootmenu_entry)); > + if (!entry) > + goto cleanup; > + > + entry->title = u16_strdup(u"UEFI Boot Manager"); > + if (!entry->title) { > + free(entry); > + goto cleanup; > + } > + > + entry->command = strdup("bootefi bootmgr"); > + if (!entry->command) { > + free(entry->title); > + free(entry); > + goto cleanup; > + } > + > + sprintf(entry->key, "%d", i); > + > + entry->num = i; > + entry->menu = menu; > + entry->type = BOOT_TYPE_NONE; > + entry->next = NULL; > + > + if (!iter) > + menu->first = entry; > + else > + iter->next = entry; > + > + iter = entry; > + ++i; > + } > + } > + > +{ > + char *p; > + char *token; > + char *boot_targets; > + int len; > + > + /* list the distro boot "boot_targets" */ > + boot_targets = env_get("boot_targets"); > + if (!boot_targets) > + goto exit_boot_targets; > + > + len = strlen(boot_targets); > + p = calloc(1, len + 1); > + strncpy(p, boot_targets, len); > + > + token = strtok(p, " "); > + > + do { > + u16 *buf; > + char *command; > + int command_size; > + > + entry = malloc(sizeof(struct bootmenu_entry)); > + if (!entry) > + goto cleanup; > + > + len = strlen(token); > + buf = calloc(1, (len + 1) * sizeof(u16)); > + entry->title = buf; > + if (!entry->title) { > + free(entry); > + goto cleanup; > + } > + utf8_utf16_strncpy(&buf, token,len); > + sprintf(entry->key, "%d", i); > + entry->num = i; > + entry->menu = menu; > + > + command_size = strlen("run bootcmd_") + len + 1; I think we are better of here with a sizeof() instead of strlen since the 'run bootcmd_' string is not expected to change > + command = calloc(1, command_size); > + if (!command) { > + free(entry->title); > + free(entry); > + goto cleanup; > + } > + snprintf(command, command_size, "run bootcmd_%s", token); > + entry->command = command; > + entry->type = BOOT_TYPE_DISTRO_BOOT; > entry->next = NULL; > > if (!iter) > @@ -345,6 +552,48 @@ static struct bootmenu_data *bootmenu_create(int delay) > > if (i == MAX_COUNT - 1) > break; > + > + token = strtok(NULL, " "); > + } while (token); > + > + free(p); > +} > + > +exit_boot_targets: > + > + /* Add UEFI Boot Manager Maintenance entry */ > + if (i <= MAX_COUNT - 1) { > + entry = malloc(sizeof(struct bootmenu_entry)); > + if (!entry) > + goto cleanup; > + > + entry->title = u16_strdup(u"UEFI Boot Manager Maintenance"); > + if (!entry->title) { > + free(entry); > + goto cleanup; > + } > + > + entry->command = strdup("echo TODO: Not implemented"); > + if (!entry->command) { > + free(entry->title); > + free(entry); > + goto cleanup; > + } Any idea of how we'll tackle that? We could export the efibootmgr functions that deal with this and use them on the edit menu ? > + > + sprintf(entry->key, "%d", i); > + > + entry->num = i; > + entry->menu = menu; > + entry->type = BOOT_TYPE_NONE; > + entry->next = NULL; > + > + if (!iter) > + menu->first = entry; > + else > + iter->next = entry; > + > + iter = entry; > + ++i; > } > > /* Add U-Boot console entry at the end */ > @@ -353,7 +602,7 @@ static struct bootmenu_data *bootmenu_create(int delay) > if (!entry) > goto cleanup; > > - entry->title = strdup("U-Boot console"); > + entry->title = u16_strdup(u"U-Boot console"); Can we add an option to prohibit the user from going back to the console? > if (!entry->title) { > free(entry); > goto cleanup; > @@ -370,6 +619,7 @@ static struct bootmenu_data *bootmenu_create(int delay) > > entry->num = i; > entry->menu = menu; > + entry->type = BOOT_TYPE_NONE; > entry->next = NULL; > > if (!iter) > @@ -427,7 +677,7 @@ static void bootmenu_show(int delay) > { > int init = 0; > void *choice = NULL; > - char *title = NULL; > + u16 *title = NULL; > char *command = NULL; > struct menu *menu; > struct bootmenu_data *bootmenu; > @@ -478,7 +728,7 @@ static void bootmenu_show(int delay) > > if (menu_get_choice(menu, &choice)) { > iter = choice; > - title = strdup(iter->title); > + title = u16_strdup(iter->title); > command = strdup(iter->command); > } > > @@ -493,7 +743,7 @@ cleanup: > } > > if (title && command) { > - debug("Starting entry '%s'\n", title); > + debug("Starting entry '%ls'\n", title); > free(title); > run_command(command, 0); > free(command); > -- > 2.17.1 > Cheers /Ilias