[PATCH V2 0/4] efi/x86: move efi bgrt init code to early init

[PATCH V2 0/4] efi/x86: move efi bgrt init code to early init

[Dave Young]

Hi,

Here the the update of the series for moving bgrt init code to early init.

Main changes is:
- Move the 1st patch to the last because it does not block the 2nd patch
any more with Peter's patch to prune invlid memmap entries:
https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91
a35445229
But it is still tood to have since efi_mem_reserve only cares about boot related
mem ranges.

- Other comments about code itself, details please the the patches themselves.

 arch/x86/include/asm/efi.h       |    1 
 arch/x86/kernel/acpi/boot.c      |   12 +++++++
 arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
 arch/x86/platform/efi/efi.c      |   26 +++--------------
 arch/x86/platform/efi/quirks.c   |    2 -
 drivers/acpi/bgrt.c              |   28 +++++++++++++-----
 drivers/firmware/efi/fake_mem.c  |    3 +
 drivers/firmware/efi/memmap.c    |   22 +++++++++++++-
 include/linux/efi-bgrt.h         |    7 +---
 include/linux/efi.h              |    5 +--
 init/main.c                      |    1 
 11 files changed, 92 insertions(+), 74 deletions(-)

Thanks
Dave

[PATCH V2 1/4] efi/x86: move efi bgrt init code to early init code

[Dave Young]

[-- Attachment #1: efi-bgrt-fix.patch --]
[-- Type: text/plain, Size: 8343 bytes --]

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 <dyoung@redhat.com>
---
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(-)

--- linux-x86.orig/arch/x86/kernel/acpi/boot.c
+++ linux-x86/arch/x86/kernel/acpi/boot.c
@@ -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;
 }
 
+#ifdef CONFIG_ACPI_BGRT
+static int __init acpi_parse_bgrt(struct acpi_table_header *table)
+{
+	efi_bgrt_init(table);
+	return 0;
+}
+#endif
+
 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
+	acpi_table_parse("BGRT", acpi_parse_bgrt);
+#endif
 
 	if (!acpi_noirq)
 		x86_init.pci.init = pci_acpi_init;
--- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
+++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
@@ -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));
 }
--- linux-x86.orig/drivers/acpi/bgrt.c
+++ linux-x86/drivers/acpi/bgrt.c
@@ -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);
--- linux-x86.orig/include/linux/efi-bgrt.h
+++ linux-x86/include/linux/efi-bgrt.h
@@ -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 */
 
--- linux-x86.orig/arch/x86/platform/efi/efi.c
+++ linux-x86/arch/x86/platform/efi/efi.c
@@ -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;
--- linux-x86.orig/init/main.c
+++ linux-x86/init/main.c
@@ -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();
 	}
 

Re: [PATCH V2 1/4] efi/x86: move efi bgrt init code to early init code

[Nicolai Stange]

Reviewed-and-tested-by: Nicolai Stange <nicstange@gmail.com>

Re: [PATCH V2 1/4] efi/x86: move efi bgrt init code to early init code

[Ard Biesheuvel]

On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> 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 <dyoung@redhat.com>
> ---
> 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(-)
>
> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
> +++ linux-x86/arch/x86/kernel/acpi/boot.c
> @@ -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;
>  }
>
> +#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

>  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))

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

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

>         if (!acpi_noirq)
>                 x86_init.pci.init = pci_acpi_init;
> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
> @@ -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));
>  }
> --- linux-x86.orig/drivers/acpi/bgrt.c
> +++ linux-x86/drivers/acpi/bgrt.c
> @@ -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);
> --- linux-x86.orig/include/linux/efi-bgrt.h
> +++ linux-x86/include/linux/efi-bgrt.h
> @@ -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 */
>
> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> +++ linux-x86/arch/x86/platform/efi/efi.c
> @@ -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;
> --- linux-x86.orig/init/main.c
> +++ linux-x86/init/main.c
> @@ -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();
>         }
>
>
>

Re: [PATCH V2 1/4] efi/x86: move efi bgrt init code to early init code

[Ard Biesheuvel]

On 16 January 2017 at 02:45, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 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 <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> 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(-)
>
> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
> +++ linux-x86/arch/x86/kernel/acpi/boot.c
> @@ -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;
>  }
>
> +#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

>  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))

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

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

>         if (!acpi_noirq)
>                 x86_init.pci.init = pci_acpi_init;
> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
> @@ -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));
>  }
> --- linux-x86.orig/drivers/acpi/bgrt.c
> +++ linux-x86/drivers/acpi/bgrt.c
> @@ -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);
> --- linux-x86.orig/include/linux/efi-bgrt.h
> +++ linux-x86/include/linux/efi-bgrt.h
> @@ -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 */
>
> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> +++ linux-x86/arch/x86/platform/efi/efi.c
> @@ -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;
> --- linux-x86.orig/init/main.c
> +++ linux-x86/init/main.c
> @@ -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();
>         }
>
>
>

Re: [PATCH V2 1/4] efi/x86: move efi bgrt init code to early init code

[Dave Young]

On 01/17/17 at 05:10pm, Ard Biesheuvel wrote:
> On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> > 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 <dyoung@redhat.com>
> > ---
> > 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(-)
> >
> > --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
> > +++ linux-x86/arch/x86/kernel/acpi/boot.c
> > @@ -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;
> >  }
> >
> > +#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
> 
> >  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))
> 
> > +       acpi_table_parse("BGRT", acpi_parse_bgrt);
> > +#endif
> >
> 
> and perhaps we should add a #define for ACPI_SIG_BGRT as well?

There is ACPI_SIG_BGRT already in acpi header file, will use it and
switch to if (IS_ENABLED..) as you mentioned.

> 
> >         if (!acpi_noirq)
> >                 x86_init.pci.init = pci_acpi_init;
> > --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
> > +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
> > @@ -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));
> >  }
> > --- linux-x86.orig/drivers/acpi/bgrt.c
> > +++ linux-x86/drivers/acpi/bgrt.c
> > @@ -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);
> > --- linux-x86.orig/include/linux/efi-bgrt.h
> > +++ linux-x86/include/linux/efi-bgrt.h
> > @@ -5,16 +5,15 @@
> >
> >  #include <linux/acpi.h>

Will move above including before #ifdef CONFIG_ACPI_BGRT because #else
dummy function declaration use the argument type. Or there will be a
build warning.

> >
> > -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 */
> >
> > --- linux-x86.orig/arch/x86/platform/efi/efi.c
> > +++ linux-x86/arch/x86/platform/efi/efi.c
> > @@ -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;
> > --- linux-x86.orig/init/main.c
> > +++ linux-x86/init/main.c
> > @@ -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();
> >         }
> >
> >
> >

Re: [PATCH V2 1/4] efi/x86: move efi bgrt init code to early init code

[Dave Young]

On 01/17/17 at 05:10pm, Ard Biesheuvel wrote:
> On 16 January 2017 at 02:45, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > 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 <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > 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(-)
> >
> > --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
> > +++ linux-x86/arch/x86/kernel/acpi/boot.c
> > @@ -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;
> >  }
> >
> > +#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
> 
> >  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))
> 
> > +       acpi_table_parse("BGRT", acpi_parse_bgrt);
> > +#endif
> >
> 
> and perhaps we should add a #define for ACPI_SIG_BGRT as well?

There is ACPI_SIG_BGRT already in acpi header file, will use it and
switch to if (IS_ENABLED..) as you mentioned.

> 
> >         if (!acpi_noirq)
> >                 x86_init.pci.init = pci_acpi_init;
> > --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
> > +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
> > @@ -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));
> >  }
> > --- linux-x86.orig/drivers/acpi/bgrt.c
> > +++ linux-x86/drivers/acpi/bgrt.c
> > @@ -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);
> > --- linux-x86.orig/include/linux/efi-bgrt.h
> > +++ linux-x86/include/linux/efi-bgrt.h
> > @@ -5,16 +5,15 @@
> >
> >  #include <linux/acpi.h>

Will move above including before #ifdef CONFIG_ACPI_BGRT because #else
dummy function declaration use the argument type. Or there will be a
build warning.

> >
> > -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 */
> >
> > --- linux-x86.orig/arch/x86/platform/efi/efi.c
> > +++ linux-x86/arch/x86/platform/efi/efi.c
> > @@ -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;
> > --- linux-x86.orig/init/main.c
> > +++ linux-x86/init/main.c
> > @@ -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();
> >         }
> >
> >
> >

[PATCH V3 1/4] efi/x86: move efi bgrt init code to early init code

[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 <dyoung@redhat.com>
---
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(-)

--- linux-x86.orig/arch/x86/kernel/acpi/boot.c
+++ linux-x86/arch/x86/kernel/acpi/boot.c
@@ -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;
--- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
+++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
@@ -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));
 }
--- linux-x86.orig/drivers/acpi/bgrt.c
+++ linux-x86/drivers/acpi/bgrt.c
@@ -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);
--- linux-x86.orig/include/linux/efi-bgrt.h
+++ linux-x86/include/linux/efi-bgrt.h
@@ -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 */
 
--- linux-x86.orig/arch/x86/platform/efi/efi.c
+++ linux-x86/arch/x86/platform/efi/efi.c
@@ -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;
--- linux-x86.orig/init/main.c
+++ linux-x86/init/main.c
@@ -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();
 	}
 

Re: [PATCH V3 1/4] efi/x86: move efi bgrt init code to early init code

[Ard Biesheuvel]

On 18 January 2017 at 13:48, Dave Young <dyoung@redhat.com> wrote:
> 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 <dyoung@redhat.com>

This patch looks fine to me know

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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.


> ---
> 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(-)
>
> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
> +++ linux-x86/arch/x86/kernel/acpi/boot.c
> @@ -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;
> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
> @@ -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));
>  }
> --- linux-x86.orig/drivers/acpi/bgrt.c
> +++ linux-x86/drivers/acpi/bgrt.c
> @@ -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);
> --- linux-x86.orig/include/linux/efi-bgrt.h
> +++ linux-x86/include/linux/efi-bgrt.h
> @@ -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 */
>
> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> +++ linux-x86/arch/x86/platform/efi/efi.c
> @@ -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;
> --- linux-x86.orig/init/main.c
> +++ linux-x86/init/main.c
> @@ -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();
>         }
>

Re: [PATCH V3 1/4] efi/x86: move efi bgrt init code to early init code

[Bhupesh Sharma]

On Wed, Jan 18, 2017 at 7:30 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 18 January 2017 at 13:48, Dave Young <dyoung@redhat.com> wrote:
>> 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 <dyoung@redhat.com>
>
> This patch looks fine to me know
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> 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?).

Regards,
Bhupesh

> - an ack from the ACPI maintainers (cc'ed)
>
> Thanks,
> Ard.
>
>
>> ---
>> 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(-)
>>
>> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
>> +++ linux-x86/arch/x86/kernel/acpi/boot.c
>> @@ -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;
>> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
>> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
>> @@ -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));
>>  }
>> --- linux-x86.orig/drivers/acpi/bgrt.c
>> +++ linux-x86/drivers/acpi/bgrt.c
>> @@ -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);
>> --- linux-x86.orig/include/linux/efi-bgrt.h
>> +++ linux-x86/include/linux/efi-bgrt.h
>> @@ -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 */
>>
>> --- linux-x86.orig/arch/x86/platform/efi/efi.c
>> +++ linux-x86/arch/x86/platform/efi/efi.c
>> @@ -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;
>> --- linux-x86.orig/init/main.c
>> +++ linux-x86/init/main.c
>> @@ -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();
>>         }
>>

Re: [PATCH V3 1/4] efi/x86: move efi bgrt init code to early init code

[Ard Biesheuvel]

On 18 January 2017 at 19:24, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> On Wed, Jan 18, 2017 at 7:30 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 18 January 2017 at 13:48, Dave Young <dyoung@redhat.com> wrote:
>>> 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 <dyoung@redhat.com>
>>
>> This patch looks fine to me know
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> 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?).
>

Thanks!

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

Rafael, Len? Any objections?

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


>>> --->>> 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(-)
>>>
>>> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
>>> +++ linux-x86/arch/x86/kernel/acpi/boot.c
>>> @@ -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;
>>> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
>>> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
>>> @@ -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));
>>>  }
>>> --- linux-x86.orig/drivers/acpi/bgrt.c
>>> +++ linux-x86/drivers/acpi/bgrt.c
>>> @@ -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);
>>> --- linux-x86.orig/include/linux/efi-bgrt.h
>>> +++ linux-x86/include/linux/efi-bgrt.h
>>> @@ -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 */
>>>
>>> --- linux-x86.orig/arch/x86/platform/efi/efi.c
>>> +++ linux-x86/arch/x86/platform/efi/efi.c
>>> @@ -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;
>>> --- linux-x86.orig/init/main.c
>>> +++ linux-x86/init/main.c
>>> @@ -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.
>>
>>
>>> ---
>>> 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(-)
>>>
>>> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
>>> +++ linux-x86/arch/x86/kernel/acpi/boot.c
>>> @@ -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;
>>> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
>>> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
>>> @@ -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));
>>>  }
>>> --- linux-x86.orig/drivers/acpi/bgrt.c
>>> +++ linux-x86/drivers/acpi/bgrt.c
>>> @@ -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);
>>> --- linux-x86.orig/include/linux/efi-bgrt.h
>>> +++ linux-x86/include/linux/efi-bgrt.h
>>> @@ -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 */
>>>
>>> --- linux-x86.orig/arch/x86/platform/efi/efi.c
>>> +++ linux-x86/arch/x86/platform/efi/efi.c
>>> @@ -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;
>>> --- linux-x86.orig/init/main.c
>>> +++ linux-x86/init/main.c
>>> @@ -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();
>>>         }
>>>

Re: [PATCH V3 1/4] efi/x86: move efi bgrt init code to early init code

[Dave Young]

On 01/19/17 at 12:48pm, Ard Biesheuvel wrote:
> On 18 January 2017 at 19:24, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> > On Wed, Jan 18, 2017 at 7:30 PM, Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> >> On 18 January 2017 at 13:48, Dave Young <dyoung@redhat.com> wrote:
> >>> 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 <dyoung@redhat.com>
> >>
> >> This patch looks fine to me know
> >>
> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> 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?).
> >
> 
> Thanks!
> 
> >
> >> - an ack from the ACPI maintainers (cc'ed)
> >>
> 
> Rafael, Len? Any objections?


Ping..

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

Ard, thanks, just ignore the last one 4/4 if you think it is risky or
unnecessary. 

> 
> 
> >>> --->>> 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(-)
> >>>
> >>> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
> >>> +++ linux-x86/arch/x86/kernel/acpi/boot.c
> >>> @@ -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;
> >>> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
> >>> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
> >>> @@ -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));
> >>>  }
> >>> --- linux-x86.orig/drivers/acpi/bgrt.c
> >>> +++ linux-x86/drivers/acpi/bgrt.c
> >>> @@ -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);
> >>> --- linux-x86.orig/include/linux/efi-bgrt.h
> >>> +++ linux-x86/include/linux/efi-bgrt.h
> >>> @@ -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 */
> >>>
> >>> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> >>> +++ linux-x86/arch/x86/platform/efi/efi.c
> >>> @@ -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;
> >>> --- linux-x86.orig/init/main.c
> >>> +++ linux-x86/init/main.c
> >>> @@ -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.
> >>
> >>
> >>> ---
> >>> 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(-)
> >>>
> >>> --- linux-x86.orig/arch/x86/kernel/acpi/boot.c
> >>> +++ linux-x86/arch/x86/kernel/acpi/boot.c
> >>> @@ -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;
> >>> --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c
> >>> +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c
> >>> @@ -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));
> >>>  }
> >>> --- linux-x86.orig/drivers/acpi/bgrt.c
> >>> +++ linux-x86/drivers/acpi/bgrt.c
> >>> @@ -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);
> >>> --- linux-x86.orig/include/linux/efi-bgrt.h
> >>> +++ linux-x86/include/linux/efi-bgrt.h
> >>> @@ -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 */
> >>>
> >>> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> >>> +++ linux-x86/arch/x86/platform/efi/efi.c
> >>> @@ -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;
> >>> --- linux-x86.orig/init/main.c
> >>> +++ linux-x86/init/main.c
> >>> @@ -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();
> >>>         }
> >>>

Thanks
Dave

[PATCH V2 2/4] efi/x86: move efi_print_memmap to drivers/firmware/efi/memmap.c

[Dave Young]

[-- Attachment #1: efi-add-print-memmap.patch --]
[-- Type: text/plain, Size: 2541 bytes --]

Signed-off-by: Dave Young <dyoung@redhat.com>
---
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(-)

--- linux-x86.orig/arch/x86/platform/efi/efi.c
+++ linux-x86/arch/x86/platform/efi/efi.c
@@ -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)) {
--- linux-x86.orig/drivers/firmware/efi/memmap.c
+++ linux-x86/drivers/firmware/efi/memmap.c
@@ -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
--- linux-x86.orig/arch/x86/include/asm/efi.h
+++ linux-x86/arch/x86/include/asm/efi.h
@@ -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);
--- linux-x86.orig/include/linux/efi.h
+++ linux-x86/include/linux/efi.h
@@ -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);

Re: [PATCH V2 2/4] efi/x86: move efi_print_memmap to drivers/firmware/efi/memmap.c

[Nicolai Stange]

Reviewed-and-tested-by: Nicolai Stange <nicstange@gmail.com>

Re: [PATCH V2 2/4] efi/x86: move efi_print_memmap to drivers/firmware/efi/memmap.c

[Ard Biesheuvel]

On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> Signed-off-by: Dave Young <dyoung@redhat.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
> 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(-)
>
> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> +++ linux-x86/arch/x86/platform/efi/efi.c
> @@ -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)) {
> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -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
> --- linux-x86.orig/arch/x86/include/asm/efi.h
> +++ linux-x86/arch/x86/include/asm/efi.h
> @@ -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);
> --- linux-x86.orig/include/linux/efi.h
> +++ linux-x86/include/linux/efi.h
> @@ -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);
>
>

Re: [PATCH V2 2/4] efi/x86: move efi_print_memmap to drivers/firmware/efi/memmap.c

[Ard Biesheuvel]

On 16 January 2017 at 02:45, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> ---
> 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(-)
>
> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> +++ linux-x86/arch/x86/platform/efi/efi.c
> @@ -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)) {
> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -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
> --- linux-x86.orig/arch/x86/include/asm/efi.h
> +++ linux-x86/arch/x86/include/asm/efi.h
> @@ -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);
> --- linux-x86.orig/include/linux/efi.h
> +++ linux-x86/include/linux/efi.h
> @@ -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);
>
>

[PATCH V2 3/4] efi/x86: add debug code to print cooked memmap

[Dave Young]

[-- Attachment #1: efi-print-memmap-after-merge-ranges.patch --]
[-- Type: text/plain, Size: 636 bytes --]

It is not obvious if the reserved boot area are added correctly, add a
efi_print_memmap to print the new memmap.

Signed-off-by: Dave Young <dyoung@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/platform/efi/efi.c |    5 +++++
 1 file changed, 5 insertions(+)

--- linux-x86.orig/arch/x86/platform/efi/efi.c
+++ linux-x86/arch/x86/platform/efi/efi.c
@@ -943,6 +943,11 @@ static void __init __efi_enter_virtual_m
 		return;
 	}
 
+	if (efi_enabled(EFI_DBG)) {
+		pr_info("EFI runtime memory map:\n");
+		efi_print_memmap();
+	}
+
 	BUG_ON(!efi.systab);
 
 	if (efi_setup_page_tables(pa, 1 << pg_shift)) {

[PATCH V2 3/4] efi/x86: add debug code to print cooked memmap

[Dave Young]

[-- Attachment #1: efi-print-memmap-after-merge-ranges.patch --]
[-- Type: text/plain, Size: 694 bytes --]

It is not obvious if the reserved boot area are added correctly, add a
efi_print_memmap to print the new memmap.

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/x86/platform/efi/efi.c |    5 +++++
 1 file changed, 5 insertions(+)

--- linux-x86.orig/arch/x86/platform/efi/efi.c
+++ linux-x86/arch/x86/platform/efi/efi.c
@@ -943,6 +943,11 @@ static void __init __efi_enter_virtual_m
 		return;
 	}
 
+	if (efi_enabled(EFI_DBG)) {
+		pr_info("EFI runtime memory map:\n");
+		efi_print_memmap();
+	}
+
 	BUG_ON(!efi.systab);
 
 	if (efi_setup_page_tables(pa, 1 << pg_shift)) {

Re: [PATCH V2 3/4] efi/x86: add debug code to print cooked memmap

[Nicolai Stange]

Reviewed-and-tested-by: Nicolai Stange <nicstange@gmail.com>

[PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

[-- Attachment #1: efi-memmap-insert-only-handle-boot-ranges.patch --]
[-- Type: text/plain, Size: 2671 bytes --]

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 <dyoung@redhat.com>
---
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(-)

--- linux-x86.orig/drivers/firmware/efi/memmap.c
+++ linux-x86/drivers/firmware/efi/memmap.c
@@ -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;
 
--- linux-x86.orig/arch/x86/platform/efi/quirks.c
+++ linux-x86/arch/x86/platform/efi/quirks.c
@@ -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);
--- linux-x86.orig/drivers/firmware/efi/fake_mem.c
+++ linux-x86/drivers/firmware/efi/fake_mem.c
@@ -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);
--- linux-x86.orig/include/linux/efi.h
+++ linux-x86/include/linux/efi.h
@@ -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);

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Nicolai Stange]

Reviewed-and-tested-by: Nicolai Stange <nicstange@gmail.com>

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Nicolai Stange]

Reviewed-and-tested-by: Nicolai Stange <nicstange-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Ard Biesheuvel]

On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> 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.

> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
> 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(-)
>
> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -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;
>
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -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);
> --- linux-x86.orig/drivers/firmware/efi/fake_mem.c
> +++ linux-x86/drivers/firmware/efi/fake_mem.c
> @@ -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);
> --- linux-x86.orig/include/linux/efi.h
> +++ linux-x86/include/linux/efi.h
> @@ -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);
>
>

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Nicolai Stange]

On Tue, Jan 17 2017, Ard Biesheuvel wrote:

> On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
>> 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

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Nicolai Stange]

On Tue, Jan 17 2017, Ard Biesheuvel wrote:

> On 16 January 2017 at 02:45, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> 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

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/17/17 at 08:48pm, Nicolai Stange wrote:
> On Tue, Jan 17 2017, Ard Biesheuvel wrote:
> 
> > On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> >> 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.

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(-)

--- linux-x86.orig/arch/x86/platform/efi/quirks.c
+++ linux-x86/arch/x86/platform/efi/quirks.c
@@ -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);
+	new_phys = efi_memmap_alloc(num_entries);
 	if (!new_phys) {
 		pr_err("Failed to allocate new EFI memmap\n");
 		return;

> 
> 
> Thanks,
> 
> Nicolai

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/18/17 at 07:01pm, Dave Young wrote:
> On 01/17/17 at 08:48pm, Nicolai Stange wrote:
> > On Tue, Jan 17 2017, Ard Biesheuvel wrote:
> > 
> > > On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> > >> 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.
> 
> 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(-)
> 
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -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.

> +	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.

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

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/18/17 at 07:01pm, Dave Young wrote:
> On 01/17/17 at 08:48pm, Nicolai Stange wrote:
> > On Tue, Jan 17 2017, Ard Biesheuvel wrote:
> > 
> > > On 16 January 2017 at 02:45, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > >> 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.
> 
> 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(-)
> 
> --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> +++ linux-x86/arch/x86/platform/efi/quirks.c
> @@ -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.

> +	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.

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

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/18/17 at 07:06pm, Dave Young wrote:
> On 01/18/17 at 07:01pm, Dave Young wrote:
> > On 01/17/17 at 08:48pm, Nicolai Stange wrote:
> > > On Tue, Jan 17 2017, Ard Biesheuvel wrote:
> > > 
> > > > On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> > > >> 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.
> > 
> > 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.

Please ignore the comment, your patch already addressed this, that
means it is still necessary after bgrt being moved early because
efi_free_boot_services still need it.

Apologize for the noise..
> > 
> > ---
> >  arch/x86/platform/efi/quirks.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> > +++ linux-x86/arch/x86/platform/efi/quirks.c
> > @@ -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.
> 
> > +	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.
> 
> >  	if (!new_phys) {
> >  		pr_err("Failed to allocate new EFI memmap\n");
> >  		return;
> > 
> > > 
> > > 
> > > Thanks,
> > > 
> > > Nicolai

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/17/17 at 05:13pm, Ard Biesheuvel wrote:
> On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> > 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.

efi_mem_reserve is designed to address the boot service memory reservation
issue but I'm not sure if we could have other requirement in the future,
then the efi_mem_reserve itself at least the function comment need an
update also. Anyway I have no strong opinion about this patch..

> 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > ---
> > 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(-)
> >
> > --- linux-x86.orig/drivers/firmware/efi/memmap.c
> > +++ linux-x86/drivers/firmware/efi/memmap.c
> > @@ -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;
> >
> > --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> > +++ linux-x86/arch/x86/platform/efi/quirks.c
> > @@ -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);
> > --- linux-x86.orig/drivers/firmware/efi/fake_mem.c
> > +++ linux-x86/drivers/firmware/efi/fake_mem.c
> > @@ -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);
> > --- linux-x86.orig/include/linux/efi.h
> > +++ linux-x86/include/linux/efi.h
> > @@ -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);
> >
> >

Thanks
Dave

Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/17/17 at 05:13pm, Ard Biesheuvel wrote:
> On 16 January 2017 at 02:45, Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > 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.

efi_mem_reserve is designed to address the boot service memory reservation
issue but I'm not sure if we could have other requirement in the future,
then the efi_mem_reserve itself at least the function comment need an
update also. Anyway I have no strong opinion about this patch..

> 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > 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(-)
> >
> > --- linux-x86.orig/drivers/firmware/efi/memmap.c
> > +++ linux-x86/drivers/firmware/efi/memmap.c
> > @@ -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;
> >
> > --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> > +++ linux-x86/arch/x86/platform/efi/quirks.c
> > @@ -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);
> > --- linux-x86.orig/drivers/firmware/efi/fake_mem.c
> > +++ linux-x86/drivers/firmware/efi/fake_mem.c
> > @@ -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);
> > --- linux-x86.orig/include/linux/efi.h
> > +++ linux-x86/include/linux/efi.h
> > @@ -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);
> >
> >

Thanks
Dave

Re: [PATCH V2 0/4] efi/x86: move efi bgrt init code to early init

[Ard Biesheuvel]

On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> Hi,
>
> Here the the update of the series for moving bgrt init code to early init.
>
> Main changes is:
> - Move the 1st patch to the last because it does not block the 2nd patch
> any more with Peter's patch to prune invlid memmap entries:
> https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91
> a35445229
> But it is still tood to have since efi_mem_reserve only cares about boot related
> mem ranges.
>
> - Other comments about code itself, details please the the patches themselves.
>
>  arch/x86/include/asm/efi.h       |    1
>  arch/x86/kernel/acpi/boot.c      |   12 +++++++
>  arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
>  arch/x86/platform/efi/efi.c      |   26 +++--------------
>  arch/x86/platform/efi/quirks.c   |    2 -
>  drivers/acpi/bgrt.c              |   28 +++++++++++++-----
>  drivers/firmware/efi/fake_mem.c  |    3 +
>  drivers/firmware/efi/memmap.c    |   22 +++++++++++++-
>  include/linux/efi-bgrt.h         |    7 +---
>  include/linux/efi.h              |    5 +--
>  init/main.c                      |    1
>  11 files changed, 92 insertions(+), 74 deletions(-)
>

Dave,

I have pushed these to efi/next: please double check if I did it
correctly. I had some trouble applying these given that I rebased
efi/next onto -rc4. However, the fact that you are not using standard
git cover letters and emails doesn't help things, so could you
*please* use standard git send-email to post to linux-efi in the
future? Thanks.

Ard.

Re: [PATCH V2 0/4] efi/x86: move efi bgrt init code to early init

[Dave Young]

On 01/27/17 at 05:03pm, Ard Biesheuvel wrote:
> On 16 January 2017 at 02:45, Dave Young <dyoung@redhat.com> wrote:
> > Hi,
> >
> > Here the the update of the series for moving bgrt init code to early init.
> >
> > Main changes is:
> > - Move the 1st patch to the last because it does not block the 2nd patch
> > any more with Peter's patch to prune invlid memmap entries:
> > https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91
> > a35445229
> > But it is still tood to have since efi_mem_reserve only cares about boot related
> > mem ranges.
> >
> > - Other comments about code itself, details please the the patches themselves.
> >
> >  arch/x86/include/asm/efi.h       |    1
> >  arch/x86/kernel/acpi/boot.c      |   12 +++++++
> >  arch/x86/platform/efi/efi-bgrt.c |   59 ++++++++++++++++-----------------------
> >  arch/x86/platform/efi/efi.c      |   26 +++--------------
> >  arch/x86/platform/efi/quirks.c   |    2 -
> >  drivers/acpi/bgrt.c              |   28 +++++++++++++-----
> >  drivers/firmware/efi/fake_mem.c  |    3 +
> >  drivers/firmware/efi/memmap.c    |   22 +++++++++++++-
> >  include/linux/efi-bgrt.h         |    7 +---
> >  include/linux/efi.h              |    5 +--
> >  init/main.c                      |    1
> >  11 files changed, 92 insertions(+), 74 deletions(-)
> >
> 
> Dave,
> 
> I have pushed these to efi/next: please double check if I did it
> correctly. I had some trouble applying these given that I rebased
> efi/next onto -rc4. However, the fact that you are not using standard
> git cover letters and emails doesn't help things, so could you
> *please* use standard git send-email to post to linux-efi in the
> future? Thanks.

Ard, apologize for late reply due to a one week holiday. I double-checked
the efi-next commits, I think they are correct. As for the email format
I use quilt to send the series so that the cover letter is not git
formatted. The patches are based on mainline linus tree, maybe this is
the reason of the trouble I will check if it is efi/next mergeble with
"git am" before sending out next time and will switch to git-send-email
if it does not work.

Thanks
Dave