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 72E5DC433EF for ; Tue, 10 May 2022 09:38:46 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id F39008426D; Tue, 10 May 2022 11:38:43 +0200 (CEST) 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="D2VzQlae"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 3ED0384203; Tue, 10 May 2022 11:38:41 +0200 (CEST) Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) (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 CFA7084135 for ; Tue, 10 May 2022 11:38:35 +0200 (CEST) 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-pj1-x102a.google.com with SMTP id x88so3858004pjj.1 for ; Tue, 10 May 2022 02:38:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=swZwNtFcz7VzRr/AaygLYC+c1CRMM5nx0vGOCbd1PJ8=; b=D2VzQlaejjKq7Dv3sulZfQrzjyK+SVdG0QC89bfuVMxococ26R8So3uE57/B3/GPX9 evHb5IRD8dSULviTUlbVGKJQbV7Eomd77WHz1q61RfLKrLUShgsZf4e6Om2xK+sicli5 StF6Kn39G044+mBl79OAQX2oNvpTfnh34IbHyDbugVe5RIpKnkm9dqQNJbGiXNYmzFs/ W5kHw1/6q6zKZTErYUL00N4Hzq2eTCa1jZQccDyjtnYnHS5e11Z8gYjMsKnuvs270zHZ GaJ30FBFGPIWz8GIaF2f99W/ggn1/GatrcLWc3sa71kyJCZ9R1m4Ii9FPGB4gk1DGzmy u0+g== 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 :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=swZwNtFcz7VzRr/AaygLYC+c1CRMM5nx0vGOCbd1PJ8=; b=YZwQ2js2O+azmrAfgWasPlOqWmV8QOvVVsY7MVUNP8znKcCW6uv1OaiFumaO0JCIJb yaQaukQWlclvD0FS+noFOhKhFALUyNPM5L9reUuqQ3DeIu6ZhcHBhwx2+/nCNRaXUKoS kogWUYmQwzv7MIFXLSG3VKDEhdO+93gY4D2lzQeEVEV6EhFJfEoqRbhe4MqyQ/NMAARs hLh9UVoXYonSoCn9psre00mPv1rEUvDM8x3BGIcu92+XTm0Ww6PapLbG/FiPSMQl6cSP GnpF825nF1pHmZlJT8uRaOa6Ekln+LKhS4NCvzlZ2ep6wh29ST9iZuYlwQOa+aVAC6L3 IW8A== X-Gm-Message-State: AOAM532H7fl8T2W8RFCLlenm05ncHMnWLjZTJw5qms0Q05NFikAd2Qna UzV6UPfPkYGTptwWELtB3B/c9g== X-Google-Smtp-Source: ABdhPJx/85uORdAdAfrKii+BBPJuU8nJtfcPMx56/VICwjPmpnwh5gK/cplHhTwLowllkO34ifXC5A== X-Received: by 2002:a17:902:b698:b0:158:faee:442f with SMTP id c24-20020a170902b69800b00158faee442fmr20525114pls.75.1652175514007; Tue, 10 May 2022 02:38:34 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:ccaa:947e:3a8b:3c7e]) by smtp.gmail.com with ESMTPSA id h7-20020aa796c7000000b0050dc76281e3sm10190418pfq.189.2022.05.10.02.38.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 May 2022 02:38:33 -0700 (PDT) Date: Tue, 10 May 2022 18:38:30 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: mark.kettenis@xs4all.nl, u-boot@lists.denx.de Subject: Re: [PATCH] efi_loader: fix a problem in loading an image from a short-path Message-ID: <20220510093830.GA75330@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , mark.kettenis@xs4all.nl, u-boot@lists.denx.de References: <20220428045012.52286-1-takahiro.akashi@linaro.org> <0b10c3a4-4844-d031-e750-4d99478a593a@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0b10c3a4-4844-d031-e750-4d99478a593a@gmx.de> 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 Fri, Apr 29, 2022 at 12:57:15PM +0200, Heinrich Schuchardt wrote: > On 4/28/22 06:50, AKASHI Takahiro wrote: > > Booting from a short-form device path which starts with the first element > > being a File Path Media Device Path failed because it doesn't contain > > any valid device with simple file system protocol and efi_dp_find_obj() > > in efi_load_image_from_path() will return NULL. > > For instance, > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi > > -> shortened version: /\helloworld.efi > > Thanks for enabling this. > > Please, enhance test/py/tests/test_efi_bootmgr to test booting from a > filename only device path. Okay, but you should have added tests in your patch. > > > > With this patch applied, all the media devices with simple file system > > protocol are enumerated and the boot manager attempts to boot temporarily > > generated device paths one-by-one. > > > > This new implementation is still a bit incompatible with the UEFI > > specification in terms of: > > * not creating real boot options > > The UEFI specification has: > > "These new boot options must not be saved to non volatile storage, > and may not be added to BootOrder." > > Is there really something missing? When the boot manager attempts to boot a short-form File Path Media Device Path, it will enumerate all removable media devices, followed by all fixed media devices, creating boot options for each device. ^^^^^^^^^^^^^^^^^^^^^ > > * not distinguishing removable media and fix media > > struct blk_desc has field 'removable' which is mirrored in struct > efi_disk_obj. Instead of relying on such an internal structure, we can and should use a public interface, efi_block_io(->media.removable_media). > mmc_bind() always sets removable to 1 irrespective of the device being > eMMC or SD-card. I guess this would need to be fixed. > > > (See section 3.1.3 "Boot Options".) > > > > But it still gives us a closer and better solution than the current. > > > > Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form device-path") > > Signed-off-by: AKASHI Takahiro > > --- > > include/efi_loader.h | 3 ++ > > lib/efi_loader/efi_boottime.c | 87 +++++++++++++++++++++++++++++++---- > > lib/efi_loader/efi_file.c | 35 ++++++++++---- > > 3 files changed, 107 insertions(+), 18 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index ba79a9afb404..9730c1375a55 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -661,6 +661,9 @@ void efi_signal_event(struct efi_event *event); > > struct efi_simple_file_system_protocol *efi_simple_file_system( > > struct blk_desc *desc, int part, struct efi_device_path *dp); > > > > +/* open file from simple file system */ > > +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v, > > + struct efi_device_path *fp); > > /* open file from device-path: */ > > struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp); > > > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > > index 5bcb8253edba..39b0e8f7ade0 100644 > > --- a/lib/efi_loader/efi_boottime.c > > +++ b/lib/efi_loader/efi_boottime.c > > @@ -1868,19 +1868,21 @@ out: > > } > > > > /** > > - * efi_load_image_from_file() - load an image from file system > > + * __efi_load_image_from_file() - load an image from file system > > * > > * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the > > * callers obligation to update the memory type as needed. > > * > > + * @v: simple file system > > Please, use a meaningful variable name. I will fully re-write this patch. While efi boot manager should be responsible for handling a short-form device path starting with a file path, my current implementation is made in efi_load_image_from_path() hence LoadImage API. Instead, the logic of enumerating file_system_protocol devices will be added to try_load_entry()/efi_bootmgr_load(). -Takahiro Akashi > > * @file_path: the path of the image to load > > * @buffer: buffer containing the loaded image > > * @size: size of the loaded image > > * Return: status code > > */ > > static > > -efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, > > - void **buffer, efi_uintn_t *size) > > +efi_status_t __efi_load_image_from_file(struct efi_simple_file_system_protocol *v, > > + struct efi_device_path *file_path, > > + void **buffer, efi_uintn_t *size) > > { > > struct efi_file_handle *f; > > efi_status_t ret; > > @@ -1888,7 +1890,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, > > efi_uintn_t bs; > > > > /* Open file */ > > - f = efi_file_from_path(file_path); > > + if (v) > > + f = efi_file_from_fs(v, file_path); > > + else > > + /* file_path should have a device path */ > > + f = efi_file_from_path(file_path); > > if (!f) > > return EFI_NOT_FOUND; > > > > @@ -1921,6 +1927,64 @@ error: > > return ret; > > } > > > > +/** > > + * efi_load_image_from_file() - load an image from file system > > + * > > + * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the > > + * callers obligation to update the memory type as needed. > > + * > > + * @file_path: the path of the image to load > > + * @buffer: buffer containing the loaded image > > + * @size: size of the loaded image > > + * Return: status code > > + */ > > +static > > +efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, > > + void **buffer, efi_uintn_t *size) > > +{ > > + efi_uintn_t no_handles; > > + efi_handle_t *handles; > > + struct efi_handler *handler; > > + struct efi_simple_file_system_protocol *fs; > > + int i; > > + efi_status_t ret; > > + > > + /* if a file_path contains a device path */ > > + if (!EFI_DP_TYPE(file_path, MEDIA_DEVICE, FILE_PATH)) > > + return __efi_load_image_from_file(NULL, file_path, buffer, size); > > + > > + /* no explicit device specified */ > > + ret = EFI_CALL(efi_locate_handle_buffer( > > + BY_PROTOCOL, > > + &efi_simple_file_system_protocol_guid, > > + NULL, > > + &no_handles, > > + &handles)); > > + if (ret != EFI_SUCCESS) > > + return ret; > > + if (!no_handles) > > + return EFI_NOT_FOUND; > > + > > + for (i = 0; i < no_handles; i++) { > > + ret = efi_search_protocol(handles[i], > > + &efi_simple_file_system_protocol_guid, > > + &handler); > > + if (ret != EFI_SUCCESS) > > + /* unlikely */ > > + continue; > > + > > + fs = handler->protocol_interface; > > + if (!fs) > > + continue; > > + > > + ret = __efi_load_image_from_file(fs, file_path, buffer, size); > > + if (ret == EFI_SUCCESS) > > + return ret; > > + } > > + > > + return EFI_NOT_FOUND; > > +} > > + > > /** > > * efi_load_image_from_path() - load an image using a file path > > * > > @@ -1940,7 +2004,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy, > > { > > efi_handle_t device; > > efi_status_t ret; > > - struct efi_device_path *dp, *rem; > > + struct efi_device_path *rem; > > struct efi_load_file_protocol *load_file_protocol = NULL; > > efi_uintn_t buffer_size; > > uint64_t addr, pages; > > @@ -1950,12 +2014,15 @@ efi_status_t efi_load_image_from_path(bool boot_policy, > > *buffer = NULL; > > *size = 0; > > > > - dp = file_path; > > - device = efi_dp_find_obj(dp, NULL, &rem); > > - ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid, > > - NULL); > > + /* try first for simple file system protocols */ > > + ret = efi_load_image_from_file(file_path, buffer, size); > > if (ret == EFI_SUCCESS) > > - return efi_load_image_from_file(file_path, buffer, size); > > + return ret; > > + > > + /* TODO: does this really make sense? */ > > + device = efi_dp_find_obj(file_path, NULL, &rem); > > + if (!device) > > + return EFI_NOT_FOUND; > > > > ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL); > > if (ret == EFI_SUCCESS) { > > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c > > index 7a7077e6d032..2d6a432b168b 100644 > > --- a/lib/efi_loader/efi_file.c > > +++ b/lib/efi_loader/efi_file.c > > @@ -1083,16 +1083,16 @@ static const struct efi_file_handle efi_file_handle_protocol = { > > /** > > * efi_file_from_path() - open file via device path > > * > > - * @fp: device path > > + * @v: simple file system > > + * @fp: file path > > * Return: EFI_FILE_PROTOCOL for the file or NULL > > */ > > -struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) > > +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v, > > + struct efi_device_path *fp) > > { > > - struct efi_simple_file_system_protocol *v; > > struct efi_file_handle *f; > > efi_status_t ret; > > > > - v = efi_fs_from_path(fp); > > if (!v) > > return NULL; > > > > @@ -1100,10 +1100,6 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) > > if (ret != EFI_SUCCESS) > > return NULL; > > > > - /* Skip over device-path nodes before the file path. */ > > - while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH)) > > - fp = efi_dp_next(fp); > > - > > /* > > * Step through the nodes of the directory path until the actual file > > * node is reached which is the final node in the device path. > > @@ -1138,6 +1134,29 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) > > return f; > > } > > > > +/** > > + * efi_file_from_path() - open file via device path > > + * > > + * @fp: device path > > 'fp' does not resonate 'device path'. How about using dp? > > > + * Return: EFI_FILE_PROTOCOL for the file or NULL > > + */ > > +struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp) > > +{ > > + struct efi_simple_file_system_protocol *v; > > Please, provide a meaningful variable name. > > Best regards > > Heinrich > > > + > > + v = efi_fs_from_path(fp); > > + if (!v) > > + return NULL; > > + > > + /* Skip over device-path nodes before the file path. */ > > + while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH)) > > + fp = efi_dp_next(fp); > > + if (!fp) > > + return NULL; > > + > > + return efi_file_from_fs(v, fp); > > +} > > + > > static efi_status_t EFIAPI > > efi_open_volume(struct efi_simple_file_system_protocol *this, > > struct efi_file_handle **root) >