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 06964C433EF for ; Thu, 10 Mar 2022 02:42:17 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 632FE839B7; Thu, 10 Mar 2022 03:42:15 +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="cYTKyr2r"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6E726839B7; Thu, 10 Mar 2022 03:42:13 +0100 (CET) Received: from mail-pg1-x52e.google.com (mail-pg1-x52e.google.com [IPv6:2607:f8b0:4864:20::52e]) (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 F3C0080F92 for ; Thu, 10 Mar 2022 03:42:07 +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=takahiro.akashi@linaro.org Received: by mail-pg1-x52e.google.com with SMTP id t187so3563236pgb.1 for ; Wed, 09 Mar 2022 18:42:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=hQf+fE0pbOQ6zrTIgOtoOs/xapgRhOelJzZb8Ap6hIc=; b=cYTKyr2rndN3b9gstnM31KlFptoVwBPS/FumLWJryKW/90ZNf9OPs/FoFq+oSVNNvT H0iYgRg8plSUy7JjiPh8O8ODiqBj83CJm3X7ASQMaQiioRudmhack1eApwtO4HbvirkJ GQKRXn2nHbzithGYqDumuh+uDQ6yy7644C9JW41NsI91lZ22azK42mnuLRrnRzNPQCJv BPeRj2ztqPZoQj78Sxyv29UBmxBIf7TJFghXCZFwoOCF7iz/J8aiNbc3tovL6s0upel7 9DpEn6S22ag6zVGy5RrJvQiI8zuT298DAvJvhYPalYmiRCS0h4Z54XvfcGLbSzLp6jc8 5ajA== 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:subject:message-id:mail-followup-to :references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=hQf+fE0pbOQ6zrTIgOtoOs/xapgRhOelJzZb8Ap6hIc=; b=D5T/ZN4ekzRaBBJ94bEuAJiSgKLX6Xl5DWfihAtGSuZIp5weMqqJt/f8SAtx1h9HLB +lxD4D6axEHgmGfFKX7vdaBI7oCdepG/WV+xRm8eY1TwWSYYox3+qXY5c4bQkJtg1LpJ 5kgumckj2yp+wO7pHh40PjgDgRyvswP4RCyNhwsFZPYDkKlKQT+xTKBEJFqFPTJ2ICmt UZ+abI2rF1+HQs1qg0SwcD8MUjLGL74uneZ/pycWFJeoflJvIv9LuP2xp/gjRO80kFqz KGf4SdP7QLKhBd8NHl1vtm/M7zwWOyVJVxFfL8/+VRGCURFm8pNuxppg+bO9kjgvy/TG CCsw== X-Gm-Message-State: AOAM5324AMzIBmrYe9RnDgsQgUvm6wYjqWSrbKppaxsB0qo0tczt22Vl qALN+8RY4Waf/38u1szuRVGE7w== X-Google-Smtp-Source: ABdhPJwX03Tzv91+cBIIcL3HPYfBcH68KzK5IyiUBmWU5yPcxs7h0w+SP8QTNiNPrc2X3Db3NZo1lA== X-Received: by 2002:a65:6bd4:0:b0:374:1fe3:e18a with SMTP id e20-20020a656bd4000000b003741fe3e18amr2234229pgw.621.1646880121314; Wed, 09 Mar 2022 18:42:01 -0800 (PST) Received: from laputa (p914133-ipoe.ipoe.ocn.ne.jp. [153.243.15.132]) by smtp.gmail.com with ESMTPSA id w27-20020a637b1b000000b00380437ab89asm3633317pgc.50.2022.03.09.18.41.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 09 Mar 2022 18:42:01 -0800 (PST) Date: Thu, 10 Mar 2022 11:41:56 +0900 From: Takahiro Akashi To: Ilias Apalodimas , Masahisa Kojima , u-boot@lists.denx.de, Heinrich Schuchardt , Simon Glass , Francois Ozog , Mark Kettenis Subject: Re: [RFC PATCH v3 2/2] bootmenu: add UEFI and disto_boot entries Message-ID: <20220310024156.GB64795@laputa> Mail-Followup-To: Takahiro Akashi , Ilias Apalodimas , Masahisa Kojima , u-boot@lists.denx.de, Heinrich Schuchardt , Simon Glass , Francois Ozog , Mark Kettenis References: <20220308140745.26180-1-masahisa.kojima@linaro.org> <20220308140745.26180-3-masahisa.kojima@linaro.org> <20220310015057.GA64795@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220310015057.GA64795@laputa> 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 On Thu, Mar 10, 2022 at 10:50:57AM +0900, Takahiro Akashi wrote: > On Wed, Mar 09, 2022 at 04:34:42PM +0200, Ilias Apalodimas wrote: > > 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. > > To be clear, "if no boot option is defined" is wrong. > What the specification requires is that, if a file name is missing > in a device path defined in a boot option, a default path/name must be > appended to the path before trying to load an image from that media. One more thing: What the current distro_bootcmd does is to try to a binary with the default file name in an order specified by "boot_targets" variable. Here is another inconsistency with UEFI specification. -Takahiro Akashi > > 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 ? > > # efibootmgr, if it means efi_bootmgr.c, doesn't have such a function. > > I hope that it is just a matter of time :) > > I think that the display format of the menu should be improved so that users > easily understand that the list of UEFI boot options are listed in the order > that "BootOrder" specifies. > > -Takahiro Akashi > > > > > > + > > > + 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