efi/x86: move efi bgrt init code to early init

Before invoking the arch specific handler, efi_mem_reserve() reserves the given memory region through memblock.

efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time memblock is dead and it should not be used any more.

efi bgrt code depend on acpi intialization to get the bgrt acpi table, moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve in efi bgrt code still use memblock safely.

Signed-off-by Dave Young <[email protected]>

---
v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c

 arch/x86/kernel/acpi/boot.c      |   12 +++++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |    7 +---
 init/main.c                      |    1
 6 files changed, 60 insertions(+), 52 deletions(-)

arch/x86/kernel/acpi/boot.c +1 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,14 @@ int __init early_acpi_boot_init(void)
        return 0;

arch/x86/kernel/acpi/boot.c +0 -0

        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +3 -4

@@ -5,16 +5,15 @@
 
 #include <linux/acpi.h>
 
-void efi_bgrt_init(void);
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -546,11 +546,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c +0 -1

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 
        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

Quoted 34 lines · Dave Young

Before invoking the arch specific handler, efi_mem_reserve() reserves the given memory region through memblock.

efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time memblock is dead and it should not be used any more.

efi bgrt code depend on acpi intialization to get the bgrt acpi table, moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve in efi bgrt code still use memblock safely.

Signed-off-by Dave Young <[email protected]>

---
v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c

 arch/x86/kernel/acpi/boot.c      |   12 +++++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |    7 +---
 init/main.c                      |    1
 6 files changed, 60 insertions(+), 52 deletions(-)

arch/x86/kernel/acpi/boot.c +1 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,14 @@ int __init early_acpi_boot_init(void)
        return 0;

arch/x86/kernel/acpi/boot.c int __init early_acpi_boot_init(void) · line 1568

Quoted 10 lines

arch/x86/kernel/acpi/boot.c +8 -0

 }
 
+#ifdef CONFIG_ACPI_BGRT
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+       efi_bgrt_init(table);
+       return 0;
+}
+#endif
+

Please drop the #ifdef / #endif

arch/x86/kernel/acpi/boot.c int __init acpi_boot_init(void) · line 1593

Quoted 8 lines

arch/x86/kernel/acpi/boot.c +1 -0

 int __init acpi_boot_init(void)
 {
        /* those are executed after early-quirks are executed */

@@ -1581,6 +1590,9 @@ int __init acpi_boot_init(void)
        acpi_process_madt();
 
        acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
+#ifdef CONFIG_ACPI_BGRT

Please replace with

if (IS_ENABLED(CONFIG_ACPI_BGRT))

arch/x86/kernel/acpi/boot.c int __init acpi_boot_init(void) · line 1596

Quoted 3 lines

arch/x86/kernel/acpi/boot.c +2 -0

+       acpi_table_parse("BGRT", acpi_parse_bgrt);
+#endif
 

and perhaps we should add a #define for ACPI_SIG_BGRT as well?

Quoted 196 lines

arch/x86/kernel/acpi/boot.c +0 -0

        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +0 -0

@@ -5,16 +5,15 @@
 

include/linux/efi-bgrt.h line 6

Quoted 1 line

include/linux/efi-bgrt.h +0 -0

 #include <linux/acpi.h>

Quoted 43 lines

include/linux/efi-bgrt.h +3 -4

-void efi_bgrt_init(void);
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -546,11 +546,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c +0 -1

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 
        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

Before invoking the arch specific handler, efi_mem_reserve() reserves the given memory region through memblock.

efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time memblock is dead and it should not be used any more.

efi bgrt code depend on acpi intialization to get the bgrt acpi table, moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve in efi bgrt code still use memblock safely.

Signed-off-by Dave Young <[email protected]>

---
v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c
v2->v3: drop #ifdef added before; efi-bgrt.h build warning fix
since only changed this patch, so I just only resend this one.

 arch/x86/kernel/acpi/boot.c      |    9 +++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |   11 +++----
 init/main.c                      |    1
 6 files changed, 59 insertions(+), 54 deletions(-)

arch/x86/kernel/acpi/boot.c +9 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,12 @@ int __init early_acpi_boot_init(void)
        return 0;
 }
 
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+       efi_bgrt_init(table);
+       return 0;
+}
+
 int __init acpi_boot_init(void)
 {
        /* those are executed after early-quirks are executed */

@@ -1581,6 +1588,8 @@ int __init acpi_boot_init(void)
        acpi_process_madt();
 
        acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
+       if (IS_ENABLED(CONFIG_ACPI_BGRT))
+               acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 
        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +5 -6

@@ -1,20 +1,19 @@
 #ifndef _LINUX_EFI_BGRT_H
 #define _LINUX_EFI_BGRT_H
 
-#ifdef CONFIG_ACPI_BGRT
-
 #include <linux/acpi.h>
 
-void efi_bgrt_init(void);
+#ifdef CONFIG_ACPI_BGRT
+
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -542,11 +542,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c +0 -1

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 
        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

Quoted 11 lines · Dave Young

Before invoking the arch specific handler, efi_mem_reserve() reserves the given memory region through memblock.

efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time memblock is dead and it should not be used any more.

efi bgrt code depend on acpi intialization to get the bgrt acpi table, moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve in efi bgrt code still use memblock safely.

Signed-off-by Dave Young <[email protected]>

This patch looks fine to me know

Reviewed-by Ard Biesheuvel <[email protected]>

but before applying it, I'd like

- Bhupesh to confirm that this patch is a move in the right direction with regard to enabling BGRT on arm64/ACPI,

- an ack from the ACPI maintainers (cc'ed)

Thanks,
Ard.

Quoted 286 lines

---
v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c
v2->v3: drop #ifdef added before; efi-bgrt.h build warning fix
since only changed this patch, so I just only resend this one.

 arch/x86/kernel/acpi/boot.c      |    9 +++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |   11 +++----
 init/main.c                      |    1
 6 files changed, 59 insertions(+), 54 deletions(-)

arch/x86/kernel/acpi/boot.c +9 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,12 @@ int __init early_acpi_boot_init(void)
        return 0;
 }
 
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+       efi_bgrt_init(table);
+       return 0;
+}
+
 int __init acpi_boot_init(void)
 {
        /* those are executed after early-quirks are executed */

@@ -1581,6 +1588,8 @@ int __init acpi_boot_init(void)
        acpi_process_madt();
 
        acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
+       if (IS_ENABLED(CONFIG_ACPI_BGRT))
+               acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 
        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +5 -6

@@ -1,20 +1,19 @@
 #ifndef _LINUX_EFI_BGRT_H
 #define _LINUX_EFI_BGRT_H
 
-#ifdef CONFIG_ACPI_BGRT
-
 #include <linux/acpi.h>
 
-void efi_bgrt_init(void);
+#ifdef CONFIG_ACPI_BGRT
+
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -542,11 +542,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c +0 -1

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 
        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

Quoted 13 lines · Ard Biesheuvel

Quoted 11 lines · Dave Young

Before invoking the arch specific handler, efi_mem_reserve() reserves the given memory region through memblock.

efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time memblock is dead and it should not be used any more.

efi bgrt code depend on acpi intialization to get the bgrt acpi table, moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve in efi bgrt code still use memblock safely.

Signed-off-by Dave Young <[email protected]>

Quoted 8 lines

This patch looks fine to me know

Reviewed-by Ard Biesheuvel <[email protected]>

but before applying it, I'd like

- Bhupesh to confirm that this patch is a move in the right direction with regard to enabling BGRT on arm64/ACPI,

I gave the changes from Dave a try on top of the following combination:
4.10-rc3 + efi/next patches not available in 4.10-rc3 + Nicolai's patches

and was able to get the BGRT table working properly with OVMF on a QEMU-x86_64 machine. So you can add my tested-by for this patch series.

I think this is the right direction for the ARM64 BGRT handling patches as well and I will post a RFC in two phases as suggested by Ard, once Dave's patches are accepted (in efi/next?).

Quoted 3 lines

- an ack from the ACPI maintainers (cc'ed)

Quoted 578 lines

Quoted 278 lines

--->>> v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c
v2->v3: drop #ifdef added before; efi-bgrt.h build warning fix
since only changed this patch, so I just only resend this one.

 arch/x86/kernel/acpi/boot.c      |    9 +++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |   11 +++----
 init/main.c                      |    1
 6 files changed, 59 insertions(+), 54 deletions(-)

arch/x86/kernel/acpi/boot.c +9 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,12 @@ int __init early_acpi_boot_init(void)
        return 0;
 }
 
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+       efi_bgrt_init(table);
+       return 0;
+}
+
 int __init acpi_boot_init(void)
 {
        /* those are executed after early-quirks are executed */

@@ -1581,6 +1588,8 @@ int __init acpi_boot_init(void)
        acpi_process_madt();
 
        acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
+       if (IS_ENABLED(CONFIG_ACPI_BGRT))
+               acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 
        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +5 -6

@@ -1,20 +1,19 @@
 #ifndef _LINUX_EFI_BGRT_H
 #define _LINUX_EFI_BGRT_H
 
-#ifdef CONFIG_ACPI_BGRT
-
 #include <linux/acpi.h>
 
-void efi_bgrt_init(void);
+#ifdef CONFIG_ACPI_BGRT
+
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -542,11 +542,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c asmlinkage __visible void __init start_k · line 668

Quoted 7 lines

init/main.c +0 -1

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 
        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

- an ack from the ACPI maintainers (cc'ed)

Thanks,
Ard.

Quoted 286 lines

---
v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c
v2->v3: drop #ifdef added before; efi-bgrt.h build warning fix
since only changed this patch, so I just only resend this one.

 arch/x86/kernel/acpi/boot.c      |    9 +++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |   11 +++----
 init/main.c                      |    1
 6 files changed, 59 insertions(+), 54 deletions(-)

arch/x86/kernel/acpi/boot.c +9 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,12 @@ int __init early_acpi_boot_init(void)
        return 0;
 }
 
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+       efi_bgrt_init(table);
+       return 0;
+}
+
 int __init acpi_boot_init(void)
 {
        /* those are executed after early-quirks are executed */

@@ -1581,6 +1588,8 @@ int __init acpi_boot_init(void)
        acpi_process_madt();
 
        acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
+       if (IS_ENABLED(CONFIG_ACPI_BGRT))
+               acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 
        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +5 -6

@@ -1,20 +1,19 @@
 #ifndef _LINUX_EFI_BGRT_H
 #define _LINUX_EFI_BGRT_H
 
-#ifdef CONFIG_ACPI_BGRT
-
 #include <linux/acpi.h>
 
-void efi_bgrt_init(void);
+#ifdef CONFIG_ACPI_BGRT
+
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -542,11 +542,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c +0 -1

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 
        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

Quoted 27 lines · Bhupesh Sharma

Quoted 13 lines · Ard Biesheuvel

Quoted 11 lines · Dave Young

Before invoking the arch specific handler, efi_mem_reserve() reserves the given memory region through memblock.

efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time memblock is dead and it should not be used any more.

efi bgrt code depend on acpi intialization to get the bgrt acpi table, moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve in efi bgrt code still use memblock safely.

Signed-off-by Dave Young <[email protected]>

Quoted 8 lines

This patch looks fine to me know

Reviewed-by Ard Biesheuvel <[email protected]>

but before applying it, I'd like

- Bhupesh to confirm that this patch is a move in the right direction with regard to enabling BGRT on arm64/ACPI,

I gave the changes from Dave a try on top of the following combination:
4.10-rc3 + efi/next patches not available in 4.10-rc3 + Nicolai's patches

Quoted 8 lines

and was able to get the BGRT table working properly with OVMF on a QEMU-x86_64 machine. So you can add my tested-by for this patch series.

I think this is the right direction for the ARM64 BGRT handling patches as well and I will post a RFC in two phases as suggested by Ard, once Dave's patches are accepted (in efi/next?).

Thanks!

Quoted 3 lines

Quoted 3 lines

- an ack from the ACPI maintainers (cc'ed)

Rafael, Len? Any objections?

If not, I will go ahead and queue this for v4.11

Quoted 575 lines

Quoted 575 lines

Quoted 280 lines

--->>> v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c
v2->v3: drop #ifdef added before; efi-bgrt.h build warning fix
since only changed this patch, so I just only resend this one.

 arch/x86/kernel/acpi/boot.c      |    9 +++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |   11 +++----
 init/main.c                      |    1
 6 files changed, 59 insertions(+), 54 deletions(-)

arch/x86/kernel/acpi/boot.c +9 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,12 @@ int __init early_acpi_boot_init(void)
        return 0;
 }
 
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+       efi_bgrt_init(table);
+       return 0;
+}
+
 int __init acpi_boot_init(void)
 {
        /* those are executed after early-quirks are executed */

@@ -1581,6 +1588,8 @@ int __init acpi_boot_init(void)
        acpi_process_madt();
 
        acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
+       if (IS_ENABLED(CONFIG_ACPI_BGRT))
+               acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 
        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +5 -6

@@ -1,20 +1,19 @@
 #ifndef _LINUX_EFI_BGRT_H
 #define _LINUX_EFI_BGRT_H
 
-#ifdef CONFIG_ACPI_BGRT
-
 #include <linux/acpi.h>
 
-void efi_bgrt_init(void);
+#ifdef CONFIG_ACPI_BGRT
+
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -542,11 +542,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c asmlinkage __visible void __init start_k · line 668

Quoted 7 lines

init/main.c +0 -1

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 
        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

- an ack from the ACPI maintainers (cc'ed)

Thanks,
Ard.

Quoted 281 lines

---
v1->v2: efi_bgrt_init: check table length first before copying bgrt table
error checking in drivers/acpi/bgrt.c
v2->v3: drop #ifdef added before; efi-bgrt.h build warning fix
since only changed this patch, so I just only resend this one.

 arch/x86/kernel/acpi/boot.c      |    9 +++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |    5 ---
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 include/linux/efi-bgrt.h         |   11 +++----
 init/main.c                      |    1
 6 files changed, 59 insertions(+), 54 deletions(-)

arch/x86/kernel/acpi/boot.c +9 -0

@@ -35,6 +35,7 @@
 #include <linux/bootmem.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
+#include <linux/efi-bgrt.h>
 
 #include <asm/irqdomain.h>
 #include <asm/pci_x86.h>

@@ -1557,6 +1558,12 @@ int __init early_acpi_boot_init(void)
        return 0;
 }
 
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+       efi_bgrt_init(table);
+       return 0;
+}
+
 int __init acpi_boot_init(void)
 {
        /* those are executed after early-quirks are executed */

@@ -1581,6 +1588,8 @@ int __init acpi_boot_init(void)
        acpi_process_madt();
 
        acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
+       if (IS_ENABLED(CONFIG_ACPI_BGRT))
+               acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 
        if (!acpi_noirq)
                x86_init.pci.init = pci_acpi_init;

arch/x86/platform/efi/efi-bgrt.c +25 -34

@@ -19,8 +19,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 
-struct acpi_table_bgrt *bgrt_tab;
-void *__initdata bgrt_image;
+struct acpi_table_bgrt bgrt_tab;
 size_t __initdata bgrt_image_size;
 
 struct bmp_header {

@@ -28,66 +27,58 @@ struct bmp_header {
        u32 size;
 } __packed;
 
-void __init efi_bgrt_init(void)
+void __init efi_bgrt_init(struct acpi_table_header *table)
 {
-       acpi_status status;
        void *image;
        struct bmp_header bmp_header;
+       struct acpi_table_bgrt *bgrt = &bgrt_tab;
 
        if (acpi_disabled)
                return;
 
-       status = acpi_get_table("BGRT", 0,
-                               (struct acpi_table_header **)&bgrt_tab);
-       if (ACPI_FAILURE(status))
-               return;
-
-       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+       if (table->length < sizeof(bgrt_tab)) {
                pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-                      bgrt_tab->header.length, sizeof(*bgrt_tab));
+                      table->length, sizeof(bgrt_tab));
                return;
        }
-       if (bgrt_tab->version != 1) {
+       *bgrt = *(struct acpi_table_bgrt *)table;
+       if (bgrt->version != 1) {
                pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-                      bgrt_tab->version);
-               return;
+                      bgrt->version);
+               goto out;
        }
-       if (bgrt_tab->status & 0xfe) {
+       if (bgrt->status & 0xfe) {
                pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-                      bgrt_tab->status);
-               return;
+                      bgrt->status);
+               goto out;
        }
-       if (bgrt_tab->image_type != 0) {
+       if (bgrt->image_type != 0) {
                pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-                      bgrt_tab->image_type);
-               return;
+                      bgrt->image_type);
+               goto out;
        }
-       if (!bgrt_tab->image_address) {
+       if (!bgrt->image_address) {
                pr_notice("Ignoring BGRT: null image address\n");
-               return;
+               goto out;
        }
 
-       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+       image = early_memremap(bgrt->image_address, sizeof(bmp_header));
        if (!image) {
                pr_notice("Ignoring BGRT: failed to map image header memory\n");
-               return;
+               goto out;
        }
 
        memcpy(&bmp_header, image, sizeof(bmp_header));
-       memunmap(image);
+       early_memunmap(image, sizeof(bmp_header));
        if (bmp_header.id != 0x4d42) {
                pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
                        bmp_header.id);
-               return;
+               goto out;
        }
        bgrt_image_size = bmp_header.size;
+       efi_mem_reserve(bgrt->image_address, bgrt_image_size);
 
-       bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB);
-       if (!bgrt_image) {
-               pr_notice("Ignoring BGRT: failed to map image memory\n");
-               bgrt_image = NULL;
-               return;
-       }
-
-       efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size);
+       return;
+out:
+       memset(bgrt, 0, sizeof(bgrt_tab));
 }

drivers/acpi/bgrt.c +20 -8

@@ -15,40 +15,41 @@
 #include <linux/sysfs.h>
 #include <linux/efi-bgrt.h>
 
+static void *bgrt_image;
 static struct kobject *bgrt_kobj;
 
 static ssize_t show_version(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_status(struct device *dev,
                           struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status);
 }
 static DEVICE_ATTR(status, S_IRUGO, show_status, NULL);
 
 static ssize_t show_type(struct device *dev,
                         struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type);
 }
 static DEVICE_ATTR(type, S_IRUGO, show_type, NULL);
 
 static ssize_t show_xoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x);
 }
 static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL);
 
 static ssize_t show_yoffset(struct device *dev,
                            struct device_attribute *attr, char *buf)
 {
-       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y);
+       return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y);
 }
 static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL);
 

@@ -84,15 +85,24 @@ static int __init bgrt_init(void)
 {
        int ret;
 
-       if (!bgrt_image)
+       if (!bgrt_tab.image_address)
                return -ENODEV;
 
+       bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size,
+                             MEMREMAP_WB);
+       if (!bgrt_image) {
+               pr_notice("Ignoring BGRT: failed to map image memory\n");
+               return -ENOMEM;
+       }
+
        bin_attr_image.private = bgrt_image;
        bin_attr_image.size = bgrt_image_size;
 
        bgrt_kobj = kobject_create_and_add("bgrt", acpi_kobj);
-       if (!bgrt_kobj)
-               return -EINVAL;
+       if (!bgrt_kobj) {
+               ret = -EINVAL;
+               goto out_memmap;
+       }
 
        ret = sysfs_create_group(bgrt_kobj, &bgrt_attribute_group);
        if (ret)

@@ -102,6 +112,8 @@ static int __init bgrt_init(void)
 
 out_kobject:
        kobject_put(bgrt_kobj);
+out_memmap:
+       memunmap(bgrt_image);
        return ret;
 }
 device_initcall(bgrt_init);

include/linux/efi-bgrt.h +5 -6

@@ -1,20 +1,19 @@
 #ifndef _LINUX_EFI_BGRT_H
 #define _LINUX_EFI_BGRT_H
 
-#ifdef CONFIG_ACPI_BGRT
-
 #include <linux/acpi.h>
 
-void efi_bgrt_init(void);
+#ifdef CONFIG_ACPI_BGRT
+
+void efi_bgrt_init(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
-extern void *bgrt_image;
 extern size_t bgrt_image_size;
-extern struct acpi_table_bgrt *bgrt_tab;
+extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(void) {}
+static inline void efi_bgrt_init(struct acpi_table_header *table) {}
 
 #endif /* !CONFIG_ACPI_BGRT */
 

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

@@ -542,11 +542,6 @@ void __init efi_init(void)
                efi_print_memmap();
 }
 
-void __init efi_late_init(void)
-{
-       efi_bgrt_init();
-}
-
 void __init efi_set_executable(efi_memory_desc_t *md, bool executable)
 {
        u64 addr, npages;

init/main.c +0 -0

@@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k
        sfi_init_late();
 

Quoted 5 lines

Quoted 5 lines

init/main.c asmlinkage __visible void __init start_k · line 668

Quoted 5 lines

init/main.c +0 -1

        if (efi_enabled(EFI_RUNTIME_SERVICES)) {
-               efi_late_init();
                efi_free_boot_services();
        }
 

Signed-off-by Dave Young <[email protected]>

---
v1->v2: move efi_print_memmap declaration to general header file

 arch/x86/include/asm/efi.h    |    1 -
 arch/x86/platform/efi/efi.c   |   16 ----------------
 drivers/firmware/efi/memmap.c |   16 ++++++++++++++++
 include/linux/efi.h           |    1 +
 4 files changed, 17 insertions(+), 17 deletions(-)

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

@@ -278,22 +278,6 @@ static void __init efi_clean_memmap(void
        }
 }
 
-void __init efi_print_memmap(void)
-{
-       efi_memory_desc_t *md;
-       int i = 0;
-
-       for_each_efi_memory_desc(md) {
-               char buf[64];
-
-               pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
-                       i++, efi_md_typeattr_format(buf, sizeof(buf), md),
-                       md->phys_addr,
-                       md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
-                       (md->num_pages >> (20 - EFI_PAGE_SHIFT)));
-       }
-}
-
 static int __init efi_systab_init(void *phys)
 {
        if (efi_enabled(EFI_64BIT)) {

drivers/firmware/efi/memmap.c +16 -0

@@ -10,6 +10,22 @@
 #include <linux/io.h>
 #include <asm/early_ioremap.h>
 
+void __init efi_print_memmap(void)
+{
+       efi_memory_desc_t *md;
+       int i = 0;
+
+       for_each_efi_memory_desc(md) {
+               char buf[64];
+
+               pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
+                       i++, efi_md_typeattr_format(buf, sizeof(buf), md),
+                       md->phys_addr,
+                       md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
+                       (md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+       }
+}
+
 /**
  * __efi_memmap_init - Common code for mapping the EFI memory map
  * @data: EFI memory map data

arch/x86/include/asm/efi.h +0 -1

@@ -116,7 +116,6 @@ extern void __init efi_set_executable(ef
 extern int __init efi_memblock_x86_reserve_range(void);
 extern pgd_t * __init efi_call_phys_prolog(void);
 extern void __init efi_call_phys_epilog(pgd_t *save_pgd);
-extern void __init efi_print_memmap(void);
 extern void __init efi_memory_uc(u64 addr, unsigned long size);
 extern void __init efi_map_region(efi_memory_desc_t *md);
 extern void __init efi_map_region_fixed(efi_memory_desc_t *md);

include/linux/efi.h +1 -0

@@ -949,6 +949,7 @@ static inline efi_status_t efi_query_var
        return EFI_SUCCESS;
 }
 #endif
+extern void __init efi_print_memmap(void);
 extern void __iomem *efi_lookup_mapped_addr(u64 phys_addr);
 
 extern int __init efi_memmap_init_early(struct efi_memory_map_data *data);

efi_mem_reserve cares only about boot services regions, for making sure later efi_free_boot_services does not free areas which are still useful, such as bgrt image buffer.

So add a new argument to efi_memmap_insert for this purpose.

Signed-off-by Dave Young <[email protected]>

---
v1->v2: only check EFI_BOOT_SERVICES_CODE/_DATA

 arch/x86/platform/efi/quirks.c  |    2 +-
 drivers/firmware/efi/fake_mem.c |    3 ++-
 drivers/firmware/efi/memmap.c   |    6 +++++-
 include/linux/efi.h             |    4 ++--
 4 files changed, 10 insertions(+), 5 deletions(-)

drivers/firmware/efi/memmap.c +5 -1

@@ -229,7 +229,7 @@ int __init efi_memmap_split_count(efi_me
  * to see how large @buf needs to be.
  */
 void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
-                             struct efi_mem_range *mem)
+                             struct efi_mem_range *mem, bool boot_only)
 {
        u64 m_start, m_end, m_attr;
        efi_memory_desc_t *md;

@@ -262,6 +262,10 @@ void __init efi_memmap_insert(struct efi
                start = md->phys_addr;
                end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
 
+               if (boot_only && !(md->type == EFI_BOOT_SERVICES_CODE ||
+                                  md->type == EFI_BOOT_SERVICES_DATA))
+                       continue;
+
                if (m_start <= start && end <= m_end)
                        md->attribute |= m_attr;
 

arch/x86/platform/efi/quirks.c +1 -1

@@ -226,7 +226,7 @@ void __init efi_arch_mem_reserve(phys_ad
                return;
        }
 
-       efi_memmap_insert(&efi.memmap, new, &mr);
+       efi_memmap_insert(&efi.memmap, new, &mr, true);
        early_memunmap(new, new_size);
 
        efi_memmap_install(new_phys, num_entries);

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

@@ -85,7 +85,8 @@ void __init efi_fake_memmap(void)
        }
 
        for (i = 0; i < nr_fake_mem; i++)
-               efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i]);
+               efi_memmap_insert(&efi.memmap, new_memmap, &fake_mems[i],
+                                 false);
 
        /* swap into new EFI memmap */
        early_memunmap(new_memmap, efi.memmap.desc_size * new_nr_map);

include/linux/efi.h +2 -2

@@ -959,8 +959,8 @@ extern int __init efi_memmap_install(phy
 extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
                                         struct range *range);
 extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
-                                    void *buf, struct efi_mem_range *mem);
-
+                                    void *buf, struct efi_mem_range *mem,
+                                    bool boot_only);
 extern int efi_config_init(efi_config_table_type_t *arch_tables);
 #ifdef CONFIG_EFI_ESRT
 extern void __init efi_esrt_init(void);

Quoted 6 lines · Dave Young

efi_mem_reserve cares only about boot services regions, for making sure later efi_free_boot_services does not free areas which are still useful, such as bgrt image buffer.

So add a new argument to efi_memmap_insert for this purpose.

So what happens is we try to efi_mem_reserve() a regions that is not bootservices code or data?
We shouldn't simply ignore it, because it is a serious condition.

Quoted 11 lines · Ard Biesheuvel

Quoted 6 lines · Dave Young

efi_mem_reserve cares only about boot services regions, for making sure later efi_free_boot_services does not free areas which are still useful, such as bgrt image buffer.

So add a new argument to efi_memmap_insert for this purpose.

So what happens is we try to efi_mem_reserve() a regions that is not bootservices code or data?
We shouldn't simply ignore it, because it is a serious condition.

The efi_mem_desc_lookup() call in efi_arch_mem_reserve() wouldn't return anything and the latter would

pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);

then.

This is so because efi_mem_desc_lookup() searches only for regions that either
- are of type EFI_BOOT_SERVICES_DATA or EFI_RUNTIME_SERVICES_DATA
- or which have EFI_MEMORY_RUNTIME set already:

	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
	    md->type != EFI_BOOT_SERVICES_DATA &&
	    md->type != EFI_RUNTIME_SERVICES_DATA) {
		continue;
	}

For EFI_RUNTIME_SERVICES_DATA and EFI_MEMORY_RUNTIME, efi_arch_mem_reserve() would be a nop.

So we're fine here? Do you want to have a more descriptive error message than "Failed to lookup EFI memory descriptor"?

For the other checks you suggested in that other thread, i.e. for the post-slab_is_available() condition and so: let me wait until Dave's series has stabilized (or even picked) and I'll submit patches for what remains to be sanity checked then.

Also, since Dave eliminated the need for late efi_mem_reserve()'s, my 20b1e22d01a4 ("x86/efi: Don't allocate memmap through memblock after mm_init()") should certainly get reverted at some point.

Thanks,

Nicolai

Quoted 39 lines · Nicolai Stange

Quoted 11 lines · Ard Biesheuvel

Quoted 6 lines · Dave Young

efi_mem_reserve cares only about boot services regions, for making sure later efi_free_boot_services does not free areas which are still useful, such as bgrt image buffer.

So add a new argument to efi_memmap_insert for this purpose.

So what happens is we try to efi_mem_reserve() a regions that is not bootservices code or data?
We shouldn't simply ignore it, because it is a serious condition.

The efi_mem_desc_lookup() call in efi_arch_mem_reserve() wouldn't return anything and the latter would

pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);

then.

This is so because efi_mem_desc_lookup() searches only for regions that either
- are of type EFI_BOOT_SERVICES_DATA or EFI_RUNTIME_SERVICES_DATA
- or which have EFI_MEMORY_RUNTIME set already:

	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
	    md->type != EFI_BOOT_SERVICES_DATA &&
	    md->type != EFI_RUNTIME_SERVICES_DATA) {
		continue;
	}

For EFI_RUNTIME_SERVICES_DATA and EFI_MEMORY_RUNTIME, efi_arch_mem_reserve() would be a nop.

So we're fine here? Do you want to have a more descriptive error message than "Failed to lookup EFI memory descriptor"?

Quoted 8 lines

For the other checks you suggested in that other thread, i.e. for the post-slab_is_available() condition and so: let me wait until Dave's series has stabilized (or even picked) and I'll submit patches for what remains to be sanity checked then.

Also, since Dave eliminated the need for late efi_mem_reserve()'s, my 20b1e22d01a4 ("x86/efi: Don't allocate memmap through memblock after mm_init()") should certainly get reverted at some point.

While testing my patches with latest edk2, I found another thing to be fixed, I will repost bgrt patch according to Ard's comment tomorrow, maybe with below patch as another fix to the memblock_alloc late callback.

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

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

 	if (!new_phys) {
 		pr_err("Failed to allocate new EFI memmap\n");
 		return;

Quoted 6 lines

Thanks,

Nicolai

Quoted 45 lines · Dave Young

Quoted 44 lines · Nicolai Stange

Quoted 11 lines · Ard Biesheuvel

Quoted 6 lines · Dave Young

efi_mem_reserve cares only about boot services regions, for making sure later efi_free_boot_services does not free areas which are still useful, such as bgrt image buffer.

So add a new argument to efi_memmap_insert for this purpose.

So what happens is we try to efi_mem_reserve() a regions that is not bootservices code or data?
We shouldn't simply ignore it, because it is a serious condition.

The efi_mem_desc_lookup() call in efi_arch_mem_reserve() wouldn't return anything and the latter would

pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);

then.

This is so because efi_mem_desc_lookup() searches only for regions that either
- are of type EFI_BOOT_SERVICES_DATA or EFI_RUNTIME_SERVICES_DATA
- or which have EFI_MEMORY_RUNTIME set already:

	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
	    md->type != EFI_BOOT_SERVICES_DATA &&
	    md->type != EFI_RUNTIME_SERVICES_DATA) {
		continue;
	}

For EFI_RUNTIME_SERVICES_DATA and EFI_MEMORY_RUNTIME, efi_arch_mem_reserve() would be a nop.

So we're fine here? Do you want to have a more descriptive error message than "Failed to lookup EFI memory descriptor"?

For the other checks you suggested in that other thread, i.e. for the post-slab_is_available() condition and so: let me wait until Dave's series has stabilized (or even picked) and I'll submit patches for what remains to be sanity checked then.

Quoted 8 lines

Quoted 3 lines

Also, since Dave eliminated the need for late efi_mem_reserve()'s, my 20b1e22d01a4 ("x86/efi: Don't allocate memmap through memblock after mm_init()") should certainly get reverted at some point.

While testing my patches with latest edk2, I found another thing to be fixed, I will repost bgrt patch according to Ard's comment tomorrow, maybe with below patch as another fix to the memblock_alloc late callback.

arch/x86/platform/efi/quirks.c void __init efi_free_boot_services(void) · line 358

Quoted 12 lines

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

arch/x86/platform/efi/quirks.c +0 -1

@@ -355,7 +355,7 @@ void __init efi_free_boot_services(void)
 	}
 
 	new_size = efi.memmap.desc_size * num_entries;
-	new_phys = kzalloc(new_size, GFP_KERNEL);

Oops, it was memblock_alloc(), this is a middle test code used for debugging. Just ignore it.

arch/x86/platform/efi/quirks.c void __init efi_free_boot_services(void) · line 358

Quoted 1 line

arch/x86/platform/efi/quirks.c +1 -0

+	new_phys = efi_memmap_alloc(num_entries);

Maybe a efi_memmap_late_alloc is enough, will see if I can get a better version, maybe your previous patch can be dropped partially or just kept.

Quoted 9 lines

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

 	if (!new_phys) {
 		pr_err("Failed to allocate new EFI memmap\n");
 		return;

Quoted 6 lines

Thanks,

Nicolai