Re: [PATCH v4 3/7] efi_loader: Update efi_run_image() to accept image and device path

[Heinrich Schuchardt]

On 1/9/25 16:02, Simon Glass 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 <s...@chromium.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
---

(no changes since v2)

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 428991df88f..e3ac27b03fa 100644
--- a/lib/efi_loader/efi_bootbin.c
+++ b/lib/efi_loader/efi_bootbin.c
@@ -157,9 +157,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)

Please, use parameter names that make it clear that we are dealing with
device paths, e.g. dp_dev and dp_img.

  {
        efi_handle_t mem_handle = NULL, handle;
        struct efi_device_path *file_path = NULL;
@@ -167,7 +171,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,
@@ -188,9 +192,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");
        }

@@ -227,7 +230,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
   *
@@ -236,7 +239,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)

This is an unrelated change.
What is the point of adding a _ptr suffix here?
We are only dealing with pointers in the EFI sub-system and keep sandbox
virtual addresses out.

Best regards

Heinrich

  {
        efi_status_t ret;

@@ -252,5 +255,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);
  }