[PATCH v3 6/7] efi_loader: Pass in the required parameters from EFI bootmeth

[Heinrich Schuchardt]

On 12/19/24 03:38, Simon Glass wrote:
> 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 at chromium.org>
> ---
>
> (no changes since v2)
>
> 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

Please, split the patch into logical units.

>
>   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 f836aa655f5..fa7f66c9a0b 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;
> @@ -103,8 +69,6 @@ static int efiload_read_file(struct bootflow *bflow, ulong addr)
>   		return log_msg_ret("read", ret);
>   	bflow->buf = map_sysmem(addr, bflow->size);
>
> -	set_efi_bootdev(desc, bflow);
> -
>   	return 0;
>   }
>
> @@ -342,17 +306,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 9afbec35ebf..a9325b1a9d0 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -20,6 +20,7 @@
>   #include <linux/oid_registry.h>
>
>   struct blk_desc;
> +struct bootflow;
>   struct jmp_buf_data;
>
>   #if CONFIG_IS_ENABLED(EFI_LOADER)
> @@ -578,6 +579,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 8332993f421..b5d15a723b4 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;
> @@ -285,3 +288,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);

Where do you handle network devices?

Best regards

Heinrich

> +
> +		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;
> +}
> +