efi_loader: enough UEFI for standard distro boot

Prep work for next patch.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi.h      | 4 ++++
 include/part.h     | 3 ++-
 include/part_efi.h | 4 ----
 3 files changed, 6 insertions(+), 5 deletions(-)

include/efi.h +4 -0

index 02b78b31b1..87b0b43f20 100644
@@ -28,6 +28,10 @@
 
 struct efi_device_path;
 
+typedef struct {
+	u8 b[16];
+} efi_guid_t;
+
 #define EFI_BITS_PER_LONG	BITS_PER_LONG
 
 /*

include/part.h +2 -1

index 86117a7ce5..b2e820ef8a 100644
@@ -280,8 +280,9 @@ struct part_driver {
 #define U_BOOT_PART_TYPE(__name)					\
 	ll_entry_declare(struct part_driver, __name, part_driver)
 
-#if CONFIG_IS_ENABLED(EFI_PARTITION)
 #include <part_efi.h>
+
+#if CONFIG_IS_ENABLED(EFI_PARTITION)
 /* disk/part_efi.c */
 /**
  * write_gpt_table() - Write the GUID Partition Table to disk

include/part_efi.h +0 -0

index 317c044795..31e6bc6e14 100644

Quoted 45 lines · Rob Clark

Prep work for next patch.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi.h      | 4 ++++
 include/part.h     | 3 ++-
 include/part_efi.h | 4 ----
 3 files changed, 6 insertions(+), 5 deletions(-)

include/efi.h +4 -0

index 02b78b31b1..87b0b43f20 100644
@@ -28,6 +28,10 @@
 
 struct efi_device_path;
 
+typedef struct {
+	u8 b[16];
+} efi_guid_t;
+
 #define EFI_BITS_PER_LONG	BITS_PER_LONG
 
 /*

include/part.h +2 -1

index 86117a7ce5..b2e820ef8a 100644
@@ -280,8 +280,9 @@ struct part_driver {
 #define U_BOOT_PART_TYPE(__name)					\
 	ll_entry_declare(struct part_driver, __name, part_driver)
 
-#if CONFIG_IS_ENABLED(EFI_PARTITION)
 #include <part_efi.h>
+
+#if CONFIG_IS_ENABLED(EFI_PARTITION)
 /* disk/part_efi.c */
 /**
  * write_gpt_table() - Write the GUID Partition Table to disk

include/part_efi.h +0 -0

index 317c044795..31e6bc6e14 100644

include/part_efi.h line 63

Quoted 10 lines

include/part_efi.h +0 -4

@@ -58,10 +58,6 @@
 /* linux/include/efi.h */
 typedef u16 efi_char16_t;
 
-typedef struct {
-	u8 b[16];
-} efi_guid_t;
-
 /* based on linux/include/genhd.h */
 struct partition {
 	u8 boot_ind;		/* 0x80 - active */

Compiled and tested with and without CONFIG_EFI_PARTITION.

Reviewed-by Heinrich Schuchardt <[email protected]>

Prep work for next patch.

Signed-off-by Rob Clark <[email protected]>

Reviewed-by Heinrich Schuchardt <[email protected]>

From: Peter Jones <[email protected]>

EFI client programs need the signature information from the partition table to determine the disk a partition is on, so we need to fill that in here.

Signed-off-by Peter Jones <[email protected]>

[separated from efi_loader part, and fixed build-errors for non-
CONFIG_EFI_PARTITION case]

Signed-off-by Rob Clark <[email protected]>

From: Peter Jones <[email protected]>

EFI client programs need the signature information from the partition table to determine the disk a partition is on, so we need to fill that in here.

Signed-off-by Peter Jones <[email protected]>

[separated from efi_loader part, and fixed build-errors for non-
CONFIG_EFI_PARTITION case]

Signed-off-by Rob Clark <[email protected]>

Quoted 10 lines · Rob Clark

From: Peter Jones <[email protected]>

EFI client programs need the signature information from the partition table to determine the disk a partition is on, so we need to fill that in here.

Signed-off-by Peter Jones <[email protected]>

[separated from efi_loader part, and fixed build-errors for non-
CONFIG_EFI_PARTITION case]

Signed-off-by Rob Clark <[email protected]>

This is commit ff98cb90514d9b78 in mainline now and breaks all mx6 SPL boots:

U-Boot SPL 2017.09-00221-g0d6ab32 (Oct 02 2017 - 15:13:19) Trying to boot from MMC1
(hangs here)

Does anyone know how to fix this problem?

That would explain what I'm seeing as we pull this patch set in to 2017.09 for EFI boot support on aarch64 platforms.

Quoted 2 lines · Peter Robinson

That would explain what I'm seeing as we pull this patch set in to 2017.09 for EFI boot support on aarch64 platforms.

Locally I am applying the following hack, so that I can continue to work on mx6 with SPL:
https://pastebin.com/MX8tK76X

FWIW, on cubox the current status is looping, and your kludge I just get no output :( But there may be some other problems here too as I'm

Quoted 10 lines · Rob Clark

From: Peter Jones <[email protected]>

EFI client programs need the signature information from the partition table to determine the disk a partition is on, so we need to fill that in here.

Signed-off-by Peter Jones <[email protected]>

[separated from efi_loader part, and fixed build-errors for non-
CONFIG_EFI_PARTITION case]

Signed-off-by Rob Clark <[email protected]>

This is commit ff98cb90514d9b78 in mainline now and breaks all mx6 SPL boots:

U-Boot SPL 2017.09-00221-g0d6ab32 (Oct 02 2017 - 15:13:19) Trying to boot from MMC1
(hangs here)

Does anyone know how to fix this problem?

not sure to what extent debug()/printf() works from SPL, but if it does maybe adding this patch with and without that commit and comparing logs would give some hint:

I don't really see why this commit would cause problems, but I guess either something unexpected going on w/ partitions or block device driver on this platform..

All of the device-path related structures should be packed. UEFI defines the device-path as a byte-aligned data structure.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi_api.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

include/efi_api.h +0 -0

index ec1b321e8e..175341348e 100644

include/efi_api.h +2 -2

-};
+} __packed;
 
 #define DEVICE_PATH_TYPE_MESSAGING_DEVICE	0x03
 #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR	0x0b

@@ -297,7 +297,7 @@ struct efi_device_path_mac_addr {
 	struct efi_device_path dp;
 	struct efi_mac_addr mac;
 	u8 if_type;
-};
+} __packed;
 
 #define DEVICE_PATH_TYPE_MEDIA_DEVICE		0x04
 #  define DEVICE_PATH_SUB_TYPE_FILE_PATH	0x04

include/efi_api.h +1 -1

-};
+} __packed;
 
 #define BLOCK_IO_GUID \
 	EFI_GUID(0x964e5b21, 0x6459, 0x11d2, \

Quoted 19 lines · Rob Clark

All of the device-path related structures should be packed. UEFI defines the device-path as a byte-aligned data structure.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi_api.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

include/efi_api.h +1 -1

index ec1b321e8e..175341348e 100644
@@ -284,11 +284,11 @@ struct efi_device_path {
      u8 type;
      u8 sub_type;
      u16 length;
-};
+} __packed;
 

include/efi_api.h struct efi_device_path { · line 290

Quoted 2 lines

include/efi_api.h +0 -0

 struct efi_mac_addr {
      u8 addr[32];

Hello Alex, hello Rob,

32 bytes is more than the 6 bytes than we need.
Why was the structure defined this way?

Quoted 12 lines

include/efi_api.h +2 -2

-};
+} __packed;
 
 #define DEVICE_PATH_TYPE_MESSAGING_DEVICE    0x03
 #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR  0x0b

@@ -297,7 +297,7 @@ struct efi_device_path_mac_addr {
      struct efi_device_path dp;
      struct efi_mac_addr mac;
      u8 if_type;
-};
+} __packed;
 

include/efi_api.h struct efi_device_path_mac_addr { · line 307

Quoted 6 lines

include/efi_api.h +0 -0

 #define DEVICE_PATH_TYPE_MEDIA_DEVICE                0x04
 #  define DEVICE_PATH_SUB_TYPE_FILE_PATH     0x04

@@ -305,7 +305,7 @@ struct efi_device_path_mac_addr {
 struct efi_device_path_file_path {
      struct efi_device_path dp;
      u16 str[32];

Where do we get this 32 from?
Couldn't file/directory names be shorter (1 character) or longer (255 characters)?

Quoted 19 lines · Rob Clark

All of the device-path related structures should be packed. UEFI defines the device-path as a byte-aligned data structure.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi_api.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

include/efi_api.h +1 -1

index ec1b321e8e..175341348e 100644
@@ -284,11 +284,11 @@ struct efi_device_path {
 	u8 type;
 	u8 sub_type;
 	u16 length;
-};
+} __packed;
 

include/efi_api.h struct efi_device_path { · line 290

Quoted 2 lines

include/efi_api.h +0 -0

 struct efi_mac_addr {
 	u8 addr[32];

Hello Alex, hello Rob,

32 bytes is more than the 6 bytes than we need.
Why was the structure defined this way?

Quoted 13 lines

include/efi_api.h +2 -2

-};
+} __packed;
 
 #define DEVICE_PATH_TYPE_MESSAGING_DEVICE	0x03
 #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR	0x0b

@@ -297,7 +297,7 @@ struct efi_device_path_mac_addr {
 	struct efi_device_path dp;
 	struct efi_mac_addr mac;
 	u8 if_type;
-};
+} __packed;
 

include/efi_api.h struct efi_device_path_mac_addr { · line 307

Quoted 6 lines

include/efi_api.h +0 -0

 #define DEVICE_PATH_TYPE_MEDIA_DEVICE		0x04
 #  define DEVICE_PATH_SUB_TYPE_FILE_PATH	0x04

@@ -305,7 +305,7 @@ struct efi_device_path_mac_addr {
 struct efi_device_path_file_path {
 	struct efi_device_path dp;
 	u16 str[32];

Where do we get this 32 from?
Couldn't file/directory names be shorter (1 character) or longer (255 characters)?

All of the device-path related structures should be packed. UEFI defines the device-path as a byte-aligned data structure.

Signed-off-by Rob Clark <[email protected]>

From: Peter Jones <[email protected]>

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Rob Clark <[email protected]>

Helpers to construct device-paths from devices, partitions, files, and for parsing and manipulating device-paths.

For non-legacy devices, this will use u-boot's device-model to construct device-paths which include bus hierarchy to construct device-paths. For legacy devices we still fake it, but slightly more convincingly.

Signed-off-by Rob Clark <[email protected]>

Quoted 9 lines · Rob Clark

Helpers to construct device-paths from devices, partitions, files, and for parsing and manipulating device-paths.

For non-legacy devices, this will use u-boot's device-model to construct device-paths which include bus hierarchy to construct device-paths. For legacy devices we still fake it, but slightly more convincingly.

Signed-off-by Rob Clark <[email protected]>

This patch gives me checkpatch warnings left and right (unsigned vs unsigned int, double blank lines, double assignments, unsafe define, ...). I'll pull it in for now since it seems to be functionally correct, but please fix up the warnings in a follow-up patch.

Alex

WARNING: Adding new packed members is to be done with care #56: FILE: include/efi_api.h:340:
+} __packed;

CHECK: Please don't use multiple blank lines
#69: FILE: include/efi_loader.h:200:

+

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #74: FILE: include/efi_loader.h:205:
+unsigned efi_dp_size(const struct efi_device_path *dp);

CHECK: Please don't use multiple blank lines
#81: FILE: include/efi_loader.h:212:

(quoted diff) +2 -0

+
+

CHECK: Macro argument reuse '_dp' - possible side-effects? #91: FILE: include/efi_loader.h:222:

include/efi_loader.h +3 -0

+#define EFI_DP_TYPE(_dp, _type, _subtype) \
+       (((_dp)->type == DEVICE_PATH_TYPE_##_type) && \
+        ((_dp)->sub_type == DEVICE_PATH_SUB_TYPE_##_subtype))

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #136:
new file mode 100644

CHECK: Comparison to NULL could be written "!dp"
#195: FILE: lib/efi_loader/efi_device_path.c:55:
+ if (dp == NULL)

CHECK: Please don't use multiple blank lines
#232: FILE: lib/efi_loader/efi_device_path.c:92:

(quoted diff) +2 -0

+
+

CHECK: Please don't use multiple blank lines
#301: FILE: lib/efi_loader/efi_device_path.c:161:

(quoted diff) +2 -0

+
+

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #320: FILE: lib/efi_loader/efi_device_path.c:180:
+unsigned efi_dp_size(const struct efi_device_path *dp)

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #322: FILE: lib/efi_loader/efi_device_path.c:182:
+ unsigned sz = 0;

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #335: FILE: lib/efi_loader/efi_device_path.c:195:
+ unsigned sz = efi_dp_size(dp) + sizeof(END);

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #357: FILE: lib/efi_loader/efi_device_path.c:217:
+ unsigned sz1 = efi_dp_size(dp1);

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #358: FILE: lib/efi_loader/efi_device_path.c:218:
+ unsigned sz2 = efi_dp_size(dp2);

WARNING: Missing a blank line after declarations
#360: FILE: lib/efi_loader/efi_device_path.c:220:

lib/efi_loader/efi_device_path.c new file +2 -0

+               void *p = dp_alloc(sz1 + sz2 + sizeof(END));
+               memcpy(p, dp1, sz1);

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #379: FILE: lib/efi_loader/efi_device_path.c:239:
+ unsigned sz = node->length;

WARNING: Missing a blank line after declarations
#381: FILE: lib/efi_loader/efi_device_path.c:241:

lib/efi_loader/efi_device_path.c new file +2 -0

+               void *p = dp_alloc(sz + sizeof(END));
+               memcpy(p, node, sz);

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #386: FILE: lib/efi_loader/efi_device_path.c:246:
+ unsigned sz = efi_dp_size(dp);

WARNING: Missing a blank line after declarations
#388: FILE: lib/efi_loader/efi_device_path.c:248:

lib/efi_loader/efi_device_path.c new file +2 -0

+               void *p = dp_alloc(sz + node->length + sizeof(END));
+               memcpy(p, dp, sz);

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #401: FILE: lib/efi_loader/efi_device_path.c:261:
+static unsigned dp_size(struct udevice *dev)

CHECK: multiple assignments should be avoided
#484: FILE: lib/efi_loader/efi_device_path.c:344:
+ start = buf = dp_alloc(dp_size(dev) + sizeof(END));

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #492: FILE: lib/efi_loader/efi_device_path.c:352:
+static unsigned dp_part_size(struct blk_desc *desc, int part)

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #494: FILE: lib/efi_loader/efi_device_path.c:354:
+ unsigned dpsize;

CHECK: Please don't use multiple blank lines
#581: FILE: lib/efi_loader/efi_device_path.c:441:

(quoted diff) +2 -0

+
+

CHECK: multiple assignments should be avoided
#587: FILE: lib/efi_loader/efi_device_path.c:447:
+ start = buf = dp_alloc(dp_part_size(desc, part) + sizeof(END));

WARNING: Missing a blank line after declarations
#601: FILE: lib/efi_loader/efi_device_path.c:461:

lib/efi_loader/efi_device_path.c new file +2 -0

+               char c = *(path++);
+               if (c == '/')

CHECK: Alignment should match open parenthesis
#613: FILE: lib/efi_loader/efi_device_path.c:473:

lib/efi_loader/efi_device_path.c new file +2 -0

+struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
+               const char *path)

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #617: FILE: lib/efi_loader/efi_device_path.c:477:
+ unsigned dpsize = 0, fpsize;

CHECK: multiple assignments should be avoided
#625: FILE: lib/efi_loader/efi_device_path.c:485:
+ start = buf = dp_alloc(dpsize + sizeof(END));

WARNING: Prefer 'unsigned int' to bare use of 'unsigned' #648: FILE: lib/efi_loader/efi_device_path.c:508:
+ unsigned dpsize = 0;

CHECK: multiple assignments should be avoided
#659: FILE: lib/efi_loader/efi_device_path.c:519:
+ start = buf = dp_alloc(dpsize + sizeof(END));

total: 0 errors, 19 warnings, 12 checks, 644 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_ETHER_ADDR_COPY USLEEP_RANGE

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

Helpers to construct device-paths from devices, partitions, files, and for parsing and manipulating device-paths.

For non-legacy devices, this will use u-boot's device-model to construct device-paths which include bus hierarchy to construct device-paths. For legacy devices we still fake it, but slightly more convincingly.

Signed-off-by Rob Clark <[email protected]>

Helpers to construct device-paths from devices, partitions, files, and for parsing and manipulating device-paths.

For non-legacy devices, this will use u-boot's device-model to construct device-paths which include bus hierarchy to construct device-paths. For legacy devices we still fake it, but slightly more convincingly.

Signed-off-by Rob Clark <[email protected]>

---
 include/efi_api.h                |  10 +
 include/efi_loader.h             |  26 ++
 lib/efi_loader/Makefile          |   2 +-
 lib/efi_loader/efi_boottime.c    |  13 +-
 lib/efi_loader/efi_device_path.c | 563 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 611 insertions(+), 3 deletions(-)
 create mode 100644 lib/efi_loader/efi_device_path.c

include/efi_api.h +10 -0

index b761cf4822..4e27c82129 100644
@@ -314,6 +314,7 @@ struct efi_device_path_acpi_path {
 #define DEVICE_PATH_TYPE_MESSAGING_DEVICE	0x03
 #  define DEVICE_PATH_SUB_TYPE_MSG_USB		0x05
 #  define DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR	0x0b
+#  define DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS	0x0f
 #  define DEVICE_PATH_SUB_TYPE_MSG_SD		0x1a
 #  define DEVICE_PATH_SUB_TYPE_MSG_MMC		0x1d
 

@@ -329,6 +330,15 @@ struct efi_device_path_mac_addr {
 	u8 if_type;
 } __packed;
 
+struct efi_device_path_usb_class {
+	struct efi_device_path dp;
+	u16 vendor_id;
+	u16 product_id;
+	u8 device_class;
+	u8 device_subclass;
+	u8 device_protocol;
+} __packed;
+
 struct efi_device_path_sd_mmc_path {
 	struct efi_device_path dp;
 	u8 slot_number;

include/efi_loader.h +26 -0

index 1179234f68..d052b03ab7 100644
@@ -197,6 +197,32 @@ extern void *efi_bounce_buffer;
 #define EFI_LOADER_BOUNCE_BUFFER_SIZE (64 * 1024 * 1024)
 #endif
 
+
+struct efi_device_path *efi_dp_next(const struct efi_device_path *dp);
+int efi_dp_match(struct efi_device_path *a, struct efi_device_path *b);
+struct efi_object *efi_dp_find_obj(struct efi_device_path *dp,
+				   struct efi_device_path **rem);
+unsigned efi_dp_size(const struct efi_device_path *dp);
+struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp);
+struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,
+				      const struct efi_device_path *dp2);
+struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
+					   const struct efi_device_path *node);
+
+
+struct efi_device_path *efi_dp_from_dev(struct udevice *dev);
+struct efi_device_path *efi_dp_from_part(struct blk_desc *desc, int part);
+struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
+					 const char *path);
+struct efi_device_path *efi_dp_from_eth(void);
+void efi_dp_split_file_path(struct efi_device_path *full_path,
+			    struct efi_device_path **device_path,
+			    struct efi_device_path **file_path);
+
+#define EFI_DP_TYPE(_dp, _type, _subtype) \
+	(((_dp)->type == DEVICE_PATH_TYPE_##_type) && \
+	 ((_dp)->sub_type == DEVICE_PATH_SUB_TYPE_##_subtype))
+
 /* Convert strings from normal C strings to uEFI strings */
 static inline void ascii2unicode(u16 *unicode, const char *ascii)
 {

lib/efi_loader/Makefile +1 -1

index 30bf343a36..f35e5ce8a8 100644
@@ -15,7 +15,7 @@ always := $(efiprogs-y)
 
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
-obj-y += efi_memory.o efi_device_path_to_text.o
+obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
 obj-$(CONFIG_PARTITIONS) += efi_disk.o

lib/efi_loader/efi_boottime.c +0 -0

index 43f32385fa..b962b62a97 100644

lib/efi_loader/efi_boottime.c +6 -0

+	if (!efiobj)
+		return EFI_EXIT(EFI_NOT_FOUND);
+
+	*device = efiobj->handle;
+
+	return EFI_EXIT(EFI_SUCCESS);
 }
 
 /* Collapses configuration table entries, removing index i */

lib/efi_loader/efi_device_path.c new file +146 -0

new file mode 100644
index 0000000000..5d5c3b3464
@@ -0,0 +1,563 @@
+/*
+ * EFI device path from u-boot device-model mapping
+ *
+ * (C) Copyright 2017 Rob Clark
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <blk.h>
+#include <dm.h>
+#include <usb.h>
+#include <mmc.h>
+#include <efi_loader.h>
+#include <inttypes.h>
+#include <part.h>
+
+/* template END node: */
+static const struct efi_device_path END = {
+	.type     = DEVICE_PATH_TYPE_END,
+	.sub_type = DEVICE_PATH_SUB_TYPE_END,
+	.length   = sizeof(END),
+};
+
+#define U_BOOT_GUID \
+	EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
+		 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b)
+
+/* template ROOT node: */
+static const struct efi_device_path_vendor ROOT = {
+	.dp = {
+		.type     = DEVICE_PATH_TYPE_HARDWARE_DEVICE,
+		.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR,
+		.length   = sizeof(ROOT),
+	},
+	.guid = U_BOOT_GUID,
+};
+
+static void *dp_alloc(size_t sz)
+{
+	void *buf;
+
+	if (efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, sz, &buf) != EFI_SUCCESS)
+		return NULL;
+
+	return buf;
+}
+
+/*
+ * Iterate to next block in device-path, terminating (returning NULL)
+ * at /End* node.
+ */
+struct efi_device_path *efi_dp_next(const struct efi_device_path *dp)
+{
+	if (dp == NULL)
+		return NULL;
+	if (dp->type == DEVICE_PATH_TYPE_END)
+		return NULL;
+	dp = ((void *)dp) + dp->length;
+	if (dp->type == DEVICE_PATH_TYPE_END)
+		return NULL;
+	return (struct efi_device_path *)dp;
+}
+
+/*
+ * Compare two device-paths, stopping when the shorter of the two hits
+ * an End* node.  This is useful to, for example, compare a device-path
+ * representing a device with one representing a file on the device, or
+ * a device with a parent device.
+ */
+int efi_dp_match(struct efi_device_path *a, struct efi_device_path *b)
+{
+	while (1) {
+		int ret;
+
+		ret = memcmp(&a->length, &b->length, sizeof(a->length));
+		if (ret)
+			return ret;
+
+		ret = memcmp(a, b, a->length);
+		if (ret)
+			return ret;
+
+		a = efi_dp_next(a);
+		b = efi_dp_next(b);
+
+		if (!a || !b)
+			return 0;
+	}
+}
+
+
+/*
+ * See UEFI spec (section 3.1.2, about short-form device-paths..
+ * tl;dr: we can have a device-path that starts with a USB WWID
+ * or USB Class node, and a few other cases which don't encode
+ * the full device path with bus hierarchy:
+ *
+ *   - MESSAGING:USB_WWID
+ *   - MESSAGING:USB_CLASS
+ *   - MEDIA:FILE_PATH
+ *   - MEDIA:HARD_DRIVE
+ *   - MESSAGING:URI
+ */
+static struct efi_device_path *shorten_path(struct efi_device_path *dp)
+{
+	while (dp) {
+		/*
+		 * TODO: Add MESSAGING:USB_WWID and MESSAGING:URI..
+		 * in practice fallback.efi just uses MEDIA:HARD_DRIVE
+		 * so not sure when we would see these other cases.
+		 */
+		if (EFI_DP_TYPE(dp, MESSAGING_DEVICE, MSG_USB_CLASS) ||
+		    EFI_DP_TYPE(dp, MEDIA_DEVICE, HARD_DRIVE_PATH) ||
+		    EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH))
+			return dp;
+
+		dp = efi_dp_next(dp);
+	}
+
+	return dp;
+}
+
+static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path,
+				   struct efi_device_path **rem)
+{
+	struct efi_object *efiobj;
+
+	list_for_each_entry(efiobj, &efi_obj_list, link) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
+			struct efi_handler *handler = &efiobj->protocols[i];
+			struct efi_device_path *obj_dp;
+
+			if (!handler->guid)
+				break;
+
+			if (guidcmp(handler->guid, &efi_guid_device_path))
+				continue;
+
+			obj_dp = handler->protocol_interface;
+
+			do {
+				if (efi_dp_match(dp, obj_dp) == 0) {
+					if (rem) {

lib/efi_loader/efi_device_path.c new file +407 -0

+
+	return NULL;
+}
+
+
+/*
+ * Find an efiobj from device-path, if 'rem' is not NULL, returns the
+ * remaining part of the device path after the matched object.
+ */
+struct efi_object *efi_dp_find_obj(struct efi_device_path *dp,
+				   struct efi_device_path **rem)
+{
+	struct efi_object *efiobj;
+
+	efiobj = find_obj(dp, false, rem);
+
+	if (!efiobj)
+		efiobj = find_obj(dp, true, rem);
+
+	return efiobj;
+}
+
+/* return size not including End node: */
+unsigned efi_dp_size(const struct efi_device_path *dp)
+{
+	unsigned sz = 0;
+
+	while (dp) {
+		sz += dp->length;
+		dp = efi_dp_next(dp);
+	}
+
+	return sz;
+}
+
+struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)
+{
+	struct efi_device_path *ndp;
+	unsigned sz = efi_dp_size(dp) + sizeof(END);
+
+	if (!dp)
+		return NULL;
+
+	ndp = dp_alloc(sz);
+	memcpy(ndp, dp, sz);
+
+	return ndp;
+}
+
+struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,
+				      const struct efi_device_path *dp2)
+{
+	struct efi_device_path *ret;
+
+	if (!dp1) {
+		ret = efi_dp_dup(dp2);
+	} else if (!dp2) {
+		ret = efi_dp_dup(dp1);
+	} else {
+		/* both dp1 and dp2 are non-null */
+		unsigned sz1 = efi_dp_size(dp1);
+		unsigned sz2 = efi_dp_size(dp2);
+		void *p = dp_alloc(sz1 + sz2 + sizeof(END));
+		memcpy(p, dp1, sz1);
+		memcpy(p + sz1, dp2, sz2);
+		memcpy(p + sz1 + sz2, &END, sizeof(END));
+		ret = p;
+	}
+
+	return ret;
+}
+
+struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
+					   const struct efi_device_path *node)
+{
+	struct efi_device_path *ret;
+
+	if (!node && !dp) {
+		ret = efi_dp_dup(&END);
+	} else if (!node) {
+		ret = efi_dp_dup(dp);
+	} else if (!dp) {
+		unsigned sz = node->length;
+		void *p = dp_alloc(sz + sizeof(END));
+		memcpy(p, node, sz);
+		memcpy(p + sz, &END, sizeof(END));
+		ret = p;
+	} else {
+		/* both dp and node are non-null */
+		unsigned sz = efi_dp_size(dp);
+		void *p = dp_alloc(sz + node->length + sizeof(END));
+		memcpy(p, dp, sz);
+		memcpy(p + sz, node, node->length);
+		memcpy(p + sz + node->length, &END, sizeof(END));
+		ret = p;
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_DM
+/* size of device-path not including END node for device and all parents
+ * up to the root device.
+ */
+static unsigned dp_size(struct udevice *dev)
+{
+	if (!dev || !dev->driver)
+		return sizeof(ROOT);
+
+	switch (dev->driver->id) {
+	case UCLASS_ROOT:
+	case UCLASS_SIMPLE_BUS:
+		/* stop traversing parents at this point: */
+		return sizeof(ROOT);
+	case UCLASS_MMC:
+		return dp_size(dev->parent) +
+			sizeof(struct efi_device_path_sd_mmc_path);
+	case UCLASS_MASS_STORAGE:
+	case UCLASS_USB_HUB:
+		return dp_size(dev->parent) +
+			sizeof(struct efi_device_path_usb_class);
+	default:
+		/* just skip over unknown classes: */
+		return dp_size(dev->parent);
+	}
+}
+
+static void *dp_fill(void *buf, struct udevice *dev)
+{
+	if (!dev || !dev->driver)
+		return buf;
+
+	switch (dev->driver->id) {
+	case UCLASS_ROOT:
+	case UCLASS_SIMPLE_BUS: {
+		/* stop traversing parents at this point: */
+		struct efi_device_path_vendor *vdp = buf;
+		*vdp = ROOT;
+		return &vdp[1];
+	}
+#if defined(CONFIG_DM_MMC) && defined(CONFIG_MMC)
+	case UCLASS_MMC: {
+		struct efi_device_path_sd_mmc_path *sddp =
+			dp_fill(buf, dev->parent);
+		struct mmc *mmc = mmc_get_mmc_dev(dev);
+		struct blk_desc *desc = mmc_get_blk_desc(mmc);
+
+		sddp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+		sddp->dp.sub_type = (desc->if_type == IF_TYPE_MMC) ?
+			DEVICE_PATH_SUB_TYPE_MSG_MMC :
+			DEVICE_PATH_SUB_TYPE_MSG_SD;
+		sddp->dp.length   = sizeof(*sddp);
+		sddp->slot_number = dev->seq;
+
+		return &sddp[1];
+	}
+#endif
+	case UCLASS_MASS_STORAGE:
+	case UCLASS_USB_HUB: {
+		struct efi_device_path_usb_class *udp =
+			dp_fill(buf, dev->parent);
+		struct usb_device *udev = dev_get_parent_priv(dev);
+		struct usb_device_descriptor *desc = &udev->descriptor;
+
+		udp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+		udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS;
+		udp->dp.length   = sizeof(*udp);
+		udp->vendor_id   = desc->idVendor;
+		udp->product_id  = desc->idProduct;
+		udp->device_class    = desc->bDeviceClass;
+		udp->device_subclass = desc->bDeviceSubClass;
+		udp->device_protocol = desc->bDeviceProtocol;
+
+		return &udp[1];
+	}
+	default:
+		debug("unhandled device class: %s (%u)\n",
+		      dev->name, dev->driver->id);
+		return dp_fill(buf, dev->parent);
+	}
+}
+
+/* Construct a device-path from a device: */
+struct efi_device_path *efi_dp_from_dev(struct udevice *dev)
+{
+	void *buf, *start;
+
+	start = buf = dp_alloc(dp_size(dev) + sizeof(END));
+	buf = dp_fill(buf, dev);
+	*((struct efi_device_path *)buf) = END;
+
+	return start;
+}
+#endif
+
+static unsigned dp_part_size(struct blk_desc *desc, int part)
+{
+	unsigned dpsize;
+
+#ifdef CONFIG_BLK
+	dpsize = dp_size(desc->bdev->parent);
+#else
+	dpsize = sizeof(ROOT) + sizeof(struct efi_device_path_usb);
+#endif
+
+	if (part == 0) /* the actual disk, not a partition */
+		return dpsize;
+
+	if (desc->part_type == PART_TYPE_ISO)
+		dpsize += sizeof(struct efi_device_path_cdrom_path);
+	else
+		dpsize += sizeof(struct efi_device_path_hard_drive_path);
+
+	return dpsize;
+}
+
+static void *dp_part_fill(void *buf, struct blk_desc *desc, int part)
+{
+	disk_partition_t info;
+
+#ifdef CONFIG_BLK
+	buf = dp_fill(buf, desc->bdev->parent);
+#else
+	/*
+	 * We *could* make a more accurate path, by looking at if_type
+	 * and handling all the different cases like we do for non-
+	 * legacy (ie CONFIG_BLK=y) case.  But most important thing
+	 * is just to have a unique device-path for if_type+devnum.
+	 * So map things to a fictional USB device:
+	 */
+	struct efi_device_path_usb *udp;
+
+	memcpy(buf, &ROOT, sizeof(ROOT));
+	buf += sizeof(ROOT);
+
+	udp = buf;
+	udp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+	udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB;
+	udp->dp.length = sizeof(*udp);
+	udp->parent_port_number = desc->if_type;
+	udp->usb_interface = desc->devnum;
+	buf = &udp[1];
+#endif
+
+	if (part == 0) /* the actual disk, not a partition */
+		return buf;
+
+	part_get_info(desc, part, &info);
+
+	if (desc->part_type == PART_TYPE_ISO) {
+		struct efi_device_path_cdrom_path *cddp = buf;
+
+		cddp->boot_entry = part - 1;
+		cddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
+		cddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_CDROM_PATH;
+		cddp->dp.length = sizeof(*cddp);
+		cddp->partition_start = info.start;
+		cddp->partition_end = info.size;
+
+		buf = &cddp[1];
+	} else {
+		struct efi_device_path_hard_drive_path *hddp = buf;
+
+		hddp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
+		hddp->dp.sub_type = DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH;
+		hddp->dp.length = sizeof(*hddp);
+		hddp->partition_number = part - 1;
+		hddp->partition_start = info.start;
+		hddp->partition_end = info.size;
+		if (desc->part_type == PART_TYPE_EFI)
+			hddp->partmap_type = 2;
+		else
+			hddp->partmap_type = 1;
+		hddp->signature_type = desc->sig_type;
+		if (hddp->signature_type != 0)
+			memcpy(hddp->partition_signature, &desc->guid_sig,
+			       sizeof(hddp->partition_signature));
+
+		buf = &hddp[1];
+	}
+
+	return buf;
+}
+
+
+/* Construct a device-path from a partition on a blk device: */
+struct efi_device_path *efi_dp_from_part(struct blk_desc *desc, int part)
+{
+	void *buf, *start;
+
+	start = buf = dp_alloc(dp_part_size(desc, part) + sizeof(END));
+
+	buf = dp_part_fill(buf, desc, part);
+
+	*((struct efi_device_path *)buf) = END;
+
+	return start;
+}
+
+/* convert path to an UEFI style path (ie. DOS style backslashes and utf16) */
+static void path_to_uefi(u16 *uefi, const char *path)
+{
+	while (*path) {
+		char c = *(path++);
+		if (c == '/')
+			c = '\\';
+		*(uefi++) = c;
+	}
+	*uefi = '\0';
+}
+
+/*
+ * If desc is NULL, this creates a path with only the file component,
+ * otherwise it creates a full path with both device and file components
+ */
+struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
+		const char *path)
+{
+	struct efi_device_path_file_path *fp;
+	void *buf, *start;
+	unsigned dpsize = 0, fpsize;
+
+	if (desc)
+		dpsize = dp_part_size(desc, part);
+
+	fpsize = sizeof(struct efi_device_path) + 2 * (strlen(path) + 1);
+	dpsize += fpsize;
+
+	start = buf = dp_alloc(dpsize + sizeof(END));
+
+	if (desc)
+		buf = dp_part_fill(buf, desc, part);
+
+	/* add file-path: */
+	fp = buf;
+	fp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
+	fp->dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
+	fp->dp.length = fpsize;
+	path_to_uefi(fp->str, path);
+	buf += fpsize;
+
+	*((struct efi_device_path *)buf) = END;
+
+	return start;
+}
+
+#ifdef CONFIG_NET
+struct efi_device_path *efi_dp_from_eth(void)
+{
+	struct efi_device_path_mac_addr *ndp;
+	void *buf, *start;
+	unsigned dpsize = 0;
+
+	assert(eth_get_dev());
+
+#ifdef CONFIG_DM_ETH
+	dpsize += dp_size(eth_get_dev());
+#else
+	dpsize += sizeof(ROOT);
+#endif
+	dpsize += sizeof(*ndp);
+
+	start = buf = dp_alloc(dpsize + sizeof(END));
+
+#ifdef CONFIG_DM_ETH
+	buf = dp_fill(buf, eth_get_dev());
+#else
+	memcpy(buf, &ROOT, sizeof(ROOT));
+	buf += sizeof(ROOT);
+#endif
+
+	ndp = buf;
+	ndp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+	ndp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR;
+	ndp->dp.length = sizeof(*ndp);
+	memcpy(ndp->mac.addr, eth_get_ethaddr(), ARP_HLEN);
+	buf = &ndp[1];
+
+	*((struct efi_device_path *)buf) = END;
+
+	return start;
+}
+#endif
+
+/*
+ * Helper to split a full device path (containing both device and file
+ * parts) into it's constituent parts.
+ */
+void efi_dp_split_file_path(struct efi_device_path *full_path,
+			    struct efi_device_path **device_path,
+			    struct efi_device_path **file_path)
+{
+	struct efi_device_path *p, *dp, *fp;
+
+	dp = efi_dp_dup(full_path);
+	p = dp;
+	while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH))
+		p = efi_dp_next(p);
+	fp = efi_dp_dup(p);
+
+	p->type = DEVICE_PATH_TYPE_END;
+	p->sub_type = DEVICE_PATH_SUB_TYPE_END;
+	p->length = sizeof(*p);
+
+	*device_path = dp;
+	*file_path = fp;
+}

This is really the same thing as the efi_device_path struct.

Signed-off-by Rob Clark <[email protected]>

It needs to handle more device-path node types, and also multiple levels of path hierarchy. To simplify this, initially construct utf8 string to a temporary buffer, and then allocate the real utf16 buffer that is returned. This should be mostly for debugging or at least not critical- path so an extra copy won't hurt, and is saner than the alternative.

Signed-off-by Rob Clark <[email protected]>

Also, create disk objects for the disk itself, in addition to the partitions. (UEFI terminology is a bit confusing, a "disk" object is really a partition.) This helps grub properly identify the boot device since it is trying to match up partition "disk" object with it's parent device.

Now instead of seeing devices like:

/File(sdhci at 07864000.blk)/EndEntire
/File(usb_mass_storage.lun0)/EndEntire

You see:

/ACPI(133741d0,0)/UnknownMessaging(1d)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(0,800,64000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(1,64800,200000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/UnknownMessaging(1d)/HD(2,264800,19a000,dd904a8c00000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(0,800,60000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(1,61000,155000,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(2,20fa800,1bbf8800,38ca680200000000,1,1)/EndEntire /ACPI(133741d0,0)/USB(0,0)/USB(0,0)/USB(0,0)/HD(3,1b6800,1f44000,38ca680200000000,1,1)/EndEntire

This is on a board with single USB disk and single sd-card. The UnknownMessaging(1d) node in the device-path is the MMC device, but grub_efi_print_device_path() hasn't been updated yet for some of the newer device-path sub-types.

This patch is inspired by a patch originally from Peter Jones, but re-worked to use efi_device_path, so it doesn't much resemble the original.

Signed-off-by Rob Clark <[email protected]>

Signed-off-by Rob Clark <[email protected]>

Get rid of the hacky fake boot-device and duplicate device-path constructing (which needs to match what efi_disk and efi_net do). Instead convert over to use efi_device_path helpers to construct device-paths, and use that to look up the actual boot device.

Also, extract out a helper to plug things in properly to the loaded_image. In a following patch we'll want to re-use this in efi_load_image() to handle the case of loading an image from a file_path.

Signed-off-by Rob Clark <[email protected]>

fallback.efi (and probably other things) use UEFI's simple-file-system protocol and file support to search for OS's to boot.

Signed-off-by Rob Clark <[email protected]>

Previously we only supported the case when the EFI application loaded the image into memory for us. But fallback.efi does not do this.

Signed-off-by Rob Clark <[email protected]>

This avoids printf() spam about file reads (such as loading an image) into unaligned buffers (and the associated memcpy()). And generally seems like a good idea.

Signed-off-by Rob Clark <[email protected]>

Add EFI variable support, mapping to u-boot environment variables. Variables are pretty important for setting up boot order, among other things. If the board supports saveenv, then it will be called in ExitBootServices() to persist variables set by the efi payload. (For example, fallback.efi configuring BootOrder and BootXXXX load-option variables.)

Variables are *not* currently exposed at runtime, post ExitBootServices. On boards without a dedicated device for storage, which the loaded OS is not trying to also use, this is rather tricky. One idea, at least for boards that can persist RAM across reboot, is to keep a "journal" of modified variables in RAM, and then turn halt into a reboot into u-boot, plus store variables, plus halt. Whatever the solution, it likely involves some per-board support.

Mapping between EFI variables and u-boot variables:

efi_$guid_$varname = {attributes}(type)value

For example:

efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported=

     "{ro,boot,run}(blob)0000000000000000"
  efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_BootOrder=
     "(blob)00010000"

The attributes are a comma separated list of these possible attributes:

+ ro - read-only
+ boot - boot-services access
+ run - runtime access

NOTE: with current implementation, no variables are available after ExitBootServices, and all are persisted (if possible).

If not specified, the attributes default to "{boot}".

The required type is one of:

+ utf8 - raw utf8 string
+ blob - arbitrary length hex string

Signed-off-by Rob Clark <[email protected]>

lib/efi_loader/efi_boottime.c +1 -0

index ec40f41bcb..c406ff82ff 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <environment.h>
 #include <malloc.h>
 #include <asm/global_data.h>
 #include <libfdt_env.h>

This was merged as
ad644e7c18238026fecc647f62584d87d2c5b0a3
efi_loader: efi variable support

Quoted 13 lines

lib/efi_loader/efi_boottime.c +1 -0

index ec40f41bcb..c406ff82ff 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <environment.h>
 #include <malloc.h>
 #include <asm/global_data.h>
 #include <libfdt_env.h>

lib/efi_loader/efi_boottime.c static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle, · line 948

Quoted 6 lines

lib/efi_loader/efi_boottime.c +3 -0

@@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 {
 	EFI_ENTRY("%p, %ld", image_handle, map_key);
 
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
+	/* save any EFI variables that have been written: */
+	env_save();

You save all changed variables and not specifically those bound to EFI. This is completely unexpected behavior for the user and not covered by the commit message.

Furthermore you call env_save even if no EFI variable has been touched at all.

This leads to writing to flash on every boot via EFI. Depending on technology this might ruin the flash after a few hundred reboots.

Quoted 28 lines · Heinrich Schuchardt

This was merged as
ad644e7c18238026fecc647f62584d87d2c5b0a3
efi_loader: efi variable support

Quoted 13 lines

lib/efi_loader/efi_boottime.c +1 -0

index ec40f41bcb..c406ff82ff 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <environment.h>
 #include <malloc.h>
 #include <asm/global_data.h>
 #include <libfdt_env.h>

lib/efi_loader/efi_boottime.c static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle, · line 948

Quoted 6 lines

lib/efi_loader/efi_boottime.c +3 -0

@@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 {
     EFI_ENTRY("%p, %ld", image_handle, map_key);
 
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
+    /* save any EFI variables that have been written: */
+    env_save();

You save all changed variables and not specifically those bound to EFI. This is completely unexpected behavior for the user and not covered by the commit message.

Quoted 5 lines

Furthermore you call env_save even if no EFI variable has been touched at all.

This leads to writing to flash on every boot via EFI. Depending on technology this might ruin the flash after a few hundred reboots.

I agree that overwriting flash on every boot isn't a terribly smart idea.

Also saving random environment variables that are set while we are in a distro loop to persistent storage isn't great either.

I agree that we should probably improve this code to only save efi variables.

Rob, how bad would it be for Fedora if we remove the persistent variable store call (just the env_save() line) for 2017.11 to not kill people's storage devices? Then for the next version tackle it for real and only save efi variable changes here and only if anything really did change.

Quoted 42 lines · Alexander Graf

Quoted 28 lines · Heinrich Schuchardt

This was merged as
ad644e7c18238026fecc647f62584d87d2c5b0a3
efi_loader: efi variable support

Quoted 13 lines

lib/efi_loader/efi_boottime.c +1 -0

index ec40f41bcb..c406ff82ff 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <environment.h>
 #include <malloc.h>
 #include <asm/global_data.h>
 #include <libfdt_env.h>

lib/efi_loader/efi_boottime.c static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle, · line 948

Quoted 6 lines

lib/efi_loader/efi_boottime.c +3 -0

@@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 {
     EFI_ENTRY("%p, %ld", image_handle, map_key);
 
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
+    /* save any EFI variables that have been written: */
+    env_save();

You save all changed variables and not specifically those bound to EFI. This is completely unexpected behavior for the user and not covered by the commit message.

Quoted 5 lines

Furthermore you call env_save even if no EFI variable has been touched at all.

This leads to writing to flash on every boot via EFI. Depending on technology this might ruin the flash after a few hundred reboots.

I agree that overwriting flash on every boot isn't a terribly smart idea.

Also saving random environment variables that are set while we are in a distro loop to persistent storage isn't great either.

Quoted 8 lines

I agree that we should probably improve this code to only save efi variables.

Rob, how bad would it be for Fedora if we remove the persistent variable store call (just the env_save() line) for 2017.11 to not kill people's storage devices? Then for the next version tackle it for real and only save efi variable changes here and only if anything really did change.

As I mentioned on #u-boot, I think we can just comment the env_save().. or perhaps make it a config option. It will be mildly annoying, since it means every boot is "first boot", ie. falling back to fallback.efi to populate BootOrder/BootNNNN variables, and never using bootmgr. But since we can't set EFI variables from userspace yet, it isn't really a regression (yet).

Longer term, maybe we need to split out EFI variables from u-boot env vars.. I thought it would be nice to store them together, since it gives a convenient way to set EFI variables from script or cmdline. But at least a subset of the EFI vars should be persisted to reach EBBR, and if that breaks expectations about u-boot env vars, then I guess we need to split them.

Side note, devices that can't repeatedly save env (even if we split out the EFI variables to be stored separately) might be problematic with EFI_LOADER. Maybe we need a build option they can set to have a more crippled version of EFI_LOADER, so as to not penalize more capable devices.

Quoted 52 lines · Rob Clark

Quoted 50 lines · Alexander Graf

Quoted 28 lines · Heinrich Schuchardt

This was merged as
ad644e7c18238026fecc647f62584d87d2c5b0a3
efi_loader: efi variable support

Quoted 13 lines

lib/efi_loader/efi_boottime.c +1 -0

index ec40f41bcb..c406ff82ff 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <environment.h>
 #include <malloc.h>
 #include <asm/global_data.h>
 #include <libfdt_env.h>

lib/efi_loader/efi_boottime.c static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle, · line 948

Quoted 6 lines

lib/efi_loader/efi_boottime.c +3 -0

@@ -942,6 +943,11 @@ static efi_status_t EFIAPI efi_exit_boot_services(void *image_handle,
 {
     EFI_ENTRY("%p, %ld", image_handle, map_key);
 
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
+    /* save any EFI variables that have been written: */
+    env_save();

You save all changed variables and not specifically those bound to EFI. This is completely unexpected behavior for the user and not covered by the commit message.

Quoted 5 lines

Furthermore you call env_save even if no EFI variable has been touched at all.

This leads to writing to flash on every boot via EFI. Depending on technology this might ruin the flash after a few hundred reboots.

I agree that overwriting flash on every boot isn't a terribly smart idea.

Also saving random environment variables that are set while we are in a distro loop to persistent storage isn't great either.

I agree that we should probably improve this code to only save efi variables.

Rob, how bad would it be for Fedora if we remove the persistent variable store call (just the env_save() line) for 2017.11 to not kill people's storage devices? Then for the next version tackle it for real and only save efi variable changes here and only if anything really did change.

Quoted 6 lines

As I mentioned on #u-boot, I think we can just comment the env_save().. or perhaps make it a config option. It will be mildly annoying, since it means every boot is "first boot", ie. falling back to fallback.efi to populate BootOrder/BootNNNN variables, and never using bootmgr. But since we can't set EFI variables from userspace yet, it isn't really a regression (yet).

Ok, I sent a patch to remove the env_save() line.

Quoted 7 lines

Longer term, maybe we need to split out EFI variables from u-boot env vars.. I thought it would be nice to store them together, since it gives a convenient way to set EFI variables from script or cmdline. But at least a subset of the EFI vars should be persisted to reach EBBR, and if that breaks expectations about u-boot env vars, then I guess we need to split them.

I really like the sharing of the env space. We just need to have a different write mechanism for EFI variables: Instead of writing the current env, we could for example reread the original env, copy all current efi variables into that and write it back?

Quoted 7 lines

Side note, devices that can't repeatedly save env (even if we split out the EFI variables to be stored separately) might be problematic with EFI_LOADER. Maybe we need a build option they can set to have a more crippled version of EFI_LOADER, so as to not penalize more capable devices.

Every flash device self-destroys :).

Really, it boils down to the amount of saves. Writing to flash on every boot is a pretty bad idea. If we simply only write when a variable write actually occurred (which in most cases won't), then we should be ok on all devices I think.

The current implementation of saving of EFI variables has unwanted side effects:

- Writing to flash on every boot may harm the memory.
- All variables are saved. Not only the EFI ones.
- Variables are saved even if there is not change.

So let us disable saving for now until we have a complete solution.

This will also mean every boot is "first boot", ie. falling back to fallback.efi to populate BootOrder/BootNNNN variables, and never using bootmgr.

Fixes ad644e7c1823 efi_loader: efi variable support

Signed-off-by Heinrich Schuchardt <[email protected]>

---
 lib/efi_loader/efi_boottime.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)