[RFC 0/3] Change logic of EFI LoadFile2 protocol for initrd loading

[RFC 0/3] Change logic of EFI LoadFile2 protocol for initrd loading

[Ilias Apalodimas]

Hi, 

This is the revised version of [1].

After the discussion we had with Akashi and Heinrich, storing a device path
in the variable is preferable to a custom U-Boot string.

In hindsight and after reading the EFI spec a bit further, using a custom 
EFI variable to hold that device path doesn't look great either, we can do 
better than that.
Turns out we can use the array of device paths defined in the EFI spec and
store OS specific device paths [2].
So I converted this to an RFC instead, exploring this idea. The variabale is 
now stored as the second array member in the EFI_LOAD_OPTIONS and later 
retrieved in order to load the file when the kernel requests it.
Another big change is that installing the EFI protocol itself will cause an
error if the file's not found and the bootmgr will fallback to other valid 
boot options.

The efi selftest for the load option itself is not included in the RFC.
I'll go ahead and change it if there's general agreement on the feature.

[1] https://lists.denx.de/pipermail/u-boot/2020-December/436080.html
[2] https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf section 3.1.3

Ilias Apalodimas (3):
  efi_loader: Introduce helper functions for EFI
  efi_loader: efi_loader: Replace config option for initrd loading
  efidebug: add multiple device path instances on Boot####

 cmd/bootefi.c                    |   3 +
 cmd/efidebug.c                   |  89 ++++++++++++++++--
 include/efi_helper.h             |  23 +++++
 include/efi_loader.h             |   1 +
 lib/efi_loader/Kconfig           |  13 +--
 lib/efi_loader/efi_bootmgr.c     |   3 +
 lib/efi_loader/efi_helper.c      | 146 +++++++++++++++++++++++++++++
 lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++---------------
 8 files changed, 341 insertions(+), 91 deletions(-)
 create mode 100644 include/efi_helper.h
 create mode 100644 lib/efi_loader/efi_helper.c

-- 
2.30.0.rc2

[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

A following patch introduces a different logic for loading initrd's
based on the EFI_LOAD_FILE2_PROTOCOL.
Since similar logic can be applied in the future for other system files
(i.e DTBs), let's add some helper functions which will retrieve and
parse file paths stored in EFI load options.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_helper.h        |  23 ++++++
 lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)
 create mode 100644 include/efi_helper.h
 create mode 100644 lib/efi_loader/efi_helper.c

diff --git a/include/efi_helper.h b/include/efi_helper.h
new file mode 100644
index 000000000000..4e6bd2f036df
--- /dev/null
+++ b/include/efi_helper.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Linaro Limited
+ */
+
+#if !defined _EFI_HELPER_H_
+#define _EFI_HELPER_H
+
+#include <efi.h>
+#include <efi_api.h>
+
+enum load_option_dp_type {
+	BOOT_IMAGE_DP,
+	INITRD_DP,
+	DTB_DP,
+};
+
+void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
+struct efi_device_path *efi_get_fp_from_boot(int idx);
+struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
+					       efi_uintn_t *size, int idx);
+
+#endif
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
new file mode 100644
index 000000000000..e2437a4f698b
--- /dev/null
+++ b/lib/efi_loader/efi_helper.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020, Linaro Limited
+ */
+
+#include <common.h>
+#include <env.h>
+#include <malloc.h>
+#include <dm.h>
+#include <fs.h>
+#include <efi_helper.h>
+#include <efi_loader.h>
+#include <efi_variable.h>
+
+/**
+ * efi_get_var() - read value of an EFI variable
+ *
+ * @name:	variable name
+ * @start:	vendor GUID
+ * @size:	size of allocated buffer
+ *
+ * Return:	buffer with variable data or NULL
+ */
+void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
+{
+	efi_status_t ret;
+	void *buf = NULL;
+
+	*size = 0;
+	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		buf = malloc(*size);
+		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
+	}
+
+	if (ret != EFI_SUCCESS) {
+		free(buf);
+		*size = 0;
+		return NULL;
+	}
+
+	return buf;
+}
+
+/**
+ * efi_dp_instance_by_idx() - Get a file path with a specific index
+ *
+ * @name:	device path array
+ * @size:	size of the discovered device path
+ * @idx:	index of the device path
+ *
+ * Return:	device path or NULL. Caller must free the returned value
+ */
+
+struct
+efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
+					efi_uintn_t *size, int idx)
+{
+	struct efi_device_path *instance = NULL;
+	efi_uintn_t instance_size = 0;
+
+	if (!efi_dp_is_multi_instance(dp))
+		return NULL;
+
+	while (idx >= 0 && dp) {
+		instance = efi_dp_get_next_instance(&dp, &instance_size);
+		if (idx && instance) {
+			efi_free_pool(instance);
+			instance_size = 0;
+			instance = NULL;
+		}
+		idx--;
+	}
+	*size = instance_size;
+
+	return instance;
+}
+
+/**
+ * create_boot_var_indexed() - Return Boot#### name were #### is replaced by
+ *			       the value of BootCurrent
+ *
+ * @var_name:		variable name
+ * @var_name_size:	size of var_name
+ *
+ * Return:	Status code
+ */
+static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size)
+{
+	efi_uintn_t boot_order_size;
+	efi_status_t ret;
+	u16 boot_order;
+	u16 *pos;
+
+	boot_order_size = sizeof(boot_order);
+	ret = efi_get_variable_int(L"BootCurrent",
+				   &efi_global_variable_guid, NULL,
+				   &boot_order_size, &boot_order, NULL);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	pos = efi_create_indexed_name(var_name, var_name_size, "Boot",
+				      boot_order);
+	if (!pos) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * efi_get_fp_from_var() - Retrieve and return a device path from an EFI
+ *			   Boot### variable
+ *
+ * @idx:	index of the instance to retrieve
+ *
+ * Return:	device path of NULL. Caller must free the returned value
+ */
+struct efi_device_path *efi_get_fp_from_boot(int idx)
+{
+	struct efi_device_path *file_path;
+	struct efi_load_option lo;
+	void *var_value = NULL;
+	efi_uintn_t size;
+	efi_status_t ret;
+	u16 var_name[16];
+
+	ret = create_boot_var_indexed(var_name, sizeof(var_name));
+	if (ret != EFI_SUCCESS)
+		return NULL;
+
+	var_value = efi_get_var(var_name, &efi_global_variable_guid, &size);
+	if (!var_value)
+		return NULL;
+
+	efi_deserialize_load_option(&lo, var_value, &size);
+	file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx);
+	if (!file_path) {
+		printf("Instance not found\n");
+		return NULL;
+	}
+
+	return file_path;
+}
-- 
2.30.0.rc2

[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Heinrich Schuchardt]

On 13.01.21 12:11, Ilias Apalodimas wrote:
> A following patch introduces a different logic for loading initrd's
> based on the EFI_LOAD_FILE2_PROTOCOL.
> Since similar logic can be applied in the future for other system files
> (i.e DTBs), let's add some helper functions which will retrieve and
> parse file paths stored in EFI load options.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  include/efi_helper.h        |  23 ++++++
>  lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 169 insertions(+)
>  create mode 100644 include/efi_helper.h
>  create mode 100644 lib/efi_loader/efi_helper.c
>
> diff --git a/include/efi_helper.h b/include/efi_helper.h
> new file mode 100644
> index 000000000000..4e6bd2f036df
> --- /dev/null
> +++ b/include/efi_helper.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#if !defined _EFI_HELPER_H_
> +#define _EFI_HELPER_H
> +
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +enum load_option_dp_type {
> +	BOOT_IMAGE_DP,
> +	INITRD_DP,
> +	DTB_DP,
> +};
> +
> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
> +struct efi_device_path *efi_get_fp_from_boot(int idx);
> +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> +					       efi_uintn_t *size, int idx);
> +
> +#endif
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> new file mode 100644
> index 000000000000..e2437a4f698b
> --- /dev/null
> +++ b/lib/efi_loader/efi_helper.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <env.h>
> +#include <malloc.h>
> +#include <dm.h>
> +#include <fs.h>
> +#include <efi_helper.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +
> +/**
> + * efi_get_var() - read value of an EFI variable
> + *
> + * @name:	variable name
> + * @start:	vendor GUID
> + * @size:	size of allocated buffer
> + *
> + * Return:	buffer with variable data or NULL
> + */
> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
> +{
> +	efi_status_t ret;
> +	void *buf = NULL;
> +
> +	*size = 0;
> +	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		buf = malloc(*size);

Please, always check the output of malloc(), e.g.

		if (!buf)
			ret = EFI_OUT_OF_RESOURCES;
		else

> +		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	}
> +
> +	if (ret != EFI_SUCCESS) {
> +		free(buf);
> +		*size = 0;
> +		return NULL;
> +	}
> +
> +	return buf;
> +}
> +
> +/**
> + * efi_dp_instance_by_idx() - Get a file path with a specific index
> + *
> + * @name:	device path array
> + * @size:	size of the discovered device path
> + * @idx:	index of the device path
> + *
> + * Return:	device path or NULL. Caller must free the returned value
> + */
> +
> +struct
> +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> +					efi_uintn_t *size, int idx)
> +{

idx should be of an unsigned type as we cannot have negative indices.

> +	struct efi_device_path *instance = NULL;
> +	efi_uintn_t instance_size = 0;
> +
> +	if (!efi_dp_is_multi_instance(dp))

Given a device path with one instance, why don't you allow me to read
index 0? I assume this check can be removed.

> +		return NULL;
> +
> +	while (idx >= 0 && dp) {
> +		instance = efi_dp_get_next_instance(&dp, &instance_size);
> +		if (idx && instance) {
> +			efi_free_pool(instance);
> +			instance_size = 0;
> +			instance = NULL;
> +		}
> +		idx--;
> +	}
> +	*size = instance_size;
> +
> +	return instance;

This can be simplified with unsigned idx:

for (; dp; --idx) {
	instance = efi_dp_get_next_instance(&dp, size);
	if (!idx)  {
		return instance;
	efi_free_pool(instance);
}
return NULL;

> +}
> +
> +/**
> + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by
> + *			       the value of BootCurrent
> + *
> + * @var_name:		variable name
> + * @var_name_size:	size of var_name
> + *
> + * Return:	Status code
> + */
> +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size)
> +{
> +	efi_uintn_t boot_order_size;

You are reading BootCurrent, not BootOrder.

> +	efi_status_t ret;
> +	u16 boot_order;

Same here.

> +	u16 *pos;
> +
> +	boot_order_size = sizeof(boot_order);
> +	ret = efi_get_variable_int(L"BootCurrent",
> +				   &efi_global_variable_guid, NULL,
> +				   &boot_order_size, &boot_order, NULL);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	pos = efi_create_indexed_name(var_name, var_name_size, "Boot",
> +				      boot_order);
> +	if (!pos) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +/**
> + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI
> + *			   Boot### variable
> + *
> + * @idx:	index of the instance to retrieve
> + *
> + * Return:	device path of NULL. Caller must free the returned value

%s/of/or/

> + */
> +struct efi_device_path *efi_get_fp_from_boot(int idx)
> +{
> +	struct efi_device_path *file_path;
> +	struct efi_load_option lo;
> +	void *var_value = NULL;
> +	efi_uintn_t size;
> +	efi_status_t ret;
> +	u16 var_name[16];
> +
> +	ret = create_boot_var_indexed(var_name, sizeof(var_name));
> +	if (ret != EFI_SUCCESS)
> +		return NULL;
> +
> +	var_value = efi_get_var(var_name, &efi_global_variable_guid, &size);
> +	if (!var_value)
> +		return NULL;
> +
> +	efi_deserialize_load_option(&lo, var_value, &size);
> +	file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx);
> +	if (!file_path) {
> +		printf("Instance not found\n");

This message is not enough for a user to take action. I suggest

("Missing file path with index %d in %ls", idx, var_name)

We want to use logging. I assume this is not an error. Can we use
log_debug() here?

> +		return NULL;
> +	}
> +
> +	return file_path;
> +}
>

Some other functions would fit into the same C module. Candidates are:

* efi_create_indexed_name()
* efi_deserialize_load_option()
* efi_serialize_load_option()

But that can be done in a separate patch.

Best regards

Heinrich

[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

Hi Heinrich,
> > +	efi_status_t ret;
> > +	void *buf = NULL;
> > +
> > +	*size = 0;
> > +	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> > +	if (ret == EFI_BUFFER_TOO_SMALL) {
> > +		buf = malloc(*size);
> 
> Please, always check the output of malloc(), e.g.
> 
> 		if (!buf)
> 			ret = EFI_OUT_OF_RESOURCES;
> 		else
> 

I just moved the function from lib/efi_loader/efi_bootmgr.c (check for
get_var()) and completely missed that. I'll fix it on the next revision.

> > +		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> > +	}
> > +
> > +	if (ret != EFI_SUCCESS) {
> > +		free(buf);
> > +		*size = 0;
> > +		return NULL;
> > +	}
> > +
> > +	return buf;
> > +}
> > +
> > +/**
> > + * efi_dp_instance_by_idx() - Get a file path with a specific index
> > + *
> > + * @name:	device path array
> > + * @size:	size of the discovered device path
> > + * @idx:	index of the device path
> > + *
> > + * Return:	device path or NULL. Caller must free the returned value
> > + */
> > +
> > +struct
> > +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> > +					efi_uintn_t *size, int idx)
> > +{
> 
> idx should be of an unsigned type as we cannot have negative indices.
> 
> > +	struct efi_device_path *instance = NULL;
> > +	efi_uintn_t instance_size = 0;
> > +
> > +	if (!efi_dp_is_multi_instance(dp))
> 
> Given a device path with one instance, why don't you allow me to read
> index 0? I assume this check can be removed.
> 

Yea, but why would you ever use that? you can just retrieve the deserialized
device path directly if there are no other instances.

> > +		return NULL;
> > +
> > +	while (idx >= 0 && dp) {
> > +		instance = efi_dp_get_next_instance(&dp, &instance_size);
> > +		if (idx && instance) {
> > +			efi_free_pool(instance);
> > +			instance_size = 0;
> > +			instance = NULL;
> > +		}
> > +		idx--;
> > +	}
> > +	*size = instance_size;
> > +
> > +	return instance;
> 
> This can be simplified with unsigned idx:
> 
> for (; dp; --idx) {
> 	instance = efi_dp_get_next_instance(&dp, size);
> 	if (!idx)  {
> 		return instance;
> 	efi_free_pool(instance);
> }
> return NULL;

Ok I'll have a look

> 
> > +}
> > +
> > +/**
> > + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by
> > + *			       the value of BootCurrent
> > + *
> > + * @var_name:		variable name
> > + * @var_name_size:	size of var_name
> > + *
> > + * Return:	Status code
> > + */
> > +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size)
> > +{
> > +	efi_uintn_t boot_order_size;
> 
> You are reading BootCurrent, not BootOrder.
> 
> > +	efi_status_t ret;
> > +	u16 boot_order;
> 
> Same here.
> 

Ok

> > +	if (!file_path) {
> > +		printf("Instance not found\n");
> 
> This message is not enough for a user to take action. I suggest
> 
> ("Missing file path with index %d in %ls", idx, var_name)
> 
> We want to use logging. I assume this is not an error. Can we use
> log_debug() here?
> 


That's a leftover from my opwn debugging that I neglected to remove prior to
the RFC. I'll just add a log_debug()

> > +		return NULL;
> > +	}
> > +
> > +	return file_path;
> > +}
> >
> 
> Some other functions would fit into the same C module. Candidates are:
> 
> * efi_create_indexed_name()
> * efi_deserialize_load_option()
> * efi_serialize_load_option()
> 
> But that can be done in a separate patch.

Yea, that's the idea, we can also use the efi_get_var() in multiple places,
but I'll send different patches for that, once we merge this.

I assume we agree on the architecture. If so I'll fix the selftests and post a
proper patch

Regards
/Ilias
> 
> Best regards
> 
> Heinrich

[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[AKASHI Takahiro]

On Wed, Jan 13, 2021 at 01:11:47PM +0200, Ilias Apalodimas wrote:
> A following patch introduces a different logic for loading initrd's
> based on the EFI_LOAD_FILE2_PROTOCOL.
> Since similar logic can be applied in the future for other system files
> (i.e DTBs),

In this respect,

> let's add some helper functions which will retrieve and
> parse file paths stored in EFI load options.
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  include/efi_helper.h        |  23 ++++++
>  lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 169 insertions(+)
>  create mode 100644 include/efi_helper.h
>  create mode 100644 lib/efi_loader/efi_helper.c
> 
> diff --git a/include/efi_helper.h b/include/efi_helper.h
> new file mode 100644
> index 000000000000..4e6bd2f036df
> --- /dev/null
> +++ b/include/efi_helper.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#if !defined _EFI_HELPER_H_
> +#define _EFI_HELPER_H
> +
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +enum load_option_dp_type {
> +	BOOT_IMAGE_DP,
> +	INITRD_DP,
> +	DTB_DP,
> +};
> +
> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
> +struct efi_device_path *efi_get_fp_from_boot(int idx);
> +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> +					       efi_uintn_t *size, int idx);
> +
> +#endif
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> new file mode 100644
> index 000000000000..e2437a4f698b
> --- /dev/null
> +++ b/lib/efi_loader/efi_helper.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <env.h>
> +#include <malloc.h>
> +#include <dm.h>
> +#include <fs.h>
> +#include <efi_helper.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +
> +/**
> + * efi_get_var() - read value of an EFI variable
> + *
> + * @name:	variable name
> + * @start:	vendor GUID
> + * @size:	size of allocated buffer
> + *
> + * Return:	buffer with variable data or NULL
> + */
> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
> +{
> +	efi_status_t ret;
> +	void *buf = NULL;
> +
> +	*size = 0;
> +	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		buf = malloc(*size);
> +		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	}
> +
> +	if (ret != EFI_SUCCESS) {
> +		free(buf);
> +		*size = 0;
> +		return NULL;
> +	}
> +
> +	return buf;
> +}
> +
> +/**
> + * efi_dp_instance_by_idx() - Get a file path with a specific index
> + *
> + * @name:	device path array
> + * @size:	size of the discovered device path
> + * @idx:	index of the device path
> + *
> + * Return:	device path or NULL. Caller must free the returned value
> + */
> +
> +struct
> +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> +					efi_uintn_t *size, int idx)

The type of "idx" should be 'enum load_option_dp_type'.

Currently, "idx" is used as an index into the array of device paths,
but given each device path is set to have its own guid, "idx" should be
unlinked from the 'order' within the array.

Even if you don't want so, this function should at least take care of
some special cases like
<execution image> + (no initrd) + <dtb>
Alternatively, "END device path" can be put at the second place.

I expect that handling those corner cases should be described explicitly.

> +{
> +	struct efi_device_path *instance = NULL;
> +	efi_uintn_t instance_size = 0;
> +
> +	if (!efi_dp_is_multi_instance(dp))
> +		return NULL;
> +
> +	while (idx >= 0 && dp) {
> +		instance = efi_dp_get_next_instance(&dp, &instance_size);
> +		if (idx && instance) {
> +			efi_free_pool(instance);
> +			instance_size = 0;
> +			instance = NULL;
> +		}
> +		idx--;
> +	}
> +	*size = instance_size;
> +
> +	return instance;
> +}
> +
> +/**
> + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by
> + *			       the value of BootCurrent
> + *
> + * @var_name:		variable name
> + * @var_name_size:	size of var_name
> + *
> + * Return:	Status code
> + */
> +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size)
> +{
> +	efi_uintn_t boot_order_size;
> +	efi_status_t ret;
> +	u16 boot_order;
> +	u16 *pos;
> +
> +	boot_order_size = sizeof(boot_order);
> +	ret = efi_get_variable_int(L"BootCurrent",
> +				   &efi_global_variable_guid, NULL,
> +				   &boot_order_size, &boot_order, NULL);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	pos = efi_create_indexed_name(var_name, var_name_size, "Boot",
> +				      boot_order);
> +	if (!pos) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +/**
> + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI
> + *			   Boot### variable
> + *
> + * @idx:	index of the instance to retrieve
> + *
> + * Return:	device path of NULL. Caller must free the returned value
> + */
> +struct efi_device_path *efi_get_fp_from_boot(int idx)

Are you sure that this function is called only when "BootCurrent"
is appropriately set?

> +{
> +	struct efi_device_path *file_path;
> +	struct efi_load_option lo;
> +	void *var_value = NULL;
> +	efi_uintn_t size;
> +	efi_status_t ret;
> +	u16 var_name[16];

The size of var_name: 16 -> 8

-Takahiro Akashi

> +	ret = create_boot_var_indexed(var_name, sizeof(var_name));
> +	if (ret != EFI_SUCCESS)
> +		return NULL;
> +
> +	var_value = efi_get_var(var_name, &efi_global_variable_guid, &size);
> +	if (!var_value)
> +		return NULL;
> +
> +	efi_deserialize_load_option(&lo, var_value, &size);
> +	file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx);
> +	if (!file_path) {
> +		printf("Instance not found\n");
> +		return NULL;
> +	}
> +
> +	return file_path;
> +}
> -- 
> 2.30.0.rc2
> 

[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Heinrich Schuchardt]

Am 14. Januar 2021 05:48:28 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>On Wed, Jan 13, 2021 at 01:11:47PM +0200, Ilias Apalodimas wrote:
>> A following patch introduces a different logic for loading initrd's
>> based on the EFI_LOAD_FILE2_PROTOCOL.
>> Since similar logic can be applied in the future for other system
>files
>> (i.e DTBs),
>
>In this respect,
>
>> let's add some helper functions which will retrieve and
>> parse file paths stored in EFI load options.
>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> ---
>>  include/efi_helper.h        |  23 ++++++
>>  lib/efi_loader/efi_helper.c | 146
>++++++++++++++++++++++++++++++++++++
>>  2 files changed, 169 insertions(+)
>>  create mode 100644 include/efi_helper.h
>>  create mode 100644 lib/efi_loader/efi_helper.c
>> 
>> diff --git a/include/efi_helper.h b/include/efi_helper.h
>> new file mode 100644
>> index 000000000000..4e6bd2f036df
>> --- /dev/null
>> +++ b/include/efi_helper.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (c) 2020, Linaro Limited
>> + */
>> +
>> +#if !defined _EFI_HELPER_H_
>> +#define _EFI_HELPER_H
>> +
>> +#include <efi.h>
>> +#include <efi_api.h>
>> +
>> +enum load_option_dp_type {
>> +	BOOT_IMAGE_DP,
>> +	INITRD_DP,
>> +	DTB_DP,
>> +};
>> +
>> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t
>*size);
>> +struct efi_device_path *efi_get_fp_from_boot(int idx);
>> +struct efi_device_path *efi_dp_instance_by_idx(struct
>efi_device_path *dp,
>> +					       efi_uintn_t *size, int idx);
>> +
>> +#endif
>> diff --git a/lib/efi_loader/efi_helper.c
>b/lib/efi_loader/efi_helper.c
>> new file mode 100644
>> index 000000000000..e2437a4f698b
>> --- /dev/null
>> +++ b/lib/efi_loader/efi_helper.c
>> @@ -0,0 +1,146 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2020, Linaro Limited
>> + */
>> +
>> +#include <common.h>
>> +#include <env.h>
>> +#include <malloc.h>
>> +#include <dm.h>
>> +#include <fs.h>
>> +#include <efi_helper.h>
>> +#include <efi_loader.h>
>> +#include <efi_variable.h>
>> +
>> +/**
>> + * efi_get_var() - read value of an EFI variable
>> + *
>> + * @name:	variable name
>> + * @start:	vendor GUID
>> + * @size:	size of allocated buffer
>> + *
>> + * Return:	buffer with variable data or NULL
>> + */
>> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t
>*size)
>> +{
>> +	efi_status_t ret;
>> +	void *buf = NULL;
>> +
>> +	*size = 0;
>> +	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
>> +	if (ret == EFI_BUFFER_TOO_SMALL) {
>> +		buf = malloc(*size);
>> +		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
>> +	}
>> +
>> +	if (ret != EFI_SUCCESS) {
>> +		free(buf);
>> +		*size = 0;
>> +		return NULL;
>> +	}
>> +
>> +	return buf;
>> +}
>> +
>> +/**
>> + * efi_dp_instance_by_idx() - Get a file path with a specific index
>> + *
>> + * @name:	device path array
>> + * @size:	size of the discovered device path
>> + * @idx:	index of the device path
>> + *
>> + * Return:	device path or NULL. Caller must free the returned value
>> + */
>> +
>> +struct
>> +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
>> +					efi_uintn_t *size, int idx)
>
>The type of "idx" should be 'enum load_option_dp_type'.

There is nothing boot manager specific in this function. You can use it to access any multiple device path.

An enum or #define can be used on the caller side.

Should this function be moved to efi_devicepath.c?

Best regards

Heinrich

>
>Currently, "idx" is used as an index into the array of device paths,
>but given each device path is set to have its own guid, "idx" should be
>unlinked from the 'order' within the array.
>
>Even if you don't want so, this function should at least take care of
>some special cases like
><execution image> + (no initrd) + <dtb>
>Alternatively, "END device path" can be put at the second place.
>
>I expect that handling those corner cases should be described
>explicitly.
>
>> +{
>> +	struct efi_device_path *instance = NULL;
>> +	efi_uintn_t instance_size = 0;
>> +
>> +	if (!efi_dp_is_multi_instance(dp))
>> +		return NULL;
>> +
>> +	while (idx >= 0 && dp) {
>> +		instance = efi_dp_get_next_instance(&dp, &instance_size);
>> +		if (idx && instance) {
>> +			efi_free_pool(instance);
>> +			instance_size = 0;
>> +			instance = NULL;
>> +		}
>> +		idx--;
>> +	}
>> +	*size = instance_size;
>> +
>> +	return instance;
>> +}
>> +
>> +/**
>> + * create_boot_var_indexed() - Return Boot#### name were #### is
>replaced by
>> + *			       the value of BootCurrent
>> + *
>> + * @var_name:		variable name
>> + * @var_name_size:	size of var_name
>> + *
>> + * Return:	Status code
>> + */
>> +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t
>var_name_size)
>> +{
>> +	efi_uintn_t boot_order_size;
>> +	efi_status_t ret;
>> +	u16 boot_order;
>> +	u16 *pos;
>> +
>> +	boot_order_size = sizeof(boot_order);
>> +	ret = efi_get_variable_int(L"BootCurrent",
>> +				   &efi_global_variable_guid, NULL,
>> +				   &boot_order_size, &boot_order, NULL);
>> +	if (ret != EFI_SUCCESS)
>> +		goto out;
>> +
>> +	pos = efi_create_indexed_name(var_name, var_name_size, "Boot",
>> +				      boot_order);
>> +	if (!pos) {
>> +		ret = EFI_OUT_OF_RESOURCES;
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +/**
>> + * efi_get_fp_from_var() - Retrieve and return a device path from an
>EFI
>> + *			   Boot### variable
>> + *
>> + * @idx:	index of the instance to retrieve
>> + *
>> + * Return:	device path of NULL. Caller must free the returned value
>> + */
>> +struct efi_device_path *efi_get_fp_from_boot(int idx)
>
>Are you sure that this function is called only when "BootCurrent"
>is appropriately set?
>
>> +{
>> +	struct efi_device_path *file_path;
>> +	struct efi_load_option lo;
>> +	void *var_value = NULL;
>> +	efi_uintn_t size;
>> +	efi_status_t ret;
>> +	u16 var_name[16];
>
>The size of var_name: 16 -> 8
>
>-Takahiro Akashi
>
>> +	ret = create_boot_var_indexed(var_name, sizeof(var_name));
>> +	if (ret != EFI_SUCCESS)
>> +		return NULL;
>> +
>> +	var_value = efi_get_var(var_name, &efi_global_variable_guid,
>&size);
>> +	if (!var_value)
>> +		return NULL;
>> +
>> +	efi_deserialize_load_option(&lo, var_value, &size);
>> +	file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx);
>> +	if (!file_path) {
>> +		printf("Instance not found\n");
>> +		return NULL;
>> +	}
>> +
>> +	return file_path;
>> +}
>> -- 
>> 2.30.0.rc2
>> 

[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

[...]
> >> + * @size:	size of the discovered device path
> >> + * @idx:	index of the device path
> >> + *
> >> + * Return:	device path or NULL. Caller must free the returned value
> >> + */
> >> +
> >> +struct
> >> +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> >> +					efi_uintn_t *size, int idx)
> >
> >The type of "idx" should be 'enum load_option_dp_type'.
> 
> There is nothing boot manager specific in this function. You can use it to access any multiple device path.
> 
> An enum or #define can be used on the caller side.
> 
> Should this function be moved to efi_devicepath.c?

Sure it's generic enough to be used for device paths with multiple instances.

Regards
/Ilias

> Best regards
> 
> Heinrich
> 
 
 [...]

[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

Akashi-san,

> > +					efi_uintn_t *size, int idx)
[...]
> 
> The type of "idx" should be 'enum load_option_dp_type'.
> 
> Currently, "idx" is used as an index into the array of device paths,
> but given each device path is set to have its own guid, "idx" should be
> unlinked from the 'order' within the array.
> 
> Even if you don't want so, this function should at least take care of
> some special cases like
> <execution image> + (no initrd) + <dtb>
> Alternatively, "END device path" can be put at the second place.
> 
> I expect that handling those corner cases should be described explicitly.
> 

True, I'll refactor this a bit and allow us to extend it to load DTBs in the
future.

> > +{
> > +	struct efi_device_path *instance = NULL;
> > +

[...]

> > +/**
> > + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI
> > + *			   Boot### variable
> > + *
> > + * @idx:	index of the instance to retrieve
> > + *
> > + * Return:	device path of NULL. Caller must free the returned value
> > + */
> > +struct efi_device_path *efi_get_fp_from_boot(int idx)
> 
> Are you sure that this function is called only when "BootCurrent"
> is appropriately set?
> 

Yea, this is either used on the function called by the EFI-stub or
try_load_entry() where the BootCurrent value is set explicitly.

Regards
/Ilias

[RFC PATCH 2/3] efi_loader: efi_loader: Replace config option for initrd loading

[Ilias Apalodimas]

Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
unconditionally. Although we correctly return various EFI exit codes
depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the
kernel loader, only falls back to the cmdline interpreted initrd if the
protocol is not installed.

This creates a problem for EFI installers, since they won't be able to
load their own initrd and continue the installation. It also makes the
feature hard to use, since we can either have a single initrd or we have
to recompile u-boot if the filename changes.

So let's introduce a different logic that will decouple the initrd
path from the config option we currently have.
When defining a UEFI BootXXXX we can use the filepathlist and store
a file path pointing to our initrd. Specifically the EFI spec describes:

"The first element of the array is a device path that describes the device
and location of the Image for this load option. Other device paths may
optionally exist in the FilePathList, but their usage is OSV specific"

When the EFI application is launched through the bootmgr, we'll try to
interpret the extra device path. If that points to a file that exists on
our disk, we'll now install the load_file2 and the efi-stub will be able
to use it.

This opens up another path using U-Boot and defines a new boot flow.
A user will be able to control the kernel/initrd pairs without explicit
cmdline args or GRUB.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 cmd/bootefi.c                    |   3 +
 include/efi_loader.h             |   1 +
 lib/efi_loader/Kconfig           |  13 +--
 lib/efi_loader/efi_bootmgr.c     |   3 +
 lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++---------------
 5 files changed, 91 insertions(+), 83 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index fdf909f8da2c..053927d5d986 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -357,6 +357,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
 
 	free(load_options);
 
+	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
+		efi_initrd_deregister();
+
 	return ret;
 }
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 4719fa93f06d..5d2e161963c3 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -432,6 +432,7 @@ efi_status_t efi_net_register(void);
 /* Called by bootefi to make the watchdog available */
 efi_status_t efi_watchdog_register(void);
 efi_status_t efi_initrd_register(void);
+void efi_initrd_deregister(void);
 /* Called by bootefi to make SMBIOS tables available */
 /**
  * efi_acpi_register() - write out ACPI tables
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index fdf245dea30b..597a3ee86c88 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -307,14 +307,11 @@ config EFI_LOAD_FILE2_INITRD
 	help
 	  Expose a EFI_FILE_LOAD2_PROTOCOL that the Linux UEFI stub can
 	  use to load the initial ramdisk. Once this is enabled using
-	  initrd=<ramdisk> will stop working.
-
-config EFI_INITRD_FILESPEC
-	string "initramfs path"
-	default "host 0:1 initrd"
-	depends on EFI_LOAD_FILE2_INITRD
-	help
-	  Full path of the initramfs file, e.g. mmc 0:2 initramfs.cpio.gz.
+	  initrd=<ramdisk> will stop working. The protocol will only be
+	  installed if bootmgr is used and the file is found on the defined
+	  path. A boot entry of Boot0001 will try to match Initrd0001 and use
+	  it. Initrd EFI variable format should be '<dev> <part> <filename>'
+	  i.e 'mmc 0:1 boot/initrd'
 
 config EFI_SECURE_BOOT
 	bool "Enable EFI secure boot support"
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 0fe503a7f376..aa5d521535ee 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -222,6 +222,9 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
 		ret = efi_set_variable_int(L"BootCurrent",
 					   &efi_global_variable_guid,
 					   attributes, sizeof(n), &n, false);
+		/* try to register load file2 for initrd's */
+		if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
+			ret |= efi_initrd_register();
 		if (ret != EFI_SUCCESS) {
 			if (EFI_CALL(efi_unload_image(*handle))
 			    != EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index b9ee8839054f..bb0c5e63b65c 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -5,7 +5,9 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <efi_helper.h>
 #include <efi_load_initrd.h>
+#include <efi_variable.h>
 #include <fs.h>
 #include <malloc.h>
 #include <mapmem.h>
@@ -39,41 +41,10 @@ static const struct efi_initrd_dp dp = {
 	}
 };
 
-/**
- * get_file_size() - retrieve the size of initramfs, set efi status on error
- *
- * @dev:			device to read from, e.g. "mmc"
- * @part:			device partition, e.g. "0:1"
- * @file:			name of file
- * @status:			EFI exit code in case of failure
- *
- * Return:			size of file
- */
-static loff_t get_file_size(const char *dev, const char *part, const char *file,
-			    efi_status_t *status)
-{
-	loff_t sz = 0;
-	int ret;
-
-	ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
-	if (ret) {
-		*status = EFI_NO_MEDIA;
-		goto out;
-	}
-
-	ret = fs_size(file, &sz);
-	if (ret) {
-		sz = 0;
-		*status = EFI_NOT_FOUND;
-		goto out;
-	}
-
-out:
-	return sz;
-}
+static efi_handle_t efi_initrd_handle;
 
 /**
- * efi_load_file2initrd() - load initial RAM disk
+ * efi_load_file2_initrd() - load initial RAM disk
  *
  * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL
  * in order to load an initial RAM disk requested by the Linux kernel stub.
@@ -93,21 +64,15 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 		      struct efi_device_path *file_path, bool boot_policy,
 		      efi_uintn_t *buffer_size, void *buffer)
 {
-	char *filespec;
 	efi_status_t status = EFI_NOT_FOUND;
-	loff_t file_sz = 0, read_sz = 0;
-	char *dev, *part, *file;
-	char *pos;
-	int ret;
+	struct efi_device_path *initrd_path = NULL;
+	struct efi_file_info *info = NULL;
+	struct efi_file_handle *f;
+	efi_uintn_t bs;
 
 	EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
 		  buffer_size, buffer);
 
-	filespec = strdup(CONFIG_EFI_INITRD_FILESPEC);
-	if (!filespec)
-		goto out;
-	pos = filespec;
-
 	if (!this || this != &efi_lf2_protocol ||
 	    !buffer_size) {
 		status = EFI_INVALID_PARAMETER;
@@ -125,51 +90,82 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 		goto out;
 	}
 
-	/*
-	 * expect a string with three space separated parts:
-	 *
-	 * * a block device type, e.g. "mmc"
-	 * * a device and partition identifier, e.g. "0:1"
-	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
-	 */
-	dev = strsep(&pos, " ");
-	if (!dev)
+	initrd_path = efi_get_fp_from_boot(INITRD_DP);
+	if (!initrd_path) {
+		status = EFI_NOT_FOUND;
 		goto out;
-	part = strsep(&pos, " ");
-	if (!part)
+	}
+
+	/* Open file */
+	f = efi_file_from_path(initrd_path);
+	if (!f) {
+		status = EFI_NOT_FOUND;
 		goto out;
-	file = strsep(&pos, " ");
-	if (!file)
+	}
+
+	/* Get file size */
+	bs = 0;
+	EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid,
+				     &bs, info));
+	if (status != EFI_BUFFER_TOO_SMALL) {
+		status = EFI_DEVICE_ERROR;
 		goto out;
+	}
 
-	file_sz = get_file_size(dev, part, file, &status);
-	if (!file_sz)
+	info = malloc(bs);
+	EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs,
+				     info));
+	if (status != EFI_SUCCESS)
 		goto out;
 
-	if (!buffer || *buffer_size < file_sz) {
+	bs = info->file_size;
+	//efi_load_image_from_file
+	if (!buffer || *buffer_size < bs) {
 		status = EFI_BUFFER_TOO_SMALL;
-		*buffer_size = file_sz;
+		*buffer_size = bs;
 	} else {
-		ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
-		if (ret) {
-			status = EFI_NO_MEDIA;
-			goto out;
-		}
-
-		ret = fs_read(file, map_to_sysmem(buffer), 0, *buffer_size,
-			      &read_sz);
-		if (ret || read_sz != file_sz)
-			goto out;
-		*buffer_size = read_sz;
-
-		status = EFI_SUCCESS;
+		EFI_CALL(status = f->read(f, &bs, (void *)(uintptr_t)buffer));
+		*buffer_size = bs;
 	}
 
 out:
-	free(filespec);
+	free(info);
+	efi_free_pool(initrd_path);
+	EFI_CALL(f->close(f));
 	return EFI_EXIT(status);
 }
 
+/**
+ * check_initrd() - Determine if the file defined as an initrd in Boot####
+ *		    load_options device path is present
+ *
+ * Return:	status code
+ */
+static efi_status_t check_initrd(void)
+{
+	struct efi_device_path *file_path;
+	struct efi_file_handle *f;
+
+	/*
+	 * if bootmgr is setup with and initrd, that will be the second
+	 * device path instance in our load options located in Boot####
+	 */
+	file_path = efi_get_fp_from_boot(INITRD_DP);
+	if (!file_path)
+		return EFI_NOT_FOUND;
+
+	f = efi_file_from_path(file_path);
+	if (!f) {
+		efi_free_pool(file_path);
+		return EFI_NOT_FOUND;
+	}
+
+	EFI_CALL(f->close(f));
+	efi_free_pool(file_path);
+
+	return EFI_SUCCESS;
+}
+
 /**
  * efi_initrd_register() - create handle for loading initial RAM disk
  *
@@ -182,9 +178,12 @@ out:
  */
 efi_status_t efi_initrd_register(void)
 {
-	efi_handle_t efi_initrd_handle = NULL;
 	efi_status_t ret;
 
+	ret = check_initrd();
+	if (ret != EFI_SUCCESS)
+		return ret;
+
 	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
 		       (&efi_initrd_handle,
 			/* initramfs */
@@ -196,3 +195,8 @@ efi_status_t efi_initrd_register(void)
 
 	return ret;
 }
+
+void efi_initrd_deregister(void)
+{
+	efi_delete_handle(efi_initrd_handle);
+}
-- 
2.30.0.rc2

[RFC PATCH 2/3] efi_loader: efi_loader: Replace config option for initrd loading

[Heinrich Schuchardt]

On 13.01.21 12:11, Ilias Apalodimas wrote:
> Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
> unconditionally. Although we correctly return various EFI exit codes
> depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the
> kernel loader, only falls back to the cmdline interpreted initrd if the
> protocol is not installed.
>
> This creates a problem for EFI installers, since they won't be able to
> load their own initrd and continue the installation. It also makes the
> feature hard to use, since we can either have a single initrd or we have
> to recompile u-boot if the filename changes.
>
> So let's introduce a different logic that will decouple the initrd
> path from the config option we currently have.
> When defining a UEFI BootXXXX we can use the filepathlist and store
> a file path pointing to our initrd. Specifically the EFI spec describes:
>
> "The first element of the array is a device path that describes the device
> and location of the Image for this load option. Other device paths may
> optionally exist in the FilePathList, but their usage is OSV specific"
>
> When the EFI application is launched through the bootmgr, we'll try to
> interpret the extra device path. If that points to a file that exists on
> our disk, we'll now install the load_file2 and the efi-stub will be able
> to use it.
>
> This opens up another path using U-Boot and defines a new boot flow.
> A user will be able to control the kernel/initrd pairs without explicit
> cmdline args or GRUB.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/bootefi.c                    |   3 +
>  include/efi_loader.h             |   1 +
>  lib/efi_loader/Kconfig           |  13 +--
>  lib/efi_loader/efi_bootmgr.c     |   3 +
>  lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++---------------
>  5 files changed, 91 insertions(+), 83 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index fdf909f8da2c..053927d5d986 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -357,6 +357,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>
>  	free(load_options);
>
> +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> +		efi_initrd_deregister();
> +
>  	return ret;
>  }
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 4719fa93f06d..5d2e161963c3 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -432,6 +432,7 @@ efi_status_t efi_net_register(void);
>  /* Called by bootefi to make the watchdog available */
>  efi_status_t efi_watchdog_register(void);
>  efi_status_t efi_initrd_register(void);
> +void efi_initrd_deregister(void);
>  /* Called by bootefi to make SMBIOS tables available */
>  /**
>   * efi_acpi_register() - write out ACPI tables
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index fdf245dea30b..597a3ee86c88 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -307,14 +307,11 @@ config EFI_LOAD_FILE2_INITRD
>  	help
>  	  Expose a EFI_FILE_LOAD2_PROTOCOL that the Linux UEFI stub can
>  	  use to load the initial ramdisk. Once this is enabled using
> -	  initrd=<ramdisk> will stop working.
> -
> -config EFI_INITRD_FILESPEC
> -	string "initramfs path"
> -	default "host 0:1 initrd"
> -	depends on EFI_LOAD_FILE2_INITRD
> -	help
> -	  Full path of the initramfs file, e.g. mmc 0:2 initramfs.cpio.gz.
> +	  initrd=<ramdisk> will stop working. The protocol will only be

How about

"Linux v5.7 and later can make use of this option. If the boot option
selected by the UEFI boot manager specifies an existing file to be used
as initial RAM disk, a Linux specific Load File2 protocol will be
installed and Linux 5.7+ will ignore any initrd=<ramdisk> command line
argument."

> +	  installed if bootmgr is used and the file is found on the defined
> +	  path. A boot entry of Boot0001 will try to match Initrd0001 and use

This does not match the implementation.

> +	  it. Initrd EFI variable format should be '<dev> <part> <filename>'
> +	  i.e 'mmc 0:1 boot/initrd'

"The efidebug command can be used to specify the file with the initial
RAM disk."

>
>  config EFI_SECURE_BOOT
>  	bool "Enable EFI secure boot support"
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 0fe503a7f376..aa5d521535ee 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -222,6 +222,9 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
>  		ret = efi_set_variable_int(L"BootCurrent",
>  					   &efi_global_variable_guid,
>  					   attributes, sizeof(n), &n, false);
> +		/* try to register load file2 for initrd's */
> +		if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> +			ret |= efi_initrd_register();
>  		if (ret != EFI_SUCCESS) {
>  			if (EFI_CALL(efi_unload_image(*handle))
>  			    != EFI_SUCCESS)
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index b9ee8839054f..bb0c5e63b65c 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -5,7 +5,9 @@
>
>  #include <common.h>
>  #include <efi_loader.h>
> +#include <efi_helper.h>
>  #include <efi_load_initrd.h>
> +#include <efi_variable.h>
>  #include <fs.h>
>  #include <malloc.h>
>  #include <mapmem.h>
> @@ -39,41 +41,10 @@ static const struct efi_initrd_dp dp = {
>  	}
>  };
>
> -/**
> - * get_file_size() - retrieve the size of initramfs, set efi status on error
> - *
> - * @dev:			device to read from, e.g. "mmc"
> - * @part:			device partition, e.g. "0:1"
> - * @file:			name of file
> - * @status:			EFI exit code in case of failure
> - *
> - * Return:			size of file
> - */
> -static loff_t get_file_size(const char *dev, const char *part, const char *file,
> -			    efi_status_t *status)
> -{
> -	loff_t sz = 0;
> -	int ret;
> -
> -	ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
> -	if (ret) {
> -		*status = EFI_NO_MEDIA;
> -		goto out;
> -	}
> -
> -	ret = fs_size(file, &sz);
> -	if (ret) {
> -		sz = 0;
> -		*status = EFI_NOT_FOUND;
> -		goto out;
> -	}
> -
> -out:
> -	return sz;
> -}
> +static efi_handle_t efi_initrd_handle;
>
>  /**
> - * efi_load_file2initrd() - load initial RAM disk
> + * efi_load_file2_initrd() - load initial RAM disk
>   *
>   * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL
>   * in order to load an initial RAM disk requested by the Linux kernel stub.
> @@ -93,21 +64,15 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  		      struct efi_device_path *file_path, bool boot_policy,
>  		      efi_uintn_t *buffer_size, void *buffer)
>  {
> -	char *filespec;
>  	efi_status_t status = EFI_NOT_FOUND;

In all our other coding this variable is called ret.
I would prefer to do the same here too.

Best regards

Heinrich

> -	loff_t file_sz = 0, read_sz = 0;
> -	char *dev, *part, *file;
> -	char *pos;
> -	int ret;
> +	struct efi_device_path *initrd_path = NULL;
> +	struct efi_file_info *info = NULL;
> +	struct efi_file_handle *f;
> +	efi_uintn_t bs;
>
>  	EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
>  		  buffer_size, buffer);
>
> -	filespec = strdup(CONFIG_EFI_INITRD_FILESPEC);
> -	if (!filespec)
> -		goto out;
> -	pos = filespec;
> -
>  	if (!this || this != &efi_lf2_protocol ||
>  	    !buffer_size) {
>  		status = EFI_INVALID_PARAMETER;
> @@ -125,51 +90,82 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  		goto out;
>  	}
>
> -	/*
> -	 * expect a string with three space separated parts:
> -	 *
> -	 * * a block device type, e.g. "mmc"
> -	 * * a device and partition identifier, e.g. "0:1"
> -	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
> -	 */
> -	dev = strsep(&pos, " ");
> -	if (!dev)
> +	initrd_path = efi_get_fp_from_boot(INITRD_DP);
> +	if (!initrd_path) {
> +		status = EFI_NOT_FOUND;
>  		goto out;
> -	part = strsep(&pos, " ");
> -	if (!part)
> +	}
> +
> +	/* Open file */
> +	f = efi_file_from_path(initrd_path);
> +	if (!f) {
> +		status = EFI_NOT_FOUND;
>  		goto out;
> -	file = strsep(&pos, " ");
> -	if (!file)
> +	}
> +
> +	/* Get file size */
> +	bs = 0;
> +	EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid,
> +				     &bs, info));
> +	if (status != EFI_BUFFER_TOO_SMALL) {
> +		status = EFI_DEVICE_ERROR;
>  		goto out;
> +	}
>
> -	file_sz = get_file_size(dev, part, file, &status);
> -	if (!file_sz)
> +	info = malloc(bs);
> +	EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs,
> +				     info));
> +	if (status != EFI_SUCCESS)
>  		goto out;
>
> -	if (!buffer || *buffer_size < file_sz) {
> +	bs = info->file_size;
> +	//efi_load_image_from_file
> +	if (!buffer || *buffer_size < bs) {
>  		status = EFI_BUFFER_TOO_SMALL;
> -		*buffer_size = file_sz;
> +		*buffer_size = bs;
>  	} else {
> -		ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
> -		if (ret) {
> -			status = EFI_NO_MEDIA;
> -			goto out;
> -		}
> -
> -		ret = fs_read(file, map_to_sysmem(buffer), 0, *buffer_size,
> -			      &read_sz);
> -		if (ret || read_sz != file_sz)
> -			goto out;
> -		*buffer_size = read_sz;
> -
> -		status = EFI_SUCCESS;
> +		EFI_CALL(status = f->read(f, &bs, (void *)(uintptr_t)buffer));
> +		*buffer_size = bs;
>  	}
>
>  out:
> -	free(filespec);
> +	free(info);
> +	efi_free_pool(initrd_path);
> +	EFI_CALL(f->close(f));
>  	return EFI_EXIT(status);
>  }
>
> +/**
> + * check_initrd() - Determine if the file defined as an initrd in Boot####
> + *		    load_options device path is present
> + *
> + * Return:	status code
> + */
> +static efi_status_t check_initrd(void)
> +{
> +	struct efi_device_path *file_path;
> +	struct efi_file_handle *f;
> +
> +	/*
> +	 * if bootmgr is setup with and initrd, that will be the second
> +	 * device path instance in our load options located in Boot####
> +	 */
> +	file_path = efi_get_fp_from_boot(INITRD_DP);
> +	if (!file_path)
> +		return EFI_NOT_FOUND;
> +
> +	f = efi_file_from_path(file_path);
> +	if (!f) {
> +		efi_free_pool(file_path);
> +		return EFI_NOT_FOUND;
> +	}
> +
> +	EFI_CALL(f->close(f));
> +	efi_free_pool(file_path);
> +
> +	return EFI_SUCCESS;
> +}
> +
>  /**
>   * efi_initrd_register() - create handle for loading initial RAM disk
>   *
> @@ -182,9 +178,12 @@ out:
>   */
>  efi_status_t efi_initrd_register(void)
>  {
> -	efi_handle_t efi_initrd_handle = NULL;
>  	efi_status_t ret;
>
> +	ret = check_initrd();
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
>  	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
>  		       (&efi_initrd_handle,
>  			/* initramfs */
> @@ -196,3 +195,8 @@ efi_status_t efi_initrd_register(void)
>
>  	return ret;
>  }
> +
> +void efi_initrd_deregister(void)
> +{
> +	efi_delete_handle(efi_initrd_handle);
> +}
>

[RFC PATCH 2/3] efi_loader: efi_loader: Replace config option for initrd loading

[Ilias Apalodimas]

> > +	  initrd=<ramdisk> will stop working. The protocol will only be
[...]
> 
> How about
> 
> "Linux v5.7 and later can make use of this option. If the boot option
> selected by the UEFI boot manager specifies an existing file to be used
> as initial RAM disk, a Linux specific Load File2 protocol will be
> installed and Linux 5.7+ will ignore any initrd=<ramdisk> command line
> argument."
> 
> > +	  installed if bootmgr is used and the file is found on the defined
> > +	  path. A boot entry of Boot0001 will try to match Initrd0001 and use
> 
> This does not match the implementation.
> 
> > +	  it. Initrd EFI variable format should be '<dev> <part> <filename>'
> > +	  i.e 'mmc 0:1 boot/initrd'
> 
> "The efidebug command can be used to specify the file with the initial
> RAM disk."

Seems like I missed some text while amending the description. 
Your proposal sounds good I'll update it.

[...]
> 
> In all our other coding this variable is called ret.
> I would prefer to do the same here too.

No problem

Cheers
/Ilias
> 
> Best regards
> 

[RFC PATCH 2/3] efi_loader: efi_loader: Replace config option for initrd loading

[AKASHI Takahiro]

Ilias,

On Wed, Jan 13, 2021 at 01:11:48PM +0200, Ilias Apalodimas wrote:
> Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
> unconditionally. Although we correctly return various EFI exit codes
> depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the
> kernel loader, only falls back to the cmdline interpreted initrd if the
> protocol is not installed.
> 
> This creates a problem for EFI installers, since they won't be able to
> load their own initrd and continue the installation. It also makes the
> feature hard to use, since we can either have a single initrd or we have
> to recompile u-boot if the filename changes.
> 
> So let's introduce a different logic that will decouple the initrd
> path from the config option we currently have.
> When defining a UEFI BootXXXX we can use the filepathlist and store
> a file path pointing to our initrd. Specifically the EFI spec describes:
> 
> "The first element of the array is a device path that describes the device
> and location of the Image for this load option. Other device paths may
> optionally exist in the FilePathList, but their usage is OSV specific"

I wonder what "OSV specific" does and should mean.
Apparently, using a "array of device paths" is U-Boot specific for now
and any distro who wants to use U-Boot as an EFI boot loader needs to
(at least, preferably) take care of this. It would be sad
that the installation process cannot be EFI-implementation agnostic
in terms of EFI's purpose.

So do you intend to propose your idea as a common practice to linux community?

> When the EFI application is launched through the bootmgr, we'll try to
> interpret the extra device path. If that points to a file that exists on
> our disk, we'll now install the load_file2 and the efi-stub will be able
> to use it.
> 
> This opens up another path using U-Boot and defines a new boot flow.
> A user will be able to control the kernel/initrd pairs without explicit
> cmdline args or GRUB.
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/bootefi.c                    |   3 +
>  include/efi_loader.h             |   1 +
>  lib/efi_loader/Kconfig           |  13 +--
>  lib/efi_loader/efi_bootmgr.c     |   3 +
>  lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++---------------
>  5 files changed, 91 insertions(+), 83 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index fdf909f8da2c..053927d5d986 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -357,6 +357,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
>  
>  	free(load_options);
>  
> +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> +		efi_initrd_deregister();
> +

I understand why you want do "deregister" the initrd handle,
but the handle for the loaded image is still valid at this point.
So it looks inconsistent from the viewpoint of API's.

>  	return ret;
>  }
>  
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 4719fa93f06d..5d2e161963c3 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -432,6 +432,7 @@ efi_status_t efi_net_register(void);
>  /* Called by bootefi to make the watchdog available */
>  efi_status_t efi_watchdog_register(void);
>  efi_status_t efi_initrd_register(void);
> +void efi_initrd_deregister(void);
>  /* Called by bootefi to make SMBIOS tables available */
>  /**
>   * efi_acpi_register() - write out ACPI tables
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index fdf245dea30b..597a3ee86c88 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -307,14 +307,11 @@ config EFI_LOAD_FILE2_INITRD
>  	help
>  	  Expose a EFI_FILE_LOAD2_PROTOCOL that the Linux UEFI stub can
>  	  use to load the initial ramdisk. Once this is enabled using
> -	  initrd=<ramdisk> will stop working.
> -
> -config EFI_INITRD_FILESPEC
> -	string "initramfs path"
> -	default "host 0:1 initrd"
> -	depends on EFI_LOAD_FILE2_INITRD
> -	help
> -	  Full path of the initramfs file, e.g. mmc 0:2 initramfs.cpio.gz.
> +	  initrd=<ramdisk> will stop working. The protocol will only be
> +	  installed if bootmgr is used and the file is found on the defined
> +	  path. A boot entry of Boot0001 will try to match Initrd0001 and use
> +	  it. Initrd EFI variable format should be '<dev> <part> <filename>'
> +	  i.e 'mmc 0:1 boot/initrd'
>  
>  config EFI_SECURE_BOOT
>  	bool "Enable EFI secure boot support"
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 0fe503a7f376..aa5d521535ee 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -222,6 +222,9 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
>  		ret = efi_set_variable_int(L"BootCurrent",
>  					   &efi_global_variable_guid,
>  					   attributes, sizeof(n), &n, false);
> +		/* try to register load file2 for initrd's */
> +		if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> +			ret |= efi_initrd_register();

I think that, even if the config(EFI_LOAD_FILE at _INITRD) is turned on,
we should be allowed to boot linux without initrd if we want.
In addition, we may want to boot non-linux images by using U-boot 
with the config enabled.

>  		if (ret != EFI_SUCCESS) {
>  			if (EFI_CALL(efi_unload_image(*handle))
>  			    != EFI_SUCCESS)
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index b9ee8839054f..bb0c5e63b65c 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -5,7 +5,9 @@
>  
>  #include <common.h>
>  #include <efi_loader.h>
> +#include <efi_helper.h>
>  #include <efi_load_initrd.h>
> +#include <efi_variable.h>
>  #include <fs.h>
>  #include <malloc.h>
>  #include <mapmem.h>
> @@ -39,41 +41,10 @@ static const struct efi_initrd_dp dp = {
>  	}
>  };
>  
> -/**
> - * get_file_size() - retrieve the size of initramfs, set efi status on error
> - *
> - * @dev:			device to read from, e.g. "mmc"
> - * @part:			device partition, e.g. "0:1"
> - * @file:			name of file
> - * @status:			EFI exit code in case of failure
> - *
> - * Return:			size of file
> - */
> -static loff_t get_file_size(const char *dev, const char *part, const char *file,
> -			    efi_status_t *status)
> -{
> -	loff_t sz = 0;
> -	int ret;
> -
> -	ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
> -	if (ret) {
> -		*status = EFI_NO_MEDIA;
> -		goto out;
> -	}
> -
> -	ret = fs_size(file, &sz);
> -	if (ret) {
> -		sz = 0;
> -		*status = EFI_NOT_FOUND;
> -		goto out;
> -	}
> -
> -out:
> -	return sz;
> -}
> +static efi_handle_t efi_initrd_handle;
>  
>  /**
> - * efi_load_file2initrd() - load initial RAM disk
> + * efi_load_file2_initrd() - load initial RAM disk
>   *
>   * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL
>   * in order to load an initial RAM disk requested by the Linux kernel stub.
> @@ -93,21 +64,15 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  		      struct efi_device_path *file_path, bool boot_policy,
>  		      efi_uintn_t *buffer_size, void *buffer)
>  {
> -	char *filespec;
>  	efi_status_t status = EFI_NOT_FOUND;
> -	loff_t file_sz = 0, read_sz = 0;
> -	char *dev, *part, *file;
> -	char *pos;
> -	int ret;
> +	struct efi_device_path *initrd_path = NULL;
> +	struct efi_file_info *info = NULL;
> +	struct efi_file_handle *f;
> +	efi_uintn_t bs;
>  
>  	EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
>  		  buffer_size, buffer);
>  
> -	filespec = strdup(CONFIG_EFI_INITRD_FILESPEC);
> -	if (!filespec)
> -		goto out;
> -	pos = filespec;
> -
>  	if (!this || this != &efi_lf2_protocol ||
>  	    !buffer_size) {
>  		status = EFI_INVALID_PARAMETER;
> @@ -125,51 +90,82 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  		goto out;
>  	}
>  
> -	/*
> -	 * expect a string with three space separated parts:
> -	 *
> -	 * * a block device type, e.g. "mmc"
> -	 * * a device and partition identifier, e.g. "0:1"
> -	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
> -	 */
> -	dev = strsep(&pos, " ");
> -	if (!dev)
> +	initrd_path = efi_get_fp_from_boot(INITRD_DP);
> +	if (!initrd_path) {
> +		status = EFI_NOT_FOUND;
>  		goto out;
> -	part = strsep(&pos, " ");
> -	if (!part)
> +	}
> +
> +	/* Open file */
> +	f = efi_file_from_path(initrd_path);
> +	if (!f) {
> +		status = EFI_NOT_FOUND;
>  		goto out;

The logic here (efi_get_fp_from_boot, then efi_file_from_path)
is also seen in check_initrd(). Pls refactor the code.

-Takahiro Akashi


> -	file = strsep(&pos, " ");
> -	if (!file)
> +	}
> +
> +	/* Get file size */
> +	bs = 0;
> +	EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid,
> +				     &bs, info));
> +	if (status != EFI_BUFFER_TOO_SMALL) {
> +		status = EFI_DEVICE_ERROR;
>  		goto out;
> +	}
>  
> -	file_sz = get_file_size(dev, part, file, &status);
> -	if (!file_sz)
> +	info = malloc(bs);
> +	EFI_CALL(status = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs,
> +				     info));
> +	if (status != EFI_SUCCESS)
>  		goto out;
>  
> -	if (!buffer || *buffer_size < file_sz) {
> +	bs = info->file_size;
> +	//efi_load_image_from_file
> +	if (!buffer || *buffer_size < bs) {
>  		status = EFI_BUFFER_TOO_SMALL;
> -		*buffer_size = file_sz;
> +		*buffer_size = bs;
>  	} else {
> -		ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
> -		if (ret) {
> -			status = EFI_NO_MEDIA;
> -			goto out;
> -		}
> -
> -		ret = fs_read(file, map_to_sysmem(buffer), 0, *buffer_size,
> -			      &read_sz);
> -		if (ret || read_sz != file_sz)
> -			goto out;
> -		*buffer_size = read_sz;
> -
> -		status = EFI_SUCCESS;
> +		EFI_CALL(status = f->read(f, &bs, (void *)(uintptr_t)buffer));
> +		*buffer_size = bs;
>  	}
>  
>  out:
> -	free(filespec);
> +	free(info);
> +	efi_free_pool(initrd_path);
> +	EFI_CALL(f->close(f));
>  	return EFI_EXIT(status);
>  }
>  
> +/**
> + * check_initrd() - Determine if the file defined as an initrd in Boot####
> + *		    load_options device path is present
> + *
> + * Return:	status code
> + */
> +static efi_status_t check_initrd(void)
> +{
> +	struct efi_device_path *file_path;
> +	struct efi_file_handle *f;
> +
> +	/*
> +	 * if bootmgr is setup with and initrd, that will be the second
> +	 * device path instance in our load options located in Boot####
> +	 */
> +	file_path = efi_get_fp_from_boot(INITRD_DP);
> +	if (!file_path)
> +		return EFI_NOT_FOUND;
> +
> +	f = efi_file_from_path(file_path);
> +	if (!f) {
> +		efi_free_pool(file_path);
> +		return EFI_NOT_FOUND;
> +	}
> +
> +	EFI_CALL(f->close(f));
> +	efi_free_pool(file_path);
> +
> +	return EFI_SUCCESS;
> +}
> +
>  /**
>   * efi_initrd_register() - create handle for loading initial RAM disk
>   *
> @@ -182,9 +178,12 @@ out:
>   */
>  efi_status_t efi_initrd_register(void)
>  {
> -	efi_handle_t efi_initrd_handle = NULL;
>  	efi_status_t ret;
>  
> +	ret = check_initrd();
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
>  	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
>  		       (&efi_initrd_handle,
>  			/* initramfs */
> @@ -196,3 +195,8 @@ efi_status_t efi_initrd_register(void)
>  
>  	return ret;
>  }
> +
> +void efi_initrd_deregister(void)
> +{
> +	efi_delete_handle(efi_initrd_handle);
> +}
> -- 
> 2.30.0.rc2
> 

[RFC PATCH 2/3] efi_loader: efi_loader: Replace config option for initrd loading

[Ilias Apalodimas]

Akashi-san,

On Thu, Jan 14, 2021 at 01:23:30PM +0900, AKASHI Takahiro wrote:
> Ilias,
> 
> On Wed, Jan 13, 2021 at 01:11:48PM +0200, Ilias Apalodimas wrote:
> > Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
> > unconditionally. Although we correctly return various EFI exit codes
> > depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the
> > kernel loader, only falls back to the cmdline interpreted initrd if the
> > protocol is not installed.
> > 
> > This creates a problem for EFI installers, since they won't be able to
> > load their own initrd and continue the installation. It also makes the
> > feature hard to use, since we can either have a single initrd or we have
> > to recompile u-boot if the filename changes.
> > 
> > So let's introduce a different logic that will decouple the initrd
> > path from the config option we currently have.
> > When defining a UEFI BootXXXX we can use the filepathlist and store
> > a file path pointing to our initrd. Specifically the EFI spec describes:
> > 
> > "The first element of the array is a device path that describes the device
> > and location of the Image for this load option. Other device paths may
> > optionally exist in the FilePathList, but their usage is OSV specific"
> 
> I wonder what "OSV specific" does and should mean.

Judging from the context is used, I assumed it meant OS vendor.

> Apparently, using a "array of device paths" is U-Boot specific for now
> and any distro who wants to use U-Boot as an EFI boot loader needs to
> (at least, preferably) take care of this. It would be sad
> that the installation process cannot be EFI-implementation agnostic
> in terms of EFI's purpose.
> 

That's the reason I changed the approach completely. This one adheres to the
EFI spec compared to the previous version that used EFI variables.

> So do you intend to propose your idea as a common practice to linux community?

Yes, although that depends on the definition of the linux community.
The kernel itself is irrelevant here. We need to make sure though that
applications like efibootmgr follow the same principle.

> 
> > When the EFI application is launched through the bootmgr, we'll try to
> > interpret the extra device path. If that points to a file that exists on
> > our disk, we'll now install the load_file2 and the efi-stub will be able
> > to use it.
> > 
> > This opens up another path using U-Boot and defines a new boot flow.
> > A user will be able to control the kernel/initrd pairs without explicit
> > cmdline args or GRUB.
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  cmd/bootefi.c                    |   3 +
> >  include/efi_loader.h             |   1 +
> >  lib/efi_loader/Kconfig           |  13 +--
> >  lib/efi_loader/efi_bootmgr.c     |   3 +
> >  lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++---------------
> >  5 files changed, 91 insertions(+), 83 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index fdf909f8da2c..053927d5d986 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -357,6 +357,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
> >  
> >  	free(load_options);
> >  
> > +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> > +		efi_initrd_deregister();
> > +
> 
> I understand why you want do "deregister" the initrd handle,
> but the handle for the loaded image is still valid at this point.
> So it looks inconsistent from the viewpoint of API's.
> 

Hmm why? Won't this code be called after efi_exit()? In that case the loaded
image handle is deleted as well.

> >  	return ret;
> >  }

[...]

> > @@ -222,6 +222,9 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> >  		ret = efi_set_variable_int(L"BootCurrent",
> >  					   &efi_global_variable_guid,
> >  					   attributes, sizeof(n), &n, false);
> > +		/* try to register load file2 for initrd's */
> > +		if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> > +			ret |= efi_initrd_register();
> 
> I think that, even if the config(EFI_LOAD_FILE at _INITRD) is turned on,
> we should be allowed to boot linux without initrd if we want.
> In addition, we may want to boot non-linux images by using U-boot 
> with the config enabled.
> 

That was the case with the previous patchset.  The efi_initrd_register() was
always returning EFI_SUCCESS. Heinrich asked me respect the bootmgr behavior,
since a distro without initrd will most likely fail to boot, so he wanted to
allow fallbacks on the bootmgr.

The reasoning is that the kernel, in EFI mode, has 3 ways of discovering an
iniitrd.
1. Fixups on initrd-start/end in the DTB. In that case the stub is unaware of
how the DTB gets loaded
2. initrd=XXXX in the command line. In that case the kernel uses
EFI_SIMPLE_FILESYSYSTEM_PROTOCOL to load the parsed file. 
3. The load option this patch implements via LOAD_FILE2

If the user specifies an initrd, he needs it to boot and it's unlikely to specify 
both the initrd=xxxxx and add it in the Boot#### variable. 

I think I can fix the use case and return EFI_SUCCESS from
efi_initrd_register() if the extra instance is not found in the device path
array. In that case a user will be able to boot without it, but if the device
path is specified, the file *must* be there. If it's not the bootmgr will 
fallback to the next available boot option.

> >  		if (ret != EFI_SUCCESS) {
> > +		status = EFI_NOT_FOUND;

[...]

> >  		goto out;
> 
> The logic here (efi_get_fp_from_boot, then efi_file_from_path)
> is also seen in check_initrd(). Pls refactor the code.

check_initrd() closes the file handle though and frees the device path. It's
supposed to serve as a wrapper for 'is the file present', while in the second
function we need the device path and the file handle to load the file.

Thanks for taking the time to review this.

Regards
/Ilias

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[Ilias Apalodimas]

The UEFI spec allow a packed array of UEFI device paths in the
FilePathList[] of an EFI_LOAD_OPTION. The first file path must
describe the laoded image but the rest are OS specific.
Previous patches parse the device path and try to use the second
member of the array as an initrd. So let's modify efidebug slightly
and install the second file described in the command line as the
initrd device path.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 81 insertions(+), 8 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 5fb7b1e3c6a9..8d62981aca92 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -8,6 +8,7 @@
 #include <charset.h>
 #include <common.h>
 #include <command.h>
+#include <efi_helper.h>
 #include <efi_loader.h>
 #include <efi_rng.h>
 #include <exports.h>
@@ -17,6 +18,7 @@
 #include <mapmem.h>
 #include <search.h>
 #include <linux/ctype.h>
+#include <linux/err.h>
 
 #define BS systab.boottime
 #define RT systab.runtime
@@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
 	return CMD_RET_SUCCESS;
 }
 
+/**
+ * add_initrd_instance() - Append a device path to load_options pointing to an
+ *			   inirtd
+ *
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ * @file_path	Existing device path, the new instance will be appended
+ * Return:	Pointer to the device path or ERR_PTR
+ *
+ */
+static struct efi_device_path *add_initrd_instance(int argc, char *const argv[],
+						   struct efi_device_path *file_path)
+{
+	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
+	struct efi_device_path *final_fp = NULL;
+	efi_status_t ret;
+
+	if (argc < 8)
+		return ERR_PTR(-EINVAL);
+
+	ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp,
+			       &tmp_fp);
+	if (ret != EFI_SUCCESS) {
+		printf("Cannot create device path for \"%s %s\"\n",
+		       argv[6], argv[7]);
+		goto out;
+	}
+
+	final_fp = efi_dp_append_instance(file_path, tmp_fp);
+
+out:
+	efi_free_pool(tmp_dp);
+	efi_free_pool(tmp_fp);
+	return final_fp ? final_fp : ERR_PTR(-EINVAL);
+}
+
 /**
  * do_efi_boot_add() - set UEFI load option
  *
@@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
  *
  * Implement efidebug "boot add" sub-command. Create or change UEFI load option.
  *
- *     efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
+ * Without initrd:
+ * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
+ *
+ * With initrd:
+ * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options>
  */
 static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
 			   int argc, char *const argv[])
@@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
 	size_t label_len, label_len16;
 	u16 *label;
 	struct efi_device_path *device_path = NULL, *file_path = NULL;
+	struct efi_device_path *final_fp = NULL;
 	struct efi_load_option lo;
 	void *data = NULL;
 	efi_uintn_t size;
 	efi_status_t ret;
 	int r = CMD_RET_SUCCESS;
 
-	if (argc < 6 || argc > 7)
+	if (argc < 6 || argc > 9)
 		return CMD_RET_USAGE;
 
 	id = (int)simple_strtoul(argv[1], &endp, 16);
@@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
 	/* attributes */
 	lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */
 
+	/* optional data */
+	if (argc == 6)
+		lo.optional_data = NULL;
+	else
+		lo.optional_data = (const u8 *)argv[6];
+
 	/* label */
 	label_len = strlen(argv[2]);
 	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
@@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
 		r = CMD_RET_FAILURE;
 		goto out;
 	}
+
+	/* add initrd instance in device path */
+	if (argc >= 9) {
+		final_fp = add_initrd_instance(argc, argv, file_path);
+		if (IS_ERR(final_fp)) {
+			r = CMD_RET_FAILURE;
+			goto out;
+		}
+
+		/* add_initrd_instance allocates a new device path */
+		efi_free_pool(file_path);
+		file_path = final_fp;
+
+		/* update optional data */
+		if (argc == 9)
+			lo.optional_data = NULL;
+		else
+			lo.optional_data = (const u8 *)argv[9];
+	}
+
 	lo.file_path = file_path;
 	lo.file_path_length = efi_dp_size(file_path)
 				+ sizeof(struct efi_device_path); /* for END */
 
-	/* optional data */
-	if (argc == 6)
-		lo.optional_data = NULL;
-	else
-		lo.optional_data = (const u8 *)argv[6];
 
 	size = efi_serialize_load_option(&lo, (u8 **)&data);
 	if (!size) {
@@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag,
  */
 static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
 {
+	struct efi_device_path *initrd_path = NULL;
 	struct efi_load_option lo;
 	char *label, *p;
 	size_t label_len16, label_len;
 	u16 *dp_str;
 	efi_status_t ret;
+	efi_uintn_t initrd_dp_size;
 
 	ret = efi_deserialize_load_option(&lo, data, size);
 	if (ret != EFI_SUCCESS) {
@@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
 	dp_str = efi_dp_str(lo.file_path);
 	printf("  file_path: %ls\n", dp_str);
 	efi_free_pool(dp_str);
+	initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP);
+	if (initrd_path) {
+		dp_str = efi_dp_str(initrd_path);
+		printf("  initrd_path: %ls\n", dp_str);
+		efi_free_pool(dp_str);
+		efi_free_pool(initrd_path);
+	}
 
 	printf("  data:\n");
 	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
@@ -1583,7 +1656,7 @@ static char efidebug_help_text[] =
 #endif
 
 U_BOOT_CMD(
-	efidebug, 10, 0, do_efidebug,
+	efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug,
 	"Configure UEFI environment",
 	efidebug_help_text
 );
-- 
2.30.0.rc2

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[Heinrich Schuchardt]

On 13.01.21 12:11, Ilias Apalodimas wrote:
> The UEFI spec allow a packed array of UEFI device paths in the
> FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> describe the laoded image but the rest are OS specific.
> Previous patches parse the device path and try to use the second
> member of the array as an initrd. So let's modify efidebug slightly
> and install the second file described in the command line as the
> initrd device path.

Please, describe the syntax of the efidebug command in the commit message.

>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 81 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 5fb7b1e3c6a9..8d62981aca92 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -8,6 +8,7 @@
>  #include <charset.h>
>  #include <common.h>
>  #include <command.h>
> +#include <efi_helper.h>
>  #include <efi_loader.h>
>  #include <efi_rng.h>
>  #include <exports.h>
> @@ -17,6 +18,7 @@
>  #include <mapmem.h>
>  #include <search.h>
>  #include <linux/ctype.h>
> +#include <linux/err.h>
>
>  #define BS systab.boottime
>  #define RT systab.runtime
> @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
>  	return CMD_RET_SUCCESS;
>  }
>
> +/**
> + * add_initrd_instance() - Append a device path to load_options pointing to an
> + *			   inirtd
> + *
> + * @argc:	Number of arguments
> + * @argv:	Argument array
> + * @file_path	Existing device path, the new instance will be appended
> + * Return:	Pointer to the device path or ERR_PTR
> + *
> + */
> +static struct efi_device_path *add_initrd_instance(int argc, char *const argv[],
> +						   struct efi_device_path *file_path)
> +{
> +	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
> +	struct efi_device_path *final_fp = NULL;
> +	efi_status_t ret;
> +
> +	if (argc < 8)
> +		return ERR_PTR(-EINVAL);
> +
> +	ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp,
> +			       &tmp_fp);
> +	if (ret != EFI_SUCCESS) {
> +		printf("Cannot create device path for \"%s %s\"\n",
> +		       argv[6], argv[7]);
> +		goto out;
> +	}
> +
> +	final_fp = efi_dp_append_instance(file_path, tmp_fp);
> +
> +out:
> +	efi_free_pool(tmp_dp);
> +	efi_free_pool(tmp_fp);
> +	return final_fp ? final_fp : ERR_PTR(-EINVAL);
> +}
> +
>  /**
>   * do_efi_boot_add() - set UEFI load option
>   *
> @@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
>   *
>   * Implement efidebug "boot add" sub-command. Create or change UEFI load option.
>   *
> - *     efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
> + * Without initrd:
> + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
> + *
> + * With initrd:
> + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options>
>   */
>  static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  			   int argc, char *const argv[])
> @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  	size_t label_len, label_len16;
>  	u16 *label;
>  	struct efi_device_path *device_path = NULL, *file_path = NULL;
> +	struct efi_device_path *final_fp = NULL;
>  	struct efi_load_option lo;
>  	void *data = NULL;
>  	efi_uintn_t size;
>  	efi_status_t ret;
>  	int r = CMD_RET_SUCCESS;
>
> -	if (argc < 6 || argc > 7)
> +	if (argc < 6 || argc > 9)
>  		return CMD_RET_USAGE;
>
>  	id = (int)simple_strtoul(argv[1], &endp, 16);
> @@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  	/* attributes */
>  	lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */
>
> +	/* optional data */
> +	if (argc == 6)
> +		lo.optional_data = NULL;
> +	else
> +		lo.optional_data = (const u8 *)argv[6];
> +
>  	/* label */
>  	label_len = strlen(argv[2]);
>  	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
> @@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  		r = CMD_RET_FAILURE;
>  		goto out;
>  	}
> +
> +	/* add initrd instance in device path */
> +	if (argc >= 9) {
> +		final_fp = add_initrd_instance(argc, argv, file_path);
> +		if (IS_ERR(final_fp)) {
> +			r = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +
> +		/* add_initrd_instance allocates a new device path */
> +		efi_free_pool(file_path);
> +		file_path = final_fp;
> +
> +		/* update optional data */
> +		if (argc == 9)
> +			lo.optional_data = NULL;
> +		else
> +			lo.optional_data = (const u8 *)argv[9];
> +	}
> +
>  	lo.file_path = file_path;
>  	lo.file_path_length = efi_dp_size(file_path)
>  				+ sizeof(struct efi_device_path); /* for END */
>
> -	/* optional data */
> -	if (argc == 6)
> -		lo.optional_data = NULL;
> -	else
> -		lo.optional_data = (const u8 *)argv[6];
>
>  	size = efi_serialize_load_option(&lo, (u8 **)&data);
>  	if (!size) {
> @@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag,
>   */
>  static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
>  {
> +	struct efi_device_path *initrd_path = NULL;
>  	struct efi_load_option lo;
>  	char *label, *p;
>  	size_t label_len16, label_len;
>  	u16 *dp_str;
>  	efi_status_t ret;
> +	efi_uintn_t initrd_dp_size;
>
>  	ret = efi_deserialize_load_option(&lo, data, size);
>  	if (ret != EFI_SUCCESS) {
> @@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
>  	dp_str = efi_dp_str(lo.file_path);
>  	printf("  file_path: %ls\n", dp_str);
>  	efi_free_pool(dp_str);
> +	initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP);
> +	if (initrd_path) {
> +		dp_str = efi_dp_str(initrd_path);
> +		printf("  initrd_path: %ls\n", dp_str);
> +		efi_free_pool(dp_str);
> +		efi_free_pool(initrd_path);
> +	}
>
>  	printf("  data:\n");
>  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] =

Where is the change to efidebug_help_text[]?

Best regards

Heinrich

>  #endif
>
>  U_BOOT_CMD(
> -	efidebug, 10, 0, do_efidebug,
> +	efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug,
>  	"Configure UEFI environment",
>  	efidebug_help_text
>  );
>

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[Ilias Apalodimas]

On Wed, Jan 13, 2021 at 02:13:44PM +0100, Heinrich Schuchardt wrote:
> On 13.01.21 12:11, Ilias Apalodimas wrote:
> > The UEFI spec allow a packed array of UEFI device paths in the
> > FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> > describe the laoded image but the rest are OS specific.
> > Previous patches parse the device path and try to use the second
> > member of the array as an initrd. So let's modify efidebug slightly
> > and install the second file described in the command line as the
> > initrd device path.
> 
> Please, describe the syntax of the efidebug command in the commit message.

sure 

[...]

> > +	}
> >
> >  	printf("  data:\n");
> >  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> > @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] =
> 
> Where is the change to efidebug_help_text[]?

Thanks, I'll add that on the next version

Cheers
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> >  #endif
> >
> >  U_BOOT_CMD(
> > -	efidebug, 10, 0, do_efidebug,
> > +	efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug,
> >  	"Configure UEFI environment",
> >  	efidebug_help_text
> >  );
> >
> 

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[AKASHI Takahiro]

Ilias,

On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:
> The UEFI spec allow a packed array of UEFI device paths in the
> FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> describe the laoded image but the rest are OS specific.
> Previous patches parse the device path and try to use the second
> member of the array as an initrd. So let's modify efidebug slightly
> and install the second file described in the command line as the
> initrd device path.

I have a concern about your proposed command line syntax.
It takes a lot of parameters as a whole which makes it
hard to understand it at a glance, easily overflowing
the width of terminal window.

It will even get worse if we want to take dtb as a third
device path, and what if we want to specify dtb, but not initrd?

Moreover, if we want to add support for non-linux executabes which
utilize extra device paths (neither initrd nor dtb) in a boot
load option, the syntax will be problematic.

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 81 insertions(+), 8 deletions(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 5fb7b1e3c6a9..8d62981aca92 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -8,6 +8,7 @@
>  #include <charset.h>
>  #include <common.h>
>  #include <command.h>
> +#include <efi_helper.h>
>  #include <efi_loader.h>
>  #include <efi_rng.h>
>  #include <exports.h>
> @@ -17,6 +18,7 @@
>  #include <mapmem.h>
>  #include <search.h>
>  #include <linux/ctype.h>
> +#include <linux/err.h>
>  
>  #define BS systab.boottime
>  #define RT systab.runtime
> @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
>  	return CMD_RET_SUCCESS;
>  }
>  
> +/**
> + * add_initrd_instance() - Append a device path to load_options pointing to an
> + *			   inirtd
> + *
> + * @argc:	Number of arguments
> + * @argv:	Argument array
> + * @file_path	Existing device path, the new instance will be appended
> + * Return:	Pointer to the device path or ERR_PTR
> + *
> + */
> +static struct efi_device_path *add_initrd_instance(int argc, char *const argv[],
> +						   struct efi_device_path *file_path)
> +{
> +	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
> +	struct efi_device_path *final_fp = NULL;
> +	efi_status_t ret;
> +
> +	if (argc < 8)
> +		return ERR_PTR(-EINVAL);
> +
> +	ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp,
> +			       &tmp_fp);
> +	if (ret != EFI_SUCCESS) {
> +		printf("Cannot create device path for \"%s %s\"\n",
> +		       argv[6], argv[7]);
> +		goto out;
> +	}
> +
> +	final_fp = efi_dp_append_instance(file_path, tmp_fp);
> +
> +out:
> +	efi_free_pool(tmp_dp);
> +	efi_free_pool(tmp_fp);
> +	return final_fp ? final_fp : ERR_PTR(-EINVAL);
> +}
> +
>  /**
>   * do_efi_boot_add() - set UEFI load option
>   *
> @@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
>   *
>   * Implement efidebug "boot add" sub-command. Create or change UEFI load option.
>   *
> - *     efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
> + * Without initrd:
> + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
> + *
> + * With initrd:
> + * efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options>
>   */
>  static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  			   int argc, char *const argv[])
> @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  	size_t label_len, label_len16;
>  	u16 *label;
>  	struct efi_device_path *device_path = NULL, *file_path = NULL;
> +	struct efi_device_path *final_fp = NULL;
>  	struct efi_load_option lo;
>  	void *data = NULL;
>  	efi_uintn_t size;
>  	efi_status_t ret;
>  	int r = CMD_RET_SUCCESS;
>  
> -	if (argc < 6 || argc > 7)
> +	if (argc < 6 || argc > 9)
>  		return CMD_RET_USAGE;
>  
>  	id = (int)simple_strtoul(argv[1], &endp, 16);
> @@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  	/* attributes */
>  	lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */
>  
> +	/* optional data */
> +	if (argc == 6)
> +		lo.optional_data = NULL;
> +	else
> +		lo.optional_data = (const u8 *)argv[6];
> +
>  	/* label */
>  	label_len = strlen(argv[2]);
>  	label_len16 = utf8_utf16_strnlen(argv[2], label_len);
> @@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  		r = CMD_RET_FAILURE;
>  		goto out;
>  	}
> +
> +	/* add initrd instance in device path */
> +	if (argc >= 9) {
> +		final_fp = add_initrd_instance(argc, argv, file_path);

We'd better pass argv[6], argv[7] and argv[8] explicitly here,
which will make the code readable and easily extended to support
dtb in the future.
then,
    argc -= 3;
    argv += 3;

> +		if (IS_ERR(final_fp)) {
> +			r = CMD_RET_FAILURE;
> +			goto out;
> +		}
> +
> +		/* add_initrd_instance allocates a new device path */
> +		efi_free_pool(file_path);
> +		file_path = final_fp;
> +
> +		/* update optional data */

Then, the code should be moved out of this 'if' clause.

> +		if (argc == 9)
> +			lo.optional_data = NULL;
> +		else
> +			lo.optional_data = (const u8 *)argv[9];
> +	}
> +
>  	lo.file_path = file_path;
>  	lo.file_path_length = efi_dp_size(file_path)
>  				+ sizeof(struct efi_device_path); /* for END */
>  
> -	/* optional data */
> -	if (argc == 6)
> -		lo.optional_data = NULL;
> -	else
> -		lo.optional_data = (const u8 *)argv[6];

The code should remain here for non-initrd case.

-Takahiro Akashi

>  
>  	size = efi_serialize_load_option(&lo, (u8 **)&data);
>  	if (!size) {
> @@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag,
>   */
>  static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
>  {
> +	struct efi_device_path *initrd_path = NULL;
>  	struct efi_load_option lo;
>  	char *label, *p;
>  	size_t label_len16, label_len;
>  	u16 *dp_str;
>  	efi_status_t ret;
> +	efi_uintn_t initrd_dp_size;
>  
>  	ret = efi_deserialize_load_option(&lo, data, size);
>  	if (ret != EFI_SUCCESS) {
> @@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
>  	dp_str = efi_dp_str(lo.file_path);
>  	printf("  file_path: %ls\n", dp_str);
>  	efi_free_pool(dp_str);
> +	initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP);
> +	if (initrd_path) {
> +		dp_str = efi_dp_str(initrd_path);
> +		printf("  initrd_path: %ls\n", dp_str);
> +		efi_free_pool(dp_str);
> +		efi_free_pool(initrd_path);
> +	}
>  
>  	printf("  data:\n");
>  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> @@ -1583,7 +1656,7 @@ static char efidebug_help_text[] =
>  #endif
>  
>  U_BOOT_CMD(
> -	efidebug, 10, 0, do_efidebug,
> +	efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug,
>  	"Configure UEFI environment",
>  	efidebug_help_text
>  );
> -- 
> 2.30.0.rc2
> 

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[Ilias Apalodimas]

On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote:
> Ilias,
> 
> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:
> > The UEFI spec allow a packed array of UEFI device paths in the
> > FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> > describe the laoded image but the rest are OS specific.
> > Previous patches parse the device path and try to use the second
> > member of the array as an initrd. So let's modify efidebug slightly
> > and install the second file described in the command line as the
> > initrd device path.
> 
> I have a concern about your proposed command line syntax.
> It takes a lot of parameters as a whole which makes it
> hard to understand it at a glance, easily overflowing
> the width of terminal window.
> 
> It will even get worse if we want to take dtb as a third
> device path, and what if we want to specify dtb, but not initrd?
> 
> Moreover, if we want to add support for non-linux executabes which
> utilize extra device paths (neither initrd nor dtb) in a boot
> load option, the syntax will be problematic.
> 

Maybe we should add explicit commands in efidebug then?
Something like:
efidebug initrd add 0002 virtio 1 initrd_file
efidebug dtb add 0002 virtio 1 dtb

That would untangle the do_efi_boot_add() function, make our lives easier on
adding things like 'kernel <no initrd> valid dtb' and should be much easier 
to use. The user will just have to make sure the boot order numbers match when 
adding files

[...]

Cheers
/Ilias

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[Heinrich Schuchardt]

On 14.01.21 10:55, Ilias Apalodimas wrote:
> On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote:
>> Ilias,
>>
>> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:
>>> The UEFI spec allow a packed array of UEFI device paths in the
>>> FilePathList[] of an EFI_LOAD_OPTION. The first file path must
>>> describe the laoded image but the rest are OS specific.
>>> Previous patches parse the device path and try to use the second
>>> member of the array as an initrd. So let's modify efidebug slightly
>>> and install the second file described in the command line as the
>>> initrd device path.
>>
>> I have a concern about your proposed command line syntax.
>> It takes a lot of parameters as a whole which makes it
>> hard to understand it at a glance, easily overflowing
>> the width of terminal window.
>>
>> It will even get worse if we want to take dtb as a third
>> device path, and what if we want to specify dtb, but not initrd?
>>
>> Moreover, if we want to add support for non-linux executabes which
>> utilize extra device paths (neither initrd nor dtb) in a boot
>> load option, the syntax will be problematic.
>>
>
> Maybe we should add explicit commands in efidebug then?
> Something like:
> efidebug initrd add 0002 virtio 1 initrd_file
> efidebug dtb add 0002 virtio 1 dtb

Why "add"? If no file is provided, we could empty the device path.

All other boot related subcommands start with efidebug boot. So how about:

efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
efidebug boot initrd 00B1 mmc 0:1 initrd-5.99
efidebug boot dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
efidebug boot order 00B1

What will happen if you pass the following command next:

efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99.1 UUID=1234

Will initrd and and dtb be kept?

What will happen if you start with dtb or with initrd and not with add?

A device path with kernel.efi, no initrd, and dtb would have an initrd
device path with only an end node. Should we install a LOAD FILE2
protocol in this case to disallow initrd on the command line?

The boot options are relevant for all users, while the rest of efidebug
is more developers oriented. Should we separate the boot related
commands from the rest of efidebug and build with the new command by
default?

bootopt add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
bootopt initrd 00B1 mmc 0:1 initrd-5.99
bootopt dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
bootopt order 00B1

Best regards

Heinrich

>
> That would untangle the do_efi_boot_add() function, make our lives easier on
> adding things like 'kernel <no initrd> valid dtb' and should be much easier
> to use. The user will just have to make sure the boot order numbers match when
> adding files
>
> [...]
>
> Cheers
> /Ilias
>

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[Ilias Apalodimas]

On Thu, Jan 14, 2021 at 01:07:42PM +0100, Heinrich Schuchardt wrote:
> On 14.01.21 10:55, Ilias Apalodimas wrote:
> > On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote:
> >> Ilias,
> >>
> >> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:
> >>> The UEFI spec allow a packed array of UEFI device paths in the
> >>> FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> >>> describe the laoded image but the rest are OS specific.
> >>> Previous patches parse the device path and try to use the second
> >>> member of the array as an initrd. So let's modify efidebug slightly
> >>> and install the second file described in the command line as the
> >>> initrd device path.
> >>
> >> I have a concern about your proposed command line syntax.
> >> It takes a lot of parameters as a whole which makes it
> >> hard to understand it at a glance, easily overflowing
> >> the width of terminal window.
> >>
> >> It will even get worse if we want to take dtb as a third
> >> device path, and what if we want to specify dtb, but not initrd?
> >>
> >> Moreover, if we want to add support for non-linux executabes which
> >> utilize extra device paths (neither initrd nor dtb) in a boot
> >> load option, the syntax will be problematic.
> >>
> >
> > Maybe we should add explicit commands in efidebug then?
> > Something like:
> > efidebug initrd add 0002 virtio 1 initrd_file
> > efidebug dtb add 0002 virtio 1 dtb
> 
> Why "add"? If no file is provided, we could empty the device path.
> 
> All other boot related subcommands start with efidebug boot. So how about:
> 
> efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
> efidebug boot initrd 00B1 mmc 0:1 initrd-5.99
> efidebug boot dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
> efidebug boot order 00B1
> 
> What will happen if you pass the following command next:
> 
> efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99.1 UUID=1234
> 
> Will initrd and and dtb be kept?

That's one of the reasons the RFC had everything in one line. You enforce less
options to the user, which imho is always good :).

> 
> What will happen if you start with dtb or with initrd and not with add?

IMHO we should return the usage in that case and explicitly request the boot
option to be set first. In principle the dtb and initrd are not load options. 
They are appended DTB instances in an existing load option. If you dont do 
'add' first, there's no device path to begin with.

So to answer your first question, in order to simplify the command line, I'd
suggest we overwrite the load_option when adding a new image. It's unlikely
the initrd will remain as is anyway, the dtb might remain the same, but it's
not worth the effort and complexity. You'll need a substantial amount of code 
parsing the DP instances and placing elements in top/middle/bottom etc.

> 
> A device path with kernel.efi, no initrd, and dtb would have an initrd
> device path with only an end node. Should we install a LOAD FILE2
> protocol in this case to disallow initrd on the command line?

If you do that the user won't be able to load an initrd, unless a fixup is
applied directly on the DTB. I'd avoid that, I would simply defer from
installing the protocol overall if an end node is discovered on the initrd.

> 
> The boot options are relevant for all users, while the rest of efidebug
> is more developers oriented. Should we separate the boot related
> commands from the rest of efidebug and build with the new command by
> default?
> 
> bootopt add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
> bootopt initrd 00B1 mmc 0:1 initrd-5.99
> bootopt dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
> bootopt order 00B1

+1 on that, efidebug feels a bit weird for that.
This can go in on a different patchset I guess?
It would merely mean c/p the code in a different file and 
edit a few makesfiles?

Cheers
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> >
> > That would untangle the do_efi_boot_add() function, make our lives easier on
> > adding things like 'kernel <no initrd> valid dtb' and should be much easier
> > to use. The user will just have to make sure the boot order numbers match when
> > adding files
> >
> > [...]
> >
> > Cheers
> > /Ilias
> >
> 

[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[AKASHI Takahiro]

On Thu, Jan 14, 2021 at 02:36:23PM +0200, Ilias Apalodimas wrote:
> On Thu, Jan 14, 2021 at 01:07:42PM +0100, Heinrich Schuchardt wrote:
> > On 14.01.21 10:55, Ilias Apalodimas wrote:
> > > On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote:
> > >> Ilias,
> > >>
> > >> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:
> > >>> The UEFI spec allow a packed array of UEFI device paths in the
> > >>> FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> > >>> describe the laoded image but the rest are OS specific.
> > >>> Previous patches parse the device path and try to use the second
> > >>> member of the array as an initrd. So let's modify efidebug slightly
> > >>> and install the second file described in the command line as the
> > >>> initrd device path.
> > >>
> > >> I have a concern about your proposed command line syntax.
> > >> It takes a lot of parameters as a whole which makes it
> > >> hard to understand it at a glance, easily overflowing
> > >> the width of terminal window.
> > >>
> > >> It will even get worse if we want to take dtb as a third
> > >> device path, and what if we want to specify dtb, but not initrd?
> > >>
> > >> Moreover, if we want to add support for non-linux executabes which
> > >> utilize extra device paths (neither initrd nor dtb) in a boot
> > >> load option, the syntax will be problematic.
> > >>
> > >
> > > Maybe we should add explicit commands in efidebug then?
> > > Something like:
> > > efidebug initrd add 0002 virtio 1 initrd_file
> > > efidebug dtb add 0002 virtio 1 dtb
> > 
> > Why "add"? If no file is provided, we could empty the device path.
> > 
> > All other boot related subcommands start with efidebug boot. So how about:
> > 
> > efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
> > efidebug boot initrd 00B1 mmc 0:1 initrd-5.99
> > efidebug boot dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
> > efidebug boot order 00B1

I have had the same idea in my mind.

> > What will happen if you pass the following command next:
> > 
> > efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99.1 UUID=1234
> > 
> > Will initrd and and dtb be kept?
> 
> That's one of the reasons the RFC had everything in one line. You enforce less
> options to the user, which imho is always good :).
> 
> > 
> > What will happen if you start with dtb or with initrd and not with add?
> 
> IMHO we should return the usage in that case and explicitly request the boot
> option to be set first. In principle the dtb and initrd are not load options. 
> They are appended DTB instances in an existing load option. If you dont do 
> 'add' first, there's no device path to begin with.
> 
> So to answer your first question, in order to simplify the command line, I'd
> suggest we overwrite the load_option when adding a new image. It's unlikely
> the initrd will remain as is anyway, the dtb might remain the same, but it's
> not worth the effort and complexity. You'll need a substantial amount of code 
> parsing the DP instances and placing elements in top/middle/bottom etc.
> 
> > 
> > A device path with kernel.efi, no initrd, and dtb would have an initrd
> > device path with only an end node. Should we install a LOAD FILE2
> > protocol in this case to disallow initrd on the command line?
> 
> If you do that the user won't be able to load an initrd, unless a fixup is
> applied directly on the DTB. I'd avoid that, I would simply defer from
> installing the protocol overall if an end node is discovered on the initrd.
> 
> > 
> > The boot options are relevant for all users, while the rest of efidebug
> > is more developers oriented. Should we separate the boot related
> > commands from the rest of efidebug and build with the new command by
> > default?
> > 
> > bootopt add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
> > bootopt initrd 00B1 mmc 0:1 initrd-5.99
> > bootopt dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
> > bootopt order 00B1

My long-standing hope was to step up the stage of "efidebug" from
"debug" to "normal" command. Initially, I intended to implement this
command as an alternative of EDK2's efishell (or poorman's shell),
so most of the sub commands have a similar syntax and output formats
to efisehell defined in EFI specification.
But this idea was rejected by Alex :)

I agree that "boot" sub command is a good candidate of a standalone command.
Or alternatively, we may want to add options to bootefi command.

> +1 on that, efidebug feels a bit weird for that.
> This can go in on a different patchset I guess?
> It would merely mean c/p the code in a different file and 
> edit a few makesfiles?

Anyhow, when you go this way, please don't forget to update pytest scripts
since efidebug is also used in secure boot/capsule update tests.

-Takahiro Akashi


> Cheers
> /Ilias
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > > That would untangle the do_efi_boot_add() function, make our lives easier on
> > > adding things like 'kernel <no initrd> valid dtb' and should be much easier
> > > to use. The user will just have to make sure the boot order numbers match when
> > > adding files
> > >
> > > [...]
> > >
> > > Cheers
> > > /Ilias
> > >
> >