EFI updates

From: Eric Snowberg <[email protected]>

Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()") introduced a regression on systems with large memory maps causing them to hang on boot. The first "goto get_map" that was removed from exit_boot insured there was enough room for the memory map when efi_call_early(exit_boot_services) was called. This happens when (nr_desc > ARRAY_SIZE(params->e820_table).

Chain of events:
exit_boot()
efi_exit_boot_services()
efi_get_memory_map <- at this point the mm can't grow over 8 desc priv_func()
exit_boot_func()
allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
efi_call_early(exit_boot_services) <- mm key doesn't match so retry efi_call_early(get_memory_map) <- not enough room for new mm system hangs

This patch allocates the e820 buffer before calling efi_exit_boot_services and fixes the regression.

Signed-off-by Eric Snowberg <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 24 deletions(-)

Quoted 20 lines · Ard Biesheuvel

From: Eric Snowberg <[email protected]>

Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()") introduced a regression on systems with large memory maps causing them to hang on boot. The first "goto get_map" that was removed from exit_boot insured there was enough room for the memory map when efi_call_early(exit_boot_services) was called. This happens when (nr_desc > ARRAY_SIZE(params->e820_table).

Chain of events:
exit_boot()
efi_exit_boot_services()
efi_get_memory_map <- at this point the mm can't grow over 8 desc priv_func()
exit_boot_func()
allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
efi_call_early(exit_boot_services) <- mm key doesn't match so retry efi_call_early(get_memory_map) <- not enough room for new mm system hangs

Quoted 8 lines

This patch allocates the e820 buffer before calling efi_exit_boot_services and fixes the regression.

Signed-off-by Eric Snowberg <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 24 deletions(-)

Any objections against marking this for -stable and filing it in efi/urgent? Boot hangs are show-stopper bugs, so distributions would want to backport this fix anyway.

arch/x86/boot/compressed/eboot.c static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext, · line 653

Quoted 6 lines

arch/x86/boot/compressed/eboot.c +6 -0

+     boot_map.map =          ↦
+     boot_map.map_size =     &map_size;
+     boot_map.desc_size =    &desc_size;
+     boot_map.desc_ver =     NULL;
+     boot_map.key_ptr =      NULL;
+     boot_map.buff_size =    &buff_size;

I have changed this to the canonical style, which is also used elsewhere in the file:

arch/x86/boot/compressed/eboot.c static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) · line 706

Quoted 5 lines

      map.buff_size           = &buff_size;
      priv.boot_params        = boot_params;
      priv.efi                = &boot_params->efi_info;

arch/x86/boot/compressed/eboot.c +0 -2

-     priv.e820ext            = NULL;
-     priv.e820ext_size       = 0;

I.e.:

Quoted 33 lines · Ingo Molnar

Quoted 20 lines · Ard Biesheuvel

From: Eric Snowberg <[email protected]>

Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()") introduced a regression on systems with large memory maps causing them to hang on boot. The first "goto get_map" that was removed from exit_boot insured there was enough room for the memory map when efi_call_early(exit_boot_services) was called. This happens when (nr_desc > ARRAY_SIZE(params->e820_table).

Chain of events:
exit_boot()
efi_exit_boot_services()
efi_get_memory_map <- at this point the mm can't grow over 8 desc priv_func()
exit_boot_func()
allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
efi_call_early(exit_boot_services) <- mm key doesn't match so retry efi_call_early(get_memory_map) <- not enough room for new mm system hangs

Quoted 8 lines

This patch allocates the e820 buffer before calling efi_exit_boot_services and fixes the regression.

Signed-off-by Eric Snowberg <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 24 deletions(-)

Quoted 4 lines

Any objections against marking this for -stable and filing it in efi/urgent? Boot hangs are show-stopper bugs, so distributions would want to backport this fix anyway.

No objections per se, but this is the kind of patch that might go the other way as well, so I would prefer to give it some wider coverage at first, given how quickly patches are taken into -stable.

I can make a note of it and send it to Greg halfway into the next -rc cycle.

From: Julien Thierry <[email protected]>

Closing bracket seems to end a for statement when it is actually ending the contained if. Add some brackets to have clear delimitation of each scope.

No functional change/fix, just fix the indentation.

Signed-off-by Julien Thierry <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/fdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/firmware/efi/libstub/fdt.c +1 -1

index 0c0d2312f4a8..a3614f9b5f75 100644
@@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
 	tables = (efi_config_table_t *) sys_table->tables;
 	fdt = NULL;
 
-	for (i = 0; i < sys_table->nr_tables; i++)
+	for (i = 0; i < sys_table->nr_tables; i++) {
 		if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
 			fdt = (void *) tables[i].table;
 			if (fdt_check_header(fdt) != 0) {

Quoted 28 lines · Ard Biesheuvel

From: Julien Thierry <[email protected]>

Closing bracket seems to end a for statement when it is actually ending the contained if. Add some brackets to have clear delimitation of each scope.

No functional change/fix, just fix the indentation.

Signed-off-by Julien Thierry <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/fdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/firmware/efi/libstub/fdt.c +1 -1

index 0c0d2312f4a8..a3614f9b5f75 100644
@@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
      tables = (efi_config_table_t *) sys_table->tables;
      fdt = NULL;
 
-     for (i = 0; i < sys_table->nr_tables; i++)
+     for (i = 0; i < sys_table->nr_tables; i++) {
              if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
                      fdt = (void *) tables[i].table;
                      if (fdt_check_header(fdt) != 0) {

drivers/firmware/efi/libstub/fdt.c void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size) · line 389

Quoted 6 lines

drivers/firmware/efi/libstub/fdt.c +2 -1

@@ -385,7 +385,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
                      }
                      *fdt_size = fdt_totalsize(fdt);
                      break;
-      }
+             }
+     }

So if we are doing trivial cleanups, how about the patch below on top? It cleans up this file for good. Only minimally build tested.

Thanks,

Ingo

======================>
Subject: efi/fdt: More cleanups
From: Ingo Molnar <[email protected]>

Apply a number of cleanups:

- Introduce fdt_setprop_*var() helper macros to simplify and shorten repetitive sequences - this also makes it less likely that the wrong variable size is passed in. This change makes a lot of the property-setting calls single-line and easier to read.

- Harmonize comment style: capitalization, punctuation, whitespaces, etc.

- Fix some whitespace noise in the libstub Makefile which I happened to notice.

- Use the standard tabular initialization style:

    -       map.map =       &runtime_map;
    -       map.map_size =  &map_size;
    -       map.desc_size = &desc_size;
    -       map.desc_ver =  &desc_ver;
    -       map.key_ptr =   &mmap_key;
    -       map.buff_size = &buff_size;

    +       map.map         = &runtime_map;
    +       map.map_size    = &map_size;
    +       map.desc_size   = &desc_size;
    +       map.desc_ver    = &desc_ver;
    +       map.key_ptr     = &mmap_key;
    +       map.buff_size   = &buff_size;

- Use tabular structure definition for better readability.

- Make all pr*() lines single-line, even if they marginally exceed 80 cols - this makes them visually less intrusive.

- Unbreak line breaks into single lines when the length exceeds 80 cols only marginally, for better readability.

- Move assignment closer to the actual usage site.

- Plus some other smaller cleanups, spelling fixes, etc.

No change in functionality intended.

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 drivers/firmware/efi/libstub/Makefile |    4 -
 drivers/firmware/efi/libstub/fdt.c    |  109 ++++++++++++++++------------------
 scripts/dtc/libfdt/libfdt.h           |   10 +++
 3 files changed, 64 insertions(+), 59 deletions(-)

Index: tip/drivers/firmware/efi/libstub/Makefile
===================================================================

drivers/firmware/efi/libstub/Makefile +2 -2

@@ -49,7 +49,7 @@ lib-$(CONFIG_EFI_ARMSTUB)     += arm-stub.o
 
 lib-$(CONFIG_ARM)              += arm32-stub.o
 lib-$(CONFIG_ARM64)            += arm64-stub.o
-CFLAGS_arm64-stub.o            := -DTEXT_OFFSET=$(TEXT_OFFSET)
+CFLAGS_arm64-stub.o            := -DTEXT_OFFSET=$(TEXT_OFFSET)
 
 #
 # arm64 puts the stub in the kernel proper, which will unnecessarily retain all

@@ -86,7 +86,7 @@ quiet_cmd_stubcopy = STUBCPY $@
       cmd_stubcopy = if $(STRIP) --strip-debug $(STUBCOPY_RM-y) -o $@ $<; \
                     then if $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y); \
                     then (echo >&2 "$@: absolute symbol references not allowed in the EFI stub"; \
-                          rm -f $@; /bin/false);                         \
+                          rm -f $@; /bin/false);                         \
                     else $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@; fi        \
                     else /bin/false; fi
 

Index: tip/drivers/firmware/efi/libstub/fdt.c
===================================================================

drivers/firmware/efi/libstub/fdt.c +52 -57

@@ -26,10 +26,8 @@ static void fdt_update_cell_size(efi_sys
        offset = fdt_path_offset(fdt, "/");
        /* Set the #address-cells and #size-cells values for an empty tree */
 
-       fdt_setprop_u32(fdt, offset, "#address-cells",
-                       EFI_DT_ADDR_CELLS_DEFAULT);
-
-       fdt_setprop_u32(fdt, offset, "#size-cells", EFI_DT_SIZE_CELLS_DEFAULT);
+       fdt_setprop_u32(fdt, offset, "#address-cells", EFI_DT_ADDR_CELLS_DEFAULT);
+       fdt_setprop_u32(fdt, offset, "#size-cells",    EFI_DT_SIZE_CELLS_DEFAULT);
 }
 
 static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,

@@ -42,7 +40,7 @@ static efi_status_t update_fdt(efi_syste
        u32 fdt_val32;
        u64 fdt_val64;
 
-       /* Do some checks on provided FDT, if it exists*/
+       /* Do some checks on provided FDT, if it exists: */
        if (orig_fdt) {
                if (fdt_check_header(orig_fdt)) {
                        pr_efi_err(sys_table, "Device Tree header not valid!\n");

@@ -50,7 +48,7 @@ static efi_status_t update_fdt(efi_syste
                }
                /*
                 * We don't get the size of the FDT if we get if from a
-                * configuration table.
+                * configuration table:
                 */
                if (orig_fdt_size && fdt_totalsize(orig_fdt) > orig_fdt_size) {
                        pr_efi_err(sys_table, "Truncated device tree! foo!\n");

@@ -64,8 +62,8 @@ static efi_status_t update_fdt(efi_syste
                status = fdt_create_empty_tree(fdt, new_fdt_size);
                if (status == 0) {
                        /*
-                        * Any failure from the following function is non
-                        * critical
+                        * Any failure from the following function is
+                        * non-critical:
                         */
                        fdt_update_cell_size(sys_table, fdt);
                }

@@ -86,12 +84,13 @@ static efi_status_t update_fdt(efi_syste
        if (node < 0) {
                node = fdt_add_subnode(fdt, 0, "chosen");
                if (node < 0) {
-                       status = node; /* node is error code when negative */
+                       /* 'node' is an error code when negative: */
+                       status = node;
                        goto fdt_set_fail;
                }
        }
 
-       if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
+       if (cmdline_ptr != NULL && strlen(cmdline_ptr) > 0) {
                status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
                                     strlen(cmdline_ptr) + 1);
                if (status)

@@ -103,13 +102,12 @@ static efi_status_t update_fdt(efi_syste
                u64 initrd_image_end;
                u64 initrd_image_start = cpu_to_fdt64(initrd_addr);
 
-               status = fdt_setprop(fdt, node, "linux,initrd-start",
-                                    &initrd_image_start, sizeof(u64));
+               status = fdt_setprop_var(fdt, node, "linux,initrd-start", initrd_image_start);
                if (status)
                        goto fdt_set_fail;
+
                initrd_image_end = cpu_to_fdt64(initrd_addr + initrd_size);
-               status = fdt_setprop(fdt, node, "linux,initrd-end",
-                                    &initrd_image_end, sizeof(u64));
+               status = fdt_setprop_var(fdt, node, "linux,initrd-end", initrd_image_end);
                if (status)
                        goto fdt_set_fail;
        }

@@ -117,30 +115,28 @@ static efi_status_t update_fdt(efi_syste
        /* Add FDT entries for EFI runtime services in chosen node. */
        node = fdt_subnode_offset(fdt, 0, "chosen");
        fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);
-       status = fdt_setprop(fdt, node, "linux,uefi-system-table",
-                            &fdt_val64, sizeof(fdt_val64));
+
+       status = fdt_setprop_var(fdt, node, "linux,uefi-system-table", fdt_val64);
        if (status)
                goto fdt_set_fail;
 
        fdt_val64 = U64_MAX; /* placeholder */
-       status = fdt_setprop(fdt, node, "linux,uefi-mmap-start",
-                            &fdt_val64,  sizeof(fdt_val64));
+
+       status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-start", fdt_val64);
        if (status)
                goto fdt_set_fail;
 
        fdt_val32 = U32_MAX; /* placeholder */
-       status = fdt_setprop(fdt, node, "linux,uefi-mmap-size",
-                            &fdt_val32,  sizeof(fdt_val32));
+
+       status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-size", fdt_val32);
        if (status)
                goto fdt_set_fail;
 
-       status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-size",
-                            &fdt_val32, sizeof(fdt_val32));
+       status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-desc-size", fdt_val32);
        if (status)
                goto fdt_set_fail;
 
-       status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-ver",
-                            &fdt_val32, sizeof(fdt_val32));
+       status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-desc-ver", fdt_val32);
        if (status)
                goto fdt_set_fail;
 

@@ -150,8 +146,7 @@ static efi_status_t update_fdt(efi_syste
                efi_status = efi_get_random_bytes(sys_table, sizeof(fdt_val64),
                                                  (u8 *)&fdt_val64);
                if (efi_status == EFI_SUCCESS) {
-                       status = fdt_setprop(fdt, node, "kaslr-seed",
-                                            &fdt_val64, sizeof(fdt_val64));
+                       status = fdt_setprop_var(fdt, node, "kaslr-seed", fdt_val64);
                        if (status)
                                goto fdt_set_fail;
                } else if (efi_status != EFI_NOT_FOUND) {

@@ -159,7 +154,7 @@ static efi_status_t update_fdt(efi_syste
                }
        }
 
-       /* shrink the FDT back to its minimum size */
+       /* Shrink the FDT back to its minimum size: */
        fdt_pack(fdt);
 
        return EFI_SUCCESS;

@@ -182,26 +177,26 @@ static efi_status_t update_fdt_memmap(vo
                return EFI_LOAD_ERROR;
 
        fdt_val64 = cpu_to_fdt64((unsigned long)*map->map);
-       err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-start",
-                                 &fdt_val64, sizeof(fdt_val64));
+
+       err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-start", fdt_val64);
        if (err)
                return EFI_LOAD_ERROR;
 
        fdt_val32 = cpu_to_fdt32(*map->map_size);
-       err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-size",
-                                 &fdt_val32, sizeof(fdt_val32));
+
+       err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-size", fdt_val32);
        if (err)
                return EFI_LOAD_ERROR;
 
        fdt_val32 = cpu_to_fdt32(*map->desc_size);
-       err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-desc-size",
-                                 &fdt_val32, sizeof(fdt_val32));
+
+       err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-desc-size", fdt_val32);
        if (err)
                return EFI_LOAD_ERROR;
 
        fdt_val32 = cpu_to_fdt32(*map->desc_ver);
-       err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-desc-ver",
-                                 &fdt_val32, sizeof(fdt_val32));
+
+       err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-desc-ver", fdt_val32);
        if (err)
                return EFI_LOAD_ERROR;
 

@@ -209,13 +204,13 @@ static efi_status_t update_fdt_memmap(vo
 }
 
 #ifndef EFI_FDT_ALIGN
-#define EFI_FDT_ALIGN EFI_PAGE_SIZE
+# define EFI_FDT_ALIGN EFI_PAGE_SIZE
 #endif
 
 struct exit_boot_struct {
-       efi_memory_desc_t *runtime_map;
-       int *runtime_entry_count;
-       void *new_fdt_addr;
+       efi_memory_desc_t       *runtime_map;
+       int                     *runtime_entry_count;
+       void                    *new_fdt_addr;
 };
 
 static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,

@@ -235,7 +230,7 @@ static efi_status_t exit_boot_func(efi_s
 }
 
 #ifndef MAX_FDT_SIZE
-#define MAX_FDT_SIZE   SZ_2M
+# define MAX_FDT_SIZE SZ_2M
 #endif
 
 /*

@@ -266,16 +261,16 @@ efi_status_t allocate_new_fdt_and_exit_b
        unsigned long mmap_key;
        efi_memory_desc_t *memory_map, *runtime_map;
        efi_status_t status;
-       int runtime_entry_count = 0;
+       int runtime_entry_count;
        struct efi_boot_memmap map;
        struct exit_boot_struct priv;
 
-       map.map =       &runtime_map;
-       map.map_size =  &map_size;
-       map.desc_size = &desc_size;
-       map.desc_ver =  &desc_ver;
-       map.key_ptr =   &mmap_key;
-       map.buff_size = &buff_size;
+       map.map         = &runtime_map;
+       map.map_size    = &map_size;
+       map.desc_size   = &desc_size;
+       map.desc_ver    = &desc_ver;
+       map.key_ptr     = &mmap_key;
+       map.buff_size   = &buff_size;
 
        /*
         * Get a copy of the current memory map that we will use to prepare

@@ -289,15 +284,13 @@ efi_status_t allocate_new_fdt_and_exit_b
                return status;
        }
 
-       pr_efi(sys_table,
-              "Exiting boot services and installing virtual address map...\n");
+       pr_efi(sys_table, "Exiting boot services and installing virtual address map...\n");
 
        map.map = &memory_map;
        status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
                                new_fdt_addr, max_addr);
        if (status != EFI_SUCCESS) {
-               pr_efi_err(sys_table,
-                          "Unable to allocate memory for new device tree.\n");
+               pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
                goto fail;
        }
 

@@ -318,11 +311,12 @@ efi_status_t allocate_new_fdt_and_exit_b
                goto fail_free_new_fdt;
        }
 
-       priv.runtime_map = runtime_map;
-       priv.runtime_entry_count = &runtime_entry_count;
-       priv.new_fdt_addr = (void *)*new_fdt_addr;
-       status = efi_exit_boot_services(sys_table, handle, &map, &priv,
-                                       exit_boot_func);
+       runtime_entry_count             = 0;
+       priv.runtime_map                = runtime_map;
+       priv.runtime_entry_count        = &runtime_entry_count;
+       priv.new_fdt_addr               = (void *)*new_fdt_addr;
+
+       status = efi_exit_boot_services(sys_table, handle, &map, &priv, exit_boot_func);
 
        if (status == EFI_SUCCESS) {
                efi_set_virtual_address_map_t *svam;

@@ -363,6 +357,7 @@ fail_free_new_fdt:
 
 fail:
        sys_table->boottime->free_pool(runtime_map);
+
        return EFI_LOAD_ERROR;
 }
 

@@ -373,7 +368,7 @@ void *get_fdt(efi_system_table_t *sys_ta
        void *fdt;
        int i;
 
-       tables = (efi_config_table_t *) sys_table->tables;
+       tables = (efi_config_table_t *)sys_table->tables;
        fdt = NULL;
 
        for (i = 0; i < sys_table->nr_tables; i++) {

Index: tip/scripts/dtc/libfdt/libfdt.h
===================================================================

scripts/dtc/libfdt/libfdt.h +10 -0

@@ -1213,8 +1213,14 @@ int fdt_setprop_inplace_namelen_partial(
  *     -FDT_ERR_TRUNCATED, standard meanings
  */
 #ifndef SWIG /* Not available in Python */
+
 int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
                        const void *val, int len);
+
+/* Helper macro for the usual case of using simple C variables: */
+#define fdt_setprop_inplace_var(fdt, node_offset, name, var) \
+       fdt_setprop_inplace((fdt), (node_offset), (name), &(var), sizeof(var))
+
 #endif
 
 /**

@@ -1540,6 +1546,10 @@ int fdt_setprop(void *fdt, int nodeoffse
 int fdt_setprop_placeholder(void *fdt, int nodeoffset, const char *name,
                            int len, void **prop_data);
 
+/* Helper macro for the usual case of using simple C variables: */
+#define fdt_setprop_var(fdt, node_offset, name, var) \
+       fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var))
+
 /**
  * fdt_setprop_u32 - set a property to a 32-bit integer
  * @fdt: pointer to the device tree blob

Quoted 50 lines · Ingo Molnar

Quoted 28 lines · Ard Biesheuvel

From: Julien Thierry <[email protected]>

Closing bracket seems to end a for statement when it is actually ending the contained if. Add some brackets to have clear delimitation of each scope.

No functional change/fix, just fix the indentation.

Signed-off-by Julien Thierry <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/fdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/firmware/efi/libstub/fdt.c +1 -1

index 0c0d2312f4a8..a3614f9b5f75 100644
@@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
      tables = (efi_config_table_t *) sys_table->tables;
      fdt = NULL;
 
-     for (i = 0; i < sys_table->nr_tables; i++)
+     for (i = 0; i < sys_table->nr_tables; i++) {
              if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
                      fdt = (void *) tables[i].table;
                      if (fdt_check_header(fdt) != 0) {

drivers/firmware/efi/libstub/fdt.c void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size) · line 389

Quoted 6 lines

drivers/firmware/efi/libstub/fdt.c +2 -1

@@ -385,7 +385,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
                      }
                      *fdt_size = fdt_totalsize(fdt);
                      break;
-      }
+             }
+     }

So if we are doing trivial cleanups, how about the patch below on top? It cleans up this file for good. Only minimally build tested.

Thanks,

Ingo

======================>
Subject: efi/fdt: More cleanups
From: Ingo Molnar <[email protected]>

Quoted 7 lines

Apply a number of cleanups:

- Introduce fdt_setprop_*var() helper macros to simplify and shorten repetitive sequences - this also makes it less likely that the wrong variable size is passed in. This change makes a lot of the property-setting calls single-line and easier to read.

This change looks fine to me, but scripts/dtc/libfdt/libfdt.h is part of an external library, so we'd either need to contribute it there or define the macro in a local header.

Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80 Gitweb:
https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80

Author:     Sai Praneeth Prakhya <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:30 +0100

x86/efi: Unmap EFI boot services code/data regions from efi_pgd

efi_free_boot_services(), as the name suggests, frees EFI boot services code/data regions but forgets to unmap these regions from efi_pgd. This means that any code that's running in efi_pgd address space (e.g:
any EFI runtime service) would still be able to access these regions but the contents of these regions would have long been over written by someone else. So, it's important to unmap these regions. Hence, introduce efi_unmap_pages() to unmap these regions from efi_pgd.

After unmapping EFI boot services code/data regions, any illegal access by buggy firmware to these regions would result in page fault which will be handled by EFI specific fault handler.

Quoted 19 lines

Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80 Gitweb:
https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80

Author:     Sai Praneeth Prakhya <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:30 +0100

x86/efi: Unmap EFI boot services code/data regions from efi_pgd

efi_free_boot_services(), as the name suggests, frees EFI boot services code/data regions but forgets to unmap these regions from efi_pgd. This means that any code that's running in efi_pgd address space (e.g:
any EFI runtime service) would still be able to access these regions but the contents of these regions would have long been over written by someone else. So, it's important to unmap these regions. Hence, introduce efi_unmap_pages() to unmap these regions from efi_pgd.

Quoted 3 lines

After unmapping EFI boot services code/data regions, any illegal access by buggy firmware to these regions would result in page fault which will be handled by EFI specific fault handler.

Hi Thomas and Ingo,

I recently noticed that the below commits [1] and [2] are broken when kernel command line argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending these patches to mailing list. I am working on a fix and will send it to mailing list as soon as it's ready.

Meanwhile, could you please drop these patches before sending pull request to Linus?

[1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd")
[2] Commit 7e0dabd3010d ("x86/mm/pageattr: Introduce helper function to unmap EFI boot services")

Quoted 7 lines

Hi Thomas and Ingo,

I recently noticed that the below commits [1] and [2] are broken when kernel command line argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending these patches to mailing list. I am working on a fix and will send it to mailing list as soon as it's ready.

Could you elaborate on the problem please?

Quoted 2 lines

Meanwhile, could you please drop these patches before sending pull request

to Linus?

Quoted 5 lines

[1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd") [2] Commit 7e0dabd3010d ("x86/mm/pageattr:
Introduce helper function to unmap EFI boot services")

I'd like to understand what the issue is before we drop anything.

Quoted 10 lines

Quoted 8 lines

Hi Thomas and Ingo,

I recently noticed that the below commits [1] and [2] are broken when kernel command line argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending these patches to mailing list. I am working on a fix and will send it to mailing list as soon as it's ready.

Could you elaborate on the problem please?

Sure! My bad..

Little bit of history here:
Boris with this patch set [1] introduced statically mapping EFI Runtime Services at -4G and also introduced "efi=old_map" to return to previous EFI functionality which used ioremap and __va(pa).

[3] and [4] are links to old_map_region()

The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd"), unmaps EFI boot services code/data regions *only* from efi_pgd but efi=old_map maps
EFI boot services code/data regions into swapper_pgd. Also, efi=old_map uses either
ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and doesn't use kernel_map_pages_in_pgd().

So, we need a different unmapping routine to unmap EFI boot services code/data regions from swapper_pgd if they were mapped using efi=old_map.

Quoted 17 lines

Quoted 9 lines

Quoted 7 lines

Hi Thomas and Ingo,

I recently noticed that the below commits [1] and [2] are broken when kernel command line argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending these patches to mailing list. I am working on a fix and will send it to mailing list as soon as it's ready.

Could you elaborate on the problem please?

Sure! My bad..

Little bit of history here:
Boris with this patch set [1] introduced statically mapping EFI Runtime Services at -4G and also introduced "efi=old_map" to return to previous EFI functionality which used ioremap and __va(pa).

Quoted 7 lines

[3] and [4] are links to old_map_region()

The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd"), unmaps EFI boot services code/data regions *only* from efi_pgd but efi=old_map maps EFI boot services code/data regions into swapper_pgd. Also, efi=old_map uses either ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and

doesn't use kernel_map_pages_in_pgd().

Quoted 4 lines

So, we need a different unmapping routine to unmap EFI boot services code/data regions from swapper_pgd if they were mapped using efi=old_map.

For the short term, could we simply make your changes dependent on efi != old_map? That gives us some time to fix the old_map case properly.

Quoted 31 lines

Quoted 18 lines

Quoted 10 lines

Quoted 8 lines

Hi Thomas and Ingo,

I recently noticed that the below commits [1] and [2] are broken when kernel command line argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending these patches to mailing list. I am working on a fix and will send it to mailing list as soon as it's ready.

Could you elaborate on the problem please?

Sure! My bad..

Little bit of history here:
Boris with this patch set [1] introduced statically mapping EFI Runtime Services at -4G and also introduced "efi=old_map" to return to previous EFI functionality which used ioremap and __va(pa).

Quoted 7 lines

[3] and [4] are links to old_map_region()

The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd"), unmaps EFI boot services code/data regions *only* from efi_pgd but efi=old_map maps EFI boot services code/data regions into swapper_pgd. Also, efi=old_map uses either ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and

doesn't use kernel_map_pages_in_pgd().

Quoted 4 lines

So, we need a different unmapping routine to unmap EFI boot services code/data regions from swapper_pgd if they were mapped using efi=old_map.

Quoted 2 lines

For the short term, could we simply make your changes dependent on efi != old_map? That gives us some time to fix the old_map case properly.

Yes, I think we could and it should work but I hesitated to propose it because (as you already noted) it's a short term fix and not the right fix.

Alternatively, we could also evaluate if we need to support efi=old_map case going further. I thought dropping it would be a bad idea because it changes kernel user visible interface (because it's a kernel command line argument) and never brought it up.
Not sure what Thomas, Ingo or Linus might think about dropping a kernel command line argument.

As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with it for the foreseeable future.

Quoted 2 lines · Ard Biesheuvel

As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with it for the foreseeable future.

What happened with the old apple laptops which couldn't handle high virtual mappings and needed 1:1? We don't care anymore?

Quoted 5 lines · Borislav Petkov

Quoted 2 lines · Ard Biesheuvel

As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with it for the foreseeable future.

Quoted 3 lines

What happened with the old apple laptops which couldn't handle high virtual mappings and needed 1:1? We don't care anymore?

If that is the case (I wouldn't know) then yes, there is a second reason why we need to keep this code.

Quoted 6 lines · Ard Biesheuvel

Quoted 5 lines · Borislav Petkov

Quoted 2 lines · Ard Biesheuvel

As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with it for the foreseeable future.

Quoted 6 lines

Quoted 3 lines

What happened with the old apple laptops which couldn't handle high virtual mappings and needed 1:1? We don't care anymore?

If that is the case (I wouldn't know) then yes, there is a second reason why we need to keep this code.

Fleming knows details and he's on CC, lemme "pull" him up into To: :-)

Quoted 15 lines · Borislav Petkov

Quoted 12 lines · Ard Biesheuvel

Quoted 8 lines · Borislav Petkov

Quoted 2 lines · Ard Biesheuvel

As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with it for the foreseeable future.

What happened with the old apple laptops which couldn't handle high virtual mappings and needed 1:1? We don't care anymore?

If that is the case (I wouldn't know) then yes, there is a second reason why we need to keep this code.

Quoted 2 lines

Fleming knows details and he's on CC, lemme "pull" him up into To: :-)

IIUC the 1:1 mapping and the 'old' mapping are two different things, and the new mapping also contains a 1:1 mapping of the boot services regions, at least until SetVirtualAddressMap() returns.

Quoted 6 lines

Quoted 2 lines

For the short term, could we simply make your changes dependent on efi != old_map? That gives us some time to fix the old_map case properly.

Yes, I think we could and it should work but I hesitated to propose it because (as you already noted) it's a short term fix and not the right fix.

What is the status here?

Quoted 2 lines

Alternatively, we could also evaluate if we need to support efi=old_map case

going further.

Quoted 2 lines

I thought dropping it would be a bad idea because it changes kernel user visible interface (because it's a kernel command line argument) and never

brought it up.

Quoted 3 lines

Not sure what Thomas, Ingo or Linus might think about dropping a kernel command line argument.

Dropping a command line argument is not a problem in itself, unless anyone is actively using it :-)

As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with it for the foreseeable future.

This means we need a fixes that makes your unmapping code conditional on !old_memmap. Do you have an ETA for that?

From: Arend van Spriel <[email protected]>

Since commit:

ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from
EFI variables")

we have a device driver accessing the efivars API. Several functions in the efivars API assume __efivars is set, i.e., that they will be accessed only after efivars_register() has been called. However, the following NULL pointer access was reported calling efivar_entry_size() from the brcmfmac device driver.

  Unable to handle kernel NULL pointer dereference at virtual address 00000008
  pgd = 60bfa5f1
  [00000008] *pgd=00000000
  Internal error: Oops: 5 [#1] SMP ARM
  ...
  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
  Workqueue: events request_firmware_work_func
  PC is at efivar_entry_size+0x28/0x90
  LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
  pc : [<c0c40718>]    lr : [<bf2a3ef4>]    psr: a00d0113
  sp : ede7fe28  ip : ee983410  fp : c1787f30
  r10: 00000000  r9 : 00000000  r8 : bf2b2258
  r7 : ee983000  r6 : c1604c48  r5 : ede7fe88  r4 : edf337c0
  r3 : 00000000  r2 : 00000000  r1 : ede7fe88  r0 : c17712c8
  Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
  Control: 10c5387d  Table: ad16804a  DAC: 00000051

Disassembly showed that the local static variable __efivars is NULL, which is not entirely unexpected given that it is a non-EFI platform. So add a NULL pointer check to efivar_entry_size(), and to related functions while at it. In efivars_register() a couple of sanity checks are added as well.

Cc Hans de Goede <[email protected]>

Reported-by Jon Hunter <[email protected]>

Signed-off-by Arend van Spriel <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Quoted 2 lines · Ard Biesheuvel

From: Arend van Spriel <[email protected]>

Quoted 4 lines

Since commit:

ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from
EFI variables")

This commit ID is not upstream AFAICS. Which tree is it from? Mentioning non-upstream sha1's is discouraged in changelogs, as there's no guarantee that the sha1 will make it upstream.

Quoted 29 lines

we have a device driver accessing the efivars API. Several functions in the efivars API assume __efivars is set, i.e., that they will be accessed only after efivars_register() has been called. However, the following NULL pointer access was reported calling efivar_entry_size() from the brcmfmac device driver.

  Unable to handle kernel NULL pointer dereference at virtual address 00000008
  pgd = 60bfa5f1
  [00000008] *pgd=00000000
  Internal error: Oops: 5 [#1] SMP ARM
  ...
  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
  Workqueue: events request_firmware_work_func
  PC is at efivar_entry_size+0x28/0x90
  LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
  pc : [<c0c40718>]    lr : [<bf2a3ef4>]    psr: a00d0113
  sp : ede7fe28  ip : ee983410  fp : c1787f30
  r10: 00000000  r9 : 00000000  r8 : bf2b2258
  r7 : ee983000  r6 : c1604c48  r5 : ede7fe88  r4 : edf337c0
  r3 : 00000000  r2 : 00000000  r1 : ede7fe88  r0 : c17712c8
  Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
  Control: 10c5387d  Table: ad16804a  DAC: 00000051

Disassembly showed that the local static variable __efivars is NULL, which is not entirely unexpected given that it is a non-EFI platform. So add a NULL pointer check to efivar_entry_size(), and to related functions while at it. In efivars_register() a couple of sanity checks are added as well.

Quoted 4 lines

Cc Hans de Goede <[email protected]>

Reported-by Jon Hunter <[email protected]>

Signed-off-by Arend van Spriel <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Will that new commit be backported? If yes I suppose we could mark this fix -stable too? If not then it's fine for a v4.21 merge.

The current implementation of efi_mem_reserve_persistent() is rather naive, in the sense that for each invocation, it creates a separate linked list entry to describe the reservation. Since the linked list entries themselves need to persist across subsequent kexec reboots, every reservation created this way results in two memblock_reserve() calls at the next boot.

On arm64 systems with 100s of CPUs, this may result in a excessive number of memblock reservations, and needless fragmentation.

So instead, make use of the newly updated struct linux_efi_memreserve layout to put multiple reservations into a single linked list entry. This should get rid of the numerous tiny memblock reservations, and effectively cut the total number of reservations in half on arm64 systems with many CPUs.

Tested-by Marc Zyngier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c | 20 +++++++++++++++++---
 include/linux/efi.h        |  3 +++
 2 files changed, 20 insertions(+), 3 deletions(-)

drivers/firmware/efi/efi.c +2 -1

index 80b11521627a..e90bc32c2670 100644
@@ -998,7 +998,8 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
 	struct linux_efi_memreserve *rsv;
 	int rsvsize = EFI_MEMRESERVE_SIZE(1);
-	int rc;
+	unsigned long prsv;
+	int rc, index;
 
 	if (efi_memreserve_root == (void *)ULONG_MAX)
 		return -ENODEV;

Quoted 41 lines · Ard Biesheuvel

The current implementation of efi_mem_reserve_persistent() is rather naive, in the sense that for each invocation, it creates a separate linked list entry to describe the reservation. Since the linked list entries themselves need to persist across subsequent kexec reboots, every reservation created this way results in two memblock_reserve() calls at the next boot.

On arm64 systems with 100s of CPUs, this may result in a excessive number of memblock reservations, and needless fragmentation.

So instead, make use of the newly updated struct linux_efi_memreserve layout to put multiple reservations into a single linked list entry. This should get rid of the numerous tiny memblock reservations, and effectively cut the total number of reservations in half on arm64 systems with many CPUs.

Tested-by Marc Zyngier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c | 20 +++++++++++++++++---
 include/linux/efi.h        |  3 +++
 2 files changed, 20 insertions(+), 3 deletions(-)

drivers/firmware/efi/efi.c +2 -1

index 80b11521627a..e90bc32c2670 100644
@@ -998,7 +998,8 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
      struct linux_efi_memreserve *rsv;
      int rsvsize = EFI_MEMRESERVE_SIZE(1);
-     int rc;
+     unsigned long prsv;
+     int rc, index;
 
      if (efi_memreserve_root == (void *)ULONG_MAX)
              return -ENODEV;

@@ -1009,11 +1010,24 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
                      return rc;
      }
 

drivers/firmware/efi/efi.c int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size) · line 1012

Quoted 1 line

drivers/firmware/efi/efi.c +0 -1

-     rsv = kmalloc(rsvsize, GFP_ATOMIC);

I fixed the following build warning in this patch:

drivers/firmware/efi/efi.c:1000:6: warning: unused variable ‘rsvsize’ [-Wunused-variable]

'rsvsize' got entirely orphaned by the patch, so it can be removed.

From: YiFei Zhu <[email protected]>

An affected screen resolution is 1366 x 768, which width is not divisible by 8, the default font width. On such screens, when longer lines are earlyprintk'ed, overflow-to-next-line can never trigger, due to the left-most x-coordinate of the next character always less than the screen width. Earlyprintk will infinite loop in trying to print the rest of the string but unable to, due to the line being full.

This patch makes the trigger consider the right-most x-coordinate, instead of left-most, as the value to compare against the screen width threshold.

Signed-off-by YiFei Zhu <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/platform/efi/early_printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/platform/efi/early_printk.c +0 -0

index 7476b3b097e1..7138bc7a265c 100644