[PATCH v2 0/7] efi: Start to chip away at the EFI workaround
At present there is a function, efi_set_bootdev(), which is used in various places to tell the EFI loader which device a file came from.
With bootstd, this information is available in the bootflow.
This little series provides a way for bootstd to provide the bootflow to the EFI loader, so that it is able to select the correct paths.
For now only the EFI bootmeth is updated.
Changes in v2: - Adjust argument ordering for efi_run_image() - Use efi_loader tag instead of efi - Drop unnecessary path removal - Fix 'require' typo - Move calculation of dev-name into a separate function - Add new patch to support PXE booting
Simon Glass (7): efi_loader: Refactor device and image paths into a function efi_loader: Make efi_run_image() static efi_loader: Update efi_run_image() to accept image and device path efi_loader: Add a version of efi_binary_run() with more parameters efi_loader: Move the fallback code from efi_run_image() efi_loader: Pass in the required parameters from EFI bootmeth bootmeth_efi: Support PXE booting
boot/bootmeth_efi.c | 64 +--------- include/efi_loader.h | 10 ++ lib/efi_loader/efi_bootbin.c | 239 ++++++++++++++++++++++++++--------- 3 files changed, 189 insertions(+), 124 deletions(-)
Move this code into a function so it can be called from elsewhere.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 53 +++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 16 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 7e7a6bf31aa..afb13a008c2 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -44,12 +44,46 @@ void efi_clear_bootdev(void) image_size = 0; }
+efi_status_t calculate_paths(const char *dev, const char *devnr, const char *path, + struct efi_device_path **device_pathp, + struct efi_device_path **image_pathp) + +{ + struct efi_device_path *image, *device; + efi_status_t ret; + + ret = efi_dp_from_name(dev, devnr, path, &device, &image); + if (ret != EFI_SUCCESS) + return ret; + + *device_pathp = device; + if (image) { + /* FIXME: image should not contain device */ + struct efi_device_path *image_tmp = image; + + efi_dp_split_file_path(image, &device, &image); + efi_free_pool(image_tmp); + } + *image_pathp = image; + log_debug("- boot device %pD\n", device); + if (image) + log_debug("- image %pD\n", image); + + return EFI_SUCCESS; +} + /** * efi_set_bootdev() - set boot device * * This function is called when a file is loaded, e.g. via the 'load' command. * We use the path to this file to inform the UEFI binary about the boot device. * + * For a valid image, it sets: + * - image_addr to the provided buffer + * - image_size to the provided buffer_size + * - bootefi_device_path to the EFI device-path + * - bootefi_image_path to the EFI image-path + * * @dev: device, e.g. "MMC" * @devnr: number of the device, e.g. "1:2" * @path: path to file loaded @@ -59,7 +93,6 @@ void efi_clear_bootdev(void) void efi_set_bootdev(const char *dev, const char *devnr, const char *path, void *buffer, size_t buffer_size) { - struct efi_device_path *device, *image; efi_status_t ret;
log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev, @@ -93,21 +126,9 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, image_addr = buffer; image_size = buffer_size;
- ret = efi_dp_from_name(dev, devnr, path, &device, &image); - if (ret == EFI_SUCCESS) { - bootefi_device_path = device; - if (image) { - /* FIXME: image should not contain device */ - struct efi_device_path *image_tmp = image; - - efi_dp_split_file_path(image, &device, &image); - efi_free_pool(image_tmp); - } - bootefi_image_path = image; - log_debug("- boot device %pD\n", device); - if (image) - log_debug("- image %pD\n", image); - } else { + ret = calculate_paths(dev, devnr, path, &bootefi_device_path, + &bootefi_image_path); + if (ret) { log_debug("- efi_dp_from_name() failed, err=%lx\n", ret); efi_clear_bootdev(); }
Hi Simon,
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
Move this code into a function so it can be called from elsewhere.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 53 +++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 16 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 7e7a6bf31aa..afb13a008c2 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -44,12 +44,46 @@ void efi_clear_bootdev(void) image_size = 0; }
+efi_status_t calculate_paths(const char *dev, const char *devnr, const char *path,
struct efi_device_path **device_pathp,struct efi_device_path **image_pathp)+{
struct efi_device_path *image, *device;efi_status_t ret;ret = efi_dp_from_name(dev, devnr, path, &device, &image);if (ret != EFI_SUCCESS)return ret;*device_pathp = device;if (image) {/* FIXME: image should not contain device */struct efi_device_path *image_tmp = image;efi_dp_split_file_path(image, &device, &image);efi_free_pool(image_tmp);}*image_pathp = image;log_debug("- boot device %pD\n", device);if (image)log_debug("- image %pD\n", image);return EFI_SUCCESS;+}
This needs an entry in a header file and a description. Also, we usually keep DP related functions to lib/efi_loader/efi_device_path.c
[...]
Thanks /Ilias
Hi Simon,
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
Move this code into a function so it can be called from elsewhere.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 53 +++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 16 deletions(-)
Applied to sjg/master, thanks!
This function is not called from outside this file and has no entry in the header file, so mark it static.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index afb13a008c2..ec8f4e4f7f4 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -141,7 +141,7 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, * @source_size: size of the UEFI image * Return: status code */ -efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) +static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) { efi_handle_t mem_handle = NULL, handle; struct efi_device_path *file_path = NULL;
This function is not called from outside this file and has no entry in the header file, so mark it static.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to sjg/master, thanks!
Provide these globals as parameters to this function, on the way to making it possible to start an image without relying on the globals.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Adjust argument ordering for efi_run_image()
lib/efi_loader/efi_bootbin.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index ec8f4e4f7f4..8febd325f34 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -139,9 +139,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, * * @source_buffer: memory address of the UEFI image * @source_size: size of the UEFI image + * @device: EFI device-path + * @image: EFI image-path * Return: status code */ -static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) +static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, + struct efi_device_path *device, + struct efi_device_path *image) { efi_handle_t mem_handle = NULL, handle; struct efi_device_path *file_path = NULL; @@ -149,7 +153,7 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) efi_status_t ret; u16 *load_options;
- if (!bootefi_device_path || !bootefi_image_path) { + if (!device || !image) { log_debug("Not loaded from disk\n"); /* * Special case for efi payload not loaded from disk, @@ -169,9 +173,8 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) goto out; msg_path = file_path; } else { - file_path = efi_dp_concat(bootefi_device_path, - bootefi_image_path, 0); - msg_path = bootefi_image_path; + file_path = efi_dp_concat(device, image, 0); + msg_path = image; log_debug("Loaded from disk\n"); }
@@ -208,7 +211,7 @@ out: /** * efi_binary_run() - run loaded UEFI image * - * @image: memory address of the UEFI image + * @image_ptr: memory address of the UEFI image * @size: size of the UEFI image * @fdt: device-tree * @@ -217,7 +220,7 @@ out: * * Return: status code */ -efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) { efi_status_t ret;
@@ -233,5 +236,6 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
- return efi_run_image(image, size); + return efi_run_image(image_ptr, size, bootefi_device_path, + bootefi_image_path); }
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
Provide these globals as parameters to this function, on the way to making it possible to start an image without relying on the globals.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Adjust argument ordering for efi_run_image()
lib/efi_loader/efi_bootbin.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index ec8f4e4f7f4..8febd325f34 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -139,9 +139,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
- @source_buffer: memory address of the UEFI image
- @source_size: size of the UEFI image
- @device: EFI device-path
*/
- @image: EFI image-path
- Return: status code
-static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) +static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size,
struct efi_device_path *device,struct efi_device_path *image){ efi_handle_t mem_handle = NULL, handle; struct efi_device_path *file_path = NULL; @@ -149,7 +153,7 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) efi_status_t ret; u16 *load_options;
if (!bootefi_device_path || !bootefi_image_path) {
if (!device || !image) { log_debug("Not loaded from disk\n"); /* * Special case for efi payload not loaded from disk,@@ -169,9 +173,8 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) goto out; msg_path = file_path; } else {
file_path = efi_dp_concat(bootefi_device_path,bootefi_image_path, 0);msg_path = bootefi_image_path;
file_path = efi_dp_concat(device, image, 0);msg_path = image; log_debug("Loaded from disk\n"); }@@ -208,7 +211,7 @@ out: /**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
@@ -217,7 +220,7 @@ out:
- Return: status code
*/ -efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt)
The * is enough, please drop the _ptr rename
{ efi_status_t ret;
@@ -233,5 +236,6 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image, size);
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);}
2.34.1
With the ptr rename removed Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Hi Ilias,
On Thu, 12 Dec 2024 at 00:44, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
Provide these globals as parameters to this function, on the way to making it possible to start an image without relying on the globals.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Adjust argument ordering for efi_run_image()
lib/efi_loader/efi_bootbin.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index ec8f4e4f7f4..8febd325f34 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -139,9 +139,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
- @source_buffer: memory address of the UEFI image
- @source_size: size of the UEFI image
- @device: EFI device-path
*/
- @image: EFI image-path
- Return: status code
-static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) +static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size,
struct efi_device_path *device,struct efi_device_path *image){ efi_handle_t mem_handle = NULL, handle; struct efi_device_path *file_path = NULL; @@ -149,7 +153,7 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) efi_status_t ret; u16 *load_options;
if (!bootefi_device_path || !bootefi_image_path) {
if (!device || !image) { log_debug("Not loaded from disk\n"); /* * Special case for efi payload not loaded from disk,@@ -169,9 +173,8 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) goto out; msg_path = file_path; } else {
file_path = efi_dp_concat(bootefi_device_path,bootefi_image_path, 0);msg_path = bootefi_image_path;
file_path = efi_dp_concat(device, image, 0);msg_path = image; log_debug("Loaded from disk\n"); }@@ -208,7 +211,7 @@ out: /**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
@@ -217,7 +220,7 @@ out:
- Return: status code
*/ -efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt)
The * is enough, please drop the _ptr rename
The next patch adds a new parameter called 'image', which is the name used throughout the file, so it seems better to rename this one.
{ efi_status_t ret;
@@ -233,5 +236,6 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image, size);
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);}
2.34.1
With the ptr rename removed Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
OK, dropped.
Regards, Simon
Hi Ilias,
On Thu, 12 Dec 2024 at 00:44, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
Provide these globals as parameters to this function, on the way to making it possible to start an image without relying on the globals.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Adjust argument ordering for efi_run_image()
lib/efi_loader/efi_bootbin.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
Applied to sjg/master, thanks!
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8febd325f34..8ebd48547cb 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,18 +209,22 @@ out: }
/** - * efi_binary_run() - run loaded UEFI image + * efi_binary_run_() - run loaded UEFI image * * @image_ptr: memory address of the UEFI image * @size: size of the UEFI image * @fdt: device-tree + * @device: EFI device-path + * @image: EFI image-path * * Execute an EFI binary image loaded at @image. * @size may be zero if the binary is loaded with U-Boot load command. * * Return: status code */ -efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt, + struct efi_device_path *device, + struct efi_device_path *image) { efi_status_t ret;
@@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
- return efi_run_image(image_ptr, size, bootefi_device_path, - bootefi_image_path); + return efi_run_image(image_ptr, size, device, image); +} + +/** + * efi_binary_run() - run loaded UEFI image + * + * @image: memory address of the UEFI image + * @size: size of the UEFI image + * @fdt: device-tree + * + * Execute an EFI binary image loaded at @image. + * @size may be zero if the binary is loaded with U-Boot load command. + * + * Return: status code + */ +efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +{ + return efi_binary_run_(image, size, fdt, bootefi_device_path, + bootefi_image_path); }
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8febd325f34..8ebd48547cb 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,18 +209,22 @@ out: }
/**
- efi_binary_run() - run loaded UEFI image
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
*/
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
-efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){
This needs to be static here. Not the next patch. But I don't think we need this at all (look below)
efi_status_t ret;@@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);
return efi_run_image(image_ptr, size, device, image);+}
+/**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
+efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +{
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);}
We don't need functions and wrappers for such a simple call. Just update all the callisites with the extra parameters and keep one function
Thanks /Ilias
-- 2.34.1
Hi Ilias,
On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8febd325f34..8ebd48547cb 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,18 +209,22 @@ out: }
/**
- efi_binary_run() - run loaded UEFI image
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
*/
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
-efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){
This needs to be static here. Not the next patch. But I don't think we need this at all (look below)
efi_status_t ret;@@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);
return efi_run_image(image_ptr, size, device, image);+}
+/**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
+efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +{
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);}
We don't need functions and wrappers for such a simple call. Just update all the callisites with the extra parameters and keep one function
If you look at the two call sites, you'll see that they would need to pass bootefi_device_path and bootefi_image_path in. but these are static variables in this file.
Thanks /Ilias
-- 2.34.1
Regards, Simon
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8febd325f34..8ebd48547cb 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,18 +209,22 @@ out: }
/**
- efi_binary_run() - run loaded UEFI image
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
*/
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
-efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){
This needs to be static here. Not the next patch. But I don't think we need this at all (look below)
efi_status_t ret;@@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);
return efi_run_image(image_ptr, size, device, image);+}
+/**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
+efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +{
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);}
We don't need functions and wrappers for such a simple call. Just update all the callisites with the extra parameters and keep one function
If you look at the two call sites, you'll see that they would need to pass bootefi_device_path and bootefi_image_path in. but these are static variables in this file.
Ah fair enough, then please rename efi_binary_run_ -> _efi_binary_run ands make it static on this patch
Thanks /Ilias
Thanks /Ilias
-- 2.34.1
Regards, Simon
Hi Ilias,
On Thu, 12 Dec 2024 at 08:45, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8febd325f34..8ebd48547cb 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,18 +209,22 @@ out: }
/**
- efi_binary_run() - run loaded UEFI image
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
*/
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
-efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){
This needs to be static here. Not the next patch. But I don't think we need this at all (look below)
efi_status_t ret;@@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);
return efi_run_image(image_ptr, size, device, image);+}
+/**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
+efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +{
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);}
We don't need functions and wrappers for such a simple call. Just update all the callisites with the extra parameters and keep one function
If you look at the two call sites, you'll see that they would need to pass bootefi_device_path and bootefi_image_path in. but these are static variables in this file.
Ah fair enough, then please rename efi_binary_run_ -> _efi_binary_run ands make it static on this patch
I don't mind where the underscore goes. I changed my mind about that when the dtc maintainer pointed out that C-library functions start with underscore, so putting an underscore at the start of the function is confusing things.
"The underscore prefix is reserved for functions and types used by the compiler and standard library. The standard library can use these names freely."[1]
But I don't mind. We don't even use the C library except for sandbox and tools. So let me know what you think.
Regards, SImon
[1] https://stackoverflow.com/questions/39625352/why-do-some-functions-in-c-have...
Hi Simon,
On Fri, 13 Dec 2024 at 05:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 08:45, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8febd325f34..8ebd48547cb 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,18 +209,22 @@ out: }
/**
- efi_binary_run() - run loaded UEFI image
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
*/
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
-efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){
This needs to be static here. Not the next patch. But I don't think we need this at all (look below)
efi_status_t ret;@@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);
return efi_run_image(image_ptr, size, device, image);+}
+/**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
+efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +{
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);}
We don't need functions and wrappers for such a simple call. Just update all the callisites with the extra parameters and keep one function
If you look at the two call sites, you'll see that they would need to pass bootefi_device_path and bootefi_image_path in. but these are static variables in this file.
Ah fair enough, then please rename efi_binary_run_ -> _efi_binary_run ands make it static on this patch
I don't mind where the underscore goes. I changed my mind about that when the dtc maintainer pointed out that C-library functions start with underscore, so putting an underscore at the start of the function is confusing things.
"The underscore prefix is reserved for functions and types used by the compiler and standard library. The standard library can use these names freely."[1]
But I don't mind. We don't even use the C library except for sandbox and tools. So let me know what you think.
I don't mind really. you can do __efi_binary_run() or keep whjat you have. I jsut find the leading underscores a bit easier to read and spot, but not a huge deal
Thanks /Ilias
Regards, SImon
[1] https://stackoverflow.com/questions/39625352/why-do-some-functions-in-c-have...
Hi Ilias,
On Fri, 13 Dec 2024 at 05:13, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 13 Dec 2024 at 05:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 08:45, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8febd325f34..8ebd48547cb 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,18 +209,22 @@ out: }
/**
- efi_binary_run() - run loaded UEFI image
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
*/
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
-efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) +efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){
This needs to be static here. Not the next patch. But I don't think we need this at all (look below)
efi_status_t ret;@@ -236,6 +240,23 @@ efi_status_t efi_binary_run(void *image_ptr, size_t size, void *fdt) if (ret != EFI_SUCCESS) return ret;
return efi_run_image(image_ptr, size, bootefi_device_path,bootefi_image_path);
return efi_run_image(image_ptr, size, device, image);+}
+/**
- efi_binary_run() - run loaded UEFI image
- @image: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
+efi_status_t efi_binary_run(void *image, size_t size, void *fdt) +{
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);}
We don't need functions and wrappers for such a simple call. Just update all the callisites with the extra parameters and keep one function
If you look at the two call sites, you'll see that they would need to pass bootefi_device_path and bootefi_image_path in. but these are static variables in this file.
Ah fair enough, then please rename efi_binary_run_ -> _efi_binary_run ands make it static on this patch
I don't mind where the underscore goes. I changed my mind about that when the dtc maintainer pointed out that C-library functions start with underscore, so putting an underscore at the start of the function is confusing things.
"The underscore prefix is reserved for functions and types used by the compiler and standard library. The standard library can use these names freely."[1]
But I don't mind. We don't even use the C library except for sandbox and tools. So let me know what you think.
I don't mind really. you can do __efi_binary_run() or keep whjat you have. I jsut find the leading underscores a bit easier to read and spot, but not a huge deal
Yes I find them easier to read also. I'll go with a single underscore, since __ is supposed to be for start-up code, or something.
But Tom asked me not to send anything until Wednesday and I'm heading off for a few weeks at EOW. Let's see what you decide on the big series, and then perhaps I can send both before I go.
Regards, Simon
[1] https://stackoverflow.com/questions/39625352/why-do-some-functions-in-c-have...
Hi Ilias,
On Fri, 13 Dec 2024 at 05:13, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Fri, 13 Dec 2024 at 05:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 08:45, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 00:49, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This uses a few global variables at present. With the bootflow we have the required parameters, so add a function which accepts these. Update the existing function to call the new one with the globals.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
Applied to sjg/master, thanks!
This code is only needed if an invalid image/device path is passed in. Move the code out to a caller where this can be dealt with. The normal flow will provide these parameters.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 103 ++++++++++++++++------------------- 1 file changed, 48 insertions(+), 55 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8ebd48547cb..582b2745ced 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -147,36 +147,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, struct efi_device_path *device, struct efi_device_path *image) { - efi_handle_t mem_handle = NULL, handle; - struct efi_device_path *file_path = NULL; - struct efi_device_path *msg_path; + efi_handle_t handle; + struct efi_device_path *msg_path, *file_path; efi_status_t ret; u16 *load_options;
- if (!device || !image) { - log_debug("Not loaded from disk\n"); - /* - * Special case for efi payload not loaded from disk, - * such as 'bootefi hello' or for example payload - * loaded directly into memory via JTAG, etc: - */ - file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, - source_buffer, source_size); - /* - * Make sure that device for device_path exist - * in load_image(). Otherwise, shell and grub will fail. - */ - ret = efi_install_multiple_protocol_interfaces(&mem_handle, - &efi_guid_device_path, - file_path, NULL); - if (ret != EFI_SUCCESS) - goto out; - msg_path = file_path; - } else { - file_path = efi_dp_concat(device, image, 0); - msg_path = image; - log_debug("Loaded from disk\n"); - } + file_path = efi_dp_concat(device, image, 0); + msg_path = image;
log_info("Booting %pD\n", msg_path);
@@ -195,36 +172,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, ret = do_bootefi_exec(handle, load_options);
out: - if (mem_handle) { - efi_status_t r; - - r = efi_uninstall_multiple_protocol_interfaces( - mem_handle, &efi_guid_device_path, file_path, NULL); - if (r != EFI_SUCCESS) - log_err("Uninstalling protocol interfaces failed\n"); - } - efi_free_pool(file_path);
return ret; }
-/** - * efi_binary_run_() - run loaded UEFI image - * - * @image_ptr: memory address of the UEFI image - * @size: size of the UEFI image - * @fdt: device-tree - * @device: EFI device-path - * @image: EFI image-path - * - * Execute an EFI binary image loaded at @image. - * @size may be zero if the binary is loaded with U-Boot load command. - * - * Return: status code - */ -efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt, - struct efi_device_path *device, - struct efi_device_path *image) +static efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt, + struct efi_device_path *device, + struct efi_device_path *image) { efi_status_t ret;
@@ -257,6 +211,45 @@ efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt, */ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) { - return efi_binary_run_(image, size, fdt, bootefi_device_path, - bootefi_image_path); + efi_handle_t mem_handle = NULL; + struct efi_device_path *file_path = NULL; + efi_status_t ret; + + if (!bootefi_device_path || !bootefi_image_path) { + log_debug("Not loaded from disk\n"); + /* + * Special case for efi payload not loaded from disk, + * such as 'bootefi hello' or for example payload + * loaded directly into memory via JTAG, etc: + */ + file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, image, + size); + /* + * Make sure that device for device_path exist + * in load_image(). Otherwise, shell and grub will fail. + */ + ret = efi_install_multiple_protocol_interfaces(&mem_handle, + &efi_guid_device_path, + file_path, NULL); + if (ret != EFI_SUCCESS) + goto out; + } else { + log_debug("Loaded from disk\n"); + } + + ret = efi_binary_run_(image, size, fdt, bootefi_device_path, + bootefi_image_path); +out: + if (mem_handle) { + efi_status_t r; + + r = efi_uninstall_multiple_protocol_interfaces(mem_handle, + &efi_guid_device_path, file_path, NULL); + if (r != EFI_SUCCESS) + log_err("Uninstalling protocol interfaces failed\n"); + } + efi_free_pool(file_path); + + return ret; } +
Checkpatch has errors on this. Please have a look
Also for some reason is fails to apply after patch 4.
Thanks /Ilias
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This code is only needed if an invalid image/device path is passed in. Move the code out to a caller where this can be dealt with. The normal flow will provide these parameters.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 103 ++++++++++++++++------------------- 1 file changed, 48 insertions(+), 55 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8ebd48547cb..582b2745ced 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -147,36 +147,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, struct efi_device_path *device, struct efi_device_path *image) {
efi_handle_t mem_handle = NULL, handle;struct efi_device_path *file_path = NULL;struct efi_device_path *msg_path;
efi_handle_t handle;struct efi_device_path *msg_path, *file_path; efi_status_t ret; u16 *load_options;
if (!device || !image) {log_debug("Not loaded from disk\n");/** Special case for efi payload not loaded from disk,* such as 'bootefi hello' or for example payload* loaded directly into memory via JTAG, etc:*/file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,source_buffer, source_size);/** Make sure that device for device_path exist* in load_image(). Otherwise, shell and grub will fail.*/ret = efi_install_multiple_protocol_interfaces(&mem_handle,&efi_guid_device_path,file_path, NULL);if (ret != EFI_SUCCESS)goto out;msg_path = file_path;} else {file_path = efi_dp_concat(device, image, 0);msg_path = image;log_debug("Loaded from disk\n");}
file_path = efi_dp_concat(device, image, 0);msg_path = image; log_info("Booting %pD\n", msg_path);@@ -195,36 +172,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, ret = do_bootefi_exec(handle, load_options);
out:
if (mem_handle) {efi_status_t r;r = efi_uninstall_multiple_protocol_interfaces(mem_handle, &efi_guid_device_path, file_path, NULL);if (r != EFI_SUCCESS)log_err("Uninstalling protocol interfaces failed\n");}efi_free_pool(file_path); return ret;}
-/**
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
-efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image)+static efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){ efi_status_t ret;
@@ -257,6 +211,45 @@ efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt, */ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) {
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);
efi_handle_t mem_handle = NULL;struct efi_device_path *file_path = NULL;efi_status_t ret;if (!bootefi_device_path || !bootefi_image_path) {log_debug("Not loaded from disk\n");/** Special case for efi payload not loaded from disk,* such as 'bootefi hello' or for example payload* loaded directly into memory via JTAG, etc:*/file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, image,size);/** Make sure that device for device_path exist* in load_image(). Otherwise, shell and grub will fail.*/ret = efi_install_multiple_protocol_interfaces(&mem_handle,&efi_guid_device_path,file_path, NULL);if (ret != EFI_SUCCESS)goto out;} else {log_debug("Loaded from disk\n");}ret = efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);+out:
if (mem_handle) {efi_status_t r;r = efi_uninstall_multiple_protocol_interfaces(mem_handle,&efi_guid_device_path, file_path, NULL);if (r != EFI_SUCCESS)log_err("Uninstalling protocol interfaces failed\n");}efi_free_pool(file_path);return ret;}
-- 2.34.1
Hi Ilias,
On Thu, 12 Dec 2024 at 00:53, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Checkpatch has errors on this. Please have a look
It's the brackets not matching up because of efi_uninstall_multiple_protocol_interfaces() being so long. It also complains about possible blank lines at the end of the file, but that isn't correct.
Also for some reason is fails to apply after patch 4.
It is based on top of the other series (confusion between pointers and addresses). I can send it independently if you'd like to apply this one first.
Thanks /Ilias
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This code is only needed if an invalid image/device path is passed in. Move the code out to a caller where this can be dealt with. The normal flow will provide these parameters.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 103 ++++++++++++++++------------------- 1 file changed, 48 insertions(+), 55 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8ebd48547cb..582b2745ced 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -147,36 +147,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, struct efi_device_path *device, struct efi_device_path *image) {
efi_handle_t mem_handle = NULL, handle;struct efi_device_path *file_path = NULL;struct efi_device_path *msg_path;
efi_handle_t handle;struct efi_device_path *msg_path, *file_path; efi_status_t ret; u16 *load_options;
if (!device || !image) {log_debug("Not loaded from disk\n");/** Special case for efi payload not loaded from disk,* such as 'bootefi hello' or for example payload* loaded directly into memory via JTAG, etc:*/file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,source_buffer, source_size);/** Make sure that device for device_path exist* in load_image(). Otherwise, shell and grub will fail.*/ret = efi_install_multiple_protocol_interfaces(&mem_handle,&efi_guid_device_path,file_path, NULL);if (ret != EFI_SUCCESS)goto out;msg_path = file_path;} else {file_path = efi_dp_concat(device, image, 0);msg_path = image;log_debug("Loaded from disk\n");}
file_path = efi_dp_concat(device, image, 0);msg_path = image; log_info("Booting %pD\n", msg_path);@@ -195,36 +172,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, ret = do_bootefi_exec(handle, load_options);
out:
if (mem_handle) {efi_status_t r;r = efi_uninstall_multiple_protocol_interfaces(mem_handle, &efi_guid_device_path, file_path, NULL);if (r != EFI_SUCCESS)log_err("Uninstalling protocol interfaces failed\n");}efi_free_pool(file_path); return ret;}
-/**
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
-efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image)+static efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){ efi_status_t ret;
@@ -257,6 +211,45 @@ efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt, */ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) {
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);
efi_handle_t mem_handle = NULL;struct efi_device_path *file_path = NULL;efi_status_t ret;if (!bootefi_device_path || !bootefi_image_path) {log_debug("Not loaded from disk\n");/** Special case for efi payload not loaded from disk,* such as 'bootefi hello' or for example payload* loaded directly into memory via JTAG, etc:*/file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, image,size);/** Make sure that device for device_path exist* in load_image(). Otherwise, shell and grub will fail.*/ret = efi_install_multiple_protocol_interfaces(&mem_handle,&efi_guid_device_path,file_path, NULL);if (ret != EFI_SUCCESS)goto out;} else {log_debug("Loaded from disk\n");}ret = efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);+out:
if (mem_handle) {efi_status_t r;r = efi_uninstall_multiple_protocol_interfaces(mem_handle,&efi_guid_device_path, file_path, NULL);if (r != EFI_SUCCESS)log_err("Uninstalling protocol interfaces failed\n");}efi_free_pool(file_path);return ret;}
-- 2.34.1
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 00:53, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Checkpatch has errors on this. Please have a look
It's the brackets not matching up because of efi_uninstall_multiple_protocol_interfaces() being so long. It also complains about possible blank lines at the end of the file, but that isn't correct.
Also for some reason is fails to apply after patch 4.
It is based on top of the other series (confusion between pointers and addresses). I can send it independently if you'd like to apply this one first.
Yes please, if it's not too much work. I think the other one still needs bigger adjustments
Thanks /Ilias
Thanks /Ilias
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This code is only needed if an invalid image/device path is passed in. Move the code out to a caller where this can be dealt with. The normal flow will provide these parameters.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 103 ++++++++++++++++------------------- 1 file changed, 48 insertions(+), 55 deletions(-)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8ebd48547cb..582b2745ced 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -147,36 +147,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, struct efi_device_path *device, struct efi_device_path *image) {
efi_handle_t mem_handle = NULL, handle;struct efi_device_path *file_path = NULL;struct efi_device_path *msg_path;
efi_handle_t handle;struct efi_device_path *msg_path, *file_path; efi_status_t ret; u16 *load_options;
if (!device || !image) {log_debug("Not loaded from disk\n");/** Special case for efi payload not loaded from disk,* such as 'bootefi hello' or for example payload* loaded directly into memory via JTAG, etc:*/file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,source_buffer, source_size);/** Make sure that device for device_path exist* in load_image(). Otherwise, shell and grub will fail.*/ret = efi_install_multiple_protocol_interfaces(&mem_handle,&efi_guid_device_path,file_path, NULL);if (ret != EFI_SUCCESS)goto out;msg_path = file_path;} else {file_path = efi_dp_concat(device, image, 0);msg_path = image;log_debug("Loaded from disk\n");}
file_path = efi_dp_concat(device, image, 0);msg_path = image; log_info("Booting %pD\n", msg_path);@@ -195,36 +172,13 @@ static efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size, ret = do_bootefi_exec(handle, load_options);
out:
if (mem_handle) {efi_status_t r;r = efi_uninstall_multiple_protocol_interfaces(mem_handle, &efi_guid_device_path, file_path, NULL);if (r != EFI_SUCCESS)log_err("Uninstalling protocol interfaces failed\n");}efi_free_pool(file_path); return ret;}
-/**
- efi_binary_run_() - run loaded UEFI image
- @image_ptr: memory address of the UEFI image
- @size: size of the UEFI image
- @fdt: device-tree
- @device: EFI device-path
- @image: EFI image-path
- Execute an EFI binary image loaded at @image.
- @size may be zero if the binary is loaded with U-Boot load command.
- Return: status code
- */
-efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image)+static efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt,
struct efi_device_path *device,struct efi_device_path *image){ efi_status_t ret;
@@ -257,6 +211,45 @@ efi_status_t efi_binary_run_(void *image_ptr, size_t size, void *fdt, */ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) {
return efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);
efi_handle_t mem_handle = NULL;struct efi_device_path *file_path = NULL;efi_status_t ret;if (!bootefi_device_path || !bootefi_image_path) {log_debug("Not loaded from disk\n");/** Special case for efi payload not loaded from disk,* such as 'bootefi hello' or for example payload* loaded directly into memory via JTAG, etc:*/file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, image,size);/** Make sure that device for device_path exist* in load_image(). Otherwise, shell and grub will fail.*/ret = efi_install_multiple_protocol_interfaces(&mem_handle,&efi_guid_device_path,file_path, NULL);if (ret != EFI_SUCCESS)goto out;} else {log_debug("Loaded from disk\n");}ret = efi_binary_run_(image, size, fdt, bootefi_device_path,bootefi_image_path);+out:
if (mem_handle) {efi_status_t r;r = efi_uninstall_multiple_protocol_interfaces(mem_handle,&efi_guid_device_path, file_path, NULL);if (r != EFI_SUCCESS)log_err("Uninstalling protocol interfaces failed\n");}efi_free_pool(file_path);return ret;}
-- 2.34.1
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 00:53, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Checkpatch has errors on this. Please have a look
It's the brackets not matching up because of efi_uninstall_multiple_protocol_interfaces() being so long. It also complains about possible blank lines at the end of the file, but that isn't correct.
Also for some reason is fails to apply after patch 4.
It is based on top of the other series (confusion between pointers and addresses). I can send it independently if you'd like to apply this one first.
Yes please, if it's not too much work. I think the other one still needs bigger adjustments
Thanks /Ilias
Thanks /Ilias
On Thu, 12 Dec 2024 at 00:38, Simon Glass sjg@chromium.org wrote:
This code is only needed if an invalid image/device path is passed in. Move the code out to a caller where this can be dealt with. The normal flow will provide these parameters.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_bootbin.c | 103 ++++++++++++++++------------------- 1 file changed, 48 insertions(+), 55 deletions(-)
Applied to sjg/master, thanks!
Rather than setting up the global variables and then making the call, pass them into function directly. This cleans up the code and makes it all a bit easier to understand.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Use efi_loader tag instead of efi - Drop unnecessary path removal - Fix 'require' typo - Move calculation of dev-name into a separate function
boot/bootmeth_efi.c | 49 +------------------------ include/efi_loader.h | 10 +++++ lib/efi_loader/efi_bootbin.c | 71 ++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 47 deletions(-)
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index a2998452666..b745ba8bd4b 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -52,40 +52,6 @@ static bool bootmeth_uses_network(struct bootflow *bflow) device_get_uclass_id(media) == UCLASS_ETH; }
-static void set_efi_bootdev(struct blk_desc *desc, struct bootflow *bflow) -{ - const struct udevice *media_dev; - int size = bflow->size; - const char *dev_name; - char devnum_str[9]; - char dirname[200]; - char *last_slash; - - /* - * This is a horrible hack to tell EFI about this boot device. Once we - * unify EFI with the rest of U-Boot we can clean this up. The same hack - * exists in multiple places, e.g. in the fs, tftp and load commands. - * - * Once we can clean up the EFI code to make proper use of driver model, - * this can go away. - */ - media_dev = dev_get_parent(bflow->dev); - snprintf(devnum_str, sizeof(devnum_str), "%x:%x", - desc ? desc->devnum : dev_seq(media_dev), - bflow->part); - - strlcpy(dirname, bflow->fname, sizeof(dirname)); - last_slash = strrchr(dirname, '/'); - if (last_slash) - *last_slash = '\0'; - - dev_name = device_get_uclass_id(media_dev) == UCLASS_MASS_STORAGE ? - "usb" : blk_get_uclass_name(device_get_uclass_id(media_dev)); - log_debug("setting bootdev %s, %s, %s, %p, %x\n", - dev_name, devnum_str, bflow->fname, bflow->buf, size); - efi_set_bootdev(dev_name, devnum_str, bflow->fname, bflow->buf, size); -} - static int efiload_read_file(struct bootflow *bflow, ulong addr) { struct blk_desc *desc = NULL; @@ -102,8 +68,6 @@ static int efiload_read_file(struct bootflow *bflow, ulong addr) return log_msg_ret("rdf", ret); bflow->buf = map_sysmem(addr, bflow->size);
- set_efi_bootdev(desc, bflow); - return 0; }
@@ -344,17 +308,8 @@ static int distro_efi_boot(struct udevice *dev, struct bootflow *bflow) fdt = env_get_hex("fdt_addr_r", 0); }
- if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) { - log_debug("Booting with built-in fdt\n"); - if (efi_binary_run(map_sysmem(kernel, 0), bflow->size, - EFI_FDT_USE_INTERNAL)) - return log_msg_ret("run", -EINVAL); - } else { - log_debug("Booting with external fdt\n"); - if (efi_binary_run(map_sysmem(kernel, 0), bflow->size, - map_sysmem(fdt, 0))) - return log_msg_ret("run", -EINVAL); - } + if (efi_bootflow_run(bflow)) + return log_msg_ret("run", -EINVAL);
return 0; } diff --git a/include/efi_loader.h b/include/efi_loader.h index 1e5a067deca..8a3c7273608 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -19,6 +19,7 @@ #include <linux/oid_registry.h>
struct blk_desc; +struct bootflow; struct jmp_buf_data;
#if CONFIG_IS_ENABLED(EFI_LOADER) @@ -544,6 +545,15 @@ efi_status_t efi_install_fdt(void *fdt); efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options); /* Run loaded UEFI image with given fdt */ efi_status_t efi_binary_run(void *image, size_t size, void *fdt); + +/** + * efi_bootflow_run() - Run a bootflow containing an EFI application + * + * @bootflow: Bootflow to run + * Return: Status code, something went wrong + */ +efi_status_t efi_bootflow_run(struct bootflow *bootflow); + /* Initialize variable services */ efi_status_t efi_init_variables(void); /* Notify ExitBootServices() is called */ diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 582b2745ced..8efd779d9f1 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -6,13 +6,16 @@
#define LOG_CATEGORY LOGC_EFI
+#include <bootflow.h> #include <charset.h> +#include <dm.h> #include <efi.h> #include <efi_loader.h> #include <env.h> #include <image.h> #include <log.h> #include <malloc.h> +#include <mapmem.h>
static struct efi_device_path *bootefi_image_path; static struct efi_device_path *bootefi_device_path; @@ -253,3 +256,71 @@ out: return ret; }
+/** + * calc_dev_name() - Calculate the device name to give to EFI + * + * If not supported, this shows an error. + * + * Return name, or NULL if not supported + */ +static const char *calc_dev_name(struct bootflow *bflow) +{ + const struct udevice *media_dev; + + media_dev = dev_get_parent(bflow->dev); + + if (!bflow->blk) { + log_err("Cannot boot EFI app on media '%s'\n", + dev_get_uclass_name(media_dev)); + + return NULL; + } + + if (device_get_uclass_id(media_dev) == UCLASS_MASS_STORAGE) + return "usb"; + + return blk_get_uclass_name(device_get_uclass_id(media_dev)); +} + +efi_status_t efi_bootflow_run(struct bootflow *bflow) +{ + struct efi_device_path *device, *image; + const struct udevice *media_dev; + struct blk_desc *desc = NULL; + const char *dev_name; + char devnum_str[9]; + efi_status_t ret; + void *fdt; + + media_dev = dev_get_parent(bflow->dev); + if (bflow->blk) { + desc = dev_get_uclass_plat(bflow->blk); + + snprintf(devnum_str, sizeof(devnum_str), "%x:%x", + desc ? desc->devnum : dev_seq(media_dev), bflow->part); + } else { + *devnum_str = '\0'; + } + + dev_name = calc_dev_name(bflow); + log_debug("dev_name '%s' devnum_str '%s' fname '%s' media_dev '%s'\n", + dev_name, devnum_str, bflow->fname, media_dev->name); + if (!dev_name) + return EFI_UNSUPPORTED; + ret = calculate_paths(dev_name, devnum_str, bflow->fname, &device, + &image); + if (ret) + return ret; + + if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) { + log_debug("Booting with built-in fdt\n"); + fdt = EFI_FDT_USE_INTERNAL; + } else { + log_debug("Booting with external fdt\n"); + fdt = map_sysmem(bflow->fdt_addr, 0); + } + ret = efi_binary_run_(bflow->buf, bflow->size, fdt, device, image); + + return ret; +} +
Rather than setting up the global variables and then making the call, pass them into function directly. This cleans up the code and makes it all a bit easier to understand.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Use efi_loader tag instead of efi - Drop unnecessary path removal - Fix 'require' typo - Move calculation of dev-name into a separate function
boot/bootmeth_efi.c | 49 +------------------------ include/efi_loader.h | 10 +++++ lib/efi_loader/efi_bootbin.c | 71 ++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 47 deletions(-)
Applied to sjg/master, thanks!
Finish off the implementation so it is possible to boot an EFI app over a network.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add new patch to support PXE booting
boot/bootmeth_efi.c | 15 +-------------- lib/efi_loader/efi_bootbin.c | 5 ++++- 2 files changed, 5 insertions(+), 15 deletions(-)
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index b745ba8bd4b..0c9b4c3d59d 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -210,6 +210,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) if (size <= 0) return log_msg_ret("sz", -EINVAL); bflow->size = size; + bflow->buf = map_sysmem(addr, size);
/* bootfile should be setup by dhcp */ bootfile_name = env_get("bootfile"); @@ -219,10 +220,6 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) if (!bflow->fname) return log_msg_ret("fi0", -ENOMEM);
- /* do the hideous EFI hack */ - efi_set_bootdev("Net", "", bflow->fname, map_sysmem(addr, 0), - bflow->size); - /* read the DT file also */ fdt_addr_str = env_get("fdt_addr_r"); if (!fdt_addr_str) @@ -296,16 +293,6 @@ static int distro_efi_boot(struct udevice *dev, struct bootflow *bflow) if (bflow->flags & ~BOOTFLOWF_USE_BUILTIN_FDT) fdt = bflow->fdt_addr;
- } else { - /* - * This doesn't actually work for network devices: - * - * do_bootefi_image() No UEFI binary known at 0x02080000 - * - * But this is the same behaviour for distro boot, so it can be - * fixed here. - */ - fdt = env_get_hex("fdt_addr_r", 0); }
if (efi_bootflow_run(bflow)) diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index 8efd779d9f1..d02c254c2bd 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -270,6 +270,9 @@ static const char *calc_dev_name(struct bootflow *bflow) media_dev = dev_get_parent(bflow->dev);
if (!bflow->blk) { + if (device_get_uclass_id(media_dev) == UCLASS_ETH) + return "Net"; + log_err("Cannot boot EFI app on media '%s'\n", dev_get_uclass_name(media_dev));
@@ -310,7 +313,7 @@ efi_status_t efi_bootflow_run(struct bootflow *bflow) ret = calculate_paths(dev_name, devnum_str, bflow->fname, &device, &image); if (ret) - return ret; + return EFI_UNSUPPORTED;
if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) { log_debug("Booting with built-in fdt\n");
Finish off the implementation so it is possible to boot an EFI app over a network.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add new patch to support PXE booting
boot/bootmeth_efi.c | 15 +-------------- lib/efi_loader/efi_bootbin.c | 5 ++++- 2 files changed, 5 insertions(+), 15 deletions(-)
Applied to sjg/master, thanks!
On Wed, Dec 11, 2024 at 03:38:15PM -0700, Simon Glass wrote:
At present there is a function, efi_set_bootdev(), which is used in various places to tell the EFI loader which device a file came from.
With bootstd, this information is available in the bootflow.
This little series provides a way for bootstd to provide the bootflow to the EFI loader, so that it is able to select the correct paths.
For now only the EFI bootmeth is updated.
Hi Simon,
I think as a general rule it would help everyone if you had no more than one outstanding series per subsystem per week. This is 30 efi_loader patches in 24 hours and that's _a_lot_. One could easily argue that's too much. Thanks.
Hi Tom,
On Wed, 11 Dec 2024 at 15:50, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 11, 2024 at 03:38:15PM -0700, Simon Glass wrote:
At present there is a function, efi_set_bootdev(), which is used in various places to tell the EFI loader which device a file came from.
With bootstd, this information is available in the bootflow.
This little series provides a way for bootstd to provide the bootflow to the EFI loader, so that it is able to select the correct paths.
For now only the EFI bootmeth is updated.
Hi Simon,
I think as a general rule it would help everyone if you had no more than one outstanding series per subsystem per week. This is 30 efi_loader patches in 24 hours and that's _a_lot_. One could easily argue that's too much. Thanks.
Sure, I'm happy to send fewer patches. Could you go into a little more detail about what 'per week' means in this context. Can you give me an example?
This series has been sitting around since mid-October. It just didn't get reviewed / applied. I just hit a situation where I needed PXE booting, so I added another patch and sent a v2.
Regards, Simon
On Wed, Dec 11, 2024 at 05:58:12PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 11 Dec 2024 at 15:50, Tom Rini trini@konsulko.com wrote:
On Wed, Dec 11, 2024 at 03:38:15PM -0700, Simon Glass wrote:
At present there is a function, efi_set_bootdev(), which is used in various places to tell the EFI loader which device a file came from.
With bootstd, this information is available in the bootflow.
This little series provides a way for bootstd to provide the bootflow to the EFI loader, so that it is able to select the correct paths.
For now only the EFI bootmeth is updated.
Hi Simon,
I think as a general rule it would help everyone if you had no more than one outstanding series per subsystem per week. This is 30 efi_loader patches in 24 hours and that's _a_lot_. One could easily argue that's too much. Thanks.
Sure, I'm happy to send fewer patches. Could you go into a little more detail about what 'per week' means in this context. Can you give me an example?
Sure. No new patches, no re-spinnning patches, no re-posting patches that touch lib/efi_loader/* until next Wednesday.
This series has been sitting around since mid-October. It just didn't get reviewed / applied. I just hit a situation where I needed PXE booting, so I added another patch and sent a v2.
Per the changelog, you also made a few other changes in here since v1 other than picking up Ilias' tag.
Please also note that it's been not quite a week since you posted the 46 part "pxe" series, and that needs to be broken up in to something reviewable-sized too.
participants (3)
-
Ilias Apalodimas -
Simon Glass -
Tom Rini