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

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

[Dave Young]

Hi,

Here is a patchset to move efi_bgrt_init to early code so that we can still use memblock api.

Appreciated for comments and review.

Diffstat:

 arch/x86/kernel/acpi/boot.c      |   12 +++++++++++
 arch/x86/platform/efi/efi-bgrt.c |   42
+++++++++++++--------------------------
 arch/x86/platform/efi/efi.c      |   26 ++++--------------------
 arch/x86/platform/efi/quirks.c   |    2 -
 drivers/acpi/bgrt.c              |   21 +++++++++++++------
 drivers/firmware/efi/fake_mem.c  |    3 +-
 drivers/firmware/efi/memmap.c    |   24 +++++++++++++++++++++-
 include/linux/efi-bgrt.h         |    7 ++----
 include/linux/efi.h              |    4 +--
 init/main.c                      |    1 
 10 files changed, 78 insertions(+), 64 deletions(-)

Thanks
Dave

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

[Dave Young]

[-- Attachment #1: efi-memmap-insert-fix.patch --]
[-- Type: text/plain, Size: 3954 bytes --]

There are memory ranges like below when I testing early efi_mem_reserve:

efi: mem62: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem63: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem64: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem65: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem66: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
efi: mem67: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)

So efi_memmap_insert will run into inserting same region multiple times,
also because efi_memmap_insert does not consider the duplicate ranges it will
cause memmap buffer overflow due to the size is pre-calculated, and kernel boot
fail with a panic.  We did not detect such issue because current users of
efi_mem_insert do it very late after switching to virtual mode, at that time
the new cooked efi.memmap contains only runtime needed memory ranges.

efi_mem_reserve cares only about boot services regions and maybe loader areas.
So add a new argument to efi_memmap_insert for this purpose.
 
Later patches depend on this one for moving bgrt reservation to early code.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 arch/x86/platform/efi/quirks.c  |    2 +-
 drivers/firmware/efi/fake_mem.c |    3 ++-
 drivers/firmware/efi/memmap.c   |    8 +++++++-
 include/linux/efi.h             |    4 ++--
 4 files changed, 12 insertions(+), 5 deletions(-)

--- linux-x86.orig/drivers/firmware/efi/memmap.c
+++ linux-x86/drivers/firmware/efi/memmap.c
@@ -213,7 +213,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;
@@ -246,6 +246,12 @@ 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_LOADER_DATA ||
+		    md->type == EFI_LOADER_CODE ||
+		    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
@@ -957,8 +957,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);

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

[Dave Young]

[-- Attachment #1: efi-bgrt-fix.patch --]
[-- Type: text/plain, Size: 7755 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>
---
 arch/x86/kernel/acpi/boot.c      |   12 +++++++++++
 arch/x86/platform/efi/efi-bgrt.c |   42 +++++++++++++--------------------------
 arch/x86/platform/efi/efi.c      |    5 ----
 drivers/acpi/bgrt.c              |   21 +++++++++++++------
 include/linux/efi-bgrt.h         |    7 ++----
 init/main.c                      |    1 
 6 files changed, 45 insertions(+), 43 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,53 +27,49 @@ 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;
 
 	if (acpi_disabled)
 		return;
 
-	status = acpi_get_table("BGRT", 0,
-	                        (struct acpi_table_header **)&bgrt_tab);
-	if (ACPI_FAILURE(status))
-		return;
+	bgrt_tab = *(struct acpi_table_bgrt *)table;
 
-	if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
+	if (bgrt_tab.header.length < sizeof(bgrt_tab)) {
 		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-		       bgrt_tab->header.length, sizeof(*bgrt_tab));
+		       bgrt_tab.header.length, sizeof(bgrt_tab));
 		return;
 	}
-	if (bgrt_tab->version != 1) {
+	if (bgrt_tab.version != 1) {
 		pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
-		       bgrt_tab->version);
+		       bgrt_tab.version);
 		return;
 	}
-	if (bgrt_tab->status & 0xfe) {
+	if (bgrt_tab.status & 0xfe) {
 		pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
-		       bgrt_tab->status);
+		       bgrt_tab.status);
 		return;
 	}
-	if (bgrt_tab->image_type != 0) {
+	if (bgrt_tab.image_type != 0) {
 		pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
-		       bgrt_tab->image_type);
+		       bgrt_tab.image_type);
 		return;
 	}
-	if (!bgrt_tab->image_address) {
+	if (!bgrt_tab.image_address) {
 		pr_notice("Ignoring BGRT: null image address\n");
 		return;
 	}
 
-	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
+	image = early_memremap(bgrt_tab.image_address, sizeof(bmp_header));
 	if (!image) {
 		pr_notice("Ignoring BGRT: failed to map image header memory\n");
 		return;
 	}
 
 	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);
@@ -82,12 +77,5 @@ void __init efi_bgrt_init(void)
 	}
 	bgrt_image_size = bmp_header.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);
+	efi_mem_reserve(bgrt_tab.image_address, bgrt_image_size);
 }
--- 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,9 +85,17 @@ 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");
+		bgrt_image = NULL;
+		return -ENOMEM;
+	}
+
 	bin_attr_image.private = bgrt_image;
 	bin_attr_image.size = bgrt_image_size;
 
--- 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
@@ -476,11 +476,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 3/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: 1505 bytes --]

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 arch/x86/platform/efi/efi.c   |   16 ----------------
 drivers/firmware/efi/memmap.c |   16 ++++++++++++++++
 2 files changed, 16 insertions(+), 16 deletions(-)

--- linux-x86.orig/arch/x86/platform/efi/efi.c
+++ linux-x86/arch/x86/platform/efi/efi.c
@@ -210,22 +210,6 @@ int __init efi_memblock_x86_reserve_rang
 	return 0;
 }
 
-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

[PATCH 4/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: 583 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>
---
 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
@@ -873,6 +873,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 2/4] efi/x86: move efi bgrt init code to early init code

[Dave Young]

[snip]
> --- linux-x86.orig/drivers/acpi/bgrt.c
> +++ linux-x86/drivers/acpi/bgrt.c

[snip]
>  
> @@ -84,9 +85,17 @@ 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");
> +		bgrt_image = NULL;
> +		return -ENOMEM;
> +	}
> +

Oops, later error path need unmap bgrt_image, will update in next
version after collecting more comments.

Also bgrt_image = NULL is useless, will drop it.

>  	bin_attr_image.private = bgrt_image;
>  	bin_attr_image.size = bgrt_image_size;
>  

Thanks
Dave

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

[Nicolai Stange]

Hi Dave,

On Thu, Jan 12 2017, Dave Young wrote:

> efi_mem_reserve cares only about boot services regions and maybe loader areas.
> So add a new argument to efi_memmap_insert for this purpose.

Please see below.


> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -213,7 +213,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;
> @@ -246,6 +246,12 @@ 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_LOADER_DATA ||
> +		    md->type == EFI_LOADER_CODE ||
> +		    md->type == EFI_BOOT_SERVICES_CODE ||
> +		    md->type == EFI_BOOT_SERVICES_DATA))
> +			continue;
> +


Actually, the efi_mem_desc_lookup() called from
efi_arch_memmap_reserve() will only return mds not satisfying the
following condition:

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

Furthermore, efi_arch_mem_reserve() will only accept ranges fully
contained within such a region.

I think we can make efi_arch_mem_reserve() return early if
EFI_MEMORY_RUNTIME has been set already and thus, neglect this case in
efi_memmap_insert().

I suppose that we don't want to reserve within EFI_RUNTIME_SERVICES_DATA
regions in efi_mem_reserve() either -- these won't ever get made
available as general memory anyway [1]. So efi_arch_mem_reserve() should
return early here as well imo.

So, what would remain to be handled from efi_memmap_insert() in case of
boot_only would be EFI_BOOT_SERVICES_DATA only?

(As a sidenote, Matt pointed out at [1] that the EFI_LOADER_* regions
 should be reserved early through memblock_reserve() and not through
 efi_mem_reserve()).

Thanks,

Nicolai


[1] http://lkml.kernel.org/r/20170109130702.GI16838@codeblueprint.co.uk

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

[Nicolai Stange]

On Thu, Jan 12 2017, Dave Young wrote:

> -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;
>  
>  	if (acpi_disabled)
>  		return;
>  
> -	status = acpi_get_table("BGRT", 0,
> -	                        (struct acpi_table_header **)&bgrt_tab);
> -	if (ACPI_FAILURE(status))
> -		return;


Not sure, but wouldn't it be safer to reverse the order of this assignment

> +	bgrt_tab = *(struct acpi_table_bgrt *)table;

and this length check

> -	if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
> +	if (bgrt_tab.header.length < sizeof(bgrt_tab)) {
>  		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> -		       bgrt_tab->header.length, sizeof(*bgrt_tab));
> +		       bgrt_tab.header.length, sizeof(bgrt_tab));
>  		return;
>  	}

?

Also, from here on, all error paths should zero out
bgrt_tab.image_address (or so) to signal failure to bgrt_init():
bgrt_init() now checks for !bgrt_tab.image_address whereas before it had
tested bgrt_image and the latter used to be set at the very end of
efi_bgrt_init().


> -	if (bgrt_tab->version != 1) {
> +	if (bgrt_tab.version != 1) {
>  		pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
> -		       bgrt_tab->version);
> +		       bgrt_tab.version);
>  		return;
>  	}
> -	if (bgrt_tab->status & 0xfe) {
> +	if (bgrt_tab.status & 0xfe) {
>  		pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
> -		       bgrt_tab->status);
> +		       bgrt_tab.status);
>  		return;
>  	}
> -	if (bgrt_tab->image_type != 0) {
> +	if (bgrt_tab.image_type != 0) {
>  		pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
> -		       bgrt_tab->image_type);
> +		       bgrt_tab.image_type);
>  		return;
>  	}
> -	if (!bgrt_tab->image_address) {
> +	if (!bgrt_tab.image_address) {
>  		pr_notice("Ignoring BGRT: null image address\n");
>  		return;
>  	}
>  
> -	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
> +	image = early_memremap(bgrt_tab.image_address, sizeof(bmp_header));
>  	if (!image) {
>  		pr_notice("Ignoring BGRT: failed to map image header memory\n");
>  		return;
>  	}
>  
>  	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);
> @@ -82,12 +77,5 @@ void __init efi_bgrt_init(void)
>  	}
>  	bgrt_image_size = bmp_header.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);
> +	efi_mem_reserve(bgrt_tab.image_address, bgrt_image_size);
>  }
> --- 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;

[...]

> @@ -84,9 +85,17 @@ 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");
> +		bgrt_image = NULL;
> +		return -ENOMEM;
> +	}
> +
>  	bin_attr_image.private = bgrt_image;
>  	bin_attr_image.size = bgrt_image_size;
>  

Thanks,

Nicolai

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

[Nicolai Stange]

On Thu, Jan 12 2017, Dave Young wrote:

> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  arch/x86/platform/efi/efi.c   |   16 ----------------
>  drivers/firmware/efi/memmap.c |   16 ++++++++++++++++
>  2 files changed, 16 insertions(+), 16 deletions(-)
>
> --- linux-x86.orig/arch/x86/platform/efi/efi.c
> +++ linux-x86/arch/x86/platform/efi/efi.c
> @@ -210,22 +210,6 @@ int __init efi_memblock_x86_reserve_rang
>  	return 0;
>  }
>  
> -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

Shouldn't the declaration in arch/x86/include/asm/efi.h get moved as well?

Thanks,

Nicolai

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

[Ard Biesheuvel]

Hello Dave,

On 12 January 2017 at 09:41, Dave Young <dyoung@redhat.com> wrote:
> There are memory ranges like below when I testing early efi_mem_reserve:
>
> efi: mem62: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem63: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem64: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem65: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem66: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> efi: mem67: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
>

Did you spot Peter's patch to prune invalid memmap entries?

https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91a35445229

I would expect this patch to no longer be necessary with that in place, no?


> So efi_memmap_insert will run into inserting same region multiple times,
> also because efi_memmap_insert does not consider the duplicate ranges it will
> cause memmap buffer overflow due to the size is pre-calculated, and kernel boot
> fail with a panic.  We did not detect such issue because current users of
> efi_mem_insert do it very late after switching to virtual mode, at that time
> the new cooked efi.memmap contains only runtime needed memory ranges.
>
> efi_mem_reserve cares only about boot services regions and maybe loader areas.
> So add a new argument to efi_memmap_insert for this purpose.
>
> Later patches depend on this one for moving bgrt reservation to early code.
>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  arch/x86/platform/efi/quirks.c  |    2 +-
>  drivers/firmware/efi/fake_mem.c |    3 ++-
>  drivers/firmware/efi/memmap.c   |    8 +++++++-
>  include/linux/efi.h             |    4 ++--
>  4 files changed, 12 insertions(+), 5 deletions(-)
>
> --- linux-x86.orig/drivers/firmware/efi/memmap.c
> +++ linux-x86/drivers/firmware/efi/memmap.c
> @@ -213,7 +213,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;
> @@ -246,6 +246,12 @@ 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_LOADER_DATA ||
> +                   md->type == EFI_LOADER_CODE ||
> +                   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
> @@ -957,8 +957,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 4/4] efi/x86: add debug code to print cooked memmap

[Ard Biesheuvel]

On 12 January 2017 at 09:41, Dave Young <dyoung@redhat.com> wrote:
> 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>

This sounds useful regardless of the other patches in this series:

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

Note that it is appropriate to keep this x86 specific, since we never
modify the memory map on ARM (except for the sorting that occurs in
the stub)

> ---
>  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
> @@ -873,6 +873,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 2/4] efi/x86: move efi bgrt init code to early init code

[Ard Biesheuvel]

On 12 January 2017 at 09:41, 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>

I know this is probably out of scope for you, but since we're moving
things around, any chance we could do so in a manner that will enable
BGRT support for arm64/ACPI? Happy to test/collaborate on this.


> ---
>  arch/x86/kernel/acpi/boot.c      |   12 +++++++++++
>  arch/x86/platform/efi/efi-bgrt.c |   42 +++++++++++++--------------------------
>  arch/x86/platform/efi/efi.c      |    5 ----
>  drivers/acpi/bgrt.c              |   21 +++++++++++++------
>  include/linux/efi-bgrt.h         |    7 ++----
>  init/main.c                      |    1
>  6 files changed, 45 insertions(+), 43 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,53 +27,49 @@ 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;
>
>         if (acpi_disabled)
>                 return;
>
> -       status = acpi_get_table("BGRT", 0,
> -                               (struct acpi_table_header **)&bgrt_tab);
> -       if (ACPI_FAILURE(status))
> -               return;
> +       bgrt_tab = *(struct acpi_table_bgrt *)table;
>
> -       if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
> +       if (bgrt_tab.header.length < sizeof(bgrt_tab)) {
>                 pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> -                      bgrt_tab->header.length, sizeof(*bgrt_tab));
> +                      bgrt_tab.header.length, sizeof(bgrt_tab));
>                 return;
>         }
> -       if (bgrt_tab->version != 1) {
> +       if (bgrt_tab.version != 1) {
>                 pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
> -                      bgrt_tab->version);
> +                      bgrt_tab.version);
>                 return;
>         }
> -       if (bgrt_tab->status & 0xfe) {
> +       if (bgrt_tab.status & 0xfe) {
>                 pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
> -                      bgrt_tab->status);
> +                      bgrt_tab.status);
>                 return;
>         }
> -       if (bgrt_tab->image_type != 0) {
> +       if (bgrt_tab.image_type != 0) {
>                 pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
> -                      bgrt_tab->image_type);
> +                      bgrt_tab.image_type);
>                 return;
>         }
> -       if (!bgrt_tab->image_address) {
> +       if (!bgrt_tab.image_address) {
>                 pr_notice("Ignoring BGRT: null image address\n");
>                 return;
>         }
>
> -       image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
> +       image = early_memremap(bgrt_tab.image_address, sizeof(bmp_header));
>         if (!image) {
>                 pr_notice("Ignoring BGRT: failed to map image header memory\n");
>                 return;
>         }
>
>         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);
> @@ -82,12 +77,5 @@ void __init efi_bgrt_init(void)
>         }
>         bgrt_image_size = bmp_header.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);
> +       efi_mem_reserve(bgrt_tab.image_address, bgrt_image_size);
>  }
> --- 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,9 +85,17 @@ 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");
> +               bgrt_image = NULL;
> +               return -ENOMEM;
> +       }
> +
>         bin_attr_image.private = bgrt_image;
>         bin_attr_image.size = bgrt_image_size;
>
> --- 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
> @@ -476,11 +476,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 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/12/17 at 04:15pm, Ard Biesheuvel wrote:
> Hello Dave,
> 
> On 12 January 2017 at 09:41, Dave Young <dyoung@redhat.com> wrote:
> > There are memory ranges like below when I testing early efi_mem_reserve:
> >
> > efi: mem62: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > efi: mem63: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > efi: mem64: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > efi: mem65: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > efi: mem66: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > efi: mem67: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> >
> 
> Did you spot Peter's patch to prune invalid memmap entries?
> 
> https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91a35445229
> 
> I would expect this patch to no longer be necessary with that in place, no?

Ard, good suggestion, I did not notice that patch, will try. Actually
I'm not sure about fake_mem handling, if we can filter out the invalid
ranges then it will be natural to drop this one after a test.

Thanks
Dave

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

[Dave Young]

On 01/12/17 at 12:15pm, Nicolai Stange wrote:
> Hi Dave,
> 
> On Thu, Jan 12 2017, Dave Young wrote:
> 
> > efi_mem_reserve cares only about boot services regions and maybe loader areas.
> > So add a new argument to efi_memmap_insert for this purpose.
> 
> Please see below.
> 
> 
> > --- linux-x86.orig/drivers/firmware/efi/memmap.c
> > +++ linux-x86/drivers/firmware/efi/memmap.c
> > @@ -213,7 +213,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;
> > @@ -246,6 +246,12 @@ 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_LOADER_DATA ||
> > +		    md->type == EFI_LOADER_CODE ||
> > +		    md->type == EFI_BOOT_SERVICES_CODE ||
> > +		    md->type == EFI_BOOT_SERVICES_DATA))
> > +			continue;
> > +
> 
> 
> Actually, the efi_mem_desc_lookup() called from
> efi_arch_memmap_reserve() will only return mds not satisfying the
> following condition:
> 
>   if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
>       md->type != EFI_BOOT_SERVICES_DATA &&
>       md->type != EFI_RUNTIME_SERVICES_DATA) {
>   	continue;
>   }
> 
> Furthermore, efi_arch_mem_reserve() will only accept ranges fully
> contained within such a region.
> 
> I think we can make efi_arch_mem_reserve() return early if
> EFI_MEMORY_RUNTIME has been set already and thus, neglect this case in
> efi_memmap_insert().
> 
> I suppose that we don't want to reserve within EFI_RUNTIME_SERVICES_DATA
> regions in efi_mem_reserve() either -- these won't ever get made
> available as general memory anyway [1]. So efi_arch_mem_reserve() should
> return early here as well imo.
> 
> So, what would remain to be handled from efi_memmap_insert() in case of
> boot_only would be EFI_BOOT_SERVICES_DATA only?

It sounds reasonable though I'm still not sure about EFI_LOADER*.

The main purpose of this patch is to address the invalid mem ranges
case. As Ard mentioned I will test with Peter's patch first, if it works
fine I would like to either drop this patch as a future improvement or add
it at the end of the next post.

Matt, what's your opinion about the boot_only check and the EFI_LOADERS*
question?

> 
> (As a sidenote, Matt pointed out at [1] that the EFI_LOADER_* regions
>  should be reserved early through memblock_reserve() and not through
>  efi_mem_reserve()).
> 
> Thanks,
> 
> Nicolai
> 
> 
> [1] http://lkml.kernel.org/r/20170109130702.GI16838@codeblueprint.co.uk

Thanks
Dave

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

[Dave Young]

On 01/12/17 at 04:20pm, Ard Biesheuvel wrote:
> On 12 January 2017 at 09:41, 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>
> 
> I know this is probably out of scope for you, but since we're moving
> things around, any chance we could do so in a manner that will enable
> BGRT support for arm64/ACPI? Happy to test/collaborate on this.
> 

I'm happy to do so, Bhupesh Sharma <bhsharma@redhat.com> said he had
some investigation on that already, I would like to ask him to help on that.

Already cced him..

Thanks
Dave

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

[Dave Young]

On 01/12/17 at 12:54pm, Nicolai Stange wrote:
> On Thu, Jan 12 2017, Dave Young wrote:
> 
> > -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;
> >  
> >  	if (acpi_disabled)
> >  		return;
> >  
> > -	status = acpi_get_table("BGRT", 0,
> > -	                        (struct acpi_table_header **)&bgrt_tab);
> > -	if (ACPI_FAILURE(status))
> > -		return;
> 
> 
> Not sure, but wouldn't it be safer to reverse the order of this assignment
> 
> > +	bgrt_tab = *(struct acpi_table_bgrt *)table;

Nicolai, sorry, I'm not sure I understand the comment, is it about above line?
Could you elaborate a bit?

> 
> and this length check
> 

I also do not get this :(

> > -	if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
> > +	if (bgrt_tab.header.length < sizeof(bgrt_tab)) {
> >  		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> > -		       bgrt_tab->header.length, sizeof(*bgrt_tab));
> > +		       bgrt_tab.header.length, sizeof(bgrt_tab));
> >  		return;
> >  	}
> 
> ?
> 
> Also, from here on, all error paths should zero out
> bgrt_tab.image_address (or so) to signal failure to bgrt_init():
> bgrt_init() now checks for !bgrt_tab.image_address whereas before it had
> tested bgrt_image and the latter used to be set at the very end of
> efi_bgrt_init().
> 

Will do, thanks!

> 
> > -	if (bgrt_tab->version != 1) {
> > +	if (bgrt_tab.version != 1) {
> >  		pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
> > -		       bgrt_tab->version);
> > +		       bgrt_tab.version);
> >  		return;
> >  	}
> > -	if (bgrt_tab->status & 0xfe) {
> > +	if (bgrt_tab.status & 0xfe) {
> >  		pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
> > -		       bgrt_tab->status);
> > +		       bgrt_tab.status);
> >  		return;
> >  	}
> > -	if (bgrt_tab->image_type != 0) {
> > +	if (bgrt_tab.image_type != 0) {
> >  		pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n",
> > -		       bgrt_tab->image_type);
> > +		       bgrt_tab.image_type);
> >  		return;
> >  	}
> > -	if (!bgrt_tab->image_address) {
> > +	if (!bgrt_tab.image_address) {
> >  		pr_notice("Ignoring BGRT: null image address\n");
> >  		return;
> >  	}
> >  
> > -	image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB);
> > +	image = early_memremap(bgrt_tab.image_address, sizeof(bmp_header));
> >  	if (!image) {
> >  		pr_notice("Ignoring BGRT: failed to map image header memory\n");
> >  		return;
> >  	}
> >  
> >  	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);
> > @@ -82,12 +77,5 @@ void __init efi_bgrt_init(void)
> >  	}
> >  	bgrt_image_size = bmp_header.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);
> > +	efi_mem_reserve(bgrt_tab.image_address, bgrt_image_size);
> >  }
> > --- 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;
> 
> [...]
> 
> > @@ -84,9 +85,17 @@ 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");
> > +		bgrt_image = NULL;
> > +		return -ENOMEM;
> > +	}
> > +
> >  	bin_attr_image.private = bgrt_image;
> >  	bin_attr_image.size = bgrt_image_size;
> >  
> 
> Thanks,
> 
> Nicolai

Thanks
Dave

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

[Dave Young]

On 01/12/17 at 01:08pm, Nicolai Stange wrote:
> On Thu, Jan 12 2017, Dave Young wrote:
> 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > ---
> >  arch/x86/platform/efi/efi.c   |   16 ----------------
> >  drivers/firmware/efi/memmap.c |   16 ++++++++++++++++
> >  2 files changed, 16 insertions(+), 16 deletions(-)
> >
> > --- linux-x86.orig/arch/x86/platform/efi/efi.c
> > +++ linux-x86/arch/x86/platform/efi/efi.c
> > @@ -210,22 +210,6 @@ int __init efi_memblock_x86_reserve_rang
> >  	return 0;
> >  }
> >  
> > -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
> 
> Shouldn't the declaration in arch/x86/include/asm/efi.h get moved as well?

Good catch, will change it as well

Thanks
Dave

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

[Nicolai Stange]

On Fri, Jan 13 2017, Dave Young wrote:

> On 01/12/17 at 12:54pm, Nicolai Stange wrote:
>> On Thu, Jan 12 2017, Dave Young wrote:
>> 
>> > -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;
>> >  
>> >  	if (acpi_disabled)
>> >  		return;
>> >  
>> > -	status = acpi_get_table("BGRT", 0,
>> > -	                        (struct acpi_table_header **)&bgrt_tab);
>> > -	if (ACPI_FAILURE(status))
>> > -		return;
>> 
>> 
>> Not sure, but wouldn't it be safer to reverse the order of this assignment
>> 
>> > +	bgrt_tab = *(struct acpi_table_bgrt *)table;
>
> Nicolai, sorry, I'm not sure I understand the comment, is it about above line?
> Could you elaborate a bit?
>
>> 
>> and this length check
>> 
>
> I also do not get this :(

Ah sorry, my point is this: the length check should perhaps be made
before doing the assignment to bgrt_tab because otherwise, we might end
up reading from invalid memory.

I.e. if (struct acpi_table_bgrt *)table->length < sizeof(bgrt_tab), then

  bgrt_tab = *(struct acpi_table_bgrt *)table;

would read past the table's end.

I'm not sure whether this is a real problem though -- that is, whether
this read could ever hit some unmapped memory.


>> > -	if (bgrt_tab->header.length < sizeof(*bgrt_tab)) {
>> > +	if (bgrt_tab.header.length < sizeof(bgrt_tab)) {
>> >  		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
>> > -		       bgrt_tab->header.length, sizeof(*bgrt_tab));
>> > +		       bgrt_tab.header.length, sizeof(bgrt_tab));
>> >  		return;
>> >  	}
>> 
>> ?

Thanks,

Nicolai

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

[Dave Young]

On 01/13/17 at 12:11am, Nicolai Stange wrote:
> On Fri, Jan 13 2017, Dave Young wrote:
> 
> > On 01/12/17 at 12:54pm, Nicolai Stange wrote:
> >> On Thu, Jan 12 2017, Dave Young wrote:
> >> 
> >> > -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;
> >> >  
> >> >  	if (acpi_disabled)
> >> >  		return;
> >> >  
> >> > -	status = acpi_get_table("BGRT", 0,
> >> > -	                        (struct acpi_table_header **)&bgrt_tab);
> >> > -	if (ACPI_FAILURE(status))
> >> > -		return;
> >> 
> >> 
> >> Not sure, but wouldn't it be safer to reverse the order of this assignment
> >> 
> >> > +	bgrt_tab = *(struct acpi_table_bgrt *)table;
> >
> > Nicolai, sorry, I'm not sure I understand the comment, is it about above line?
> > Could you elaborate a bit?
> >
> >> 
> >> and this length check
> >> 
> >
> > I also do not get this :(
> 
> Ah sorry, my point is this: the length check should perhaps be made
> before doing the assignment to bgrt_tab because otherwise, we might end
> up reading from invalid memory.
> 
> I.e. if (struct acpi_table_bgrt *)table->length < sizeof(bgrt_tab), then
> 
>   bgrt_tab = *(struct acpi_table_bgrt *)table;
> 
> would read past the table's end.
> 
> I'm not sure whether this is a real problem though -- that is, whether
> this read could ever hit some unmapped memory.

Nicolai, thanks for the explanation. It make sense to move it to even later
at the end of the function.

Thanks
Dave

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

[Dave Young]

On 01/13/17 at 10:21am, Dave Young wrote:
> On 01/13/17 at 12:11am, Nicolai Stange wrote:
> > On Fri, Jan 13 2017, Dave Young wrote:
> > 
> > > On 01/12/17 at 12:54pm, Nicolai Stange wrote:
> > >> On Thu, Jan 12 2017, Dave Young wrote:
> > >> 
> > >> > -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;
> > >> >  
> > >> >  	if (acpi_disabled)
> > >> >  		return;
> > >> >  
> > >> > -	status = acpi_get_table("BGRT", 0,
> > >> > -	                        (struct acpi_table_header **)&bgrt_tab);
> > >> > -	if (ACPI_FAILURE(status))
> > >> > -		return;
> > >> 
> > >> 
> > >> Not sure, but wouldn't it be safer to reverse the order of this assignment
> > >> 
> > >> > +	bgrt_tab = *(struct acpi_table_bgrt *)table;
> > >
> > > Nicolai, sorry, I'm not sure I understand the comment, is it about above line?
> > > Could you elaborate a bit?
> > >
> > >> 
> > >> and this length check
> > >> 
> > >
> > > I also do not get this :(
> > 
> > Ah sorry, my point is this: the length check should perhaps be made
> > before doing the assignment to bgrt_tab because otherwise, we might end
> > up reading from invalid memory.
> > 
> > I.e. if (struct acpi_table_bgrt *)table->length < sizeof(bgrt_tab), then
> > 
> >   bgrt_tab = *(struct acpi_table_bgrt *)table;
> > 
> > would read past the table's end.
> > 
> > I'm not sure whether this is a real problem though -- that is, whether
> > this read could ever hit some unmapped memory.
> 
> Nicolai, thanks for the explanation. It make sense to move it to even later
> at the end of the function.

Indeed assignment should be after the length checking, but with another
tmp variable the assignment to global var can be moved to the end to
avoid clear the image_address field..

> 
> Thanks
> Dave

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

[Dave Young]

On 01/13/17 at 05:20am, Dave Young wrote:
> On 01/12/17 at 04:15pm, Ard Biesheuvel wrote:
> > Hello Dave,
> > 
> > On 12 January 2017 at 09:41, Dave Young <dyoung@redhat.com> wrote:
> > > There are memory ranges like below when I testing early efi_mem_reserve:
> > >
> > > efi: mem62: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > > efi: mem63: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > > efi: mem64: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > > efi: mem65: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > > efi: mem66: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > > efi: mem67: [Reserved           |   |  |  |  |  |  |  |   |  |  |  |  ] range=[0x0000000000000000-0xffffffffffffffff] (0MB)
> > >
> > 
> > Did you spot Peter's patch to prune invalid memmap entries?
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git/commit/?h=next&id=b2a91a35445229
> > 
> > I would expect this patch to no longer be necessary with that in place, no?
> 
> Ard, good suggestion, I did not notice that patch, will try. Actually
> I'm not sure about fake_mem handling, if we can filter out the invalid
> ranges then it will be natural to drop this one after a test.

The commit works for me. I updated the series.

I addressed comments known, moved patch 1 to 4/4 with only boot service
(dropped loader areas checking), I plan to send them out next Monday
see if there are more comments. 

If anyone who want to try them now, feel free to download from below url:
http://people.redhat.com/~ruyang/efi-bgrt/

Thanks
Dave

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

[Nicolai Stange]

On Fri, Jan 13 2017, Dave Young wrote:

> On 01/13/17 at 10:21am, Dave Young wrote:
>> On 01/13/17 at 12:11am, Nicolai Stange wrote:
>> > On Fri, Jan 13 2017, Dave Young wrote:
>> > 
>> > > On 01/12/17 at 12:54pm, Nicolai Stange wrote:
>> > >> On Thu, Jan 12 2017, Dave Young wrote:
>> > >> 
>> > >> > -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;
>> > >> >  
>> > >> >  	if (acpi_disabled)
>> > >> >  		return;
>> > >> >  
>> > >> > -	status = acpi_get_table("BGRT", 0,
>> > >> > -	                        (struct acpi_table_header **)&bgrt_tab);
>> > >> > -	if (ACPI_FAILURE(status))
>> > >> > -		return;
>> > >> 
>> > >> 
>> > >> Not sure, but wouldn't it be safer to reverse the order of this
>> > >> assignment
>> > >> 
>> > >> > +	bgrt_tab = *(struct acpi_table_bgrt *)table;
>> > >
>> > > Nicolai, sorry, I'm not sure I understand the comment, is it
>> > > about above line?
>> > > Could you elaborate a bit?
>> > >
>> > >> 
>> > >> and this length check
>> > >> 
>> > >
>> > > I also do not get this :(
>> > 
>> > Ah sorry, my point is this: the length check should perhaps be made
>> > before doing the assignment to bgrt_tab because otherwise, we might end
>> > up reading from invalid memory.
>> > 
>> > I.e. if (struct acpi_table_bgrt *)table->length < sizeof(bgrt_tab), then
>> > 
>> >   bgrt_tab = *(struct acpi_table_bgrt *)table;
>> > 
>> > would read past the table's end.
>> > 
>> > I'm not sure whether this is a real problem though -- that is, whether
>> > this read could ever hit some unmapped memory.
>> 
>> Nicolai, thanks for the explanation. It make sense to move it to even later
>> at the end of the function.
>
> Indeed assignment should be after the length checking, but with another
> tmp variable the assignment to global var can be moved to the end to
> avoid clear the image_address field..

I had a look at your updated patches at
http://people.redhat.com/~ruyang/efi-bgrt/ and they look fine to me.

One minor remark:

sizeof(acpi_table_bgrt) == 56 and it might be better to avoid the extra
tmp copy in efi_bgrt_init() by
- assigning directly to bgrt_tab
- do a 'goto err' rather than a 'return' from all the error paths
- do a memset(&bgrt_tab, 0, sizeof(bgrt_tab)) at 'err:'


With the copy to the on-stack 'bgrt', gcc 6.2.0 emits this for each of
the two copies:

  41:   8a 07                   mov    (%rdi),%al
  43:   88 45 d7                mov    %al,-0x29(%rbp)
  46:   8a 47 01                mov    0x1(%rdi),%al
  49:   88 45 d6                mov    %al,-0x2a(%rbp)
  4c:   8a 47 02                mov    0x2(%rdi),%al
  4f:   88 45 d5                mov    %al,-0x2b(%rbp)
  52:   8a 47 03                mov    0x3(%rdi),%al
  55:   88 45 d4                mov    %al,-0x2c(%rbp)
  58:   8a 47 08                mov    0x8(%rdi),%al
  5b:   88 45 d3                mov    %al,-0x2d(%rbp)
  5e:   8a 47 09                mov    0x9(%rdi),%al
  61:   88 45 d2                mov    %al,-0x2e(%rbp)
  64:   8a 47 0a                mov    0xa(%rdi),%al
  67:   88 45 d1                mov    %al,-0x2f(%rbp)
  6a:   8a 47 0b                mov    0xb(%rdi),%al
  6d:   88 45 d0                mov    %al,-0x30(%rbp)
  70:   8a 47 0c                mov    0xc(%rdi),%al
  73:   88 45 cf                mov    %al,-0x31(%rbp)
  76:   8a 47 0d                mov    0xd(%rdi),%al
  79:   88 45 ce                mov    %al,-0x32(%rbp)
  7c:   8a 47 0e                mov    0xe(%rdi),%al
  7f:   88 45 cd                mov    %al,-0x33(%rbp)
  82:   8a 47 0f                mov    0xf(%rdi),%al
  85:   88 45 cc                mov    %al,-0x34(%rbp)
  88:   8a 47 10                mov    0x10(%rdi),%al
  8b:   88 45 cb                mov    %al,-0x35(%rbp)
  8e:   8a 47 11                mov    0x11(%rdi),%al
  91:   88 45 ca                mov    %al,-0x36(%rbp)
  94:   8a 47 12                mov    0x12(%rdi),%al
  97:   88 45 c9                mov    %al,-0x37(%rbp)
  9a:   8a 47 13                mov    0x13(%rdi),%al
  9d:   88 45 c8                mov    %al,-0x38(%rbp)
  a0:   8a 47 14                mov    0x14(%rdi),%al
  a3:   8a 5f 26                mov    0x26(%rdi),%bl
  a6:   0f b6 77 27             movzbl 0x27(%rdi),%esi
  aa:   4c 8b 67 28             mov    0x28(%rdi),%r12
  ae:   88 45 c7                mov    %al,-0x39(%rbp)
  b1:   8a 47 15                mov    0x15(%rdi),%al
  b4:   44 8b 6f 30             mov    0x30(%rdi),%r13d
  b8:   44 8b 7f 34             mov    0x34(%rdi),%r15d
  bc:   88 45 c6                mov    %al,-0x3a(%rbp)
  bf:   8a 47 16                mov    0x16(%rdi),%al
  c2:   88 45 c5                mov    %al,-0x3b(%rbp)
  c5:   8a 47 17                mov    0x17(%rdi),%al
  c8:   88 45 c4                mov    %al,-0x3c(%rbp)
  cb:   8b 47 18                mov    0x18(%rdi),%eax
  ce:   89 45 c0                mov    %eax,-0x40(%rbp)
  d1:   8a 47 1c                mov    0x1c(%rdi),%al
  d4:   88 45 bf                mov    %al,-0x41(%rbp)
  d7:   8a 47 1d                mov    0x1d(%rdi),%al
  da:   88 45 be                mov    %al,-0x42(%rbp)
  dd:   8a 47 1e                mov    0x1e(%rdi),%al
  e0:   88 45 bd                mov    %al,-0x43(%rbp)
  e3:   8a 47 1f                mov    0x1f(%rdi),%al
  e6:   88 45 bc                mov    %al,-0x44(%rbp)
  e9:   8b 47 20                mov    0x20(%rdi),%eax
  ec:   89 45 b8                mov    %eax,-0x48(%rbp)
  ef:   66 8b 47 24             mov    0x24(%rdi),%ax

Not sure why gcc would think that storing bgrt in reversed order on the
stack might be a good idea, but well...

Thanks,

Nicolai

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

[Dave Young]

On 01/13/17 at 01:21pm, Nicolai Stange wrote:
> On Fri, Jan 13 2017, Dave Young wrote:
> 
> > On 01/13/17 at 10:21am, Dave Young wrote:
> >> On 01/13/17 at 12:11am, Nicolai Stange wrote:
> >> > On Fri, Jan 13 2017, Dave Young wrote:
> >> > 
> >> > > On 01/12/17 at 12:54pm, Nicolai Stange wrote:
> >> > >> On Thu, Jan 12 2017, Dave Young wrote:
> >> > >> 
> >> > >> > -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;
> >> > >> >  
> >> > >> >  	if (acpi_disabled)
> >> > >> >  		return;
> >> > >> >  
> >> > >> > -	status = acpi_get_table("BGRT", 0,
> >> > >> > -	                        (struct acpi_table_header **)&bgrt_tab);
> >> > >> > -	if (ACPI_FAILURE(status))
> >> > >> > -		return;
> >> > >> 
> >> > >> 
> >> > >> Not sure, but wouldn't it be safer to reverse the order of this
> >> > >> assignment
> >> > >> 
> >> > >> > +	bgrt_tab = *(struct acpi_table_bgrt *)table;
> >> > >
> >> > > Nicolai, sorry, I'm not sure I understand the comment, is it
> >> > > about above line?
> >> > > Could you elaborate a bit?
> >> > >
> >> > >> 
> >> > >> and this length check
> >> > >> 
> >> > >
> >> > > I also do not get this :(
> >> > 
> >> > Ah sorry, my point is this: the length check should perhaps be made
> >> > before doing the assignment to bgrt_tab because otherwise, we might end
> >> > up reading from invalid memory.
> >> > 
> >> > I.e. if (struct acpi_table_bgrt *)table->length < sizeof(bgrt_tab), then
> >> > 
> >> >   bgrt_tab = *(struct acpi_table_bgrt *)table;
> >> > 
> >> > would read past the table's end.
> >> > 
> >> > I'm not sure whether this is a real problem though -- that is, whether
> >> > this read could ever hit some unmapped memory.
> >> 
> >> Nicolai, thanks for the explanation. It make sense to move it to even later
> >> at the end of the function.
> >
> > Indeed assignment should be after the length checking, but with another
> > tmp variable the assignment to global var can be moved to the end to
> > avoid clear the image_address field..
> 
> I had a look at your updated patches at
> http://people.redhat.com/~ruyang/efi-bgrt/ and they look fine to me.

Many thanks~

> 
> One minor remark:
> 
> sizeof(acpi_table_bgrt) == 56 and it might be better to avoid the extra
> tmp copy in efi_bgrt_init() by
> - assigning directly to bgrt_tab
> - do a 'goto err' rather than a 'return' from all the error paths
> - do a memset(&bgrt_tab, 0, sizeof(bgrt_tab)) at 'err:'

Updated in V2, indeed text size shrunk from 1199 to 762.

> 
> 
> With the copy to the on-stack 'bgrt', gcc 6.2.0 emits this for each of
> the two copies:
> 
>   41:   8a 07                   mov    (%rdi),%al
>   43:   88 45 d7                mov    %al,-0x29(%rbp)
>   46:   8a 47 01                mov    0x1(%rdi),%al
>   49:   88 45 d6                mov    %al,-0x2a(%rbp)
>   4c:   8a 47 02                mov    0x2(%rdi),%al
>   4f:   88 45 d5                mov    %al,-0x2b(%rbp)
>   52:   8a 47 03                mov    0x3(%rdi),%al
>   55:   88 45 d4                mov    %al,-0x2c(%rbp)
>   58:   8a 47 08                mov    0x8(%rdi),%al
>   5b:   88 45 d3                mov    %al,-0x2d(%rbp)
>   5e:   8a 47 09                mov    0x9(%rdi),%al
>   61:   88 45 d2                mov    %al,-0x2e(%rbp)
>   64:   8a 47 0a                mov    0xa(%rdi),%al
>   67:   88 45 d1                mov    %al,-0x2f(%rbp)
>   6a:   8a 47 0b                mov    0xb(%rdi),%al
>   6d:   88 45 d0                mov    %al,-0x30(%rbp)
>   70:   8a 47 0c                mov    0xc(%rdi),%al
>   73:   88 45 cf                mov    %al,-0x31(%rbp)
>   76:   8a 47 0d                mov    0xd(%rdi),%al
>   79:   88 45 ce                mov    %al,-0x32(%rbp)
>   7c:   8a 47 0e                mov    0xe(%rdi),%al
>   7f:   88 45 cd                mov    %al,-0x33(%rbp)
>   82:   8a 47 0f                mov    0xf(%rdi),%al
>   85:   88 45 cc                mov    %al,-0x34(%rbp)
>   88:   8a 47 10                mov    0x10(%rdi),%al
>   8b:   88 45 cb                mov    %al,-0x35(%rbp)
>   8e:   8a 47 11                mov    0x11(%rdi),%al
>   91:   88 45 ca                mov    %al,-0x36(%rbp)
>   94:   8a 47 12                mov    0x12(%rdi),%al
>   97:   88 45 c9                mov    %al,-0x37(%rbp)
>   9a:   8a 47 13                mov    0x13(%rdi),%al
>   9d:   88 45 c8                mov    %al,-0x38(%rbp)
>   a0:   8a 47 14                mov    0x14(%rdi),%al
>   a3:   8a 5f 26                mov    0x26(%rdi),%bl
>   a6:   0f b6 77 27             movzbl 0x27(%rdi),%esi
>   aa:   4c 8b 67 28             mov    0x28(%rdi),%r12
>   ae:   88 45 c7                mov    %al,-0x39(%rbp)
>   b1:   8a 47 15                mov    0x15(%rdi),%al
>   b4:   44 8b 6f 30             mov    0x30(%rdi),%r13d
>   b8:   44 8b 7f 34             mov    0x34(%rdi),%r15d
>   bc:   88 45 c6                mov    %al,-0x3a(%rbp)
>   bf:   8a 47 16                mov    0x16(%rdi),%al
>   c2:   88 45 c5                mov    %al,-0x3b(%rbp)
>   c5:   8a 47 17                mov    0x17(%rdi),%al
>   c8:   88 45 c4                mov    %al,-0x3c(%rbp)
>   cb:   8b 47 18                mov    0x18(%rdi),%eax
>   ce:   89 45 c0                mov    %eax,-0x40(%rbp)
>   d1:   8a 47 1c                mov    0x1c(%rdi),%al
>   d4:   88 45 bf                mov    %al,-0x41(%rbp)
>   d7:   8a 47 1d                mov    0x1d(%rdi),%al
>   da:   88 45 be                mov    %al,-0x42(%rbp)
>   dd:   8a 47 1e                mov    0x1e(%rdi),%al
>   e0:   88 45 bd                mov    %al,-0x43(%rbp)
>   e3:   8a 47 1f                mov    0x1f(%rdi),%al
>   e6:   88 45 bc                mov    %al,-0x44(%rbp)
>   e9:   8b 47 20                mov    0x20(%rdi),%eax
>   ec:   89 45 b8                mov    %eax,-0x48(%rbp)
>   ef:   66 8b 47 24             mov    0x24(%rdi),%ax
> 
> Not sure why gcc would think that storing bgrt in reversed order on the
> stack might be a good idea, but well...

I have no idea about this..

> 
> Thanks,
> 
> Nicolai

Thanks
Dave

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

[Bhupesh Sharma]

Thanks Dave.

On Fri, Jan 13, 2017 at 3:03 AM, Dave Young <dyoung@redhat.com> wrote:
> On 01/12/17 at 04:20pm, Ard Biesheuvel wrote:
>> On 12 January 2017 at 09:41, 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>
>>
>> I know this is probably out of scope for you, but since we're moving
>> things around, any chance we could do so in a manner that will enable
>> BGRT support for arm64/ACPI? Happy to test/collaborate on this.
>>
>
> I'm happy to do so, Bhupesh Sharma <bhsharma@redhat.com> said he had
> some investigation on that already, I would like to ask him to help on that.
>
> Already cced him..


Hi Ard,

I have started working on an implementation where most of the BGRT
code which exists inside 'arch/x86/platform/efi-bgrt.c' can be reused
for ARM/ARM64.

I am testing a RFC approach for the same using Qemu for AARCH64. I
have sent out a patch to enable BGRT support in ArmVirtPkg (see [1])

I have one question regarding the placement of the early bgrt handling
code so that it can be reused on both arm/arm64 and x86:

- Should I consider moving the current code from
arch/x86/platform/efi-bgrt.c to outside arch/x86 so that it can be
used by both the ARCHs or should I reuse the existing x86 stuff in a
ARM specific way - no mem_remap for e.g. in a find inside arch/arm -
say efi-arm-bgrt.c

Suggestions?


[1] https://lists.01.org/pipermail/edk2-devel/2017-January/006588.html

Regards,
Bhupesh

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

[Ard Biesheuvel]

On 16 January 2017 at 15:15, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> Thanks Dave.
>
> On Fri, Jan 13, 2017 at 3:03 AM, Dave Young <dyoung@redhat.com> wrote:
>> On 01/12/17 at 04:20pm, Ard Biesheuvel wrote:
>>> On 12 January 2017 at 09:41, 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>
>>>
>>> I know this is probably out of scope for you, but since we're moving
>>> things around, any chance we could do so in a manner that will enable
>>> BGRT support for arm64/ACPI? Happy to test/collaborate on this.
>>>
>>
>> I'm happy to do so, Bhupesh Sharma <bhsharma@redhat.com> said he had
>> some investigation on that already, I would like to ask him to help on that.
>>
>> Already cced him..
>
>
> Hi Ard,
>
> I have started working on an implementation where most of the BGRT
> code which exists inside 'arch/x86/platform/efi-bgrt.c' can be reused
> for ARM/ARM64.
>
> I am testing a RFC approach for the same using Qemu for AARCH64. I
> have sent out a patch to enable BGRT support in ArmVirtPkg (see [1])
>
> I have one question regarding the placement of the early bgrt handling
> code so that it can be reused on both arm/arm64 and x86:
>
> - Should I consider moving the current code from
> arch/x86/platform/efi-bgrt.c to outside arch/x86 so that it can be
> used by both the ARCHs or should I reuse the existing x86 stuff in a
> ARM specific way - no mem_remap for e.g. in a find inside arch/arm -
> say efi-arm-bgrt.c
>

-ENOPARSE

Please first move the code from x86 to a generic location
(drivers/firmware/efi or drivers/acpi), and make sure that it still
works as expected.
Then you can modify the code so that it works for arm64 as well as
x86, in a separate patch.

Thanks,
Ard.

> Suggestions?
>
>
> [1] https://lists.01.org/pipermail/edk2-devel/2017-January/006588.html
>
> Regards,
> Bhupesh

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

[Matt Fleming]

On Fri, 13 Jan, at 05:29:52AM, Dave Young wrote:
> 
> It sounds reasonable though I'm still not sure about EFI_LOADER*.
> 
> The main purpose of this patch is to address the invalid mem ranges
> case. As Ard mentioned I will test with Peter's patch first, if it works
> fine I would like to either drop this patch as a future improvement or add
> it at the end of the next post.
> 
> Matt, what's your opinion about the boot_only check and the EFI_LOADERS*
> question?

The main reason that efi_mem_reserve() isn't used for EFI_LOADER
regions today is because we already have a mechanism for reserving it
via memblock_reserve(), which we do during a very early stage of boot
when parsing all the different types of SETUP_* objects.

It's questionable whether it would make sense to switch to
efi_mem_reserve() for EFI_LOADER regions because then you'd
potentially have different APIs for different SETUP_* objects.

As things stand today, I would suggest triggering a WARN_ON() if
someone tries to efi_mem_reserve() an EFI_LOADER region, until/unless
the day comes when a user exists in the kernel.

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

[Ard Biesheuvel]

On 27 January 2017 at 14:48, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 13 Jan, at 05:29:52AM, Dave Young wrote:
>>
>> It sounds reasonable though I'm still not sure about EFI_LOADER*.
>>
>> The main purpose of this patch is to address the invalid mem ranges
>> case. As Ard mentioned I will test with Peter's patch first, if it works
>> fine I would like to either drop this patch as a future improvement or add
>> it at the end of the next post.
>>
>> Matt, what's your opinion about the boot_only check and the EFI_LOADERS*
>> question?
>
> The main reason that efi_mem_reserve() isn't used for EFI_LOADER
> regions today is because we already have a mechanism for reserving it
> via memblock_reserve(), which we do during a very early stage of boot
> when parsing all the different types of SETUP_* objects.
>
> It's questionable whether it would make sense to switch to
> efi_mem_reserve() for EFI_LOADER regions because then you'd
> potentially have different APIs for different SETUP_* objects.
>
> As things stand today, I would suggest triggering a WARN_ON() if
> someone tries to efi_mem_reserve() an EFI_LOADER region, until/unless
> the day comes when a user exists in the kernel.

Hmm, I just queued this. Should we drop it again?

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

[Matt Fleming]

On Fri, 27 Jan, at 05:04:50PM, Ard Biesheuvel wrote:
> On 27 January 2017 at 14:48, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > On Fri, 13 Jan, at 05:29:52AM, Dave Young wrote:
> >>
> >> It sounds reasonable though I'm still not sure about EFI_LOADER*.
> >>
> >> The main purpose of this patch is to address the invalid mem ranges
> >> case. As Ard mentioned I will test with Peter's patch first, if it works
> >> fine I would like to either drop this patch as a future improvement or add
> >> it at the end of the next post.
> >>
> >> Matt, what's your opinion about the boot_only check and the EFI_LOADERS*
> >> question?
> >
> > The main reason that efi_mem_reserve() isn't used for EFI_LOADER
> > regions today is because we already have a mechanism for reserving it
> > via memblock_reserve(), which we do during a very early stage of boot
> > when parsing all the different types of SETUP_* objects.
> >
> > It's questionable whether it would make sense to switch to
> > efi_mem_reserve() for EFI_LOADER regions because then you'd
> > potentially have different APIs for different SETUP_* objects.
> >
> > As things stand today, I would suggest triggering a WARN_ON() if
> > someone tries to efi_mem_reserve() an EFI_LOADER region, until/unless
> > the day comes when a user exists in the kernel.
> 
> Hmm, I just queued this. Should we drop it again?

Does dropping it break the entire series?

Having had some time to re-read Dave's commit log, it sounds like it
just papers over a bug, which is that efi_memmap_insert() cannot deal
with reserved entries, which all look like they describe the same
region.

So I guess my question is: Shouldn't you fix that instead of requiring
the caller of efi_memmap_insert() to understand what type of entries
it's mapping?

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

[Ard Biesheuvel]

On 27 January 2017 at 22:13, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 27 Jan, at 05:04:50PM, Ard Biesheuvel wrote:
>> On 27 January 2017 at 14:48, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> > On Fri, 13 Jan, at 05:29:52AM, Dave Young wrote:
>> >>
>> >> It sounds reasonable though I'm still not sure about EFI_LOADER*.
>> >>
>> >> The main purpose of this patch is to address the invalid mem ranges
>> >> case. As Ard mentioned I will test with Peter's patch first, if it works
>> >> fine I would like to either drop this patch as a future improvement or add
>> >> it at the end of the next post.
>> >>
>> >> Matt, what's your opinion about the boot_only check and the EFI_LOADERS*
>> >> question?
>> >
>> > The main reason that efi_mem_reserve() isn't used for EFI_LOADER
>> > regions today is because we already have a mechanism for reserving it
>> > via memblock_reserve(), which we do during a very early stage of boot
>> > when parsing all the different types of SETUP_* objects.
>> >
>> > It's questionable whether it would make sense to switch to
>> > efi_mem_reserve() for EFI_LOADER regions because then you'd
>> > potentially have different APIs for different SETUP_* objects.
>> >
>> > As things stand today, I would suggest triggering a WARN_ON() if
>> > someone tries to efi_mem_reserve() an EFI_LOADER region, until/unless
>> > the day comes when a user exists in the kernel.
>>
>> Hmm, I just queued this. Should we drop it again?
>
> Does dropping it break the entire series?
>
> Having had some time to re-read Dave's commit log, it sounds like it
> just papers over a bug, which is that efi_memmap_insert() cannot deal
> with reserved entries, which all look like they describe the same
> region.
>

No, it cannot deal with bogus entries, and Peter already fixed that.
Dave confirmed that Peter's patch (the one we moved from next to
urgen) made the problem go away.

> So I guess my question is: Shouldn't you fix that instead of requiring
> the caller of efi_memmap_insert() to understand what type of entries
> it's mapping?

Indeed. So I don't think the patch is actually needed anymore