[PATCH v4 00/12] kexec kernel efi runtime support

[PATCH v4 00/12] kexec kernel efi runtime support

[Dave Young]

Hi,

Here is the V4 resend for supporting kexec kernel efi runtime.
Per pervious discussion I pass the 1st kernel efi runtime mapping
via setup_data to 2nd kernel. Besides of the runtime mapping
info I also pass the fw_vendor, runtime, config table, smbios
physical address in setup_data. EFI spec mentioned fw_vendor,
runtime, config table addresses will be converted to virt address
after entering virtual mode, but we will use it as physical address
in efi_init. For smbios EFI spec did not mention about the address
updating, but during my test on a HP workstation, the bios will
convert it to Virt addr, thus pass it in setup_data as well.

For fw_vendor, runtime, config table, I export them in /sys/firmware/
efi/, smbios is already in /sys/firmware/efi/systab.

For efi runtime mapping I add a new directory /sys/firmware/efi/
runtime-map/ like below
[dave@darkstar ~]$ tree /sys/firmware/efi/runtime-map/
/sys/firmware/efi/runtime-map/
├── 0
│   ├── attribute
│   ├── num_pages
│   ├── phys_addr
│   ├── type
│   └── virt_addr
├── 1
│   ├── attribute
│   ├── num_pages
│   ├── phys_addr
│   ├── type
│   └── virt_addr
[snip]
 
kexec-tools will assemble them as setup_data and pass to 2nd kernel.
I will send userspace patches as well.

Limitation is I only write support for x86_64, test on below machines:
Lenovo thinkpad t420
Dell inspiron 14 - 3421
HP Z420 workstation
Qemu + OVMF

Please help to review the patches.

The patches are based on matt's efi master tree

Changes from v1:
add one flag in xloadflags, so kexec-tools can safely load old kernel
without efi support.
coding style fixes
function name for map phys_addr to fixed virt_addr
Add ABI documentation for sysfs files

Changes from v2:
01/09: a new patch to remove unused variables in __map_region function
       catched by Toshi Kani
09/09: a new patch to export x86 boot_params to sysfs instead of use
       debugfs files
Matt: reuse __map_region instead do same thing in another function.
      add a wrapper function efi_map_region_fixed [02/09]
      check return value of krealloc
      sysfs dir name s/efi-runtime-map/runtime-map [06/09]
      use desc_size in efi_runtime_map
      for the xloadflags defination: +&& defined(CONFIG_KEXEC)
Greg: sysfs : one file one value for fw_vendor, runtime, tables. [05/09]
      Document them in ABI testing
HPA:  Document the new xloadflag
Also there's other function cleanup and improvement for error handling.

Changes from v3:
Greg: sysfs code move to use __ATTR_RO and attr_group
Boris: comments and code alignment

Added 3 new patches below
10-print-efi-runtime-memmap.patch
  - 10/12: print only runtime ranges in case EFI_DEBUG printing
11-reserve-setup-data-late.patch
  - fix a bug of kdump kernel, move function for reserving setup data
    ranges late after parsing memmap= cmdline params because kdump kernel
    will pass exact memmap late.
12-x86-kdebugfs-use-ioremap.patch
  - fix a bug of x86/kernel/kdebugfs.c, use ioremap instead of __va for
    low mem because __va does not work for exact memmap=

Dave Young (12):
  efi: remove unused variables in __map_region
  efi: add a wrapper function efi_map_region_fixed
  efi: reserve boot service fix
  efi: cleanup efi_enter_virtual_mode function
  efi: export more efi table variable to sysfs
  efi: export efi runtime memory mapping to sysfs
  efi: passing kexec necessary efi data via setup_data
  efi: only print saved efi runtime maps instead of all memmap ranges
    for kexec
  x86: add xloadflags bit for efi runtime support on kexec
  x86: export x86 boot_params to sysfs
  x86: reserve setup_data ranges late after parsing memmap cmdline
  x86: kdebugfs do not use __va for getting setup_data virt addr

 Documentation/ABI/testing/sysfs-firmware-efi       |  26 ++
 .../ABI/testing/sysfs-firmware-efi-runtime-map     |  45 +++
 Documentation/ABI/testing/sysfs-kernel-boot_params |  40 +++
 Documentation/x86/boot.txt                         |   3 +
 arch/x86/boot/header.S                             |   9 +-
 arch/x86/include/asm/efi.h                         |  13 +
 arch/x86/include/uapi/asm/bootparam.h              |   2 +
 arch/x86/kernel/Makefile                           |   2 +-
 arch/x86/kernel/kdebugfs.c                         |  35 +--
 arch/x86/kernel/ksysfs.c                           | 339 +++++++++++++++++++++
 arch/x86/kernel/setup.c                            |   7 +-
 arch/x86/platform/efi/efi.c                        | 313 +++++++++++++++----
 arch/x86/platform/efi/efi_32.c                     |   2 +
 arch/x86/platform/efi/efi_64.c                     |  16 +-
 drivers/firmware/efi/Kconfig                       |  10 +
 drivers/firmware/efi/Makefile                      |   1 +
 drivers/firmware/efi/efi.c                         |  47 ++-
 drivers/firmware/efi/runtime-map.c                 | 199 ++++++++++++
 include/linux/efi.h                                |   9 +
 19 files changed, 1028 insertions(+), 90 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-boot_params
 create mode 100644 arch/x86/kernel/ksysfs.c
 create mode 100644 drivers/firmware/efi/runtime-map.c

-- 
1.8.3.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH v4 02/12] efi: add a wrapper function efi_map_region_fixed

[Dave Young]

Kexec kernel will use saved runtime virtual mapping, so add a
new function efi_map_region_fixed for directly mapping a md
to md->virt.

The md is passed in from 1st kernel, the virtual addr is
saved in md->virt_addr.

Matt: coding style
      reuse __map_region
Boris: Strenthen comment lines to 80 cols.

Signed-off-by: Dave Young <dyoung@redhat.com>
Acked-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/efi.h     |  1 +
 arch/x86/platform/efi/efi_32.c |  2 ++
 arch/x86/platform/efi/efi_64.c | 10 ++++++++++
 3 files changed, 13 insertions(+)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 89a05b0..9fbaeb2 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -128,6 +128,7 @@ extern void efi_call_phys_epilog(void);
 extern void efi_unmap_memmap(void);
 extern void 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);
 extern void efi_sync_low_kernel_mappings(void);
 extern void efi_setup_page_tables(void);
 extern void __init old_map_region(efi_memory_desc_t *md);
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index e94557c..7b3ec6e 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -47,6 +47,8 @@ void __init efi_map_region(efi_memory_desc_t *md)
 	old_map_region(md);
 }
 
+void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
+
 void efi_call_phys_prelog(void)
 {
 	struct desc_ptr gdt_descr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index c5a6491..ff08cb1 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -199,6 +199,16 @@ void __init efi_map_region(efi_memory_desc_t *md)
 	md->virt_addr = efi_va;
 }
 
+/*
+ * kexec kernel will use efi_map_region_fixed to map efi runtime memory ranges.
+ * md->virt_addr is the original virtual address which had been mapped in kexec
+ * 1st kernel.
+ */
+void __init efi_map_region_fixed(efi_memory_desc_t *md)
+{
+	__map_region(md, md->virt_addr);
+}
+
 void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
 				 u32 type, u64 attribute)
 {
-- 
1.8.3.1

[PATCH v4 03/12] efi: reserve boot service fix

[Dave Young]

Current code check boot service region with kernel text region by:
start+size >= __pa_symbol(_text)
The end of the above region should be start + size - 1 instead.

I see this problem in ovmf + Fedora 19 grub boot:
text start: 1000000 md start: 800000 md size: 800000

Signed-off-by: Dave Young <dyoung@redhat.com>
Acked-by: Borislav Petkov <bp@suse.de>
Acked-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/platform/efi/efi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index f8ec4da..15e3b5e 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -438,7 +438,7 @@ void __init efi_reserve_boot_services(void)
 		 * - Not within any part of the kernel
 		 * - Not the bios reserved area
 		*/
-		if ((start+size >= __pa_symbol(_text)
+		if ((start + size > __pa_symbol(_text)
 				&& start <= __pa_symbol(_end)) ||
 			!e820_all_mapped(start, start+size, E820_RAM) ||
 			memblock_is_region_reserved(start, size)) {
-- 
1.8.3.1

[PATCH v4 01/12] efi: remove unused variables in __map_region

[Dave Young]

variables size and end is useless in this function, thus remove them.

Reported-by: Toshi Kani <toshi.kani-VXdhtT5mjnY@public.gmane.org>
Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
---
 arch/x86/platform/efi/efi_64.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index bf286c3..c5a6491 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -148,15 +148,11 @@ void efi_setup_page_tables(void)
 static void __init __map_region(efi_memory_desc_t *md, u64 va)
 {
 	pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
-	unsigned long pf = 0, size;
-	u64 end;
+	unsigned long pf = 0;
 
 	if (!(md->attribute & EFI_MEMORY_WB))
 		pf |= _PAGE_PCD;
 
-	size = md->num_pages << PAGE_SHIFT;
-	end  = va + size;
-
 	if (kernel_map_pages_in_pgd(pgd, md->phys_addr, va, md->num_pages, pf))
 		pr_warn("Error mapping PA 0x%llx -> VA 0x%llx!\n",
 			   md->phys_addr, va);
-- 
1.8.3.1

[PATCH v4 04/12] efi: cleanup efi_enter_virtual_mode function

[Dave Young]

Add two small functions:
efi_merge_regions and efi_map_regions, efi_enter_virtual_mode
calls them instead of embedding two long for loop.

v1->v2:
refresh; coding style fixes.

v2->v3:
Toshi Kani:
remove unused variable
Matt: check return value of krealloc.
v3->v4:
Boris: Stretch comment to 80 cols

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 109 +++++++++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 43 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 15e3b5e..480529a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -773,44 +773,12 @@ void __init old_map_region(efi_memory_desc_t *md)
 		       (unsigned long long)md->phys_addr);
 }
 
-/*
- * This function will switch the EFI runtime services to virtual mode.
- * Essentially, we look through the EFI memmap and map every region that
- * has the runtime attribute bit set in its memory descriptor into the
- * ->trampoline_pgd page table using a top-down VA allocation scheme.
- *
- * The old method which used to update that memory descriptor with the
- * virtual address obtained from ioremap() is still supported when the
- * kernel is booted with efi=old_map on its command line. Same old
- * method enabled the runtime services to be called without having to
- * thunk back into physical mode for every invocation.
- *
- * The new method does a pagetable switch in a preemption-safe manner
- * so that we're in a different address space when calling a runtime
- * function. For function arguments passing we do copy the PGDs of the
- * kernel page table into ->trampoline_pgd prior to each call.
- */
-void __init efi_enter_virtual_mode(void)
+/* Merge contiguous regions of the same type and attribute */
+static void __init efi_merge_regions(void)
 {
+	void *p;
 	efi_memory_desc_t *md, *prev_md = NULL;
-	void *p, *new_memmap = NULL;
-	unsigned long size;
-	efi_status_t status;
-	u64 end, systab;
-	int count = 0;
 
-	efi.systab = NULL;
-
-	/*
-	 * We don't do virtual mode, since we don't do runtime services, on
-	 * non-native EFI
-	 */
-	if (!efi_is_native()) {
-		efi_unmap_memmap();
-		return;
-	}
-
-	/* Merge contiguous regions of the same type and attribute */
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		u64 prev_size;
 		md = p;
@@ -837,6 +805,18 @@ void __init efi_enter_virtual_mode(void)
 		prev_md = md;
 
 	}
+}
+
+/*
+ * Map efi memory ranges for runtime serivce and update new_memmap with virtual
+ * addresses.
+ */
+static void * __init efi_map_regions(int *count)
+{
+	efi_memory_desc_t *md;
+	void *p, *new_memmap = NULL;
+	unsigned long size;
+	u64 end, systab;
 
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
@@ -861,14 +841,60 @@ void __init efi_enter_virtual_mode(void)
 		}
 
 		new_memmap = krealloc(new_memmap,
-				      (count + 1) * memmap.desc_size,
-				      GFP_KERNEL);
+					(*count + 1) * memmap.desc_size,
+					GFP_KERNEL);
 		if (!new_memmap)
-			goto err_out;
+			goto ret;
 
-		memcpy(new_memmap + (count * memmap.desc_size), md,
+		memcpy(new_memmap + (*count * memmap.desc_size), md,
 		       memmap.desc_size);
-		count++;
+		(*count)++;
+	}
+
+ret:
+	return new_memmap;
+}
+
+/*
+ * This function will switch the EFI runtime services to virtual mode.
+ * Essentially, we look through the EFI memmap and map every region that
+ * has the runtime attribute bit set in its memory descriptor into the
+ * ->trampoline_pgd page table using a top-down VA allocation scheme.
+ *
+ * The old method which used to update that memory descriptor with the
+ * virtual address obtained from ioremap() is still supported when the
+ * kernel is booted with efi=old_map on its command line. Same old
+ * method enabled the runtime services to be called without having to
+ * thunk back into physical mode for every invocation.
+ *
+ * The new method does a pagetable switch in a preemption-safe manner
+ * so that we're in a different address space when calling a runtime
+ * function. For function arguments passing we do copy the PGDs of the
+ * kernel page table into ->trampoline_pgd prior to each call.
+ */
+void __init efi_enter_virtual_mode(void)
+{
+	efi_status_t status;
+	void *new_memmap = NULL;
+	int count = 0;
+
+	efi.systab = NULL;
+
+	/*
+	 * We don't do virtual mode, since we don't do runtime services, on
+	 * non-native EFI
+	 */
+	if (!efi_is_native()) {
+		efi_unmap_memmap();
+		return;
+	}
+
+	efi_merge_regions();
+
+	new_memmap = efi_map_regions(&count);
+	if (!new_memmap) {
+		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
+		return;
 	}
 
 	BUG_ON(!efi.systab);
@@ -922,9 +948,6 @@ void __init efi_enter_virtual_mode(void)
 			 0, NULL);
 
 	return;
-
- err_out:
-	pr_err("Error reallocating memory, EFI runtime non-functional!\n");
 }
 
 /*
-- 
1.8.3.1

Re: [PATCH v4 04/12] efi: cleanup efi_enter_virtual_mode function

[Matt Fleming]

On Tue, 26 Nov, at 01:57:49PM, Dave Young wrote:
> Add two small functions:
> efi_merge_regions and efi_map_regions, efi_enter_virtual_mode
> calls them instead of embedding two long for loop.
> 
> v1->v2:
> refresh; coding style fixes.
> 
> v2->v3:
> Toshi Kani:
> remove unused variable
> Matt: check return value of krealloc.
> v3->v4:
> Boris: Stretch comment to 80 cols
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi.c | 109 +++++++++++++++++++++++++++-----------------
>  1 file changed, 66 insertions(+), 43 deletions(-)

[...]

> @@ -922,9 +948,6 @@ void __init efi_enter_virtual_mode(void)
>  			 0, NULL);
>  
>  	return;
> -
> - err_out:
> -	pr_err("Error reallocating memory, EFI runtime non-functional!\n");
>  }
>  
>  /*
 
Teeny, tiny comment: You could now delete the above return statement.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v4 05/12] efi: export more efi table variable to sysfs

[Dave Young]

Export fw_vendor, runtime and config tables physical
addresses to /sys/firmware/efi/systab becaue kexec
kernel will need them.

>From EFI spec these 3 variables will be updated to
virtual address after entering virtual mode. But
kernel startup code will need the physical address.

changelog:
Greg: add standalone sysfs files instead of add lines to systab
Document them as testing ABI
Greg: use group attrs and is_visible
Boris: align comments lines

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/ABI/testing/sysfs-firmware-efi | 26 ++++++++++++++++
 arch/x86/platform/efi/efi.c                  |  4 +++
 drivers/firmware/efi/efi.c                   | 44 ++++++++++++++++++++++++++++
 include/linux/efi.h                          |  3 ++
 4 files changed, 77 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi

diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
new file mode 100644
index 0000000..9252a24
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-efi
@@ -0,0 +1,26 @@
+What:		/sys/firmware/efi/fw_vendor
+Date:		Nov 2013
+Contact:	Dave Young <dyoung-8RVElS0ezzQdnm+yROfE0A@public.gmane.org>
+Description:
+		Shows the physical address of firmware vendor in the EFI
+		system table.
+Users:
+		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
+
+What:		/sys/firmware/efi/runtime
+Date:		Nov 2013
+Contact:	Dave Young <dyoung-8RVElS0ezzQdnm+yROfE0A@public.gmane.org>
+Description:
+		Shows the physical address of runtime service table in the
+		EFI system table.
+Users:
+		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
+
+What:		/sys/firmware/efi/config_table
+Date:		Nov 2013
+Contact:	Dave Young <dyoung-8RVElS0ezzQdnm+yROfE0A@public.gmane.org>
+Description:
+		Shows the physical address of config table in the EFI
+		system table.
+Users:
+		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 480529a..2a292aa 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -653,6 +653,10 @@ void __init efi_init(void)
 
 	set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);
 
+	efi.config_table = (unsigned long)efi.systab->tables;
+	efi.fw_vendor	 = (unsigned long)efi.systab->fw_vendor;
+	efi.runtime	 = (unsigned long)efi.systab->runtime;
+
 	/*
 	 * Show what we know for posterity
 	 */
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 2e2fbde..5d85956 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -32,6 +32,9 @@ struct efi __read_mostly efi = {
 	.hcdp       = EFI_INVALID_TABLE_ADDR,
 	.uga        = EFI_INVALID_TABLE_ADDR,
 	.uv_systab  = EFI_INVALID_TABLE_ADDR,
+	.fw_vendor  = EFI_INVALID_TABLE_ADDR,
+	.runtime    = EFI_INVALID_TABLE_ADDR,
+	.config_table  = EFI_INVALID_TABLE_ADDR,
 };
 EXPORT_SYMBOL(efi);
 
@@ -71,13 +74,54 @@ static ssize_t systab_show(struct kobject *kobj,
 static struct kobj_attribute efi_attr_systab =
 			__ATTR(systab, 0400, systab_show, NULL);
 
+static ssize_t fw_vendor_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "0x%lx\n", efi.fw_vendor);
+}
+
+static ssize_t runtime_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "0x%lx\n", efi.runtime);
+}
+
+static ssize_t config_table_show(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "0x%lx\n", efi.config_table);
+}
+
+static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
+static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
+static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
+
 static struct attribute *efi_subsys_attrs[] = {
 	&efi_attr_systab.attr,
+	&efi_attr_fw_vendor.attr,
+	&efi_attr_runtime.attr,
+	&efi_attr_config_table.attr,
 	NULL,	/* maybe more in the future? */
 };
 
+static umode_t efi_attr_is_visible(struct kobject *kobj,
+					struct attribute *attr, int n)
+{
+	umode_t mode = attr->mode;
+
+	if (attr == &efi_attr_fw_vendor.attr)
+		return (efi.fw_vendor == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
+	else if (attr == &efi_attr_runtime.attr)
+		return (efi.runtime == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
+	else if (attr == &efi_attr_config_table.attr)
+		return (efi.config_table == EFI_INVALID_TABLE_ADDR) ? 0 : mode;
+
+	return mode;
+}
+
 static struct attribute_group efi_subsys_attr_group = {
 	.attrs = efi_subsys_attrs,
+	.is_visible = efi_attr_is_visible,
 };
 
 static struct efivars generic_efivars;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6c0ca52..fb60b10 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -556,6 +556,9 @@ extern struct efi {
 	unsigned long hcdp;		/* HCDP table */
 	unsigned long uga;		/* UGA table */
 	unsigned long uv_systab;	/* UV system table */
+	unsigned long fw_vendor;	/* fw_vendor */
+	unsigned long runtime;		/* runtime table */
+	unsigned long config_table;	/* config tables */
 	efi_get_time_t *get_time;
 	efi_set_time_t *set_time;
 	efi_get_wakeup_time_t *get_wakeup_time;
-- 
1.8.3.1

Re: [PATCH v4 05/12] efi: export more efi table variable to sysfs

[Borislav Petkov]

On Tue, Nov 26, 2013 at 01:57:50PM +0800, Dave Young wrote:
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 2e2fbde..5d85956 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -32,6 +32,9 @@ struct efi __read_mostly efi = {
>  	.hcdp       = EFI_INVALID_TABLE_ADDR,
>  	.uga        = EFI_INVALID_TABLE_ADDR,
>  	.uv_systab  = EFI_INVALID_TABLE_ADDR,
> +	.fw_vendor  = EFI_INVALID_TABLE_ADDR,
> +	.runtime    = EFI_INVALID_TABLE_ADDR,
> +	.config_table  = EFI_INVALID_TABLE_ADDR,

See what I mean with alignment?

Now this config_table thing sticks out and completely destroys the
balance in the universe.

:-)

>  };
>  EXPORT_SYMBOL(efi);
>  
> @@ -71,13 +74,54 @@ static ssize_t systab_show(struct kobject *kobj,
>  static struct kobj_attribute efi_attr_systab =
>  			__ATTR(systab, 0400, systab_show, NULL);
>  
> +static ssize_t fw_vendor_show(struct kobject *kobj,
> +				struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "0x%lx\n", efi.fw_vendor);
> +}
> +
> +static ssize_t runtime_show(struct kobject *kobj,
> +				struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "0x%lx\n", efi.runtime);
> +}
> +
> +static ssize_t config_table_show(struct kobject *kobj,
> +				struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "0x%lx\n", efi.config_table);
> +}

Those are sooo similar, it's like the macro SHOW writes itself.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 05/12] efi: export more efi table variable to sysfs

[Dave Young]

On 11/26/13 at 09:50am, Borislav Petkov wrote:
> On Tue, Nov 26, 2013 at 01:57:50PM +0800, Dave Young wrote:
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 2e2fbde..5d85956 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -32,6 +32,9 @@ struct efi __read_mostly efi = {
> >  	.hcdp       = EFI_INVALID_TABLE_ADDR,
> >  	.uga        = EFI_INVALID_TABLE_ADDR,
> >  	.uv_systab  = EFI_INVALID_TABLE_ADDR,
> > +	.fw_vendor  = EFI_INVALID_TABLE_ADDR,
> > +	.runtime    = EFI_INVALID_TABLE_ADDR,
> > +	.config_table  = EFI_INVALID_TABLE_ADDR,
> 
> See what I mean with alignment?

Yes, I see. As for this segment I would not like change the previous code
just because of this reason

> 
> Now this config_table thing sticks out and completely destroys the
> balance in the universe.
> 
> :-)
> 
> >  };
> >  EXPORT_SYMBOL(efi);
> >  
> > @@ -71,13 +74,54 @@ static ssize_t systab_show(struct kobject *kobj,
> >  static struct kobj_attribute efi_attr_systab =
> >  			__ATTR(systab, 0400, systab_show, NULL);
> >  
> > +static ssize_t fw_vendor_show(struct kobject *kobj,
> > +				struct kobj_attribute *attr, char *buf)
> > +{
> > +	return sprintf(buf, "0x%lx\n", efi.fw_vendor);
> > +}
> > +
> > +static ssize_t runtime_show(struct kobject *kobj,
> > +				struct kobj_attribute *attr, char *buf)
> > +{
> > +	return sprintf(buf, "0x%lx\n", efi.runtime);
> > +}
> > +
> > +static ssize_t config_table_show(struct kobject *kobj,
> > +				struct kobj_attribute *attr, char *buf)
> > +{
> > +	return sprintf(buf, "0x%lx\n", efi.config_table);
> > +}
> 
> Those are sooo similar, it's like the macro SHOW writes itself.

Ok, will add a macro for the above functions..

Thanks for review
Dave

Re: [PATCH v4 05/12] efi: export more efi table variable to sysfs

[Matt Fleming]

On Tue, 26 Nov, at 01:57:50PM, Dave Young wrote:
> Export fw_vendor, runtime and config tables physical
> addresses to /sys/firmware/efi/systab becaue kexec
> kernel will need them.
 
This commit log needs updating.

> From EFI spec these 3 variables will be updated to
> virtual address after entering virtual mode. But
> kernel startup code will need the physical address.
> 
> changelog:
> Greg: add standalone sysfs files instead of add lines to systab
> Document them as testing ABI
> Greg: use group attrs and is_visible
> Boris: align comments lines
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-firmware-efi | 26 ++++++++++++++++
>  arch/x86/platform/efi/efi.c                  |  4 +++
>  drivers/firmware/efi/efi.c                   | 44 ++++++++++++++++++++++++++++
>  include/linux/efi.h                          |  3 ++
>  4 files changed, 77 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> new file mode 100644
> index 0000000..9252a24
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -0,0 +1,26 @@
> +What:		/sys/firmware/efi/fw_vendor
> +Date:		Nov 2013
> +Contact:	Dave Young <dyoung-8RVElS0ezzQdnm+yROfE0A@public.gmane.org>

redhat.org eh? Typo or legit?

> +Description:
> +		Shows the physical address of firmware vendor in the EFI
> +		system table.

It shows the physical address of the firmware vendor "entry" or "field"
in the EFI system table.

> +Users:
> +		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>

The "Kexec mailing list" isn't a user, kexec is.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 05/12] efi: export more efi table variable to sysfs

[Dave Young]

On 11/26/13 at 03:57pm, Matt Fleming wrote:
> On Tue, 26 Nov, at 01:57:50PM, Dave Young wrote:
> > Export fw_vendor, runtime and config tables physical
> > addresses to /sys/firmware/efi/systab becaue kexec
> > kernel will need them.
>  
> This commit log needs updating.

Will update

> 
> > From EFI spec these 3 variables will be updated to
> > virtual address after entering virtual mode. But
> > kernel startup code will need the physical address.
> > 
> > changelog:
> > Greg: add standalone sysfs files instead of add lines to systab
> > Document them as testing ABI
> > Greg: use group attrs and is_visible
> > Boris: align comments lines
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  Documentation/ABI/testing/sysfs-firmware-efi | 26 ++++++++++++++++
> >  arch/x86/platform/efi/efi.c                  |  4 +++
> >  drivers/firmware/efi/efi.c                   | 44 ++++++++++++++++++++++++++++
> >  include/linux/efi.h                          |  3 ++
> >  4 files changed, 77 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> > new file mode 100644
> > index 0000000..9252a24
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> > @@ -0,0 +1,26 @@
> > +What:		/sys/firmware/efi/fw_vendor
> > +Date:		Nov 2013
> > +Contact:	Dave Young <dyoung-8RVElS0ezzQdnm+yROfE0A@public.gmane.org>
> 
> redhat.org eh? Typo or legit?

It's typo, will fix

> 
> > +Description:
> > +		Shows the physical address of firmware vendor in the EFI
> > +		system table.
> 
> It shows the physical address of the firmware vendor "entry" or "field"
> in the EFI system table.

Will update

> 
> > +Users:
> > +		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
> 
> The "Kexec mailing list" isn't a user, kexec is.

Ok, will change to "Kexec"

Thanks for review
Dave

[PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

kexec kernel will need exactly same mapping for
efi runtime memory ranges. Thus here export the
runtime ranges mapping to sysfs, kexec-tools
will assemble them and pass to 2nd kernel via
setup_data.

Introducing a new directly /sys/firmware/efi/runtime-map
Just like /sys/firmware/memmap. Containing below attribute
in each file of that directory:
attribute  num_pages  phys_addr  type  virt_addr

It will not work for efi 32bit. Only x86_64 currently.

Matt: s/efi-runtime-map.c/runtime-map.c
      change dir name to runtime-map
update to use desc_size in efi_runtime_map
cleaup the code, add function efi_save_runtime_map
improve err handling

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../ABI/testing/sysfs-firmware-efi-runtime-map     |  45 +++++
 arch/x86/platform/efi/efi.c                        |  26 +++
 drivers/firmware/efi/Kconfig                       |  10 ++
 drivers/firmware/efi/Makefile                      |   1 +
 drivers/firmware/efi/efi.c                         |   3 +-
 drivers/firmware/efi/runtime-map.c                 | 199 +++++++++++++++++++++
 include/linux/efi.h                                |   6 +
 7 files changed, 289 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
 create mode 100644 drivers/firmware/efi/runtime-map.c

diff --git a/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
new file mode 100644
index 0000000..dab8d41
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
@@ -0,0 +1,45 @@
+What:		/sys/firmware/efi/runtime-map/
+Date:		Oct 2013
+Contact:	Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+Description:
+		Switching efi runtime services to virtual mode requires
+		that all efi memory ranges which has the runtime attribute
+		bit set to be mapped to virtual addresses.
+
+		In kexec kernel kernel can not entering virtual mode again
+		because there's a limitation that SetVirtualAddressMap can
+		only be called once for entering virtual mode. But kexec
+		kernel still need maintain same physical address to virtual
+		address mapping as the 1st kernel. The mappings are exported
+		to sysfs so userspace tools can reassemble them and pass them
+		into kexec kernel.
+
+		/sys/firmware/efi/runtim-map/ is what kernel export for
+		this purpose. The structure is as follows:
+
+		subdirectories are named with the number of the memory range:
+
+			/sys/firmware/efi/runtime-map/0
+			/sys/firmware/efi/runtime-map/1
+			/sys/firmware/efi/runtime-map/2
+			/sys/firmware/efi/runtime-map/3
+			...
+
+		Each subdirectory contains five files:
+
+		attribute : The attribute of the memory range.
+		num_pages : The size of the memory range in page number.
+		phys_addr : The start physical address of the memory range.
+		type      : The type of the memory range.
+		virt_addr : The start virtual address of the memory range.
+
+		Above values are all hexadecimal number with the '0x' prefix.
+
+		So, for example:
+
+			/sys/firmware/efi/runtime-map/0/attribute
+			/sys/firmware/efi/runtime-map/0/num_pages
+			/sys/firmware/efi/runtime-map/0/phys_addr
+			/sys/firmware/efi/runtime-map/0/type
+			/sys/firmware/efi/runtime-map/0/virt_addr
+                        ...
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 2a292aa..c3a2aaa 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -76,6 +76,9 @@ static __initdata efi_config_table_type_t arch_tables[] = {
 	{NULL_GUID, NULL, NULL},
 };
 
+void *efi_runtime_map;
+int nr_efi_runtime_map;
+
 /*
  * Returns 1 if 'facility' is enabled, 0 otherwise.
  */
@@ -811,6 +814,21 @@ static void __init efi_merge_regions(void)
 	}
 }
 
+static int __init efi_save_runtime_map(efi_memory_desc_t *md)
+{
+	efi_runtime_map = krealloc(efi_runtime_map,
+			(nr_efi_runtime_map + 1) *
+			memmap.desc_size, GFP_KERNEL);
+	if (!efi_runtime_map)
+		return -ENOMEM;
+
+	memcpy(efi_runtime_map + nr_efi_runtime_map * memmap.desc_size,
+		md, memmap.desc_size);
+	nr_efi_runtime_map++;
+
+	return 0;
+}
+
 /*
  * Map efi memory ranges for runtime serivce and update new_memmap with virtual
  * addresses.
@@ -821,6 +839,7 @@ static void * __init efi_map_regions(int *count)
 	void *p, *new_memmap = NULL;
 	unsigned long size;
 	u64 end, systab;
+	int error = 0;
 
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
@@ -852,9 +871,16 @@ static void * __init efi_map_regions(int *count)
 
 		memcpy(new_memmap + (*count * memmap.desc_size), md,
 		       memmap.desc_size);
+		if (!error && md->type != EFI_BOOT_SERVICES_CODE &&
+				md->type != EFI_BOOT_SERVICES_DATA)
+			error = efi_save_runtime_map(md);
 		(*count)++;
 	}
 
+	if (error) {
+		nr_efi_runtime_map = 0;
+		kfree(efi_runtime_map);
+	}
 ret:
 	return new_memmap;
 }
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index b0fc7c7..b4d01ad 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -36,4 +36,14 @@ config EFI_VARS_PSTORE_DEFAULT_DISABLE
 	  backend for pstore by default. This setting can be overridden
 	  using the efivars module's pstore_disable parameter.
 
+config EFI_RUNTIME_MAP
+	bool "Export efi runtime maps to sysfs" if EXPERT
+	default X86 && EFI
+	help
+	  Export efi runtime memory maps to /sys/firmware/efi/runtime-map.
+	  That memory map is used for example by kexec to set up efi virtual
+	  mapping the 2nd kernel, but can also be used for debugging purposes.
+
+	  See also Documentation/ABI/testing/sysfs-firmware-efi-runtime-map.
+
 endmenu
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 99245ab..ecadcc1 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -4,3 +4,4 @@
 obj-y					+= efi.o vars.o
 obj-$(CONFIG_EFI_VARS)			+= efivars.o
 obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
+obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5d85956..a480de8 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -38,7 +38,8 @@ struct efi __read_mostly efi = {
 };
 EXPORT_SYMBOL(efi);
 
-static struct kobject *efi_kobj;
+struct kobject *efi_kobj;
+EXPORT_SYMBOL_GPL(efi_kobj);
 static struct kobject *efivars_kobj;
 
 /*
diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
new file mode 100644
index 0000000..036d099
--- /dev/null
+++ b/drivers/firmware/efi/runtime-map.c
@@ -0,0 +1,199 @@
+/*
+ * linux/drivers/efi/runtime-map.c
+ * Copyright (C) 2013 Red Hat, Inc., Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License v2.0 as published by
+ * the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/string.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/efi.h>
+#include <linux/slab.h>
+
+#include <asm/setup.h>
+
+struct efi_runtime_map_entry {
+	efi_memory_desc_t md;
+	struct kobject kobj;   /* kobject for each entry */
+};
+
+static struct efi_runtime_map_entry **map_entries;
+
+static ssize_t map_attr_show(struct kobject *kobj,
+				struct attribute *attr, char *buf);
+static ssize_t type_show(struct efi_runtime_map_entry *entry, char *buf);
+static ssize_t phys_addr_show(struct efi_runtime_map_entry *entry, char *buf);
+static ssize_t virt_addr_show(struct efi_runtime_map_entry *entry, char *buf);
+static ssize_t num_pages_show(struct efi_runtime_map_entry *entry, char *buf);
+static ssize_t attribute_show(struct efi_runtime_map_entry *entry, char *buf);
+
+struct map_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct efi_runtime_map_entry *entry, char *buf);
+};
+
+static struct map_attribute map_type_attr = __ATTR_RO(type);
+static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
+static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
+static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
+static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
+
+/*
+ * These are default attributes that are added for every memmap entry.
+ */
+static struct attribute *def_attrs[] = {
+	&map_type_attr.attr,
+	&map_phys_addr_attr.attr,
+	&map_virt_addr_attr.attr,
+	&map_num_pages_attr.attr,
+	&map_attribute_attr.attr,
+	NULL
+};
+
+static const struct sysfs_ops map_attr_ops = {
+	.show = map_attr_show,
+};
+
+static inline struct efi_runtime_map_entry *to_map_entry(struct kobject *kobj)
+{
+	return container_of(kobj, struct efi_runtime_map_entry, kobj);
+}
+
+static void map_release(struct kobject *kobj)
+{
+	struct efi_runtime_map_entry *entry;
+
+	entry = to_map_entry(kobj);
+	kfree(entry);
+}
+
+static struct kobj_type __refdata map_ktype = {
+	.sysfs_ops	= &map_attr_ops,
+	.default_attrs	= def_attrs,
+	.release	= map_release,
+};
+
+static ssize_t type_show(struct efi_runtime_map_entry *entry, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0x%x\n", entry->md.type);
+}
+
+static ssize_t phys_addr_show(struct efi_runtime_map_entry *entry, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.phys_addr);
+}
+
+static ssize_t virt_addr_show(struct efi_runtime_map_entry *entry, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.virt_addr);
+}
+
+static ssize_t num_pages_show(struct efi_runtime_map_entry *entry, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.num_pages);
+}
+
+static ssize_t attribute_show(struct efi_runtime_map_entry *entry, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.attribute);
+}
+
+static inline struct map_attribute *to_map_attr(struct attribute *attr)
+{
+	return container_of(attr, struct map_attribute, attr);
+}
+
+static ssize_t map_attr_show(struct kobject *kobj,
+				struct attribute *attr, char *buf)
+{
+	struct efi_runtime_map_entry *entry = to_map_entry(kobj);
+	struct map_attribute *map_attr = to_map_attr(attr);
+
+	return map_attr->show(entry, buf);
+}
+
+static struct kset *map_kset;
+
+/*
+ * Add map entry on sysfs
+ */
+static struct efi_runtime_map_entry *add_sysfs_runtime_map_entry(int nr)
+{
+	int ret;
+	unsigned long desc_size;
+	struct efi_runtime_map_entry *entry;
+	struct efi_info *e = &boot_params.efi_info;
+
+	if (!map_kset) {
+		map_kset = kset_create_and_add("runtime-map", NULL,
+				efi_kobj);
+		if (!map_kset)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry) {
+		kset_unregister(map_kset);
+		return entry;
+	}
+
+	desc_size = e->efi_memdesc_size;
+	memcpy(&entry->md, efi_runtime_map + nr * desc_size,
+		sizeof(efi_memory_desc_t));
+
+	kobject_init(&entry->kobj, &map_ktype);
+	entry->kobj.kset = map_kset;
+	ret = kobject_add(&entry->kobj, NULL, "%d", nr);
+	if (ret) {
+		kobject_put(&entry->kobj);
+		kset_unregister(map_kset);
+		return ERR_PTR(ret);
+	}
+
+	return entry;
+}
+
+static int __init efi_runtime_map_init(void)
+{
+	int i, j, ret = 0;
+	struct efi_runtime_map_entry *entry;
+
+	if (!efi_runtime_map) {
+		pr_warn("no efi_runtime_map found\n");
+		return -EINVAL;
+	}
+
+	map_entries = kzalloc(nr_efi_runtime_map *
+		sizeof(struct efi_runtime_map_entry *), GFP_KERNEL);
+	if (!map_entries)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_efi_runtime_map; i++) {
+		entry = add_sysfs_runtime_map_entry(i);
+		if (IS_ERR(entry)) {
+			for (j = i - 1; j > 0; j--) {
+				entry = *(map_entries + j);
+				kobject_put(&entry->kobj);
+			}
+			if (map_kset)
+				kset_unregister(map_kset);
+			ret = PTR_ERR(entry);
+			goto out;
+		}
+		*(map_entries + i) = entry;
+	}
+
+out:
+	return ret;
+}
+late_initcall(efi_runtime_map_init);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index fb60b10..4f1651d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -872,4 +872,10 @@ int efivars_sysfs_init(void);
 
 #endif /* CONFIG_EFI_VARS */
 
+#ifdef CONFIG_EFI_RUNTIME_MAP
+extern void *efi_runtime_map;
+extern int nr_efi_runtime_map;
+extern struct kobject *efi_kobj;
+#endif
+
 #endif /* _LINUX_EFI_H */
-- 
1.8.3.1

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Matt Fleming]

On Tue, 26 Nov, at 01:57:51PM, Dave Young wrote:
> kexec kernel will need exactly same mapping for
> efi runtime memory ranges. Thus here export the
> runtime ranges mapping to sysfs, kexec-tools
> will assemble them and pass to 2nd kernel via
> setup_data.
> 
> Introducing a new directly /sys/firmware/efi/runtime-map

I'm not sure why the word "directly" is used here?

> Just like /sys/firmware/memmap. Containing below attribute
> in each file of that directory:
> attribute  num_pages  phys_addr  type  virt_addr
> 
> It will not work for efi 32bit. Only x86_64 currently.
 
Actually, exporting the tables does work for 32-bit, right? It's just
that kexec doesn't work for 32-bit EFI?

> Matt: s/efi-runtime-map.c/runtime-map.c
>       change dir name to runtime-map
> update to use desc_size in efi_runtime_map
> cleaup the code, add function efi_save_runtime_map
> improve err handling
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  .../ABI/testing/sysfs-firmware-efi-runtime-map     |  45 +++++
>  arch/x86/platform/efi/efi.c                        |  26 +++
>  drivers/firmware/efi/Kconfig                       |  10 ++
>  drivers/firmware/efi/Makefile                      |   1 +
>  drivers/firmware/efi/efi.c                         |   3 +-
>  drivers/firmware/efi/runtime-map.c                 | 199 +++++++++++++++++++++
>  include/linux/efi.h                                |   6 +
>  7 files changed, 289 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
>  create mode 100644 drivers/firmware/efi/runtime-map.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> new file mode 100644
> index 0000000..dab8d41
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> @@ -0,0 +1,45 @@
> +What:		/sys/firmware/efi/runtime-map/
> +Date:		Oct 2013
> +Contact:	Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> +Description:
> +		Switching efi runtime services to virtual mode requires
> +		that all efi memory ranges which has the runtime attribute
> +		bit set to be mapped to virtual addresses.
> +
> +		In kexec kernel kernel can not entering virtual mode again
> +		because there's a limitation that SetVirtualAddressMap can
> +		only be called once for entering virtual mode. But kexec
> +		kernel still need maintain same physical address to virtual
> +		address mapping as the 1st kernel. The mappings are exported
> +		to sysfs so userspace tools can reassemble them and pass them
> +		into kexec kernel.

How about,

		"The efi runtime services can only be switched to virtual
		 mode once without rebooting. The kexec kernel must maintain
		 the same physical to virtual address mappings as the first
		 kernel. The mappings are exported to sysfs so userspace tools
		 can reassemble them and pass them into the kexec kernel."

?

> +		/sys/firmware/efi/runtim-map/ is what kernel export for

				^^ runtime-map/

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

On 11/26/13 at 07:30pm, Matt Fleming wrote:
> On Tue, 26 Nov, at 01:57:51PM, Dave Young wrote:
> > kexec kernel will need exactly same mapping for
> > efi runtime memory ranges. Thus here export the
> > runtime ranges mapping to sysfs, kexec-tools
> > will assemble them and pass to 2nd kernel via
> > setup_data.
> > 
> > Introducing a new directly /sys/firmware/efi/runtime-map
> 
> I'm not sure why the word "directly" is used here?

should be directory, will fix.

> 
> > Just like /sys/firmware/memmap. Containing below attribute
> > in each file of that directory:
> > attribute  num_pages  phys_addr  type  virt_addr
> > 
> > It will not work for efi 32bit. Only x86_64 currently.
>  
> Actually, exporting the tables does work for 32-bit, right? It's just
> that kexec doesn't work for 32-bit EFI?

It does not make sense to export them for 32-bit because we do not
support kexec efi on 32-bit. Will change the description to below:

"kexec efi boot on 32-bit is not supported so I only export the tables
for 64-bit efi system."

> 
> > Matt: s/efi-runtime-map.c/runtime-map.c
> >       change dir name to runtime-map
> > update to use desc_size in efi_runtime_map
> > cleaup the code, add function efi_save_runtime_map
> > improve err handling
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../ABI/testing/sysfs-firmware-efi-runtime-map     |  45 +++++
> >  arch/x86/platform/efi/efi.c                        |  26 +++
> >  drivers/firmware/efi/Kconfig                       |  10 ++
> >  drivers/firmware/efi/Makefile                      |   1 +
> >  drivers/firmware/efi/efi.c                         |   3 +-
> >  drivers/firmware/efi/runtime-map.c                 | 199 +++++++++++++++++++++
> >  include/linux/efi.h                                |   6 +
> >  7 files changed, 289 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> >  create mode 100644 drivers/firmware/efi/runtime-map.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> > new file mode 100644
> > index 0000000..dab8d41
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> > @@ -0,0 +1,45 @@
> > +What:		/sys/firmware/efi/runtime-map/
> > +Date:		Oct 2013
> > +Contact:	Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > +Description:
> > +		Switching efi runtime services to virtual mode requires
> > +		that all efi memory ranges which has the runtime attribute
> > +		bit set to be mapped to virtual addresses.
> > +
> > +		In kexec kernel kernel can not entering virtual mode again
> > +		because there's a limitation that SetVirtualAddressMap can
> > +		only be called once for entering virtual mode. But kexec
> > +		kernel still need maintain same physical address to virtual
> > +		address mapping as the 1st kernel. The mappings are exported
> > +		to sysfs so userspace tools can reassemble them and pass them
> > +		into kexec kernel.
> 
> How about,
> 
> 		"The efi runtime services can only be switched to virtual
> 		 mode once without rebooting. The kexec kernel must maintain
> 		 the same physical to virtual address mappings as the first
> 		 kernel. The mappings are exported to sysfs so userspace tools
> 		 can reassemble them and pass them into the kexec kernel."
> 
> ?

Excellent, will above.

> 
> > +		/sys/firmware/efi/runtim-map/ is what kernel export for
> 
> 				^^ runtime-map/

Will fix the typo.

Thanks for review
Dave

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

On 11/27/13 at 11:07am, Dave Young wrote:
> > > It will not work for efi 32bit. Only x86_64 currently.
> >  
> > Actually, exporting the tables does work for 32-bit, right? It's just
> > that kexec doesn't work for 32-bit EFI?
> 
> It does not make sense to export them for 32-bit because we do not
> support kexec efi on 32-bit. Will change the description to below:
> 
> "kexec efi boot on 32-bit is not supported so I only export the tables
> for 64-bit efi system."

I thought about only enabling it for x86_64 and efi is in .config.
Rethinking about this, since exporting code will also work for 32-bit so I
will remove the lines you mentioned.

Thanks
Dave

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Borislav Petkov]

On Tue, Nov 26, 2013 at 01:57:51PM +0800, Dave Young wrote:
> kexec kernel will need exactly same mapping for
> efi runtime memory ranges. Thus here export the
> runtime ranges mapping to sysfs, kexec-tools
> will assemble them and pass to 2nd kernel via
> setup_data.
> 
> Introducing a new directly /sys/firmware/efi/runtime-map
> Just like /sys/firmware/memmap. Containing below attribute
> in each file of that directory:
> attribute  num_pages  phys_addr  type  virt_addr
> 
> It will not work for efi 32bit. Only x86_64 currently.
> 
> Matt: s/efi-runtime-map.c/runtime-map.c
>       change dir name to runtime-map
> update to use desc_size in efi_runtime_map
> cleaup the code, add function efi_save_runtime_map
> improve err handling
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  .../ABI/testing/sysfs-firmware-efi-runtime-map     |  45 +++++
>  arch/x86/platform/efi/efi.c                        |  26 +++
>  drivers/firmware/efi/Kconfig                       |  10 ++
>  drivers/firmware/efi/Makefile                      |   1 +
>  drivers/firmware/efi/efi.c                         |   3 +-
>  drivers/firmware/efi/runtime-map.c                 | 199 +++++++++++++++++++++
>  include/linux/efi.h                                |   6 +
>  7 files changed, 289 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
>  create mode 100644 drivers/firmware/efi/runtime-map.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> new file mode 100644
> index 0000000..dab8d41
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> @@ -0,0 +1,45 @@
> +What:		/sys/firmware/efi/runtime-map/
> +Date:		Oct 2013
> +Contact:	Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> +Description:
> +		Switching efi runtime services to virtual mode requires
> +		that all efi memory ranges which has the runtime attribute

s/has/have/

> +		bit set to be mapped to virtual addresses.
> +
> +		In kexec kernel kernel can not entering virtual mode again
> +		because there's a limitation that SetVirtualAddressMap can
> +		only be called once for entering virtual mode. But kexec
> +		kernel still need maintain same physical address to virtual
> +		address mapping as the 1st kernel.

Simply say

"SetVirtualAddressMap has the limitation that it can be called only once
per system lifetime so a second kernel, like kexec, needs to reuse the
exact same virtual addresses like the first kernel."


> 		The mappings are exported
> +		to sysfs so userspace tools can reassemble them and pass them
> +		into kexec kernel.
> +
> +		/sys/firmware/efi/runtim-map/ is what kernel export for
> +		this purpose. The structure is as follows:

"/sys/firmware/efi/runtime-map/ is the directory the kernel exports that
information in."

> +
> +		subdirectories are named with the number of the memory range:
> +
> +			/sys/firmware/efi/runtime-map/0
> +			/sys/firmware/efi/runtime-map/1
> +			/sys/firmware/efi/runtime-map/2
> +			/sys/firmware/efi/runtime-map/3
> +			...
> +
> +		Each subdirectory contains five files:
> +
> +		attribute : The attribute of the memory range.

				attributes

> +		num_pages : The size of the memory range in page number.

							 in 4K pages.

> +		phys_addr : The start physical address of the memory range.

			    The physical address of the memory range

> +		type      : The type of the memory range.
> +		virt_addr : The start virtual address of the memory range.

			    The virtual address...

> +
> +		Above values are all hexadecimal number with the '0x' prefix.

						 numbers

> +
> +		So, for example:
> +
> +			/sys/firmware/efi/runtime-map/0/attribute
> +			/sys/firmware/efi/runtime-map/0/num_pages
> +			/sys/firmware/efi/runtime-map/0/phys_addr
> +			/sys/firmware/efi/runtime-map/0/type
> +			/sys/firmware/efi/runtime-map/0/virt_addr
> +                        ...

What is this example supposed to show? The above is already detailed
enough.

> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 2a292aa..c3a2aaa 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -76,6 +76,9 @@ static __initdata efi_config_table_type_t arch_tables[] = {
>  	{NULL_GUID, NULL, NULL},
>  };
>  
> +void *efi_runtime_map;
> +int nr_efi_runtime_map;
> +
>  /*
>   * Returns 1 if 'facility' is enabled, 0 otherwise.
>   */
> @@ -811,6 +814,21 @@ static void __init efi_merge_regions(void)
>  	}
>  }
>  
> +static int __init efi_save_runtime_map(efi_memory_desc_t *md)

It is a static function so no need for the "efi_" prefix.

Also, it should be called save_runtime_mem_desc or so because this is
what it does.

> +{
> +	efi_runtime_map = krealloc(efi_runtime_map,
> +			(nr_efi_runtime_map + 1) *
> +			memmap.desc_size, GFP_KERNEL);

WARNING: Reusing the krealloc arg is almost always a bug
#21: FILE: arch/x86/platform/efi/efi.c:819:
+       efi_runtime_map = krealloc(efi_runtime_map,

Please run your diffs through checkpatch.pl --strict before submitting.

Btw, we use krealloc below again which should be changed too.

> +	if (!efi_runtime_map)
> +		return -ENOMEM;
> +
> +	memcpy(efi_runtime_map + nr_efi_runtime_map * memmap.desc_size,
> +		md, memmap.desc_size);
> +	nr_efi_runtime_map++;
> +
> +	return 0;
> +}
> +
>  /*
>   * Map efi memory ranges for runtime serivce and update new_memmap with virtual
>   * addresses.
> @@ -821,6 +839,7 @@ static void * __init efi_map_regions(int *count)
>  	void *p, *new_memmap = NULL;
>  	unsigned long size;
>  	u64 end, systab;
> +	int error = 0;
>  
>  	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
>  		md = p;
> @@ -852,9 +871,16 @@ static void * __init efi_map_regions(int *count)
>  
>  		memcpy(new_memmap + (*count * memmap.desc_size), md,
>  		       memmap.desc_size);
> +		if (!error && md->type != EFI_BOOT_SERVICES_CODE &&
> +				md->type != EFI_BOOT_SERVICES_DATA)
> +			error = efi_save_runtime_map(md);

I'm not sure I like this entangled stuff with the error code - I can
barely understand what's going on there - not necessarily because of
your changes but because this code is not that trivial, especially after
time has passed.

And I don't see you kfreeing efi_runtime_map in the case where the first
krealloc() fails at some iteration.

What would be much cleaner IMO would be if you let efi_map_regions()
finish, *and*, *only* if it was successful iterate again over the
regions and do efi_save_runtime_map() on the runtime ones. This will
make the code much more readable too.

>  		(*count)++;
>  	}
>  
> +	if (error) {
> +		nr_efi_runtime_map = 0;
> +		kfree(efi_runtime_map);
> +	}

This error gets overwritten each time the loop runs and you possibly
lose !0 values. What I'm suggesting above would take care of that case
too.

>  ret:
>  	return new_memmap;
>  }
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index b0fc7c7..b4d01ad 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -36,4 +36,14 @@ config EFI_VARS_PSTORE_DEFAULT_DISABLE
>  	  backend for pstore by default. This setting can be overridden
>  	  using the efivars module's pstore_disable parameter.
>  
> +config EFI_RUNTIME_MAP
> +	bool "Export efi runtime maps to sysfs" if EXPERT
> +	default X86 && EFI

This should depend on KEXEC.

> +	help
> +	  Export efi runtime memory maps to /sys/firmware/efi/runtime-map.
> +	  That memory map is used for example by kexec to set up efi virtual
> +	  mapping the 2nd kernel, but can also be used for debugging purposes.
> +
> +	  See also Documentation/ABI/testing/sysfs-firmware-efi-runtime-map.
> +
>  endmenu
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 99245ab..ecadcc1 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -4,3 +4,4 @@
>  obj-y					+= efi.o vars.o
>  obj-$(CONFIG_EFI_VARS)			+= efivars.o
>  obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
> +obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o

This will conflict with upstream:

checking file drivers/firmware/efi/Makefile
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED

It should be:

diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 9ba156d3c775..7f05c368b868 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -5,3 +5,4 @@ obj-y                                   += efi.o vars.o
 obj-$(CONFIG_EFI_VARS)                 += efivars.o
 obj-$(CONFIG_EFI_VARS_PSTORE)          += efi-pstore.o
 obj-$(CONFIG_UEFI_CPER)                        += cper.o
+obj-$(CONFIG_EFI_RUNTIME_MAP)          += runtime-map.o


> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5d85956..a480de8 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -38,7 +38,8 @@ struct efi __read_mostly efi = {
>  };
>  EXPORT_SYMBOL(efi);
>  
> -static struct kobject *efi_kobj;
> +struct kobject *efi_kobj;
> +EXPORT_SYMBOL_GPL(efi_kobj);

Can we move this private to linux/drivers/efi/runtime-map.c? If yes,
then there's no need to export it.

>  static struct kobject *efivars_kobj;
>  
>  /*
> diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> new file mode 100644
> index 0000000..036d099
> --- /dev/null
> +++ b/drivers/firmware/efi/runtime-map.c
> @@ -0,0 +1,199 @@
> +/*
> + * linux/drivers/efi/runtime-map.c
> + * Copyright (C) 2013 Red Hat, Inc., Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2.0 as published by
> + * the Free Software Foundation
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

Please drop this boilerplate text and do a single sentence saying that
it is licensed under GPL v2 and refer to COPYING in the kernel repo.

> + *
> + */
> +
> +#include <linux/string.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/efi.h>
> +#include <linux/slab.h>
> +
> +#include <asm/setup.h>
> +
> +struct efi_runtime_map_entry {
> +	efi_memory_desc_t md;
> +	struct kobject kobj;   /* kobject for each entry */
> +};
> +
> +static struct efi_runtime_map_entry **map_entries;
> +
> +static ssize_t map_attr_show(struct kobject *kobj,
> +				struct attribute *attr, char *buf);
> +static ssize_t type_show(struct efi_runtime_map_entry *entry, char *buf);
> +static ssize_t phys_addr_show(struct efi_runtime_map_entry *entry, char *buf);
> +static ssize_t virt_addr_show(struct efi_runtime_map_entry *entry, char *buf);
> +static ssize_t num_pages_show(struct efi_runtime_map_entry *entry, char *buf);
> +static ssize_t attribute_show(struct efi_runtime_map_entry *entry, char *buf);
> +
> +struct map_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct efi_runtime_map_entry *entry, char *buf);
> +};
> +
> +static struct map_attribute map_type_attr = __ATTR_RO(type);
> +static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> +static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> +static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> +static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> +
> +/*
> + * These are default attributes that are added for every memmap entry.
> + */
> +static struct attribute *def_attrs[] = {
> +	&map_type_attr.attr,
> +	&map_phys_addr_attr.attr,
> +	&map_virt_addr_attr.attr,
> +	&map_num_pages_attr.attr,
> +	&map_attribute_attr.attr,
> +	NULL
> +};
> +
> +static const struct sysfs_ops map_attr_ops = {
> +	.show = map_attr_show,
> +};
> +
> +static inline struct efi_runtime_map_entry *to_map_entry(struct kobject *kobj)
> +{
> +	return container_of(kobj, struct efi_runtime_map_entry, kobj);
> +}
> +
> +static void map_release(struct kobject *kobj)
> +{
> +	struct efi_runtime_map_entry *entry;
> +
> +	entry = to_map_entry(kobj);
> +	kfree(entry);
> +}
> +
> +static struct kobj_type __refdata map_ktype = {
> +	.sysfs_ops	= &map_attr_ops,
> +	.default_attrs	= def_attrs,
> +	.release	= map_release,
> +};
> +
> +static ssize_t type_show(struct efi_runtime_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%x\n", entry->md.type);
> +}
> +
> +static ssize_t phys_addr_show(struct efi_runtime_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.phys_addr);
> +}
> +
> +static ssize_t virt_addr_show(struct efi_runtime_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.virt_addr);
> +}
> +
> +static ssize_t num_pages_show(struct efi_runtime_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.num_pages);
> +}
> +
> +static ssize_t attribute_show(struct efi_runtime_map_entry *entry, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.attribute);
> +}

I can see the macro here writing itself too :)

> +
> +static inline struct map_attribute *to_map_attr(struct attribute *attr)
> +{
> +	return container_of(attr, struct map_attribute, attr);
> +}
> +
> +static ssize_t map_attr_show(struct kobject *kobj,
> +				struct attribute *attr, char *buf)
> +{
> +	struct efi_runtime_map_entry *entry = to_map_entry(kobj);
> +	struct map_attribute *map_attr = to_map_attr(attr);
> +
> +	return map_attr->show(entry, buf);
> +}
> +
> +static struct kset *map_kset;
> +
> +/*
> + * Add map entry on sysfs
> + */

The function name should be enough - no need for the comment.

> +static struct efi_runtime_map_entry *add_sysfs_runtime_map_entry(int nr)
> +{
> +	int ret;
> +	unsigned long desc_size;
> +	struct efi_runtime_map_entry *entry;
> +	struct efi_info *e = &boot_params.efi_info;
> +
> +	if (!map_kset) {
> +		map_kset = kset_create_and_add("runtime-map", NULL,
> +				efi_kobj);
> +		if (!map_kset)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry) {
> +		kset_unregister(map_kset);
> +		return entry;
> +	}
> +
> +	desc_size = e->efi_memdesc_size;
> +	memcpy(&entry->md, efi_runtime_map + nr * desc_size,
> +		sizeof(efi_memory_desc_t));

If you're going to break this line anyway, you don't really need the
local desc_size variable:

	memcpy(&entry->md,
	       efi_runtime_map + nr * e->efi_memdesc_size,
	       sizeof(efi_memory_desc_t));

> +
> +	kobject_init(&entry->kobj, &map_ktype);
> +	entry->kobj.kset = map_kset;
> +	ret = kobject_add(&entry->kobj, NULL, "%d", nr);
> +	if (ret) {
> +		kobject_put(&entry->kobj);
> +		kset_unregister(map_kset);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return entry;
> +}
> +
> +static int __init efi_runtime_map_init(void)
> +{
> +	int i, j, ret = 0;
> +	struct efi_runtime_map_entry *entry;
> +
> +	if (!efi_runtime_map) {
> +		pr_warn("no efi_runtime_map found\n");
> +		return -EINVAL;
> +	}
> +
> +	map_entries = kzalloc(nr_efi_runtime_map *
> +		sizeof(struct efi_runtime_map_entry *), GFP_KERNEL);

Shorten:

	map_entries = kzalloc(nr_efi_runtime_map * sizeof(entry), GFP_KERNEL);

> +	if (!map_entries)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr_efi_runtime_map; i++) {
> +		entry = add_sysfs_runtime_map_entry(i);
> +		if (IS_ERR(entry)) {
> +			for (j = i - 1; j > 0; j--) {
> +				entry = *(map_entries + j);
> +				kobject_put(&entry->kobj);
> +			}
> +			if (map_kset)
> +				kset_unregister(map_kset);
> +			ret = PTR_ERR(entry);
> +			goto out;
> +		}

You can move this error unwinding path away from the main loop:

	if (IS_ERR(entry))
		goto out;

and make it more readable.

> +		*(map_entries + i) = entry;
> +	}
> +
> +out:
> +	return ret;
> +}
> +late_initcall(efi_runtime_map_init);

Why an initcall? Can't we run this from efisubsys_init()?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

On 11/27/13 at 12:44pm, Borislav Petkov wrote:
> On Tue, Nov 26, 2013 at 01:57:51PM +0800, Dave Young wrote:
> > kexec kernel will need exactly same mapping for
> > efi runtime memory ranges. Thus here export the
> > runtime ranges mapping to sysfs, kexec-tools
> > will assemble them and pass to 2nd kernel via
> > setup_data.
> > 
> > Introducing a new directly /sys/firmware/efi/runtime-map
> > Just like /sys/firmware/memmap. Containing below attribute
> > in each file of that directory:
> > attribute  num_pages  phys_addr  type  virt_addr
> > 
> > It will not work for efi 32bit. Only x86_64 currently.
> > 
> > Matt: s/efi-runtime-map.c/runtime-map.c
> >       change dir name to runtime-map
> > update to use desc_size in efi_runtime_map
> > cleaup the code, add function efi_save_runtime_map
> > improve err handling
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../ABI/testing/sysfs-firmware-efi-runtime-map     |  45 +++++
> >  arch/x86/platform/efi/efi.c                        |  26 +++
> >  drivers/firmware/efi/Kconfig                       |  10 ++
> >  drivers/firmware/efi/Makefile                      |   1 +
> >  drivers/firmware/efi/efi.c                         |   3 +-
> >  drivers/firmware/efi/runtime-map.c                 | 199 +++++++++++++++++++++
> >  include/linux/efi.h                                |   6 +
> >  7 files changed, 289 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> >  create mode 100644 drivers/firmware/efi/runtime-map.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> > new file mode 100644
> > index 0000000..dab8d41
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map
> > @@ -0,0 +1,45 @@
> > +What:		/sys/firmware/efi/runtime-map/
> > +Date:		Oct 2013
> > +Contact:	Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > +Description:
> > +		Switching efi runtime services to virtual mode requires
> > +		that all efi memory ranges which has the runtime attribute
> 
> s/has/have/

Will fix

> 
> > +		bit set to be mapped to virtual addresses.
> > +
> > +		In kexec kernel kernel can not entering virtual mode again
> > +		because there's a limitation that SetVirtualAddressMap can
> > +		only be called once for entering virtual mode. But kexec
> > +		kernel still need maintain same physical address to virtual
> > +		address mapping as the 1st kernel.
> 
> Simply say
> 
> "SetVirtualAddressMap has the limitation that it can be called only once
> per system lifetime so a second kernel, like kexec, needs to reuse the
> exact same virtual addresses like the first kernel."
> 

Matt are suggesting to below one, is it ok to you? I'm fine with both.

The efi runtime services can only be switched to virtual
mode once without rebooting. The kexec kernel must maintain
the same physical to virtual address mappings as the first
kernel. The mappings are exported to sysfs so userspace tools
can reassemble them and pass them into the kexec kernel.

> 
> > 		The mappings are exported
> > +		to sysfs so userspace tools can reassemble them and pass them
> > +		into kexec kernel.
> > +
> > +		/sys/firmware/efi/runtim-map/ is what kernel export for
> > +		this purpose. The structure is as follows:
> 
> "/sys/firmware/efi/runtime-map/ is the directory the kernel exports that
> information in."

Ok.

> 
> > +
> > +		subdirectories are named with the number of the memory range:
> > +
> > +			/sys/firmware/efi/runtime-map/0
> > +			/sys/firmware/efi/runtime-map/1
> > +			/sys/firmware/efi/runtime-map/2
> > +			/sys/firmware/efi/runtime-map/3
> > +			...
> > +
> > +		Each subdirectory contains five files:
> > +
> > +		attribute : The attribute of the memory range.
> 
> 				attributes

Ok.

> 
> > +		num_pages : The size of the memory range in page number.
> 
> 							 in 4K pages.

Is it always 4K, how about "in pages"?

> 
> > +		phys_addr : The start physical address of the memory range.
> 
> 			    The physical address of the memory range

Ok.

> 
> > +		type      : The type of the memory range.
> > +		virt_addr : The start virtual address of the memory range.
> 
> 			    The virtual address...

Ok.

> 
> > +
> > +		Above values are all hexadecimal number with the '0x' prefix.
> 
> 						 numbers

Ok.

> 
> > +
> > +		So, for example:
> > +
> > +			/sys/firmware/efi/runtime-map/0/attribute
> > +			/sys/firmware/efi/runtime-map/0/num_pages
> > +			/sys/firmware/efi/runtime-map/0/phys_addr
> > +			/sys/firmware/efi/runtime-map/0/type
> > +			/sys/firmware/efi/runtime-map/0/virt_addr
> > +                        ...
> 
> What is this example supposed to show? The above is already detailed
> enough.

Ok, I can remove them.

> 
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index 2a292aa..c3a2aaa 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -76,6 +76,9 @@ static __initdata efi_config_table_type_t arch_tables[] = {
> >  	{NULL_GUID, NULL, NULL},
> >  };
> >  
> > +void *efi_runtime_map;
> > +int nr_efi_runtime_map;
> > +
> >  /*
> >   * Returns 1 if 'facility' is enabled, 0 otherwise.
> >   */
> > @@ -811,6 +814,21 @@ static void __init efi_merge_regions(void)
> >  	}
> >  }
> >  
> > +static int __init efi_save_runtime_map(efi_memory_desc_t *md)
> 
> It is a static function so no need for the "efi_" prefix.
> 
> Also, it should be called save_runtime_mem_desc or so because this is
> what it does.

Ok.

> 
> > +{
> > +	efi_runtime_map = krealloc(efi_runtime_map,
> > +			(nr_efi_runtime_map + 1) *
> > +			memmap.desc_size, GFP_KERNEL);
> 
> WARNING: Reusing the krealloc arg is almost always a bug
> #21: FILE: arch/x86/platform/efi/efi.c:819:
> +       efi_runtime_map = krealloc(efi_runtime_map,
> 
> Please run your diffs through checkpatch.pl --strict before submitting.
> 
> Btw, we use krealloc below again which should be changed too.

Since we do not know the exact number of ranges, introducing a list
or count the totoal range numbers before will add more code. Do you
really think it's necessary?

I tend to not fix every warnings from checkpatch if it's not a must-fix
one.
> 
> > +	if (!efi_runtime_map)
> > +		return -ENOMEM;
> > +
> > +	memcpy(efi_runtime_map + nr_efi_runtime_map * memmap.desc_size,
> > +		md, memmap.desc_size);
> > +	nr_efi_runtime_map++;
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Map efi memory ranges for runtime serivce and update new_memmap with virtual
> >   * addresses.
> > @@ -821,6 +839,7 @@ static void * __init efi_map_regions(int *count)
> >  	void *p, *new_memmap = NULL;
> >  	unsigned long size;
> >  	u64 end, systab;
> > +	int error = 0;
> >  
> >  	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> >  		md = p;
> > @@ -852,9 +871,16 @@ static void * __init efi_map_regions(int *count)
> >  
> >  		memcpy(new_memmap + (*count * memmap.desc_size), md,
> >  		       memmap.desc_size);
> > +		if (!error && md->type != EFI_BOOT_SERVICES_CODE &&
> > +				md->type != EFI_BOOT_SERVICES_DATA)
> > +			error = efi_save_runtime_map(md);
> 
> I'm not sure I like this entangled stuff with the error code - I can
> barely understand what's going on there - not necessarily because of
> your changes but because this code is not that trivial, especially after
> time has passed.

suppose kexec runtime map preparing fails we still can get 1st kernel boot ok.

> 
> And I don't see you kfreeing efi_runtime_map in the case where the first
> krealloc() fails at some iteration.

Good catch, the original krealloc error handling also need improve.
Ok, so I will improve the code by adding a link list for new_memmap
and removing the krealloc.

> 
> What would be much cleaner IMO would be if you let efi_map_regions()
> finish, *and*, *only* if it was successful iterate again over the
> regions and do efi_save_runtime_map() on the runtime ones. This will
> make the code much more readable too.

For simplifying the code, will do.

> 
> >  		(*count)++;
> >  	}
> >  
> > +	if (error) {
> > +		nr_efi_runtime_map = 0;
> > +		kfree(efi_runtime_map);
> > +	}
> 
> This error gets overwritten each time the loop runs and you possibly
> lose !0 values. What I'm suggesting above would take care of that case
> too.
> 
> >  ret:
> >  	return new_memmap;
> >  }
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index b0fc7c7..b4d01ad 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -36,4 +36,14 @@ config EFI_VARS_PSTORE_DEFAULT_DISABLE
> >  	  backend for pstore by default. This setting can be overridden
> >  	  using the efivars module's pstore_disable parameter.
> >  
> > +config EFI_RUNTIME_MAP
> > +	bool "Export efi runtime maps to sysfs" if EXPERT
> > +	default X86 && EFI
> 
> This should depend on KEXEC.

I did not add KEXEC because I thought it can be used for debugging purpose. 

> 
> > +	help
> > +	  Export efi runtime memory maps to /sys/firmware/efi/runtime-map.
> > +	  That memory map is used for example by kexec to set up efi virtual
> > +	  mapping the 2nd kernel, but can also be used for debugging purposes.
> > +
> > +	  See also Documentation/ABI/testing/sysfs-firmware-efi-runtime-map.
> > +
> >  endmenu
> > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > index 99245ab..ecadcc1 100644
> > --- a/drivers/firmware/efi/Makefile
> > +++ b/drivers/firmware/efi/Makefile
> > @@ -4,3 +4,4 @@
> >  obj-y					+= efi.o vars.o
> >  obj-$(CONFIG_EFI_VARS)			+= efivars.o
> >  obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
> > +obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
> 
> This will conflict with upstream:

Will check and update, what tree are you using?

> 
> checking file drivers/firmware/efi/Makefile
> Hunk #1 FAILED at 4.
> 1 out of 1 hunk FAILED
> 
> It should be:
> 
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 9ba156d3c775..7f05c368b868 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -5,3 +5,4 @@ obj-y                                   += efi.o vars.o
>  obj-$(CONFIG_EFI_VARS)                 += efivars.o
>  obj-$(CONFIG_EFI_VARS_PSTORE)          += efi-pstore.o
>  obj-$(CONFIG_UEFI_CPER)                        += cper.o
> +obj-$(CONFIG_EFI_RUNTIME_MAP)          += runtime-map.o
> 
> 
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 5d85956..a480de8 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -38,7 +38,8 @@ struct efi __read_mostly efi = {
> >  };
> >  EXPORT_SYMBOL(efi);
> >  
> > -static struct kobject *efi_kobj;
> > +struct kobject *efi_kobj;
> > +EXPORT_SYMBOL_GPL(efi_kobj);
> 
> Can we move this private to linux/drivers/efi/runtime-map.c? If yes,
> then there's no need to export it.

Hmm, it's created in efi.c so I think we can not move it..

> 
> >  static struct kobject *efivars_kobj;
> >  
> >  /*
> > diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> > new file mode 100644
> > index 0000000..036d099
> > --- /dev/null
> > +++ b/drivers/firmware/efi/runtime-map.c
> > @@ -0,0 +1,199 @@
> > +/*
> > + * linux/drivers/efi/runtime-map.c
> > + * Copyright (C) 2013 Red Hat, Inc., Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License v2.0 as published by
> > + * the Free Software Foundation
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> 
> Please drop this boilerplate text and do a single sentence saying that
> it is licensed under GPL v2 and refer to COPYING in the kernel repo.

Ok.

> 
> > + *
> > + */
> > +
> > +#include <linux/string.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/efi.h>
> > +#include <linux/slab.h>
> > +
> > +#include <asm/setup.h>
> > +
> > +struct efi_runtime_map_entry {
> > +	efi_memory_desc_t md;
> > +	struct kobject kobj;   /* kobject for each entry */
> > +};
> > +
> > +static struct efi_runtime_map_entry **map_entries;
> > +
> > +static ssize_t map_attr_show(struct kobject *kobj,
> > +				struct attribute *attr, char *buf);
> > +static ssize_t type_show(struct efi_runtime_map_entry *entry, char *buf);
> > +static ssize_t phys_addr_show(struct efi_runtime_map_entry *entry, char *buf);
> > +static ssize_t virt_addr_show(struct efi_runtime_map_entry *entry, char *buf);
> > +static ssize_t num_pages_show(struct efi_runtime_map_entry *entry, char *buf);
> > +static ssize_t attribute_show(struct efi_runtime_map_entry *entry, char *buf);
> > +
> > +struct map_attribute {
> > +	struct attribute attr;
> > +	ssize_t (*show)(struct efi_runtime_map_entry *entry, char *buf);
> > +};
> > +
> > +static struct map_attribute map_type_attr = __ATTR_RO(type);
> > +static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > +static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > +static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > +static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > +
> > +/*
> > + * These are default attributes that are added for every memmap entry.
> > + */
> > +static struct attribute *def_attrs[] = {
> > +	&map_type_attr.attr,
> > +	&map_phys_addr_attr.attr,
> > +	&map_virt_addr_attr.attr,
> > +	&map_num_pages_attr.attr,
> > +	&map_attribute_attr.attr,
> > +	NULL
> > +};
> > +
> > +static const struct sysfs_ops map_attr_ops = {
> > +	.show = map_attr_show,
> > +};
> > +
> > +static inline struct efi_runtime_map_entry *to_map_entry(struct kobject *kobj)
> > +{
> > +	return container_of(kobj, struct efi_runtime_map_entry, kobj);
> > +}
> > +
> > +static void map_release(struct kobject *kobj)
> > +{
> > +	struct efi_runtime_map_entry *entry;
> > +
> > +	entry = to_map_entry(kobj);
> > +	kfree(entry);
> > +}
> > +
> > +static struct kobj_type __refdata map_ktype = {
> > +	.sysfs_ops	= &map_attr_ops,
> > +	.default_attrs	= def_attrs,
> > +	.release	= map_release,
> > +};
> > +
> > +static ssize_t type_show(struct efi_runtime_map_entry *entry, char *buf)
> > +{
> > +	return snprintf(buf, PAGE_SIZE, "0x%x\n", entry->md.type);
> > +}
> > +
> > +static ssize_t phys_addr_show(struct efi_runtime_map_entry *entry, char *buf)
> > +{
> > +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.phys_addr);
> > +}
> > +
> > +static ssize_t virt_addr_show(struct efi_runtime_map_entry *entry, char *buf)
> > +{
> > +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.virt_addr);
> > +}
> > +
> > +static ssize_t num_pages_show(struct efi_runtime_map_entry *entry, char *buf)
> > +{
> > +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.num_pages);
> > +}
> > +
> > +static ssize_t attribute_show(struct efi_runtime_map_entry *entry, char *buf)
> > +{
> > +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->md.attribute);
> > +}
> 
> I can see the macro here writing itself too :)

Thanks for catching, actually I have changed it in my local tree since your
last comment about other file.

> 
> > +
> > +static inline struct map_attribute *to_map_attr(struct attribute *attr)
> > +{
> > +	return container_of(attr, struct map_attribute, attr);
> > +}
> > +
> > +static ssize_t map_attr_show(struct kobject *kobj,
> > +				struct attribute *attr, char *buf)
> > +{
> > +	struct efi_runtime_map_entry *entry = to_map_entry(kobj);
> > +	struct map_attribute *map_attr = to_map_attr(attr);
> > +
> > +	return map_attr->show(entry, buf);
> > +}
> > +
> > +static struct kset *map_kset;
> > +
> > +/*
> > + * Add map entry on sysfs
> > + */
> 
> The function name should be enough - no need for the comment.

Ok.

> 
> > +static struct efi_runtime_map_entry *add_sysfs_runtime_map_entry(int nr)
> > +{
> > +	int ret;
> > +	unsigned long desc_size;
> > +	struct efi_runtime_map_entry *entry;
> > +	struct efi_info *e = &boot_params.efi_info;
> > +
> > +	if (!map_kset) {
> > +		map_kset = kset_create_and_add("runtime-map", NULL,
> > +				efi_kobj);
> > +		if (!map_kset)
> > +			return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > +	if (!entry) {
> > +		kset_unregister(map_kset);
> > +		return entry;
> > +	}
> > +
> > +	desc_size = e->efi_memdesc_size;
> > +	memcpy(&entry->md, efi_runtime_map + nr * desc_size,
> > +		sizeof(efi_memory_desc_t));
> 
> If you're going to break this line anyway, you don't really need the
> local desc_size variable:

Ok

> 
> 	memcpy(&entry->md,
> 	       efi_runtime_map + nr * e->efi_memdesc_size,
> 	       sizeof(efi_memory_desc_t));
> 
> > +
> > +	kobject_init(&entry->kobj, &map_ktype);
> > +	entry->kobj.kset = map_kset;
> > +	ret = kobject_add(&entry->kobj, NULL, "%d", nr);
> > +	if (ret) {
> > +		kobject_put(&entry->kobj);
> > +		kset_unregister(map_kset);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	return entry;
> > +}
> > +
> > +static int __init efi_runtime_map_init(void)
> > +{
> > +	int i, j, ret = 0;
> > +	struct efi_runtime_map_entry *entry;
> > +
> > +	if (!efi_runtime_map) {
> > +		pr_warn("no efi_runtime_map found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	map_entries = kzalloc(nr_efi_runtime_map *
> > +		sizeof(struct efi_runtime_map_entry *), GFP_KERNEL);
> 
> Shorten:
> 
> 	map_entries = kzalloc(nr_efi_runtime_map * sizeof(entry), GFP_KERNEL);

Will do.

> 
> > +	if (!map_entries)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < nr_efi_runtime_map; i++) {
> > +		entry = add_sysfs_runtime_map_entry(i);
> > +		if (IS_ERR(entry)) {
> > +			for (j = i - 1; j > 0; j--) {
> > +				entry = *(map_entries + j);
> > +				kobject_put(&entry->kobj);
> > +			}
> > +			if (map_kset)
> > +				kset_unregister(map_kset);
> > +			ret = PTR_ERR(entry);
> > +			goto out;
> > +		}
> 
> You can move this error unwinding path away from the main loop:
> 
> 	if (IS_ERR(entry))
> 		goto out;
> 
> and make it more readable.

Ok.

> 
> > +		*(map_entries + i) = entry;
> > +	}
> > +
> > +out:
> > +	return ret;
> > +}
> > +late_initcall(efi_runtime_map_init);
> 
> Why an initcall? Can't we run this from efisubsys_init()?

Yes, we can, will do.

Thanks for review
Dave

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Borislav Petkov]

On Fri, Nov 29, 2013 at 05:40:35PM +0800, Dave Young wrote:
> Matt are suggesting to below one, is it ok to you? I'm fine with both.
>
> The efi runtime services can only be switched to virtual mode once
> without rebooting. The kexec kernel must maintain the same physical
> to virtual address mappings as the first kernel. The mappings are
> exported to sysfs so userspace tools can reassemble them and pass them
> into the kexec kernel.

His is explaining it better so take his :)

> Is it always 4K, how about "in pages"?

Actually no, so "in pages" is better.

> Since we do not know the exact number of ranges, introducing a list
> or count the totoal range numbers before will add more code. Do you
> really think it's necessary?
>
> I tend to not fix every warnings from checkpatch if it's not a
> must-fix one.

No need for the list - I actually meant you simply use a tmp pointer for
each krealloc invocation - just look around the tree for examples.

Also think about what happens to the pointed-to memory when krealloc
returns NULL.

> suppose kexec runtime map preparing fails we still can get 1st kernel
> boot ok.

Yeah, nothing stops that - especially if you iterate over the mapped
regions a second time separately, for kexec.

Which reminds me, all that save_runtime_map code needs to be behind
CONFIG_KEXEC. We don't want to be doing this if KEXEC is not enabled.

> > What would be much cleaner IMO would be if you let efi_map_regions()
> > finish, *and*, *only* if it was successful iterate again over the
> > regions and do efi_save_runtime_map() on the runtime ones. This will
> > make the code much more readable too.
> 
> For simplifying the code, will do.

Right, and hide it behind CONFIG_KEXEC too, while you're at it.

> I did not add KEXEC because I thought it can be used for debugging
> purpose.

mfleming is thinking about it :)

> Will check and update, what tree are you using?

Always current Linus tree and tip/master. This helps catch issues like
that.

> Hmm, it's created in efi.c so I think we can not move it..

Ok, so we apparently export naked kobjects like that. All right, fair
enough.

...

> Thanks for catching, actually I have changed it in my local tree since
> your last comment about other file.

:)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Matt Fleming]

On Fri, 29 Nov, at 12:50:29PM, Borislav Petkov wrote:
> > I did not add KEXEC because I thought it can be used for debugging
> > purpose.
> 
> mfleming is thinking about it :)
 
I finally finished my thought. The sysfs code should be automatically
enabled only if CONFIG_KEXEC is enabled, and not on my default because
it is a debug interface. While it's true that the code may be useful for
debugging the memory mappings of EFI systems, it doesn't make sense to
always turn it on just for CONFIG_X86 and CONFIG_EFI.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

On 11/29/13 at 11:59am, Matt Fleming wrote:
> On Fri, 29 Nov, at 12:50:29PM, Borislav Petkov wrote:
> > > I did not add KEXEC because I thought it can be used for debugging
> > > purpose.
> > 
> > mfleming is thinking about it :)
>  
> I finally finished my thought. The sysfs code should be automatically
> enabled only if CONFIG_KEXEC is enabled, and not on my default because
> it is a debug interface. While it's true that the code may be useful for
> debugging the memory mappings of EFI systems, it doesn't make sense to
> always turn it on just for CONFIG_X86 and CONFIG_EFI.

Ok, will enable it only for CONFIG_X86 and CONFIG_EFI and CONFIG_KEXEC

> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

On 11/29/13 at 12:50pm, Borislav Petkov wrote:
> No need for the list - I actually meant you simply use a tmp pointer for
> each krealloc invocation - just look around the tree for examples.
> 
> Also think about what happens to the pointed-to memory when krealloc
> returns NULL.

Ok, thanks for the explaination.

> 
> > suppose kexec runtime map preparing fails we still can get 1st kernel
> > boot ok.
> 
> Yeah, nothing stops that - especially if you iterate over the mapped
> regions a second time separately, for kexec.
> 
> Which reminds me, all that save_runtime_map code needs to be behind
> CONFIG_KEXEC. We don't want to be doing this if KEXEC is not enabled.

They are only called if there's setup_data SETUP_EFI passed in. Yes, I think
only kexec need that part of code. But I hesitated to mess the code with
a lot of #if. Will think about it.

Thanks
Dave

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Borislav Petkov]

On Mon, Dec 02, 2013 at 10:59:42AM +0800, Dave Young wrote:
> They are only called if there's setup_data SETUP_EFI passed in. Yes, I
> think only kexec need that part of code. But I hesitated to mess the
> code with a lot of #if. Will think about it.

Well, the accepted strategy with the efi code is to have function calls
in efi.c and then have different versions of those functions defined for
32 and 64-bit, i.e. efi_32.c and efi_64.c.

I can imagine adding an efi_kexec.c could make the whole deal very
clearly arranged without ifdeffery.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

Hi, all

Update my status:

I have finished most of thecode related changes including the krealloc
fixes (both for original code and my new code). And I'm slowly
moving the kexec related stuff to efi_kexec.c, this involves
some other cleanups, such as:
efi_systab_init:
 -> move 64bit code to efi_64.c, move 32bit code to efi_32.c

efi_setup global variable is changing to physical address, ioremapping
it when necessary instead of directly using the virt addr because
previously the virt addr will be freed until entering virtual mode,
it's possible a leak in case something is wrong in the middle also it
occupies the ioremap slot a little bit long.

There's also other problems, so I still need a few days probably
a week to carefully restructure the code.

Thanks
Dave

Sending from web interface so sorry for the email format.
----- Original Message -----
From: "Borislav Petkov" <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>
To: "Dave Young" <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org, linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "toshi kani" <toshi.kani-VXdhtT5mjnY@public.gmane.org>, matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org, greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org, x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "James Bottomley" <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>, horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org, ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org, hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org, vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Sent: Monday, December 2, 2013 5:40:20 PM
Subject: Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

On Mon, Dec 02, 2013 at 10:59:42AM +0800, Dave Young wrote:
> They are only called if there's setup_data SETUP_EFI passed in. Yes, I
> think only kexec need that part of code. But I hesitated to mess the
> code with a lot of #if. Will think about it.

Well, the accepted strategy with the efi code is to have function calls
in efi.c and then have different versions of those functions defined for
32 and 64-bit, i.e. efi_32.c and efi_64.c.

I can imagine adding an efi_kexec.c could make the whole deal very
clearly arranged without ifdeffery.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

_______________________________________________
kexec mailing list
kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Borislav Petkov]

On Sat, Dec 07, 2013 at 04:01:02AM -0500, Dave Young wrote:
> Hi, all
> 
> Update my status:
> 
> I have finished most of thecode related changes including the krealloc
> fixes (both for original code and my new code). And I'm slowly
> moving the kexec related stuff to efi_kexec.c, this involves
> some other cleanups, such as:
> efi_systab_init:
>  -> move 64bit code to efi_64.c, move 32bit code to efi_32.c
> 
> efi_setup global variable is changing to physical address, ioremapping
> it when necessary instead of directly using the virt addr because
> previously the virt addr will be freed until entering virtual mode,
> it's possible a leak in case something is wrong in the middle also it
> occupies the ioremap slot a little bit long.
> 
> There's also other problems, so I still need a few days probably
> a week to carefully restructure the code.

Those are all lofty goals but beware that trying to solve it all at
once can be very painful and can cause unnecessary work for both coders
and reviewers. Especially if the patchset grows out of proportion which
makes it much harder to review and deal with.

So, from my experience and if I were you, I'd try to address the most
important issue I'm trying to fix, have 10ish nice clean patches and put
the rest on the TODO list. This is much easier to deal with for everyone
involved than trying to solve it all at once.

Besides, if you solve it all at once, what are you going to work on
afterwards? :-)

HTH.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[H. Peter Anvin]

This is very good advice indeed.

Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
>On Sat, Dec 07, 2013 at 04:01:02AM -0500, Dave Young wrote:
>> Hi, all
>> 
>> Update my status:
>> 
>> I have finished most of thecode related changes including the
>krealloc
>> fixes (both for original code and my new code). And I'm slowly
>> moving the kexec related stuff to efi_kexec.c, this involves
>> some other cleanups, such as:
>> efi_systab_init:
>>  -> move 64bit code to efi_64.c, move 32bit code to efi_32.c
>> 
>> efi_setup global variable is changing to physical address,
>ioremapping
>> it when necessary instead of directly using the virt addr because
>> previously the virt addr will be freed until entering virtual mode,
>> it's possible a leak in case something is wrong in the middle also it
>> occupies the ioremap slot a little bit long.
>> 
>> There's also other problems, so I still need a few days probably
>> a week to carefully restructure the code.
>
>Those are all lofty goals but beware that trying to solve it all at
>once can be very painful and can cause unnecessary work for both coders
>and reviewers. Especially if the patchset grows out of proportion which
>makes it much harder to review and deal with.
>
>So, from my experience and if I were you, I'd try to address the most
>important issue I'm trying to fix, have 10ish nice clean patches and
>put
>the rest on the TODO list. This is much easier to deal with for
>everyone
>involved than trying to solve it all at once.
>
>Besides, if you solve it all at once, what are you going to work on
>afterwards? :-)
>
>HTH.

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs

[Dave Young]

On 12/07/13 at 02:30pm, Borislav Petkov wrote:
> On Sat, Dec 07, 2013 at 04:01:02AM -0500, Dave Young wrote:
> > Hi, all
> > 
> > Update my status:
> > 
> > I have finished most of thecode related changes including the krealloc
> > fixes (both for original code and my new code). And I'm slowly
> > moving the kexec related stuff to efi_kexec.c, this involves
> > some other cleanups, such as:
> > efi_systab_init:
> >  -> move 64bit code to efi_64.c, move 32bit code to efi_32.c
> > 
> > efi_setup global variable is changing to physical address, ioremapping
> > it when necessary instead of directly using the virt addr because
> > previously the virt addr will be freed until entering virtual mode,
> > it's possible a leak in case something is wrong in the middle also it
> > occupies the ioremap slot a little bit long.
> > 
> > There's also other problems, so I still need a few days probably
> > a week to carefully restructure the code.
> 
> Those are all lofty goals but beware that trying to solve it all at
> once can be very painful and can cause unnecessary work for both coders
> and reviewers. Especially if the patchset grows out of proportion which
> makes it much harder to review and deal with.
> 
> So, from my experience and if I were you, I'd try to address the most
> important issue I'm trying to fix, have 10ish nice clean patches and put
> the rest on the TODO list. This is much easier to deal with for everyone
> involved than trying to solve it all at once.
> 
> Besides, if you solve it all at once, what are you going to work on
> afterwards? :-)
> 

I like your suggestion for splitting kexec stuff to another file efi_kexec.c.
but yes it needs more work and is bloating the patchset, I'm fine to leave
it in TODO list.

Thanks
Dave

[PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

Add a new setup_data type SETUP_EFI for kexec use.
Passing the saved fw_vendor, runtime, config tables and
efi runtime mappings.

When entering virtual mode, directly mapping the efi
runtime ragions which we passed in previously. And skip
the step to call SetVirtualAddressMap.

Specially for HP z420 workstation it need another variable
saving, it's the smbios physical address, the HP bios
also update the SMBIOS address after entering virtual mode
besides of the standard fw_vendor,runtime and config table.

Tested on ovmf+qemu, lenovo thinkpad, a dell laptop and an
HP z420 workstation.

v2: refresh based on previous patch changes, code cleanup.
v3: use ioremap instead of phys_to_virt for esdata

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/include/asm/efi.h            |  12 +++
 arch/x86/include/uapi/asm/bootparam.h |   1 +
 arch/x86/kernel/setup.c               |   3 +
 arch/x86/platform/efi/efi.c           | 161 ++++++++++++++++++++++++++++++----
 4 files changed, 160 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 9fbaeb2..73d5643 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -133,6 +133,18 @@ extern void efi_sync_low_kernel_mappings(void);
 extern void efi_setup_page_tables(void);
 extern void __init old_map_region(efi_memory_desc_t *md);
 
+struct efi_setup_data {
+	u64 fw_vendor;
+	u64 runtime;
+	u64 tables;
+	u64 smbios;
+	u64 reserved[8];
+	efi_memory_desc_t map[0];
+};
+
+extern void parse_efi_setup(u64 phys_addr, u32 data_len);
+extern struct efi_setup_data *esdata;
+
 #ifdef CONFIG_EFI
 
 static inline bool efi_is_native(void)
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..ed6afc9 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,6 +6,7 @@
 #define SETUP_E820_EXT			1
 #define SETUP_DTB			2
 #define SETUP_PCI			3
+#define SETUP_EFI			4
 
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f0de629..ecf225e 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -447,6 +447,9 @@ static void __init parse_setup_data(void)
 		case SETUP_DTB:
 			add_dtb(pa_data);
 			break;
+		case SETUP_EFI:
+			parse_efi_setup(pa_data, data_len);
+			break;
 		default:
 			break;
 		}
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index c3a2aaa..fafeb40 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -78,6 +78,7 @@ static __initdata efi_config_table_type_t arch_tables[] = {
 
 void *efi_runtime_map;
 int nr_efi_runtime_map;
+struct efi_setup_data *esdata;
 
 /*
  * Returns 1 if 'facility' is enabled, 0 otherwise.
@@ -115,6 +116,32 @@ static int __init setup_storage_paranoia(char *arg)
 }
 early_param("efi_no_storage_paranoia", setup_storage_paranoia);
 
+void __init parse_efi_setup(u64 phys_addr, u32 data_len)
+{
+	int size;
+	struct setup_data *sdata;
+	u64 esdata_phys;
+
+	if (!efi_enabled(EFI_64BIT)) {
+		pr_warn("skipping setup_data on EFI 32BIT!");
+		return;
+	}
+
+	sdata = early_memremap(phys_addr, data_len);
+	if (!sdata)
+		return;
+
+	size = data_len - sizeof(struct setup_data);
+
+	esdata_phys = phys_addr + sizeof(struct setup_data);
+
+	nr_efi_runtime_map = (size - sizeof(struct efi_setup_data)) /
+			sizeof(efi_memory_desc_t);
+	early_iounmap(sdata, data_len);
+
+	/* iounmap esdata in function efi_enter_virtual_mode */
+	esdata = early_memremap(esdata_phys, size);
+}
 
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
@@ -504,8 +531,12 @@ static int __init efi_systab_init(void *phys)
 		}
 
 		efi_systab.hdr = systab64->hdr;
-		efi_systab.fw_vendor = systab64->fw_vendor;
-		tmp |= systab64->fw_vendor;
+
+		if (esdata)
+			efi_systab.fw_vendor = (unsigned long)esdata->fw_vendor;
+		else
+			efi_systab.fw_vendor = systab64->fw_vendor;
+		tmp |= efi_systab.fw_vendor;
 		efi_systab.fw_revision = systab64->fw_revision;
 		efi_systab.con_in_handle = systab64->con_in_handle;
 		tmp |= systab64->con_in_handle;
@@ -519,13 +550,21 @@ static int __init efi_systab_init(void *phys)
 		tmp |= systab64->stderr_handle;
 		efi_systab.stderr = systab64->stderr;
 		tmp |= systab64->stderr;
-		efi_systab.runtime = (void *)(unsigned long)systab64->runtime;
-		tmp |= systab64->runtime;
+		if (esdata)
+			efi_systab.runtime =
+				(void *)(unsigned long)esdata->runtime;
+		else
+			efi_systab.runtime =
+				(void *)(unsigned long)systab64->runtime;
+		tmp |= (unsigned long)efi_systab.runtime;
 		efi_systab.boottime = (void *)(unsigned long)systab64->boottime;
 		tmp |= systab64->boottime;
 		efi_systab.nr_tables = systab64->nr_tables;
-		efi_systab.tables = systab64->tables;
-		tmp |= systab64->tables;
+		if (esdata)
+			efi_systab.tables = (unsigned long)esdata->tables;
+		else
+			efi_systab.tables = systab64->tables;
+		tmp |= efi_systab.tables;
 
 		early_iounmap(systab64, sizeof(*systab64));
 #ifdef CONFIG_X86_32
@@ -631,6 +670,41 @@ static int __init efi_memmap_init(void)
 	return 0;
 }
 
+static int __init efi_reuse_config(u64 tables, int nr_tables)
+{
+	void *p, *tablep;
+	int i, sz;
+
+	if (!efi_enabled(EFI_64BIT))
+		return 0;
+
+	sz = sizeof(efi_config_table_64_t);
+
+	p = tablep = early_memremap(tables, nr_tables * sz);
+	if (!p) {
+		pr_err("Could not map Configuration table!\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < efi.systab->nr_tables; i++) {
+		efi_guid_t guid;
+
+		guid = ((efi_config_table_64_t *)p)->guid;
+
+		/*
+		HP z420 workstation smbios will be convert to
+		virtual address after enter virtual mode.
+		Thus in case kexec/kdump the physical address
+		will be passed in setup_data.
+		*/
+		if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
+			((efi_config_table_64_t *)p)->table = esdata->smbios;
+		p += sz;
+	}
+	early_iounmap(tablep, nr_tables * sz);
+	return 0;
+}
+
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
@@ -676,6 +750,9 @@ void __init efi_init(void)
 		efi.systab->hdr.revision >> 16,
 		efi.systab->hdr.revision & 0xffff, vendor);
 
+	if (esdata && esdata->smbios)
+		efi_reuse_config(efi.systab->tables, efi.systab->nr_tables);
+
 	if (efi_config_init(arch_tables))
 		return;
 
@@ -886,6 +963,43 @@ ret:
 }
 
 /*
+ * map efi regions which was passed via setup_data
+ * the virt_addr is a fixed addr which was used in
+ * 1st kernel of kexec boot.
+ */
+static void __init efi_map_regions_fixed(void)
+{
+	int i;
+	unsigned long size;
+	efi_memory_desc_t *md;
+	u64 end, systab;
+	void *p;
+
+	efi_runtime_map = kzalloc(nr_efi_runtime_map * memmap.desc_size,
+				GFP_KERNEL);
+	if (!efi_runtime_map)
+		pr_err("Out of memory, EFI runtime on nested kexec non-functional!\n");
+
+	for (i = 0, p = efi_runtime_map; i < nr_efi_runtime_map; i++) {
+		md = esdata->map + i;
+		efi_map_region_fixed(md);
+		size = md->num_pages << PAGE_SHIFT;
+		end = md->phys_addr + size;
+
+		systab = (u64) (unsigned long) efi_phys.systab;
+		if (md->phys_addr <= systab && systab < end) {
+			systab += md->virt_addr - md->phys_addr;
+			efi.systab =
+				(efi_system_table_t *) (unsigned long) systab;
+		}
+		if (efi_runtime_map) {
+			memcpy(p, md, memmap.desc_size);
+			p += memmap.desc_size;
+		}
+	}
+}
+
+/*
  * This function will switch the EFI runtime services to virtual mode.
  * Essentially, we look through the EFI memmap and map every region that
  * has the runtime attribute bit set in its memory descriptor into the
@@ -901,6 +1015,10 @@ ret:
  * so that we're in a different address space when calling a runtime
  * function. For function arguments passing we do copy the PGDs of the
  * kernel page table into ->trampoline_pgd prior to each call.
+ *
+ * Specially for kexec boot efi runtime maps in previous kernel should
+ * be passed in via setup_data. In that case runtime ranges will be mapped
+ * to fixed virtual addresses exactly same as the ones in previous kernel.
  */
 void __init efi_enter_virtual_mode(void)
 {
@@ -919,12 +1037,15 @@ void __init efi_enter_virtual_mode(void)
 		return;
 	}
 
-	efi_merge_regions();
-
-	new_memmap = efi_map_regions(&count);
-	if (!new_memmap) {
-		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
-		return;
+	if (esdata)
+		efi_map_regions_fixed();
+	else {
+		efi_merge_regions();
+		new_memmap = efi_map_regions(&count);
+		if (!new_memmap) {
+			pr_err("Error reallocating memory, EFI runtime non-functional!\n");
+			return;
+		}
 	}
 
 	BUG_ON(!efi.systab);
@@ -932,11 +1053,17 @@ void __init efi_enter_virtual_mode(void)
 	efi_setup_page_tables();
 	efi_sync_low_kernel_mappings();
 
-	status = phys_efi_set_virtual_address_map(
-		memmap.desc_size * count,
-		memmap.desc_size,
-		memmap.desc_version,
-		(efi_memory_desc_t *)__pa(new_memmap));
+	if (esdata) {
+		status = EFI_SUCCESS;
+		early_iounmap(esdata, sizeof(struct efi_setup_data) +
+			      nr_efi_runtime_map *
+			      sizeof(efi_memory_desc_t));
+	} else
+		status = phys_efi_set_virtual_address_map(
+			memmap.desc_size * count,
+			memmap.desc_size,
+			memmap.desc_version,
+			(efi_memory_desc_t *)__pa(new_memmap));
 
 	if (status != EFI_SUCCESS) {
 		pr_alert("Unable to switch EFI into virtual mode "
-- 
1.8.3.1

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Matt Fleming]

On Tue, 26 Nov, at 01:57:52PM, Dave Young wrote:
> Add a new setup_data type SETUP_EFI for kexec use.
> Passing the saved fw_vendor, runtime, config tables and
> efi runtime mappings.
> 
> When entering virtual mode, directly mapping the efi
> runtime ragions which we passed in previously. And skip
> the step to call SetVirtualAddressMap.
> 
> Specially for HP z420 workstation it need another variable
> saving, it's the smbios physical address, the HP bios
> also update the SMBIOS address after entering virtual mode
> besides of the standard fw_vendor,runtime and config table.
> 
> Tested on ovmf+qemu, lenovo thinkpad, a dell laptop and an
> HP z420 workstation.
> 
> v2: refresh based on previous patch changes, code cleanup.
> v3: use ioremap instead of phys_to_virt for esdata
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/include/asm/efi.h            |  12 +++
>  arch/x86/include/uapi/asm/bootparam.h |   1 +
>  arch/x86/kernel/setup.c               |   3 +
>  arch/x86/platform/efi/efi.c           | 161 ++++++++++++++++++++++++++++++----
>  4 files changed, 160 insertions(+), 17 deletions(-)

[...]

> +void __init parse_efi_setup(u64 phys_addr, u32 data_len)
> +{
> +	int size;
> +	struct setup_data *sdata;
> +	u64 esdata_phys;
> +
> +	if (!efi_enabled(EFI_64BIT)) {
> +		pr_warn("skipping setup_data on EFI 32BIT!");
> +		return;
> +	}

Hmm... this warning could be more informative. Perhaps something along
the lines of,

	"SETUP_EFI not supported on 32-bit\n"

because the reason we skip is because it isn't supported.

> +
> +	sdata = early_memremap(phys_addr, data_len);
> +	if (!sdata)
> +		return;
> +
> +	size = data_len - sizeof(struct setup_data);
> +
> +	esdata_phys = phys_addr + sizeof(struct setup_data);
> +
> +	nr_efi_runtime_map = (size - sizeof(struct efi_setup_data)) /
> +			sizeof(efi_memory_desc_t);
> +	early_iounmap(sdata, data_len);
> +
> +	/* iounmap esdata in function efi_enter_virtual_mode */
> +	esdata = early_memremap(esdata_phys, size);
> +}
>  
>  static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
>  {
> @@ -504,8 +531,12 @@ static int __init efi_systab_init(void *phys)
>  		}
>  
>  		efi_systab.hdr = systab64->hdr;
> -		efi_systab.fw_vendor = systab64->fw_vendor;
> -		tmp |= systab64->fw_vendor;
> +
> +		if (esdata)

Could we name this something more explicit? 'efi_setup' perhaps?

> @@ -631,6 +670,41 @@ static int __init efi_memmap_init(void)
>  	return 0;
>  }
>  
> +static int __init efi_reuse_config(u64 tables, int nr_tables)
> +{
> +	void *p, *tablep;
> +	int i, sz;
> +
> +	if (!efi_enabled(EFI_64BIT))
> +		return 0;
> +
> +	sz = sizeof(efi_config_table_64_t);
> +
> +	p = tablep = early_memremap(tables, nr_tables * sz);
> +	if (!p) {
> +		pr_err("Could not map Configuration table!\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < efi.systab->nr_tables; i++) {
> +		efi_guid_t guid;
> +
> +		guid = ((efi_config_table_64_t *)p)->guid;
> +
> +		/*
> +		HP z420 workstation smbios will be convert to
> +		virtual address after enter virtual mode.
> +		Thus in case kexec/kdump the physical address
> +		will be passed in setup_data.
> +		*/

This isn't the correct multi-line comment format,

		/*
		 * This is the preferred way to to write a multi-line
		 * comment.
		 */

> +		if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
> +			((efi_config_table_64_t *)p)->table = esdata->smbios;
> +		p += sz;
> +	}
> +	early_iounmap(tablep, nr_tables * sz);
> +	return 0;
> +}
> +
>  void __init efi_init(void)
>  {
>  	efi_char16_t *c16;
> @@ -676,6 +750,9 @@ void __init efi_init(void)
>  		efi.systab->hdr.revision >> 16,
>  		efi.systab->hdr.revision & 0xffff, vendor);
>  
> +	if (esdata && esdata->smbios)
> +		efi_reuse_config(efi.systab->tables, efi.systab->nr_tables);
> +

Would it make sense to move the ->smbios check inside efi_reuse_config()
in case we need to extend the number of tables in the future?

>  	if (efi_config_init(arch_tables))
>  		return;
>  
> @@ -886,6 +963,43 @@ ret:
>  }
>  
>  /*
> + * map efi regions which was passed via setup_data
> + * the virt_addr is a fixed addr which was used in
> + * 1st kernel of kexec boot.
> + */
> +static void __init efi_map_regions_fixed(void)
> +{
> +	int i;
> +	unsigned long size;
> +	efi_memory_desc_t *md;
> +	u64 end, systab;
> +	void *p;
> +
> +	efi_runtime_map = kzalloc(nr_efi_runtime_map * memmap.desc_size,
> +				GFP_KERNEL);
> +	if (!efi_runtime_map)
> +		pr_err("Out of memory, EFI runtime on nested kexec non-functional!\n");
> +
> +	for (i = 0, p = efi_runtime_map; i < nr_efi_runtime_map; i++) {
> +		md = esdata->map + i;

Heh, I read that as 'esdata->map + 1' the first few times.

> +		efi_map_region_fixed(md);
> +		size = md->num_pages << PAGE_SHIFT;
> +		end = md->phys_addr + size;
> +
> +		systab = (u64) (unsigned long) efi_phys.systab;
> +		if (md->phys_addr <= systab && systab < end) {
> +			systab += md->virt_addr - md->phys_addr;
> +			efi.systab =
> +				(efi_system_table_t *) (unsigned long) systab;
> +		}
> +		if (efi_runtime_map) {
> +			memcpy(p, md, memmap.desc_size);
> +			p += memmap.desc_size;
> +		}

Is this if () needed? Is it possible to enter the loop and have
'efi_runtime_map' be NULL?

> +	}
> +}
> +
> +/*
>   * This function will switch the EFI runtime services to virtual mode.
>   * Essentially, we look through the EFI memmap and map every region that
>   * has the runtime attribute bit set in its memory descriptor into the
> @@ -901,6 +1015,10 @@ ret:
>   * so that we're in a different address space when calling a runtime
>   * function. For function arguments passing we do copy the PGDs of the
>   * kernel page table into ->trampoline_pgd prior to each call.
> + *
> + * Specially for kexec boot efi runtime maps in previous kernel should
> + * be passed in via setup_data. In that case runtime ranges will be mapped
> + * to fixed virtual addresses exactly same as the ones in previous kernel.
>   */
>  void __init efi_enter_virtual_mode(void)
>  {
> @@ -919,12 +1037,15 @@ void __init efi_enter_virtual_mode(void)
>  		return;
>  	}
>  
> -	efi_merge_regions();
> -
> -	new_memmap = efi_map_regions(&count);
> -	if (!new_memmap) {
> -		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> -		return;
> +	if (esdata)
> +		efi_map_regions_fixed();
> +	else {
> +		efi_merge_regions();
> +		new_memmap = efi_map_regions(&count);
> +		if (!new_memmap) {
> +			pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> +			return;
> +		}
>  	}
>  
>  	BUG_ON(!efi.systab);
> @@ -932,11 +1053,17 @@ void __init efi_enter_virtual_mode(void)
>  	efi_setup_page_tables();
>  	efi_sync_low_kernel_mappings();
>  
> -	status = phys_efi_set_virtual_address_map(
> -		memmap.desc_size * count,
> -		memmap.desc_size,
> -		memmap.desc_version,
> -		(efi_memory_desc_t *)__pa(new_memmap));
> +	if (esdata) {
> +		status = EFI_SUCCESS;
> +		early_iounmap(esdata, sizeof(struct efi_setup_data) +
> +			      nr_efi_runtime_map *
> +			      sizeof(efi_memory_desc_t));
> +	} else
> +		status = phys_efi_set_virtual_address_map(
> +			memmap.desc_size * count,
> +			memmap.desc_size,
> +			memmap.desc_version,
> +			(efi_memory_desc_t *)__pa(new_memmap));
>  
>  	if (status != EFI_SUCCESS) {
>  		pr_alert("Unable to switch EFI into virtual mode "

Please fold the pr_alert() into the else branch so that you don't have
to do the 'status = EFI_SUCCESS' dance.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

On 11/26/13 at 10:04pm, Matt Fleming wrote:
> On Tue, 26 Nov, at 01:57:52PM, Dave Young wrote:
> > Add a new setup_data type SETUP_EFI for kexec use.
> > Passing the saved fw_vendor, runtime, config tables and
> > efi runtime mappings.
> > 
> > When entering virtual mode, directly mapping the efi
> > runtime ragions which we passed in previously. And skip
> > the step to call SetVirtualAddressMap.
> > 
> > Specially for HP z420 workstation it need another variable
> > saving, it's the smbios physical address, the HP bios
> > also update the SMBIOS address after entering virtual mode
> > besides of the standard fw_vendor,runtime and config table.
> > 
> > Tested on ovmf+qemu, lenovo thinkpad, a dell laptop and an
> > HP z420 workstation.
> > 
> > v2: refresh based on previous patch changes, code cleanup.
> > v3: use ioremap instead of phys_to_virt for esdata
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/include/asm/efi.h            |  12 +++
> >  arch/x86/include/uapi/asm/bootparam.h |   1 +
> >  arch/x86/kernel/setup.c               |   3 +
> >  arch/x86/platform/efi/efi.c           | 161 ++++++++++++++++++++++++++++++----
> >  4 files changed, 160 insertions(+), 17 deletions(-)
> 
> [...]
> 
> > +void __init parse_efi_setup(u64 phys_addr, u32 data_len)
> > +{
> > +	int size;
> > +	struct setup_data *sdata;
> > +	u64 esdata_phys;
> > +
> > +	if (!efi_enabled(EFI_64BIT)) {
> > +		pr_warn("skipping setup_data on EFI 32BIT!");
> > +		return;
> > +	}
> 
> Hmm... this warning could be more informative. Perhaps something along
> the lines of,
> 
> 	"SETUP_EFI not supported on 32-bit\n"

Will do

> 
> because the reason we skip is because it isn't supported.
> 
> > +
> > +	sdata = early_memremap(phys_addr, data_len);
> > +	if (!sdata)
> > +		return;
> > +
> > +	size = data_len - sizeof(struct setup_data);
> > +
> > +	esdata_phys = phys_addr + sizeof(struct setup_data);
> > +
> > +	nr_efi_runtime_map = (size - sizeof(struct efi_setup_data)) /
> > +			sizeof(efi_memory_desc_t);
> > +	early_iounmap(sdata, data_len);
> > +
> > +	/* iounmap esdata in function efi_enter_virtual_mode */
> > +	esdata = early_memremap(esdata_phys, size);
> > +}
> >  
> >  static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
> >  {
> > @@ -504,8 +531,12 @@ static int __init efi_systab_init(void *phys)
> >  		}
> >  
> >  		efi_systab.hdr = systab64->hdr;
> > -		efi_systab.fw_vendor = systab64->fw_vendor;
> > -		tmp |= systab64->fw_vendor;
> > +
> > +		if (esdata)
> 
> Could we name this something more explicit? 'efi_setup' perhaps?

Sure, just efi_setup_data is a bit long, will use efi_setup in next version.

> 
> > @@ -631,6 +670,41 @@ static int __init efi_memmap_init(void)
> >  	return 0;
> >  }
> >  
> > +static int __init efi_reuse_config(u64 tables, int nr_tables)
> > +{
> > +	void *p, *tablep;
> > +	int i, sz;
> > +
> > +	if (!efi_enabled(EFI_64BIT))
> > +		return 0;
> > +
> > +	sz = sizeof(efi_config_table_64_t);
> > +
> > +	p = tablep = early_memremap(tables, nr_tables * sz);
> > +	if (!p) {
> > +		pr_err("Could not map Configuration table!\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (i = 0; i < efi.systab->nr_tables; i++) {
> > +		efi_guid_t guid;
> > +
> > +		guid = ((efi_config_table_64_t *)p)->guid;
> > +
> > +		/*
> > +		HP z420 workstation smbios will be convert to
> > +		virtual address after enter virtual mode.
> > +		Thus in case kexec/kdump the physical address
> > +		will be passed in setup_data.
> > +		*/
> 
> This isn't the correct multi-line comment format,

Will update

> 
> 		/*
> 		 * This is the preferred way to to write a multi-line
> 		 * comment.
> 		 */
> 
> > +		if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
> > +			((efi_config_table_64_t *)p)->table = esdata->smbios;
> > +		p += sz;
> > +	}
> > +	early_iounmap(tablep, nr_tables * sz);
> > +	return 0;
> > +}
> > +
> >  void __init efi_init(void)
> >  {
> >  	efi_char16_t *c16;
> > @@ -676,6 +750,9 @@ void __init efi_init(void)
> >  		efi.systab->hdr.revision >> 16,
> >  		efi.systab->hdr.revision & 0xffff, vendor);
> >  
> > +	if (esdata && esdata->smbios)
> > +		efi_reuse_config(efi.systab->tables, efi.systab->nr_tables);
> > +
> 
> Would it make sense to move the ->smbios check inside efi_reuse_config()
> in case we need to extend the number of tables in the future?

Good suggestion, will do.

> 
> >  	if (efi_config_init(arch_tables))
> >  		return;
> >  
> > @@ -886,6 +963,43 @@ ret:
> >  }
> >  
> >  /*
> > + * map efi regions which was passed via setup_data
> > + * the virt_addr is a fixed addr which was used in
> > + * 1st kernel of kexec boot.
> > + */
> > +static void __init efi_map_regions_fixed(void)
> > +{
> > +	int i;
> > +	unsigned long size;
> > +	efi_memory_desc_t *md;
> > +	u64 end, systab;
> > +	void *p;
> > +
> > +	efi_runtime_map = kzalloc(nr_efi_runtime_map * memmap.desc_size,
> > +				GFP_KERNEL);
> > +	if (!efi_runtime_map)
> > +		pr_err("Out of memory, EFI runtime on nested kexec non-functional!\n");
> > +
> > +	for (i = 0, p = efi_runtime_map; i < nr_efi_runtime_map; i++) {
> > +		md = esdata->map + i;
> 
> Heh, I read that as 'esdata->map + 1' the first few times.

To make it more readable, I will change them like below:

p = efi_runtime_map;
md = efi_setup->map;
for (i = 0; i < nr_efi_runtime_map; i++) {
	[...]
	md += 1;
}

> 
> > +		efi_map_region_fixed(md);
> > +		size = md->num_pages << PAGE_SHIFT;
> > +		end = md->phys_addr + size;
> > +
> > +		systab = (u64) (unsigned long) efi_phys.systab;
> > +		if (md->phys_addr <= systab && systab < end) {
> > +			systab += md->virt_addr - md->phys_addr;
> > +			efi.systab =
> > +				(efi_system_table_t *) (unsigned long) systab;
> > +		}
> > +		if (efi_runtime_map) {
> > +			memcpy(p, md, memmap.desc_size);
> > +			p += memmap.desc_size;
> > +		}
> 
> Is this if () needed? Is it possible to enter the loop and have
> 'efi_runtime_map' be NULL?

Yes, it is needed. if efi_runtime_map kmalloc fails I only print error, do not
return so kernel can still boot, just kexec on efi will not work that has been
put in the error message.

> 
> > +	}
> > +}
> > +
> > +/*
> >   * This function will switch the EFI runtime services to virtual mode.
> >   * Essentially, we look through the EFI memmap and map every region that
> >   * has the runtime attribute bit set in its memory descriptor into the
> > @@ -901,6 +1015,10 @@ ret:
> >   * so that we're in a different address space when calling a runtime
> >   * function. For function arguments passing we do copy the PGDs of the
> >   * kernel page table into ->trampoline_pgd prior to each call.
> > + *
> > + * Specially for kexec boot efi runtime maps in previous kernel should
> > + * be passed in via setup_data. In that case runtime ranges will be mapped
> > + * to fixed virtual addresses exactly same as the ones in previous kernel.
> >   */
> >  void __init efi_enter_virtual_mode(void)
> >  {
> > @@ -919,12 +1037,15 @@ void __init efi_enter_virtual_mode(void)
> >  		return;
> >  	}
> >  
> > -	efi_merge_regions();
> > -
> > -	new_memmap = efi_map_regions(&count);
> > -	if (!new_memmap) {
> > -		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> > -		return;
> > +	if (esdata)
> > +		efi_map_regions_fixed();
> > +	else {
> > +		efi_merge_regions();
> > +		new_memmap = efi_map_regions(&count);
> > +		if (!new_memmap) {
> > +			pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> > +			return;
> > +		}
> >  	}
> >  
> >  	BUG_ON(!efi.systab);
> > @@ -932,11 +1053,17 @@ void __init efi_enter_virtual_mode(void)
> >  	efi_setup_page_tables();
> >  	efi_sync_low_kernel_mappings();
> >  
> > -	status = phys_efi_set_virtual_address_map(
> > -		memmap.desc_size * count,
> > -		memmap.desc_size,
> > -		memmap.desc_version,
> > -		(efi_memory_desc_t *)__pa(new_memmap));
> > +	if (esdata) {
> > +		status = EFI_SUCCESS;
> > +		early_iounmap(esdata, sizeof(struct efi_setup_data) +
> > +			      nr_efi_runtime_map *
> > +			      sizeof(efi_memory_desc_t));
> > +	} else
> > +		status = phys_efi_set_virtual_address_map(
> > +			memmap.desc_size * count,
> > +			memmap.desc_size,
> > +			memmap.desc_version,
> > +			(efi_memory_desc_t *)__pa(new_memmap));
> >  
> >  	if (status != EFI_SUCCESS) {
> >  		pr_alert("Unable to switch EFI into virtual mode "
> 
> Please fold the pr_alert() into the else branch so that you don't have
> to do the 'status = EFI_SUCCESS' dance.

Good suggestion, will do.

Thanks for review
Dave

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Matt Fleming]

On Wed, 27 Nov, at 12:52:37PM, Dave Young wrote:
> To make it more readable, I will change them like below:
> 
> p = efi_runtime_map;
> md = efi_setup->map;
> for (i = 0; i < nr_efi_runtime_map; i++) {
> 	[...]
> 	md += 1;
> }
 
Actually, md++ is the canonical way to write this.

> > 
> > > +		efi_map_region_fixed(md);
> > > +		size = md->num_pages << PAGE_SHIFT;
> > > +		end = md->phys_addr + size;
> > > +
> > > +		systab = (u64) (unsigned long) efi_phys.systab;
> > > +		if (md->phys_addr <= systab && systab < end) {
> > > +			systab += md->virt_addr - md->phys_addr;
> > > +			efi.systab =
> > > +				(efi_system_table_t *) (unsigned long) systab;
> > > +		}
> > > +		if (efi_runtime_map) {
> > > +			memcpy(p, md, memmap.desc_size);
> > > +			p += memmap.desc_size;
> > > +		}
> > 
> > Is this if () needed? Is it possible to enter the loop and have
> > 'efi_runtime_map' be NULL?
> 
> Yes, it is needed. if efi_runtime_map kmalloc fails I only print error, do not
> return so kernel can still boot, just kexec on efi will not work that has been
> put in the error message.
 
OK. On second thought, is there any way to turn the above code into a
call to efi_save_runtime_map()? Because you've basically duplicated that
code and I can definitely envisage the two code paths fragmenting over
time, e.g. when someone makes changes to one but not the other.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

On 11/27/13 at 10:17am, Matt Fleming wrote:
> On Wed, 27 Nov, at 12:52:37PM, Dave Young wrote:
> > To make it more readable, I will change them like below:
> > 
> > p = efi_runtime_map;
> > md = efi_setup->map;
> > for (i = 0; i < nr_efi_runtime_map; i++) {
> > 	[...]
> > 	md += 1;
> > }
>  
> Actually, md++ is the canonical way to write this.

Ok.

> 
> > > 
> > > > +		efi_map_region_fixed(md);
> > > > +		size = md->num_pages << PAGE_SHIFT;
> > > > +		end = md->phys_addr + size;
> > > > +
> > > > +		systab = (u64) (unsigned long) efi_phys.systab;
> > > > +		if (md->phys_addr <= systab && systab < end) {
> > > > +			systab += md->virt_addr - md->phys_addr;
> > > > +			efi.systab =
> > > > +				(efi_system_table_t *) (unsigned long) systab;
> > > > +		}
> > > > +		if (efi_runtime_map) {
> > > > +			memcpy(p, md, memmap.desc_size);
> > > > +			p += memmap.desc_size;
> > > > +		}
> > > 
> > > Is this if () needed? Is it possible to enter the loop and have
> > > 'efi_runtime_map' be NULL?
> > 
> > Yes, it is needed. if efi_runtime_map kmalloc fails I only print error, do not
> > return so kernel can still boot, just kexec on efi will not work that has been
> > put in the error message.
>  
> OK. On second thought, is there any way to turn the above code into a
> call to efi_save_runtime_map()? Because you've basically duplicated that
> code and I can definitely envisage the two code paths fragmenting over
> time, e.g. when someone makes changes to one but not the other.

OK, will consider to reuse the function.

Thanks
Dave

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Borislav Petkov]

On Tue, Nov 26, 2013 at 01:57:52PM +0800, Dave Young wrote:
> Add a new setup_data type SETUP_EFI for kexec use.
> Passing the saved fw_vendor, runtime, config tables and
> efi runtime mappings.
> 
> When entering virtual mode, directly mapping the efi
> runtime ragions which we passed in previously. And skip
> the step to call SetVirtualAddressMap.
> 
> Specially for HP z420 workstation it need another variable
> saving,

Why the special handling? Does that mean, this is going to be the case
for other HP UEFI implementations too?

> it's the smbios physical address, the HP bios
> also update the SMBIOS address after entering virtual mode
> besides of the standard fw_vendor,runtime and config table.
> 
> Tested on ovmf+qemu, lenovo thinkpad, a dell laptop and an
> HP z420 workstation.
> 
> v2: refresh based on previous patch changes, code cleanup.
> v3: use ioremap instead of phys_to_virt for esdata
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/include/asm/efi.h            |  12 +++
>  arch/x86/include/uapi/asm/bootparam.h |   1 +
>  arch/x86/kernel/setup.c               |   3 +
>  arch/x86/platform/efi/efi.c           | 161 ++++++++++++++++++++++++++++++----
>  4 files changed, 160 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 9fbaeb2..73d5643 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -133,6 +133,18 @@ extern void efi_sync_low_kernel_mappings(void);
>  extern void efi_setup_page_tables(void);
>  extern void __init old_map_region(efi_memory_desc_t *md);
>  
> +struct efi_setup_data {
> +	u64 fw_vendor;
> +	u64 runtime;
> +	u64 tables;
> +	u64 smbios;
> +	u64 reserved[8];

What's that for?

> +	efi_memory_desc_t map[0];
> +};
> +
> +extern void parse_efi_setup(u64 phys_addr, u32 data_len);
> +extern struct efi_setup_data *esdata;
> +
>  #ifdef CONFIG_EFI
>  
>  static inline bool efi_is_native(void)

[ … ]

> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index c3a2aaa..fafeb40 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -504,8 +531,12 @@ static int __init efi_systab_init(void *phys)
>  		}
>  
>  		efi_systab.hdr = systab64->hdr;
> -		efi_systab.fw_vendor = systab64->fw_vendor;
> -		tmp |= systab64->fw_vendor;
> +
> +		if (esdata)
> +			efi_systab.fw_vendor = (unsigned long)esdata->fw_vendor;
> +		else
> +			efi_systab.fw_vendor = systab64->fw_vendor;

		efi_systab.fw_vendor = esdata ? (unsigned long)esdata->fw_vendor
					      : systab64->fw_vendor;

> +		tmp |= efi_systab.fw_vendor;
>  		efi_systab.fw_revision = systab64->fw_revision;
>  		efi_systab.con_in_handle = systab64->con_in_handle;
>  		tmp |= systab64->con_in_handle;
> @@ -519,13 +550,21 @@ static int __init efi_systab_init(void *phys)
>  		tmp |= systab64->stderr_handle;
>  		efi_systab.stderr = systab64->stderr;
>  		tmp |= systab64->stderr;
> -		efi_systab.runtime = (void *)(unsigned long)systab64->runtime;
> -		tmp |= systab64->runtime;
> +		if (esdata)
> +			efi_systab.runtime =
> +				(void *)(unsigned long)esdata->runtime;
> +		else
> +			efi_systab.runtime =
> +				(void *)(unsigned long)systab64->runtime;

Ditto. Which would take care of these linebreaks which are ugly.

> +		tmp |= (unsigned long)efi_systab.runtime;
>  		efi_systab.boottime = (void *)(unsigned long)systab64->boottime;
>  		tmp |= systab64->boottime;
>  		efi_systab.nr_tables = systab64->nr_tables;
> -		efi_systab.tables = systab64->tables;
> -		tmp |= systab64->tables;
> +		if (esdata)
> +			efi_systab.tables = (unsigned long)esdata->tables;
> +		else
> +			efi_systab.tables = systab64->tables;

Ditto.

> +		tmp |= efi_systab.tables;
>  
>  		early_iounmap(systab64, sizeof(*systab64));
>  #ifdef CONFIG_X86_32
> @@ -631,6 +670,41 @@ static int __init efi_memmap_init(void)
>  	return 0;
>  }
>  
> +static int __init efi_reuse_config(u64 tables, int nr_tables)

Static function - no need for "efi_" prefix.

> +{
> +	void *p, *tablep;
> +	int i, sz;
> +
> +	if (!efi_enabled(EFI_64BIT))
> +		return 0;
> +
> +	sz = sizeof(efi_config_table_64_t);
> +
> +	p = tablep = early_memremap(tables, nr_tables * sz);
> +	if (!p) {
> +		pr_err("Could not map Configuration table!\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < efi.systab->nr_tables; i++) {
> +		efi_guid_t guid;
> +
> +		guid = ((efi_config_table_64_t *)p)->guid;
> +
> +		/*
> +		HP z420 workstation smbios will be convert to
> +		virtual address after enter virtual mode.
> +		Thus in case kexec/kdump the physical address
> +		will be passed in setup_data.

Is that what the commit message above says? I'm having a hard time
parsing this text.

> +		*/
> +		if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
> +			((efi_config_table_64_t *)p)->table = esdata->smbios;

...and yet we do this for *every* UEFI box. Why not HP only?

> +		p += sz;
> +	}
> +	early_iounmap(tablep, nr_tables * sz);
> +	return 0;
> +}
> +
>  void __init efi_init(void)
>  {
>  	efi_char16_t *c16;
> @@ -676,6 +750,9 @@ void __init efi_init(void)
>  		efi.systab->hdr.revision >> 16,
>  		efi.systab->hdr.revision & 0xffff, vendor);
>  
> +	if (esdata && esdata->smbios)
> +		efi_reuse_config(efi.systab->tables, efi.systab->nr_tables);
> +
>  	if (efi_config_init(arch_tables))
>  		return;
>  
> @@ -886,6 +963,43 @@ ret:
>  }
>  
>  /*
> + * map efi regions which was passed via setup_data
> + * the virt_addr is a fixed addr which was used in
> + * 1st kernel of kexec boot.
> + */

Comment to 80 cols pls.

> +static void __init efi_map_regions_fixed(void)

Also no need for "efi_" prefix here.

> +{
> +	int i;
> +	unsigned long size;
> +	efi_memory_desc_t *md;
> +	u64 end, systab;
> +	void *p;
> +
> +	efi_runtime_map = kzalloc(nr_efi_runtime_map * memmap.desc_size,
> +				GFP_KERNEL);

Arg alignment.

> +	if (!efi_runtime_map)
> +		pr_err("Out of memory, EFI runtime on nested kexec non-functional!\n");
> +
> +	for (i = 0, p = efi_runtime_map; i < nr_efi_runtime_map; i++) {
> +		md = esdata->map + i;
> +		efi_map_region_fixed(md);

Gaah, this function should probably have a retval which signalizes
success/failure. For that I should probably teach __map_region to do
that too. On the TODO list.

> +		size = md->num_pages << PAGE_SHIFT;
> +		end = md->phys_addr + size;
> +
> +		systab = (u64) (unsigned long) efi_phys.systab;
> +		if (md->phys_addr <= systab && systab < end) {
> +			systab += md->virt_addr - md->phys_addr;
> +			efi.systab =
> +				(efi_system_table_t *) (unsigned long) systab;

CHECK: No space is necessary after a cast
#219: FILE: arch/x86/platform/efi/efi.c:993:
+                       efi.systab =
+                               (efi_system_table_t *) (unsigned long) systab;


And also, those broken lines are ugly. Just let it stick out over 80 cols.

> +		}
> +		if (efi_runtime_map) {
> +			memcpy(p, md, memmap.desc_size);
> +			p += memmap.desc_size;
> +		}
> +	}
> +}
> +
> +/*
>   * This function will switch the EFI runtime services to virtual mode.
>   * Essentially, we look through the EFI memmap and map every region that
>   * has the runtime attribute bit set in its memory descriptor into the
> @@ -901,6 +1015,10 @@ ret:
>   * so that we're in a different address space when calling a runtime
>   * function. For function arguments passing we do copy the PGDs of the
>   * kernel page table into ->trampoline_pgd prior to each call.
> + *
> + * Specially for kexec boot efi runtime maps in previous kernel should

			  boot, ...

> + * be passed in via setup_data. In that case runtime ranges will be mapped

					   case, ...

> + * to fixed virtual addresses exactly same as the ones in previous kernel.

"... to the same virtual addresses as the first kernel."

>   */
>  void __init efi_enter_virtual_mode(void)
>  {
> @@ -919,12 +1037,15 @@ void __init efi_enter_virtual_mode(void)
>  		return;
>  	}
>  
> -	efi_merge_regions();
> -
> -	new_memmap = efi_map_regions(&count);
> -	if (!new_memmap) {
> -		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> -		return;
> +	if (esdata)
> +		efi_map_regions_fixed();
> +	else {
> +		efi_merge_regions();
> +		new_memmap = efi_map_regions(&count);
> +		if (!new_memmap) {
> +			pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> +			return;
> +		}

CHECK: braces {} should be used on all arms of this statement
#253: FILE: arch/x86/platform/efi/efi.c:1040:
+       if (esdata)
[...]
+       else {
[...]

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

On 11/27/13 at 03:07pm, Borislav Petkov wrote:
> On Tue, Nov 26, 2013 at 01:57:52PM +0800, Dave Young wrote:
> > Add a new setup_data type SETUP_EFI for kexec use.
> > Passing the saved fw_vendor, runtime, config tables and
> > efi runtime mappings.
> > 
> > When entering virtual mode, directly mapping the efi
> > runtime ragions which we passed in previously. And skip
> > the step to call SetVirtualAddressMap.
> > 
> > Specially for HP z420 workstation it need another variable
> > saving,
> 
> Why the special handling? Does that mean, this is going to be the case
> for other HP UEFI implementations too?

I have only one HP machine for testing, Maybe Toshi can help to verify
on other machines. Just comment out the function efi_reuse_config to see
if kexec kernel panic.

> 
> > it's the smbios physical address, the HP bios
> > also update the SMBIOS address after entering virtual mode
> > besides of the standard fw_vendor,runtime and config table.
> > 
> > Tested on ovmf+qemu, lenovo thinkpad, a dell laptop and an
> > HP z420 workstation.
> > 
> > v2: refresh based on previous patch changes, code cleanup.
> > v3: use ioremap instead of phys_to_virt for esdata
> > 
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > ---
> >  arch/x86/include/asm/efi.h            |  12 +++
> >  arch/x86/include/uapi/asm/bootparam.h |   1 +
> >  arch/x86/kernel/setup.c               |   3 +
> >  arch/x86/platform/efi/efi.c           | 161 ++++++++++++++++++++++++++++++----
> >  4 files changed, 160 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> > index 9fbaeb2..73d5643 100644
> > --- a/arch/x86/include/asm/efi.h
> > +++ b/arch/x86/include/asm/efi.h
> > @@ -133,6 +133,18 @@ extern void efi_sync_low_kernel_mappings(void);
> >  extern void efi_setup_page_tables(void);
> >  extern void __init old_map_region(efi_memory_desc_t *md);
> >  
> > +struct efi_setup_data {
> > +	u64 fw_vendor;
> > +	u64 runtime;
> > +	u64 tables;
> > +	u64 smbios;
> > +	u64 reserved[8];
> 
> What's that for?

That's reserved for future extension use, who knows if we will need
to pass other fields in the future.

> 
> > +	efi_memory_desc_t map[0];
> > +};
> > +
> > +extern void parse_efi_setup(u64 phys_addr, u32 data_len);
> > +extern struct efi_setup_data *esdata;
> > +
> >  #ifdef CONFIG_EFI
> >  
> >  static inline bool efi_is_native(void)
> 
> [ … ]
> 
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index c3a2aaa..fafeb40 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -504,8 +531,12 @@ static int __init efi_systab_init(void *phys)
> >  		}
> >  
> >  		efi_systab.hdr = systab64->hdr;
> > -		efi_systab.fw_vendor = systab64->fw_vendor;
> > -		tmp |= systab64->fw_vendor;
> > +
> > +		if (esdata)
> > +			efi_systab.fw_vendor = (unsigned long)esdata->fw_vendor;
> > +		else
> > +			efi_systab.fw_vendor = systab64->fw_vendor;
> 
> 		efi_systab.fw_vendor = esdata ? (unsigned long)esdata->fw_vendor
> 					      : systab64->fw_vendor;

Ok, will update.

> 
> > +		tmp |= efi_systab.fw_vendor;
> >  		efi_systab.fw_revision = systab64->fw_revision;
> >  		efi_systab.con_in_handle = systab64->con_in_handle;
> >  		tmp |= systab64->con_in_handle;
> > @@ -519,13 +550,21 @@ static int __init efi_systab_init(void *phys)
> >  		tmp |= systab64->stderr_handle;
> >  		efi_systab.stderr = systab64->stderr;
> >  		tmp |= systab64->stderr;
> > -		efi_systab.runtime = (void *)(unsigned long)systab64->runtime;
> > -		tmp |= systab64->runtime;
> > +		if (esdata)
> > +			efi_systab.runtime =
> > +				(void *)(unsigned long)esdata->runtime;
> > +		else
> > +			efi_systab.runtime =
> > +				(void *)(unsigned long)systab64->runtime;
> 
> Ditto. Which would take care of these linebreaks which are ugly.

Will do.

> 
> > +		tmp |= (unsigned long)efi_systab.runtime;
> >  		efi_systab.boottime = (void *)(unsigned long)systab64->boottime;
> >  		tmp |= systab64->boottime;
> >  		efi_systab.nr_tables = systab64->nr_tables;
> > -		efi_systab.tables = systab64->tables;
> > -		tmp |= systab64->tables;
> > +		if (esdata)
> > +			efi_systab.tables = (unsigned long)esdata->tables;
> > +		else
> > +			efi_systab.tables = systab64->tables;
> 
> Ditto.

Will do

> 
> > +		tmp |= efi_systab.tables;
> >  
> >  		early_iounmap(systab64, sizeof(*systab64));
> >  #ifdef CONFIG_X86_32
> > @@ -631,6 +670,41 @@ static int __init efi_memmap_init(void)
> >  	return 0;
> >  }
> >  
> > +static int __init efi_reuse_config(u64 tables, int nr_tables)
> 
> Static function - no need for "efi_" prefix.

Ok. I'm not very satisfied with the function name, any better
suggestion?

reuse_config
use_old_config_phys_addr
update_config_entry_with_phys_addr
...

> 
> > +{
> > +	void *p, *tablep;
> > +	int i, sz;
> > +
> > +	if (!efi_enabled(EFI_64BIT))
> > +		return 0;
> > +
> > +	sz = sizeof(efi_config_table_64_t);
> > +
> > +	p = tablep = early_memremap(tables, nr_tables * sz);
> > +	if (!p) {
> > +		pr_err("Could not map Configuration table!\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (i = 0; i < efi.systab->nr_tables; i++) {
> > +		efi_guid_t guid;
> > +
> > +		guid = ((efi_config_table_64_t *)p)->guid;
> > +
> > +		/*
> > +		HP z420 workstation smbios will be convert to
> > +		virtual address after enter virtual mode.
> > +		Thus in case kexec/kdump the physical address
> > +		will be passed in setup_data.
> 
> Is that what the commit message above says? I'm having a hard time
> parsing this text.

Since Matt suggest to extend the function for other possible field
other than smbios, so I would like to move comment to the front
of the function like below:
/*
 * For kexec kernel there's some special config table entries which will be
 * converted to virtual addresses after entering virtual mode. In kexec kernel
 * we need the physical addresses instead, thus passing them via setup_data
 * and update the entries to physical addresses in this function.
 *
 * Currently only handles smbios which is necessary for HP z420.
 */
static int __init efi_reuse_config(u64 tables, int nr_tables)

> 
> > +		*/
> > +		if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID))
> > +			((efi_config_table_64_t *)p)->table = esdata->smbios;
> 
> ...and yet we do this for *every* UEFI box. Why not HP only?

Is there any idea in your mind how to add the code for HP only?

> 
> > +		p += sz;
> > +	}
> > +	early_iounmap(tablep, nr_tables * sz);
> > +	return 0;
> > +}
> > +
> >  void __init efi_init(void)
> >  {
> >  	efi_char16_t *c16;
> > @@ -676,6 +750,9 @@ void __init efi_init(void)
> >  		efi.systab->hdr.revision >> 16,
> >  		efi.systab->hdr.revision & 0xffff, vendor);
> >  
> > +	if (esdata && esdata->smbios)
> > +		efi_reuse_config(efi.systab->tables, efi.systab->nr_tables);
> > +
> >  	if (efi_config_init(arch_tables))
> >  		return;
> >  
> > @@ -886,6 +963,43 @@ ret:
> >  }
> >  
> >  /*
> > + * map efi regions which was passed via setup_data
> > + * the virt_addr is a fixed addr which was used in
> > + * 1st kernel of kexec boot.
> > + */
> 
> Comment to 80 cols pls.

Will do

> 
> > +static void __init efi_map_regions_fixed(void)
> 
> Also no need for "efi_" prefix here.

Ok.

> 
> > +{
> > +	int i;
> > +	unsigned long size;
> > +	efi_memory_desc_t *md;
> > +	u64 end, systab;
> > +	void *p;
> > +
> > +	efi_runtime_map = kzalloc(nr_efi_runtime_map * memmap.desc_size,
> > +				GFP_KERNEL);
> 
> Arg alignment.

Will do.

> 
> > +	if (!efi_runtime_map)
> > +		pr_err("Out of memory, EFI runtime on nested kexec non-functional!\n");
> > +
> > +	for (i = 0, p = efi_runtime_map; i < nr_efi_runtime_map; i++) {
> > +		md = esdata->map + i;
> > +		efi_map_region_fixed(md);
> 
> Gaah, this function should probably have a retval which signalizes
> success/failure. For that I should probably teach __map_region to do
> that too. On the TODO list.

Yes, __map_region return an error code will be better. Will you send a
patch for __map_region? or I can add one more patch along with next version.

> 
> > +		size = md->num_pages << PAGE_SHIFT;
> > +		end = md->phys_addr + size;
> > +
> > +		systab = (u64) (unsigned long) efi_phys.systab;
> > +		if (md->phys_addr <= systab && systab < end) {
> > +			systab += md->virt_addr - md->phys_addr;
> > +			efi.systab =
> > +				(efi_system_table_t *) (unsigned long) systab;
> 
> CHECK: No space is necessary after a cast
> #219: FILE: arch/x86/platform/efi/efi.c:993:

Will fix

> +                       efi.systab =
> +                               (efi_system_table_t *) (unsigned long) systab;
> 
> 
> And also, those broken lines are ugly. Just let it stick out over 80 cols.

Sure.

> 
> > +		}
> > +		if (efi_runtime_map) {
> > +			memcpy(p, md, memmap.desc_size);
> > +			p += memmap.desc_size;
> > +		}
> > +	}
> > +}
> > +
> > +/*
> >   * This function will switch the EFI runtime services to virtual mode.
> >   * Essentially, we look through the EFI memmap and map every region that
> >   * has the runtime attribute bit set in its memory descriptor into the
> > @@ -901,6 +1015,10 @@ ret:
> >   * so that we're in a different address space when calling a runtime
> >   * function. For function arguments passing we do copy the PGDs of the
> >   * kernel page table into ->trampoline_pgd prior to each call.
> > + *
> > + * Specially for kexec boot efi runtime maps in previous kernel should
> 
> 			  boot, ...

Will update.

> 
> > + * be passed in via setup_data. In that case runtime ranges will be mapped
> 
> 					   case, ...
> 
> > + * to fixed virtual addresses exactly same as the ones in previous kernel.
> 
> "... to the same virtual addresses as the first kernel."

Will do.

> 
> >   */
> >  void __init efi_enter_virtual_mode(void)
> >  {
> > @@ -919,12 +1037,15 @@ void __init efi_enter_virtual_mode(void)
> >  		return;
> >  	}
> >  
> > -	efi_merge_regions();
> > -
> > -	new_memmap = efi_map_regions(&count);
> > -	if (!new_memmap) {
> > -		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> > -		return;
> > +	if (esdata)
> > +		efi_map_regions_fixed();
> > +	else {
> > +		efi_merge_regions();
> > +		new_memmap = efi_map_regions(&count);
> > +		if (!new_memmap) {
> > +			pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> > +			return;
> > +		}
> 
> CHECK: braces {} should be used on all arms of this statement
> #253: FILE: arch/x86/platform/efi/efi.c:1040:

Ok.

> +       if (esdata)
> [...]
> +       else {
> [...]
> 

Thanks for carefully review
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Borislav Petkov]

On Fri, Nov 29, 2013 at 05:14:16PM +0800, Dave Young wrote:
> That's reserved for future extension use, who knows if we will need to
> pass other fields in the future.

Hrrmmm, 8*64 = 64 Bytes?? And you can't change it later because of the
situation where machines might be using older kexec-tools?

> > Static function - no need for "efi_" prefix.
> 
> Ok. I'm not very satisfied with the function name, any better
> suggestion?
> 
> reuse_config
> use_old_config_phys_addr
> update_config_entry_with_phys_addr

Looks like we're applying quirks so apply_quirks()?

And those kexec-specific quirks need to be behind CONFIG_KEXEC too, btw.

> Since Matt suggest to extend the function for other possible field
> other than smbios, so I would like to move comment to the front
> of the function like below:
> /*
>  * For kexec kernel there's some special config table entries which will be
>  * converted to virtual addresses after entering virtual mode. In kexec kernel
>  * we need the physical addresses instead, thus passing them via setup_data
>  * and update the entries to physical addresses in this function.
>  *
>  * Currently only handles smbios which is necessary for HP z420.
>  */

That's better.

> Is there any idea in your mind how to add the code for HP only?

DMI?

> Yes, __map_region return an error code will be better. Will you send
> a patch for __map_region? or I can add one more patch along with next
> version.

I was going to wait for your patches to settle down first so that I
don't disturb your work.

But if you want to fix this, I certainly wouldn't want to stop you :)
but please do so in a prepatch before your series which addresses only
this issue.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

On 11/29/13 at 05:46pm, Borislav Petkov wrote:
> On Fri, Nov 29, 2013 at 05:14:16PM +0800, Dave Young wrote:
> > That's reserved for future extension use, who knows if we will need to
> > pass other fields in the future.
> 
> Hrrmmm, 8*64 = 64 Bytes?? And you can't change it later because of the
> situation where machines might be using older kexec-tools?

Right, compatibility is the reason.

> 
> > > Static function - no need for "efi_" prefix.
> > 
> > Ok. I'm not very satisfied with the function name, any better
> > suggestion?
> > 
> > reuse_config
> > use_old_config_phys_addr
> > update_config_entry_with_phys_addr
> 
> Looks like we're applying quirks so apply_quirks()?

Ok, it looks better, will use apply_quirks, thank you.

> 
> And those kexec-specific quirks need to be behind CONFIG_KEXEC too, btw.

All these setup_data passing, remapping etc. is mostly for kexec, add
a lot of contiditional #if #endif makes the code a mess. I would prefer
to not do this if you are not strongly object. 

> 
> > Since Matt suggest to extend the function for other possible field
> > other than smbios, so I would like to move comment to the front
> > of the function like below:
> > /*
> >  * For kexec kernel there's some special config table entries which will be
> >  * converted to virtual addresses after entering virtual mode. In kexec kernel
> >  * we need the physical addresses instead, thus passing them via setup_data
> >  * and update the entries to physical addresses in this function.
> >  *
> >  * Currently only handles smbios which is necessary for HP z420.
> >  */
> 
> That's better.
> 
> > Is there any idea in your mind how to add the code for HP only?
> 
> DMI?

I'm confused how to connect this with DMI..

> 
> > Yes, __map_region return an error code will be better. Will you send
> > a patch for __map_region? or I can add one more patch along with next
> > version.
> 
> I was going to wait for your patches to settle down first so that I
> don't disturb your work.
> 
> But if you want to fix this, I certainly wouldn't want to stop you :)
> but please do so in a prepatch before your series which addresses only
> this issue.

Ok, so I will leave this to you after this series settle down.

Thanks
Dave

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Borislav Petkov]

On Mon, Dec 02, 2013 at 10:49:15AM +0800, Dave Young wrote:
> All these setup_data passing, remapping etc. is mostly for kexec,
> add a lot of contiditional #if #endif makes the code a mess. I would
> prefer to not do this if you are not strongly object.

Right, having efi_kexec.c could solve all that, see my other mail.

> I'm confused how to connect this with DMI..

This HP platform - like any other platform out there - has specific DMI
strings identifying it. Look for examples calling dmi_check_system().

> Ok, so I will leave this to you after this series settle down.

Ok,

/me puts it on the TODO list :)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Toshi Kani]

On Fri, 2013-11-29 at 17:14 +0800, Dave Young wrote:
> On 11/27/13 at 03:07pm, Borislav Petkov wrote:
> > On Tue, Nov 26, 2013 at 01:57:52PM +0800, Dave Young wrote:
> > > Add a new setup_data type SETUP_EFI for kexec use.
> > > Passing the saved fw_vendor, runtime, config tables and
> > > efi runtime mappings.
> > > 
> > > When entering virtual mode, directly mapping the efi
> > > runtime ragions which we passed in previously. And skip
> > > the step to call SetVirtualAddressMap.
> > > 
> > > Specially for HP z420 workstation it need another variable
> > > saving,
> > 
> > Why the special handling? Does that mean, this is going to be the case
> > for other HP UEFI implementations too?
> 
> I have only one HP machine for testing, Maybe Toshi can help to verify
> on other machines. Just comment out the function efi_reuse_config to see
> if kexec kernel panic.

My system (HP prototype server) did not need the special handling in
efi_reuse_config().  I will check with firmware team to get more
information on this.

Thanks,
-Toshi

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Toshi Kani]

On Mon, 2013-12-02 at 15:33 -0700, Toshi Kani wrote:
> On Fri, 2013-11-29 at 17:14 +0800, Dave Young wrote:
> > On 11/27/13 at 03:07pm, Borislav Petkov wrote:
> > > On Tue, Nov 26, 2013 at 01:57:52PM +0800, Dave Young wrote:
> > > > Add a new setup_data type SETUP_EFI for kexec use.
> > > > Passing the saved fw_vendor, runtime, config tables and
> > > > efi runtime mappings.
> > > > 
> > > > When entering virtual mode, directly mapping the efi
> > > > runtime ragions which we passed in previously. And skip
> > > > the step to call SetVirtualAddressMap.
> > > > 
> > > > Specially for HP z420 workstation it need another variable
> > > > saving,
> > > 
> > > Why the special handling? Does that mean, this is going to be the case
> > > for other HP UEFI implementations too?
> > 
> > I have only one HP machine for testing, Maybe Toshi can help to verify
> > on other machines. Just comment out the function efi_reuse_config to see
> > if kexec kernel panic.
> 
> My system (HP prototype server) did not need the special handling in
> efi_reuse_config().  I will check with firmware team to get more
> information on this.

I have one question.  I think this special handling assumes that
efi.smbios contains a virtual address on HP z420.  So, how does the 1st
kernel set a virtual address to efi.smbios in the first place?

Thanks,
-Toshi

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

On 12/02/13 at 06:31pm, Toshi Kani wrote:
> On Mon, 2013-12-02 at 15:33 -0700, Toshi Kani wrote:
> > On Fri, 2013-11-29 at 17:14 +0800, Dave Young wrote:
> > > On 11/27/13 at 03:07pm, Borislav Petkov wrote:
> > > > On Tue, Nov 26, 2013 at 01:57:52PM +0800, Dave Young wrote:
> > > > > Add a new setup_data type SETUP_EFI for kexec use.
> > > > > Passing the saved fw_vendor, runtime, config tables and
> > > > > efi runtime mappings.
> > > > > 
> > > > > When entering virtual mode, directly mapping the efi
> > > > > runtime ragions which we passed in previously. And skip
> > > > > the step to call SetVirtualAddressMap.
> > > > > 
> > > > > Specially for HP z420 workstation it need another variable
> > > > > saving,
> > > > 
> > > > Why the special handling? Does that mean, this is going to be the case
> > > > for other HP UEFI implementations too?
> > > 
> > > I have only one HP machine for testing, Maybe Toshi can help to verify
> > > on other machines. Just comment out the function efi_reuse_config to see
> > > if kexec kernel panic.
> > 
> > My system (HP prototype server) did not need the special handling in
> > efi_reuse_config().  I will check with firmware team to get more
> > information on this.
> 
> I have one question.  I think this special handling assumes that
> efi.smbios contains a virtual address on HP z420.  So, how does the 1st
> kernel set a virtual address to efi.smbios in the first place?

Hi, Toshi,

It's originally a physical address, I think kernel does not change it.
As the fw_vendor, runtime etc. are changed to virt addr by firmware, so I
assume smbios on Hp z420 is also changed by firmware after becoming virtual mode.

Thanks
Dave

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Toshi Kani]

On Tue, 2013-12-03 at 09:56 +0800, Dave Young wrote:
> On 12/02/13 at 06:31pm, Toshi Kani wrote:
> > On Mon, 2013-12-02 at 15:33 -0700, Toshi Kani wrote:
> > > On Fri, 2013-11-29 at 17:14 +0800, Dave Young wrote:
> > > > On 11/27/13 at 03:07pm, Borislav Petkov wrote:
> > > > > On Tue, Nov 26, 2013 at 01:57:52PM +0800, Dave Young wrote:
> > > > > > Add a new setup_data type SETUP_EFI for kexec use.
> > > > > > Passing the saved fw_vendor, runtime, config tables and
> > > > > > efi runtime mappings.
> > > > > > 
> > > > > > When entering virtual mode, directly mapping the efi
> > > > > > runtime ragions which we passed in previously. And skip
> > > > > > the step to call SetVirtualAddressMap.
> > > > > > 
> > > > > > Specially for HP z420 workstation it need another variable
> > > > > > saving,
> > > > > 
> > > > > Why the special handling? Does that mean, this is going to be the case
> > > > > for other HP UEFI implementations too?
> > > > 
> > > > I have only one HP machine for testing, Maybe Toshi can help to verify
> > > > on other machines. Just comment out the function efi_reuse_config to see
> > > > if kexec kernel panic.
> > > 
> > > My system (HP prototype server) did not need the special handling in
> > > efi_reuse_config().  I will check with firmware team to get more
> > > information on this.
> > 
> > I have one question.  I think this special handling assumes that
> > efi.smbios contains a virtual address on HP z420.  So, how does the 1st
> > kernel set a virtual address to efi.smbios in the first place?
> 
> Hi, Toshi,
> 
> It's originally a physical address, I think kernel does not change it.
> As the fw_vendor, runtime etc. are changed to virt addr by firmware, so I
> assume smbios on Hp z420 is also changed by firmware after becoming virtual mode.

Hi Dave,

Oh, I think I now understand what the issue was.  The z420 firmware
updates the SMBIOS table address in the EFI system table to a virtual
address after calling EFI SetVirtualAddressMap.  So, you are passing the
original physical address of the SMBIOS table from the 1st kernel to the
2nd kernel to put it back to physical.  Is that right? 

Thanks,
-Toshi

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

Hi, Toshi

> Oh, I think I now understand what the issue was.  The z420 firmware
> updates the SMBIOS table address in the EFI system table to a virtual
> address after calling EFI SetVirtualAddressMap.  So, you are passing the
> original physical address of the SMBIOS table from the 1st kernel to the
> 2nd kernel to put it back to physical.  Is that right? 

Right.

Thanks
Dave

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Toshi Kani]

On Wed, 2013-12-04 at 10:46 +0800, Dave Young wrote:
> Hi, Toshi
> 
> > Oh, I think I now understand what the issue was.  The z420 firmware
> > updates the SMBIOS table address in the EFI system table to a virtual
> > address after calling EFI SetVirtualAddressMap.  So, you are passing the
> > original physical address of the SMBIOS table from the 1st kernel to the
> > 2nd kernel to put it back to physical.  Is that right? 
> 
> Right.

Hi Dave,

The z420 firmware is based on some UEFI core that may be used by other
vendors as well.  Since this handling is totally harmless (just
redundant), I'd suggest not to have a platform check on this handling.

Thanks,
-Toshi

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

On 12/04/13 at 09:43am, Toshi Kani wrote:
> On Wed, 2013-12-04 at 10:46 +0800, Dave Young wrote:
> > Hi, Toshi
> > 
> > > Oh, I think I now understand what the issue was.  The z420 firmware
> > > updates the SMBIOS table address in the EFI system table to a virtual
> > > address after calling EFI SetVirtualAddressMap.  So, you are passing the
> > > original physical address of the SMBIOS table from the 1st kernel to the
> > > 2nd kernel to put it back to physical.  Is that right? 
> > 
> > Right.
> 
> Hi Dave,
> 
> The z420 firmware is based on some UEFI core that may be used by other
> vendors as well.  Since this handling is totally harmless (just
> redundant), I'd suggest not to have a platform check on this handling.

I have same worry as well, so I agree with you.

Thanks
Dave

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Borislav Petkov]

On Thu, Dec 05, 2013 at 09:56:15AM +0800, Dave Young wrote:
> > The z420 firmware is based on some UEFI core that may be used by other
> > vendors as well.  Since this handling is totally harmless (just
> > redundant), I'd suggest not to have a platform check on this handling.
> 
> I have same worry as well, so I agree with you.

Btw, why does kexec need the ->smbios table at all? I see it being
passed in efi_setup_data.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Toshi Kani]

On Thu, 2013-12-05 at 12:51 +0100, Borislav Petkov wrote:
> On Thu, Dec 05, 2013 at 09:56:15AM +0800, Dave Young wrote:
> > > The z420 firmware is based on some UEFI core that may be used by other
> > > vendors as well.  Since this handling is totally harmless (just
> > > redundant), I'd suggest not to have a platform check on this handling.
> > 
> > I have same worry as well, so I agree with you.
> 
> Btw, why does kexec need the ->smbios table at all? I see it being
> passed in efi_setup_data.

Hi Boris,

The smbios in efi_setup_data is necessary for kexec to pass the physical
address of the SMBIOS table from the 1st kernel to the 2nd kernel.

The kernel boot sequence proceeds in the following order.  Step 2
requires efi.smbios to be the physical address.  However, Dave found
that EFI system table has a virtual address of SMBIOS in step 1.  Hence,
he sets it back to the physical address with the smbios in
efi_setup_data.  (When it is still the physical address, it simply sets
the same value.)

1. efi_init() - Set efi.smbios from EFI system table
2. dmi_scan_machine() - Temporary map efi.smbios to access SMBIOS table
3. efi_enter_virtual_mode() - Map EFI ranges

Thanks,
-Toshi

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Borislav Petkov]

On Thu, Dec 05, 2013 at 08:56:02AM -0700, Toshi Kani wrote:
> The smbios in efi_setup_data is necessary for kexec to pass the physical
> address of the SMBIOS table from the 1st kernel to the 2nd kernel.
> 
> The kernel boot sequence proceeds in the following order.  Step 2
> requires efi.smbios to be the physical address.  However, Dave found
> that EFI system table has a virtual address of SMBIOS in step 1.  Hence,
> he sets it back to the physical address with the smbios in
> efi_setup_data.  (When it is still the physical address, it simply sets
> the same value.)
> 
> 1. efi_init() - Set efi.smbios from EFI system table
> 2. dmi_scan_machine() - Temporary map efi.smbios to access SMBIOS table
> 3. efi_enter_virtual_mode() - Map EFI ranges

Thanks Toshi, it is all clear now.

@Dave: you might want to put Toshi's explanation in the commit message
of your patch so that we know *why* the change is needed.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data

[Dave Young]

On 12/05/13 at 09:52pm, Borislav Petkov wrote:
> On Thu, Dec 05, 2013 at 08:56:02AM -0700, Toshi Kani wrote:
> > The smbios in efi_setup_data is necessary for kexec to pass the physical
> > address of the SMBIOS table from the 1st kernel to the 2nd kernel.
> > 
> > The kernel boot sequence proceeds in the following order.  Step 2
> > requires efi.smbios to be the physical address.  However, Dave found
> > that EFI system table has a virtual address of SMBIOS in step 1.  Hence,
> > he sets it back to the physical address with the smbios in
> > efi_setup_data.  (When it is still the physical address, it simply sets
> > the same value.)
> > 
> > 1. efi_init() - Set efi.smbios from EFI system table
> > 2. dmi_scan_machine() - Temporary map efi.smbios to access SMBIOS table
> > 3. efi_enter_virtual_mode() - Map EFI ranges
> 
> Thanks Toshi, it is all clear now.
> 
> @Dave: you might want to put Toshi's explanation in the commit message
> of your patch so that we know *why* the change is needed.

Ok, will do.

Thanks
Dave

[PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec

[Dave Young]

For kexec/kdump kernel efi runtime mappings are saved, printing original whole
memmap ranges does not make sense anymore. So introduce a new function to only
print runtime maps in case kexec/kdump kernel is used.

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index fafeb40..c65b0b8 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -430,6 +430,24 @@ int __init efi_memblock_x86_reserve_range(void)
 	return 0;
 }
 
+/* for kexec kernel runtime maps are passed in setup_data */
+static void __init print_saved_runtime_map(void)
+{
+#ifdef EFI_DEBUG
+	int i;
+	efi_memory_desc_t *md;
+
+	for (i = 0; i < nr_efi_runtime_map; i++) {
+		md = esdata->map + i;
+		pr_info("mem%02u: type=%u, attr=0x%llx, "
+			"range=[0x%016llx-0x%016llx) (%lluMB)\n",
+			i, md->type, md->attribute, md->phys_addr,
+			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
+			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+	}
+#endif  /*  EFI_DEBUG  */
+}
+
 static void __init print_efi_memmap(void)
 {
 #ifdef EFI_DEBUG
@@ -782,7 +800,10 @@ void __init efi_init(void)
 		x86_platform.set_wallclock = efi_set_rtc_mmss;
 	}
 #endif
-	print_efi_memmap();
+	if (esdata)
+		print_saved_runtime_map();
+	else
+		print_efi_memmap();
 }
 
 void __init efi_late_init(void)
-- 
1.8.3.1

Re: [PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec

[Matt Fleming]

On Tue, 26 Nov, at 01:57:53PM, Dave Young wrote:
> For kexec/kdump kernel efi runtime mappings are saved, printing original whole
> memmap ranges does not make sense anymore. So introduce a new function to only
> print runtime maps in case kexec/kdump kernel is used.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  arch/x86/platform/efi/efi.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index fafeb40..c65b0b8 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -430,6 +430,24 @@ int __init efi_memblock_x86_reserve_range(void)
>  	return 0;
>  }
>  
> +/* for kexec kernel runtime maps are passed in setup_data */
> +static void __init print_saved_runtime_map(void)
> +{
> +#ifdef EFI_DEBUG
> +	int i;
> +	efi_memory_desc_t *md;
> +
> +	for (i = 0; i < nr_efi_runtime_map; i++) {
> +		md = esdata->map + i;
> +		pr_info("mem%02u: type=%u, attr=0x%llx, "
> +			"range=[0x%016llx-0x%016llx) (%lluMB)\n",
> +			i, md->type, md->attribute, md->phys_addr,
> +			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
> +			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
> +	}
> +#endif  /*  EFI_DEBUG  */
> +}
> +
>  static void __init print_efi_memmap(void)
>  {
>  #ifdef EFI_DEBUG
> @@ -782,7 +800,10 @@ void __init efi_init(void)
>  		x86_platform.set_wallclock = efi_set_rtc_mmss;
>  	}
>  #endif
> -	print_efi_memmap();
> +	if (esdata)
> +		print_saved_runtime_map();
> +	else
> +		print_efi_memmap();
>  }

Heh, you can probably already guess what I'm going to say here...

How about using a single function to dump the memory ranges irrespective
of whether the memory map comes from 'memmap' or 'esdata'? e.g.
something along the lines of,

	if (esdata)
		print_efi_memmap(esdata->map, nr_efi_runtime_map,
				 sizeof(esdata->map[0]));
	else
		print_efi_memmap(memmap.map, memmap.nr_map,
				 memmap.desc_size);

?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec

[Borislav Petkov]

On Wed, Nov 27, 2013 at 10:27:01AM +0000, Matt Fleming wrote:
> Heh, you can probably already guess what I'm going to say here...

I guessed :-)

> How about using a single function to dump the memory ranges irrespective
> of whether the memory map comes from 'memmap' or 'esdata'? e.g.
> something along the lines of,
> 
> 	if (esdata)
> 		print_efi_memmap(esdata->map, nr_efi_runtime_map,
> 				 sizeof(esdata->map[0]));
> 	else
> 		print_efi_memmap(memmap.map, memmap.nr_map,
> 				 memmap.desc_size);

And while you're at it:

WARNING: quoted string split across lines
#19: FILE: arch/x86/platform/efi/efi.c:443:
+               pr_info("mem%02u: type=%u, attr=0x%llx, "
+                       "range=[0x%016llx-0x%016llx) (%lluMB)\n",


Btw, do we really want to dump the same map again in the second kernel?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec

[Dave Young]

On 11/27/13 at 03:27pm, Borislav Petkov wrote:
> On Wed, Nov 27, 2013 at 10:27:01AM +0000, Matt Fleming wrote:
> > Heh, you can probably already guess what I'm going to say here...
> 
> I guessed :-)
> 
> > How about using a single function to dump the memory ranges irrespective
> > of whether the memory map comes from 'memmap' or 'esdata'? e.g.
> > something along the lines of,
> > 
> > 	if (esdata)
> > 		print_efi_memmap(esdata->map, nr_efi_runtime_map,
> > 				 sizeof(esdata->map[0]));
> > 	else
> > 		print_efi_memmap(memmap.map, memmap.nr_map,
> > 				 memmap.desc_size);
> 
> And while you're at it:
> 
> WARNING: quoted string split across lines
> #19: FILE: arch/x86/platform/efi/efi.c:443:
> +               pr_info("mem%02u: type=%u, attr=0x%llx, "
> +                       "range=[0x%016llx-0x%016llx) (%lluMB)\n",

I see the warnings, because it's originally in print_efi_memmap so I just keep
new function same. Anyway after switching to use one function there will be
no such warning.

> 
> 
> Btw, do we really want to dump the same map again in the second kernel?

It's for debugging purpose, I think it's helpful.

Thanks for review
Dave

Re: [PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec

[Borislav Petkov]

On Fri, Nov 29, 2013 at 04:50:50PM +0800, Dave Young wrote:
> It's for debugging purpose, I think it's helpful.

Why? The first kernel did dump it already.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec

[Dave Young]

On 11/29/13 at 05:47pm, Borislav Petkov wrote:
> On Fri, Nov 29, 2013 at 04:50:50PM +0800, Dave Young wrote:
> > It's for debugging purpose, I think it's helpful.
> 
> Why? The first kernel did dump it already.

As for the efi printk though 1st kernel already printed them out, but kexec kernel
context is diffrent from 1st kernel, it's a fresh reboot we do need reprint them.

I think kexec reboot is just like a normal reboot from user's point of view.

Thanks
Dave

Re: [PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec

[Dave Young]

On 11/27/13 at 10:27am, Matt Fleming wrote:
> On Tue, 26 Nov, at 01:57:53PM, Dave Young wrote:
> > For kexec/kdump kernel efi runtime mappings are saved, printing original whole
> > memmap ranges does not make sense anymore. So introduce a new function to only
> > print runtime maps in case kexec/kdump kernel is used.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/platform/efi/efi.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index fafeb40..c65b0b8 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -430,6 +430,24 @@ int __init efi_memblock_x86_reserve_range(void)
> >  	return 0;
> >  }
> >  
> > +/* for kexec kernel runtime maps are passed in setup_data */
> > +static void __init print_saved_runtime_map(void)
> > +{
> > +#ifdef EFI_DEBUG
> > +	int i;
> > +	efi_memory_desc_t *md;
> > +
> > +	for (i = 0; i < nr_efi_runtime_map; i++) {
> > +		md = esdata->map + i;
> > +		pr_info("mem%02u: type=%u, attr=0x%llx, "
> > +			"range=[0x%016llx-0x%016llx) (%lluMB)\n",
> > +			i, md->type, md->attribute, md->phys_addr,
> > +			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
> > +			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
> > +	}
> > +#endif  /*  EFI_DEBUG  */
> > +}
> > +
> >  static void __init print_efi_memmap(void)
> >  {
> >  #ifdef EFI_DEBUG
> > @@ -782,7 +800,10 @@ void __init efi_init(void)
> >  		x86_platform.set_wallclock = efi_set_rtc_mmss;
> >  	}
> >  #endif
> > -	print_efi_memmap();
> > +	if (esdata)
> > +		print_saved_runtime_map();
> > +	else
> > +		print_efi_memmap();
> >  }
> 
> Heh, you can probably already guess what I'm going to say here...
> 
> How about using a single function to dump the memory ranges irrespective
> of whether the memory map comes from 'memmap' or 'esdata'? e.g.
> something along the lines of,
> 
> 	if (esdata)
> 		print_efi_memmap(esdata->map, nr_efi_runtime_map,
> 				 sizeof(esdata->map[0]));
> 	else
> 		print_efi_memmap(memmap.map, memmap.nr_map,
> 				 memmap.desc_size);
> 
> ?

Yes, looks better, will do

Thanks for review
Dave

[PATCH v4 09/12] x86: add xloadflags bit for efi runtime support on kexec

[Dave Young]

Old kexec-tools can not load new kernel. The reason is previously kexec-tools
do not fill efi_info in x86 setup header thus efi init fail and switch
to noefi boot. In new kexec-tools it will by default fill efi_info and
pass other efi required infomation to 2nd kernel so kexec kernel efi
initialization will success finally.

To prevent from breaking userspace, add a new xloadflags bit so kexec-tools
will check the flag and switch to old logic.

changelog:
Matt: +&& defined(CONFIG_KEXEC)
HPA: document the flag.

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/x86/boot.txt            | 3 +++
 arch/x86/boot/header.S                | 9 ++++++++-
 arch/x86/include/uapi/asm/bootparam.h | 1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt
index f4f268c..cb81741d 100644
--- a/Documentation/x86/boot.txt
+++ b/Documentation/x86/boot.txt
@@ -608,6 +608,9 @@ Protocol:       2.12+
 	- If 1, the kernel supports the 64-bit EFI handoff entry point
           given at handover_offset + 0x200.
 
+  Bit 4 (read): XLF_EFI_KEXEC
+	- If 1, the kernel supports kexec EFI boot with EFI runtime support.
+
 Field name:	cmdline_size
 Type:		read
 Offset/size:	0x238/4
diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 9ec06a1..ec3b8ba 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -391,7 +391,14 @@ xloadflags:
 #else
 # define XLF23 0
 #endif
-			.word XLF0 | XLF1 | XLF23
+
+#if defined(CONFIG_X86_64) && defined(CONFIG_EFI) && defined(CONFIG_KEXEC)
+# define XLF4 XLF_EFI_KEXEC
+#else
+# define XLF4 0
+#endif
+
+			.word XLF0 | XLF1 | XLF23 | XLF4
 
 cmdline_size:   .long   COMMAND_LINE_SIZE-1     #length of the command line,
                                                 #added with boot protocol
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index ed6afc9..505eecf 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -24,6 +24,7 @@
 #define XLF_CAN_BE_LOADED_ABOVE_4G	(1<<1)
 #define XLF_EFI_HANDOVER_32		(1<<2)
 #define XLF_EFI_HANDOVER_64		(1<<3)
+#define XLF_EFI_KEXEC			(1<<4)
 
 #ifndef __ASSEMBLY__
 
-- 
1.8.3.1

Re: [PATCH v4 09/12] x86: add xloadflags bit for efi runtime support on kexec

[Borislav Petkov]

On Tue, Nov 26, 2013 at 01:57:54PM +0800, Dave Young wrote:
> Old kexec-tools can not load new kernel. The reason is previously kexec-tools

The reason is, kexec-tools ... previously, thus efi failed to initialize ...

> do not fill efi_info in x86 setup header thus efi init fail and switch
> to noefi boot. In new kexec-tools it will by default fill efi_info and
> pass other efi required infomation to 2nd kernel so kexec kernel efi
> initialization will success finally.

		      succeed

> 
> To prevent from breaking userspace, add a new xloadflags bit so kexec-tools
> will check the flag and switch to old logic.
> 
> changelog:
> Matt: +&& defined(CONFIG_KEXEC)
> HPA: document the flag.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Other than that:

Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 09/12] x86: add xloadflags bit for efi runtime support on kexec

[Dave Young]

On 11/27/13 at 03:27pm, Borislav Petkov wrote:
> On Tue, Nov 26, 2013 at 01:57:54PM +0800, Dave Young wrote:
> > Old kexec-tools can not load new kernel. The reason is previously kexec-tools
> 
> The reason is, kexec-tools ... previously, thus efi failed to initialize ...

Good suggestion

> 
> > do not fill efi_info in x86 setup header thus efi init fail and switch
> > to noefi boot. In new kexec-tools it will by default fill efi_info and
> > pass other efi required infomation to 2nd kernel so kexec kernel efi
> > initialization will success finally.
> 
> 		      succeed

Will fix

> 
> > 
> > To prevent from breaking userspace, add a new xloadflags bit so kexec-tools
> > will check the flag and switch to old logic.
> > 
> > changelog:
> > Matt: +&& defined(CONFIG_KEXEC)
> > HPA: document the flag.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Other than that:
> 
> Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> 

Thanks
Dave

[PATCH v4 10/12] x86: export x86 boot_params to sysfs

[Dave Young]

kexec-tools use boot_params for getting the 1st kernel hardware_subarch,
the kexec kernel efi runtime support also need read the old efi_info from
boot_params. Currently it exists in debugfs which is not a good place for
such infomation. Per HPA, we should avoid of "sploit debugfs".

In this patch /sys/kernel/boot_params are exported, also the setup_data
is exported as a subdirectory. For original debugfs since it's already
there for long time and kexec-tools is using it for hardware_subarch so
let's do not remove them for now.

Structure are like below:

/sys/kernel/boot_params
├── data		/* binary data for boot_params */
├── setup_data  	/* subdirectory for setup_data if there's any */
│   ├── 0		/* the first setup_data node */
│   │   ├── data	/* binary data for setup_data node 0 */
│   │   └── type	/* setup_data type of setup_data node 0, hex string */
|   [snip]		/* other setup_data nodes ... */
└── version		/* hex string for boot protocal version */

Changelog:
Greg: use __ATTR_RO() and group attr.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 Documentation/ABI/testing/sysfs-kernel-boot_params |  40 +++
 arch/x86/kernel/Makefile                           |   2 +-
 arch/x86/kernel/ksysfs.c                           | 339 +++++++++++++++++++++
 3 files changed, 380 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-kernel-boot_params
 create mode 100644 arch/x86/kernel/ksysfs.c

diff --git a/Documentation/ABI/testing/sysfs-kernel-boot_params b/Documentation/ABI/testing/sysfs-kernel-boot_params
new file mode 100644
index 0000000..8014a93
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-boot_params
@@ -0,0 +1,40 @@
+What:		/sys/kernel/boot_params
+Date:		November 2013
+Contact:	Dave Young <dyoung@redhat.com>
+Description:
+		The /sys/kernel/boot_params directory contains two
+		files: "data" and "version" and one subdirectory "setup_data".
+		It is used to export the kernel boot parameters of x86
+		platform to user space for kexec and debugging purpose.
+
+		If there's no setup_data in boot_params the subdirectory will
+		not be created.
+
+		"data" file is the binary representation of struct boot_params.
+
+		"version" file is the string representation of boot
+		protocol version.
+
+		"setup_data" subdirectory contains the setup_data data
+		structure in boot_params. setup_data is maintained in kernel
+		as a link list. In "setup_data" subdirectory there's one
+		subdirectory for each link list node named with the number
+		of the list nodes. The list node subdirectory contains two
+		files "type" and "data". "type" file is the string
+		representation of setup_data type. "data" file is the binary
+		representation of setup_data payload.
+
+		The whole boot_params directory structure is like below:
+		/sys/kernel/boot_params
+		├── data
+		├── setup_data
+		│   ├── 0
+		│   │   ├── data
+		│   │   └── type
+		│   └── 1
+		│       ├── data
+		│       └── type
+		└── version
+
+Users:
+		Kexec Mailing List <kexec@lists.infradead.org>
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index a5408b9..473a4bb 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -35,7 +35,7 @@ obj-y			+= alternative.o i8253.o pci-nommu.o hw_breakpoint.o
 obj-y			+= tsc.o io_delay.o rtc.o
 obj-y			+= pci-iommu_table.o
 obj-y			+= resource.o
-
+obj-$(CONFIG_SYSFS)	+= ksysfs.o
 obj-y				+= process.o
 obj-y				+= i387.o xsave.o
 obj-y				+= ptrace.o
diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
new file mode 100644
index 0000000..3f91207
--- /dev/null
+++ b/arch/x86/kernel/ksysfs.c
@@ -0,0 +1,339 @@
+/*
+ * Architecture specific sysfs attributes in /sys/kernel
+ *
+ * Copyright (C) 2007, Intel Corp.
+ *      Huang Ying <ying.huang@intel.com>
+ * Copyright (C) 2013, 2013 Red Hat, Inc.
+ *      Dave Young <dyoung@redhat.com>
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/kobject.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/init.h>
+#include <linux/stat.h>
+#include <linux/slab.h>
+#include <linux/mm.h>
+
+#include <asm/setup.h>
+
+static ssize_t version_show(struct kobject *kobj,
+					struct kobj_attribute *attr, char *buf)
+{
+	return sprintf(buf, "0x%04x\n", boot_params.hdr.version);
+}
+
+static struct kobj_attribute boot_params_version_attr = __ATTR_RO(version);
+
+static ssize_t boot_params_data_read(struct file *fp, struct kobject *kobj,
+				     struct bin_attribute *bin_attr,
+				     char *buf, loff_t off, size_t count)
+{
+	memcpy(buf, (void *)&boot_params + off, count);
+	return count;
+}
+
+static struct bin_attribute boot_params_data_attr = {
+	.attr = {
+		.name = "data",
+		.mode = S_IRUGO,
+	},
+	.read = boot_params_data_read,
+	.size = sizeof(boot_params),
+};
+
+static struct attribute *boot_params_version_attrs[] = {
+	&boot_params_version_attr.attr,
+	NULL,
+};
+
+static struct bin_attribute *boot_params_data_attrs[] = {
+	&boot_params_data_attr,
+	NULL,
+};
+
+static struct attribute_group boot_params_attr_group = {
+	.attrs = boot_params_version_attrs,
+	.bin_attrs = boot_params_data_attrs,
+};
+
+static int kobj_to_setup_data_nr(struct kobject *kobj, int *nr)
+{
+	const char *name;
+
+	name = kobject_name(kobj);
+	return kstrtoint(name, 10, nr);
+}
+
+static int get_setup_data_paddr(int nr, u64 *paddr)
+{
+	int i = 0;
+	struct setup_data *data;
+	u64 pa_data = boot_params.hdr.setup_data;
+
+	while (pa_data) {
+		if (nr == i) {
+			*paddr = pa_data;
+			return 0;
+		}
+		data = ioremap_cache(pa_data, sizeof(*data));
+		if (!data)
+			return -ENOMEM;
+
+		pa_data = data->next;
+		iounmap(data);
+		i++;
+	}
+	return -EINVAL;
+}
+
+static int __init get_setup_data_size(int nr, size_t *size)
+{
+	int i = 0;
+	struct setup_data *data;
+	u64 pa_data = boot_params.hdr.setup_data;
+
+	while (pa_data) {
+		data = ioremap_cache(pa_data, sizeof(*data));
+		if (!data)
+			return -ENOMEM;
+		if (nr == i) {
+			*size = data->len;
+			iounmap(data);
+			return 0;
+		}
+
+		pa_data = data->next;
+		iounmap(data);
+		i++;
+	}
+	return -EINVAL;
+}
+
+static ssize_t type_show(struct kobject *kobj,
+					struct kobj_attribute *attr, char *buf)
+{
+	int nr, ret;
+	u64 paddr;
+	struct setup_data *data;
+
+	ret = kobj_to_setup_data_nr(kobj, &nr);
+	if (ret)
+		return ret;
+
+	ret = get_setup_data_paddr(nr, &paddr);
+	if (ret)
+		return ret;
+	data = ioremap_cache(paddr, sizeof(*data));
+	if (!data)
+		return -ENOMEM;
+
+	ret = sprintf(buf, "0x%x\n", data->type);
+	iounmap(data);
+	return ret;
+}
+
+static ssize_t setup_data_data_read(struct file *fp,
+				    struct kobject *kobj,
+				    struct bin_attribute *bin_attr,
+				    char *buf,
+				    loff_t off, size_t count)
+{
+	int nr, ret = 0;
+	u64 paddr;
+	struct setup_data *data;
+	void *p;
+
+	ret = kobj_to_setup_data_nr(kobj, &nr);
+	if (ret)
+		return ret;
+
+	ret = get_setup_data_paddr(nr, &paddr);
+	if (ret)
+		return ret;
+	data = ioremap_cache(paddr, sizeof(*data));
+	if (!data)
+		return -ENOMEM;
+
+	if (off > data->len) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (count > data->len - off)
+		count = data->len - off;
+
+	if (!count)
+		goto out;
+
+	ret = count;
+	p = ioremap_cache(paddr + sizeof(*data), data->len);
+	if (!p) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	memcpy(buf, p + off, count);
+	iounmap(p);
+out:
+	iounmap(data);
+	return ret;
+}
+
+static struct kobj_attribute type_attr = __ATTR_RO(type);
+
+static struct bin_attribute data_attr = {
+	.attr = {
+		.name = "data",
+		.mode = S_IRUGO,
+	},
+	.read = setup_data_data_read,
+};
+
+static struct attribute *setup_data_type_attrs[] = {
+	&type_attr.attr,
+	NULL,
+};
+
+static struct bin_attribute *setup_data_data_attrs[] = {
+	&data_attr,
+	NULL,
+};
+
+static struct attribute_group setup_data_attr_group = {
+	.attrs = setup_data_type_attrs,
+	.bin_attrs = setup_data_data_attrs,
+};
+
+static int __init create_setup_data_node(struct kobject *parent,
+					struct kobject **kobjp, int nr)
+{
+	int ret = 0;
+	size_t size;
+	struct kobject *kobj;
+	char name[16]; /* should be enough for setup_data nodes numbers */
+	snprintf(name, 16, "%d", nr);
+
+	kobj = kobject_create_and_add(name, parent);
+	if (!kobj)
+		return -ENOMEM;
+
+	ret = get_setup_data_size(nr, &size);
+	if (ret)
+		goto out_kobj;
+
+	data_attr.size = size;
+	ret = sysfs_create_group(kobj, &setup_data_attr_group);
+	if (ret)
+		goto out_kobj;
+	*kobjp = kobj;
+
+	return 0;
+out_kobj:
+	kobject_put(kobj);
+	return ret;
+}
+
+static void __init cleanup_setup_data_node(struct kobject *kobj)
+{
+	sysfs_remove_group(kobj, &setup_data_attr_group);
+	kobject_put(kobj);
+}
+
+static int __init get_setup_data_total_num(u64 pa_data, int *nr)
+{
+	int ret = 0;
+	struct setup_data *data;
+
+	*nr = 0;
+	while (pa_data) {
+		*nr += 1;
+		data = ioremap_cache(pa_data, sizeof(*data));
+		if (!data) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		pa_data = data->next;
+		iounmap(data);
+	}
+
+out:
+	return ret;
+}
+
+static int __init create_setup_data_nodes(struct kobject *parent)
+{
+	struct kobject *setup_data_kobj, **kobjp;
+	u64 pa_data;
+	int i, j, nr, ret = 0;
+
+	pa_data = boot_params.hdr.setup_data;
+	if (!pa_data)
+		return 0;
+
+	setup_data_kobj = kobject_create_and_add("setup_data", parent);
+	if (!setup_data_kobj) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = get_setup_data_total_num(pa_data, &nr);
+	if (ret)
+		goto out_setup_data_kobj;
+
+	kobjp = kmalloc(sizeof(*kobjp) * nr, GFP_KERNEL);
+	if (!kobjp) {
+		ret = -ENOMEM;
+		goto out_setup_data_kobj;
+	}
+
+	for (i = 0; i < nr; i++) {
+		ret = create_setup_data_node(setup_data_kobj, kobjp + i, i);
+		if (ret)
+			goto out_clean_nodes;
+	}
+
+	kfree(kobjp);
+	return 0;
+
+out_clean_nodes:
+	for (j = i - 1; j > 0; j--)
+		cleanup_setup_data_node(*(kobjp + j));
+	kfree(kobjp);
+out_setup_data_kobj:
+	kobject_put(setup_data_kobj);
+out:
+	return ret;
+}
+
+static int __init boot_params_ksysfs_init(void)
+{
+	int ret;
+	struct kobject *boot_params_kobj;
+
+	boot_params_kobj = kobject_create_and_add("boot_params",
+						  kernel_kobj);
+	if (!boot_params_kobj) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = sysfs_create_group(boot_params_kobj, &boot_params_attr_group);
+	if (ret)
+		goto out_boot_params_kobj;
+
+	ret = create_setup_data_nodes(boot_params_kobj);
+	if (ret)
+		goto out_create_group;
+
+	return 0;
+out_create_group:
+	sysfs_remove_group(boot_params_kobj, &boot_params_attr_group);
+out_boot_params_kobj:
+	kobject_put(boot_params_kobj);
+out:
+	return ret;
+}
+
+arch_initcall(boot_params_ksysfs_init);
-- 
1.8.3.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v4 10/12] x86: export x86 boot_params to sysfs

[Matt Fleming]

On Tue, 26 Nov, at 01:57:55PM, Dave Young wrote:
> +Users:
> +		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>

"Kexec" please.

> +static ssize_t version_show(struct kobject *kobj,
> +					struct kobj_attribute *attr, char *buf)

This is pretty strange indentation. Usually we prefer to line up the
arguments like you've done elsewhere in this file.

> +{
> +	return sprintf(buf, "0x%04x\n", boot_params.hdr.version);
> +}
> +
> +static struct kobj_attribute boot_params_version_attr = __ATTR_RO(version);
> +
> +static ssize_t boot_params_data_read(struct file *fp, struct kobject *kobj,
> +				     struct bin_attribute *bin_attr,
> +				     char *buf, loff_t off, size_t count)
> +{
> +	memcpy(buf, (void *)&boot_params + off, count);
> +	return count;
> +}

Don't we need some checks here to ensure we don't read past the end of
boot_params?

> +static ssize_t type_show(struct kobject *kobj,
> +					struct kobj_attribute *attr, char *buf)

More whacky indentation.

> +static int __init create_setup_data_node(struct kobject *parent,
> +					struct kobject **kobjp, int nr)

Funky indentation.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 10/12] x86: export x86 boot_params to sysfs

[Dave Young]

On 11/27/13 at 11:20am, Matt Fleming wrote:
> On Tue, 26 Nov, at 01:57:55PM, Dave Young wrote:
> > +Users:
> > +		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
> 
> "Kexec" please.

Will change

> 
> > +static ssize_t version_show(struct kobject *kobj,
> > +					struct kobj_attribute *attr, char *buf)
> 
> This is pretty strange indentation. Usually we prefer to line up the
> arguments like you've done elsewhere in this file.

Ok.

> 
> > +{
> > +	return sprintf(buf, "0x%04x\n", boot_params.hdr.version);
> > +}
> > +
> > +static struct kobj_attribute boot_params_version_attr = __ATTR_RO(version);
> > +
> > +static ssize_t boot_params_data_read(struct file *fp, struct kobject *kobj,
> > +				     struct bin_attribute *bin_attr,
> > +				     char *buf, loff_t off, size_t count)
> > +{
> > +	memcpy(buf, (void *)&boot_params + off, count);
> > +	return count;
> > +}
> 
> Don't we need some checks here to ensure we don't read past the end of
> boot_params?

The size was added below, so I would assume file read will not beyond the
file size:
.size = sizeof(boot_params),

> 
> > +static ssize_t type_show(struct kobject *kobj,
> > +					struct kobj_attribute *attr, char *buf)
> 
> More whacky indentation.

Will fix

> 
> > +static int __init create_setup_data_node(struct kobject *parent,
> > +					struct kobject **kobjp, int nr)
> 
> Funky indentation.

Will fix

Thanks for review
Dave

Re: [PATCH v4 10/12] x86: export x86 boot_params to sysfs

[Borislav Petkov]

On Tue, Nov 26, 2013 at 01:57:55PM +0800, Dave Young wrote:
> kexec-tools use boot_params for getting the 1st kernel hardware_subarch,
> the kexec kernel efi runtime support also need read the old efi_info from
> boot_params. Currently it exists in debugfs which is not a good place for
> such infomation. Per HPA, we should avoid of "sploit debugfs".
> 
> In this patch /sys/kernel/boot_params are exported, also the setup_data
> is exported as a subdirectory. For original debugfs since it's already
> there for long time and kexec-tools is using it for hardware_subarch so
> let's do not remove them for now.

"kexec-tools is using debugfs for hardware_subarch for a long time now
so we're not removing it yet."

> Structure are like below:

	    is

> 
> /sys/kernel/boot_params
> ├── data		/* binary data for boot_params */

You mean "boot_params in binary"?

> ├── setup_data  	/* subdirectory for setup_data if there's any */

no need for that comment

> │   ├── 0		/* the first setup_data node */
> │   │   ├── data	/* binary data for setup_data node 0 */

"setup_data node 0 in binary"

> │   │   └── type	/* setup_data type of setup_data node 0, hex string */
> |   [snip]		/* other setup_data nodes ... */
> └── version		/* hex string for boot protocal version */

"boot protocol version (in hex, "0x" prefixed)"

> 
> Changelog:
> Greg: use __ATTR_RO() and group attr.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-kernel-boot_params |  40 +++
>  arch/x86/kernel/Makefile                           |   2 +-
>  arch/x86/kernel/ksysfs.c                           | 339 +++++++++++++++++++++
>  3 files changed, 380 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-kernel-boot_params
>  create mode 100644 arch/x86/kernel/ksysfs.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-boot_params b/Documentation/ABI/testing/sysfs-kernel-boot_params
> new file mode 100644
> index 0000000..8014a93
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-kernel-boot_params
> @@ -0,0 +1,40 @@
> +What:		/sys/kernel/boot_params
> +Date:		November 2013
> +Contact:	Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> +Description:
> +		The /sys/kernel/boot_params directory contains two
> +		files: "data" and "version" and one subdirectory "setup_data".
> +		It is used to export the kernel boot parameters of x86

"... of an x86 platform to userspace for kexec and debugging purposes."

> +		platform to user space for kexec and debugging purpose.
> +
> +		If there's no setup_data in boot_params the subdirectory will
> +		not be created.
> +
> +		"data" file is the binary representation of struct boot_params.
> +
> +		"version" file is the string representation of boot
> +		protocol version.
> +
> +		"setup_data" subdirectory contains the setup_data data
> +		structure in boot_params. setup_data is maintained in kernel
> +		as a link list. In "setup_data" subdirectory there's one
> +		subdirectory for each link list node named with the number
> +		of the list nodes. The list node subdirectory contains two
> +		files "type" and "data". "type" file is the string
> +		representation of setup_data type.

string or int?
> +		"data" file is the binary
> +		representation of setup_data payload.
> +
> +		The whole boot_params directory structure is like below:
> +		/sys/kernel/boot_params
> +		├── data
> +		├── setup_data
> +		│   ├── 0
> +		│   │   ├── data
> +		│   │   └── type
> +		│   └── 1
> +		│       ├── data
> +		│       └── type
> +		└── version
> +
> +Users:
> +		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index a5408b9..473a4bb 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -35,7 +35,7 @@ obj-y			+= alternative.o i8253.o pci-nommu.o hw_breakpoint.o
>  obj-y			+= tsc.o io_delay.o rtc.o
>  obj-y			+= pci-iommu_table.o
>  obj-y			+= resource.o
> -
> +obj-$(CONFIG_SYSFS)	+= ksysfs.o
>  obj-y				+= process.o
>  obj-y				+= i387.o xsave.o
>  obj-y				+= ptrace.o

Conflict with upstream, it should be:

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 9b0a34e2cd79..851dcd1218fc 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -35,6 +35,7 @@ obj-y                 += alternative.o i8253.o pci-nommu.o hw_breakpoint.o
 obj-y                  += tsc.o io_delay.o rtc.o
 obj-y                  += pci-iommu_table.o
 obj-y                  += resource.o
+obj-$(CONFIG_SYSFS)    += ksysfs.o
 
 obj-$(CONFIG_PREEMPT)  += preempt.o
 

> diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
> new file mode 100644
> index 0000000..3f91207
> --- /dev/null
> +++ b/arch/x86/kernel/ksysfs.c
> @@ -0,0 +1,339 @@
> +/*
> + * Architecture specific sysfs attributes in /sys/kernel

Architecture-specific

...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 10/12] x86: export x86 boot_params to sysfs

[Dave Young]

On 11/27/13 at 03:56pm, Borislav Petkov wrote:
> On Tue, Nov 26, 2013 at 01:57:55PM +0800, Dave Young wrote:
> > kexec-tools use boot_params for getting the 1st kernel hardware_subarch,
> > the kexec kernel efi runtime support also need read the old efi_info from
> > boot_params. Currently it exists in debugfs which is not a good place for
> > such infomation. Per HPA, we should avoid of "sploit debugfs".
> > 
> > In this patch /sys/kernel/boot_params are exported, also the setup_data
> > is exported as a subdirectory. For original debugfs since it's already
> > there for long time and kexec-tools is using it for hardware_subarch so
> > let's do not remove them for now.
> 
> "kexec-tools is using debugfs for hardware_subarch for a long time now
> so we're not removing it yet."

Will take above log, thanks.

> 
> > Structure are like below:
> 
> 	    is

Will fix

> 
> > 
> > /sys/kernel/boot_params
> > ├── data		/* binary data for boot_params */
> 
> You mean "boot_params in binary"?

Yes, "boot_params in binary" is better, will use

> 
> > ├── setup_data  	/* subdirectory for setup_data if there's any */
> 
> no need for that comment

Ok

> 
> > │   ├── 0		/* the first setup_data node */
> > │   │   ├── data	/* binary data for setup_data node 0 */
> 
> "setup_data node 0 in binary"

Will do.

> 
> > │   │   └── type	/* setup_data type of setup_data node 0, hex string */
> > |   [snip]		/* other setup_data nodes ... */
> > └── version		/* hex string for boot protocal version */
> 
> "boot protocol version (in hex, "0x" prefixed)"

Will fix.

> 
> > 
> > Changelog:
> > Greg: use __ATTR_RO() and group attr.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  Documentation/ABI/testing/sysfs-kernel-boot_params |  40 +++
> >  arch/x86/kernel/Makefile                           |   2 +-
> >  arch/x86/kernel/ksysfs.c                           | 339 +++++++++++++++++++++
> >  3 files changed, 380 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-kernel-boot_params
> >  create mode 100644 arch/x86/kernel/ksysfs.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-kernel-boot_params b/Documentation/ABI/testing/sysfs-kernel-boot_params
> > new file mode 100644
> > index 0000000..8014a93
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-kernel-boot_params
> > @@ -0,0 +1,40 @@
> > +What:		/sys/kernel/boot_params
> > +Date:		November 2013
> > +Contact:	Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > +Description:
> > +		The /sys/kernel/boot_params directory contains two
> > +		files: "data" and "version" and one subdirectory "setup_data".
> > +		It is used to export the kernel boot parameters of x86
> 
> "... of an x86 platform to userspace for kexec and debugging purposes."

Will update

> 
> > +		platform to user space for kexec and debugging purpose.
> > +
> > +		If there's no setup_data in boot_params the subdirectory will
> > +		not be created.
> > +
> > +		"data" file is the binary representation of struct boot_params.
> > +
> > +		"version" file is the string representation of boot
> > +		protocol version.
> > +
> > +		"setup_data" subdirectory contains the setup_data data
> > +		structure in boot_params. setup_data is maintained in kernel
> > +		as a link list. In "setup_data" subdirectory there's one
> > +		subdirectory for each link list node named with the number
> > +		of the list nodes. The list node subdirectory contains two
> > +		files "type" and "data". "type" file is the string
> > +		representation of setup_data type.
> 
> string or int?

It's printed as a string. Kernel data type is int, here means string from sysfs
point of view. On the other hand, "data" is a binary..

> > +		"data" file is the binary
> > +		representation of setup_data payload.
> > +
> > +		The whole boot_params directory structure is like below:
> > +		/sys/kernel/boot_params
> > +		├── data
> > +		├── setup_data
> > +		│   ├── 0
> > +		│   │   ├── data
> > +		│   │   └── type
> > +		│   └── 1
> > +		│       ├── data
> > +		│       └── type
> > +		└── version
> > +
> > +Users:
> > +		Kexec Mailing List <kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>
> > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > index a5408b9..473a4bb 100644
> > --- a/arch/x86/kernel/Makefile
> > +++ b/arch/x86/kernel/Makefile
> > @@ -35,7 +35,7 @@ obj-y			+= alternative.o i8253.o pci-nommu.o hw_breakpoint.o
> >  obj-y			+= tsc.o io_delay.o rtc.o
> >  obj-y			+= pci-iommu_table.o
> >  obj-y			+= resource.o
> > -
> > +obj-$(CONFIG_SYSFS)	+= ksysfs.o
> >  obj-y				+= process.o
> >  obj-y				+= i387.o xsave.o
> >  obj-y				+= ptrace.o
> 
> Conflict with upstream, it should be:

Will update

> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 9b0a34e2cd79..851dcd1218fc 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -35,6 +35,7 @@ obj-y                 += alternative.o i8253.o pci-nommu.o hw_breakpoint.o
>  obj-y                  += tsc.o io_delay.o rtc.o
>  obj-y                  += pci-iommu_table.o
>  obj-y                  += resource.o
> +obj-$(CONFIG_SYSFS)    += ksysfs.o
>  
>  obj-$(CONFIG_PREEMPT)  += preempt.o
>  
> 
> > diff --git a/arch/x86/kernel/ksysfs.c b/arch/x86/kernel/ksysfs.c
> > new file mode 100644
> > index 0000000..3f91207
> > --- /dev/null
> > +++ b/arch/x86/kernel/ksysfs.c
> > @@ -0,0 +1,339 @@
> > +/*
> > + * Architecture specific sysfs attributes in /sys/kernel
> 
> Architecture-specific

Ok.

Thanks a lot for the review
Dave

[PATCH v4 11/12] x86: reserve setup_data ranges late after parsing memmap cmdline

[Dave Young]

Kdump kernel using memmap=exactmap, move e820_reserve_setup_data after
parsing early params so they can be set reserved finally. Or kdump kernel
will warn about ioremap a normal ram range.

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kernel/setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index ecf225e..4a18f2f 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -927,8 +927,6 @@ void __init setup_arch(char **cmdline_p)
 	iomem_resource.end = (1ULL << boot_cpu_data.x86_phys_bits) - 1;
 	setup_memory_map();
 	parse_setup_data();
-	/* update the e820_saved too */
-	e820_reserve_setup_data();
 
 	copy_edd();
 
@@ -990,6 +988,8 @@ void __init setup_arch(char **cmdline_p)
 		early_dump_pci_devices();
 #endif
 
+	/* update the e820_saved too */
+	e820_reserve_setup_data();
 	finish_e820_parsing();
 
 	if (efi_enabled(EFI_BOOT))
-- 
1.8.3.1

Re: [PATCH v4 11/12] x86: reserve setup_data ranges late after parsing memmap cmdline

[Borislav Petkov]

On Tue, Nov 26, 2013 at 01:57:56PM +0800, Dave Young wrote:
> Kdump kernel using memmap=exactmap, move e820_reserve_setup_data after
> parsing early params so they can be set reserved finally. Or kdump kernel
> will warn about ioremap a normal ram range.

I had to stare for a while at this commit message, rereading it a couple
of times and looking at the code in order to understand why you're
moving the call further down.

Please rewrite it into something more readable.

The usual structure which you could use is:

1. Currently we have A
2. We need B
3. In order to achieve it, we do C

or something to that effect.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 11/12] x86: reserve setup_data ranges late after parsing memmap cmdline

[Dave Young]

On 11/27/13 at 04:07pm, Borislav Petkov wrote:
> On Tue, Nov 26, 2013 at 01:57:56PM +0800, Dave Young wrote:
> > Kdump kernel using memmap=exactmap, move e820_reserve_setup_data after
> > parsing early params so they can be set reserved finally. Or kdump kernel
> > will warn about ioremap a normal ram range.
> 
> I had to stare for a while at this commit message, rereading it a couple
> of times and looking at the code in order to understand why you're
> moving the call further down.
> 
> Please rewrite it into something more readable.
> 
> The usual structure which you could use is:
> 
> 1. Currently we have A
> 2. We need B
> 3. In order to achieve it, we do C
> 
> or something to that effect.

Is below changes ok to you?

Currently e820_reserve_setup_data is called before parsing early params,
it works in normal case. But for memmap=exactmap, the final memory ranges
are created after parsing memmap= cmdline params, so the previous
e820_reserve_setup_data has no effect. For example for setup_data ranges
will still be marked as normal system ram, thus when later sysfs driver
ioremap them kernel will warn about mapping normal ram.

This patch fix it by moving the e820_reserve_setup_data callback after
parsing early params so they can be set as reserved ranges and later
ioremap will be fine with it.

Thanks
Dave

Re: [PATCH v4 11/12] x86: reserve setup_data ranges late after parsing memmap cmdline

[Borislav Petkov]

On Fri, Nov 29, 2013 at 04:35:52PM +0800, Dave Young wrote:
> Is below changes ok to you?

Almost perfect! Just a micro-nitpick below:

> Currently e820_reserve_setup_data is called before parsing early params,
> it works in normal case. But for memmap=exactmap, the final memory ranges
> are created after parsing memmap= cmdline params, so the previous
> e820_reserve_setup_data has no effect. For example for setup_data ranges

s/ for/,/

> will still be marked as normal system ram, thus when later sysfs driver
> ioremap them kernel will warn about mapping normal ram.
> 
> This patch fix it by moving the e820_reserve_setup_data callback after
> parsing early params so they can be set as reserved ranges and later
> ioremap will be fine with it.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH v4 12/12] x86: kdebugfs do not use __va for getting setup_data virt addr

[Dave Young]

__va does not work in case memmap=exactmap, so let's always use ioremap_cache.

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/kernel/kdebugfs.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/kdebugfs.c b/arch/x86/kernel/kdebugfs.c
index dc1404b..185e1ca 100644
--- a/arch/x86/kernel/kdebugfs.c
+++ b/arch/x86/kernel/kdebugfs.c
@@ -33,7 +33,6 @@ static ssize_t setup_data_read(struct file *file, char __user *user_buf,
 	struct setup_data_node *node = file->private_data;
 	unsigned long remain;
 	loff_t pos = *ppos;
-	struct page *pg;
 	void *p;
 	u64 pa;
 
@@ -47,18 +46,12 @@ static ssize_t setup_data_read(struct file *file, char __user *user_buf,
 		count = node->len - pos;
 
 	pa = node->paddr + sizeof(struct setup_data) + pos;
-	pg = pfn_to_page((pa + count - 1) >> PAGE_SHIFT);
-	if (PageHighMem(pg)) {
-		p = ioremap_cache(pa, count);
-		if (!p)
-			return -ENXIO;
-	} else
-		p = __va(pa);
+	p = ioremap_cache(pa, count);
+	if (!p)
+		return -ENXIO;
 
 	remain = copy_to_user(user_buf, p, count);
-
-	if (PageHighMem(pg))
-		iounmap(p);
+	iounmap(p);
 
 	if (remain)
 		return -EFAULT;
@@ -109,7 +102,6 @@ static int __init create_setup_data_nodes(struct dentry *parent)
 	struct setup_data *data;
 	int error;
 	struct dentry *d;
-	struct page *pg;
 	u64 pa_data;
 	int no = 0;
 
@@ -126,16 +118,12 @@ static int __init create_setup_data_nodes(struct dentry *parent)
 			goto err_dir;
 		}
 
-		pg = pfn_to_page((pa_data+sizeof(*data)-1) >> PAGE_SHIFT);
-		if (PageHighMem(pg)) {
-			data = ioremap_cache(pa_data, sizeof(*data));
-			if (!data) {
-				kfree(node);
-				error = -ENXIO;
-				goto err_dir;
-			}
-		} else
-			data = __va(pa_data);
+		data = ioremap_cache(pa_data, sizeof(*data));
+		if (!data) {
+			kfree(node);
+			error = -ENXIO;
+			goto err_dir;
+		}
 
 		node->paddr = pa_data;
 		node->type = data->type;
@@ -143,8 +131,7 @@ static int __init create_setup_data_nodes(struct dentry *parent)
 		error = create_setup_data_node(d, no, node);
 		pa_data = data->next;
 
-		if (PageHighMem(pg))
-			iounmap(data);
+		iounmap(data);
 		if (error)
 			goto err_dir;
 		no++;
-- 
1.8.3.1

Re: [PATCH v4 00/12] kexec kernel efi runtime support

[Dave Young]

This is the resend of V4, there's no update for kexec-tools patches.

BTW, below is a git repo for anyone who want to test the patches:
https://github.com/daveyoung/linux.git
branch kexec-efi

To Boris: finally I created a git repo ;) Thanks.

Re: [PATCH v4 00/12] kexec kernel efi runtime support

[Matt Fleming]

On Tue, 26 Nov, at 01:57:45PM, Dave Young wrote:
> Hi,
> 
> Here is the V4 resend for supporting kexec kernel efi runtime.
> Per pervious discussion I pass the 1st kernel efi runtime mapping
> via setup_data to 2nd kernel. Besides of the runtime mapping
> info I also pass the fw_vendor, runtime, config table, smbios
> physical address in setup_data. EFI spec mentioned fw_vendor,
> runtime, config table addresses will be converted to virt address
> after entering virtual mode, but we will use it as physical address
> in efi_init. For smbios EFI spec did not mention about the address
> updating, but during my test on a HP workstation, the bios will
> convert it to Virt addr, thus pass it in setup_data as well.
 
Dave, your commits introduce a bunch of new sparse warnings,

/home/build/git/efi/arch/x86/platform/efi/efi.c:130:15: warning: incorrect type in assignment (different address spaces)
/home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:    expected struct setup_data *sdata
/home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:    got void [noderef] <asn:2>*
/home/build/git/efi/arch/x86/platform/efi/efi.c:140:23: warning: incorrect type in argument 1 (different address spaces)
/home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:    expected void [noderef] <asn:2>*addr
/home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:    got struct setup_data *sdata
/home/build/git/efi/arch/x86/platform/efi/efi.c:143:16: warning: incorrect type in assignment (different address spaces)
/home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:    expected struct efi_setup_data *[addressable] [toplevel] esdata
/home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:    got void [noderef] <asn:2>*

/home/build/git/efi/arch/x86/platform/efi/efi.c:701:20: warning: incorrect type in assignment (different address spaces)
/home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:    expected void *tablep
/home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:    got void [noderef] <asn:2>*
/home/build/git/efi/arch/x86/platform/efi/efi.c:722:23: warning: incorrect type in argument 1 (different address spaces)
/home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:    expected void [noderef] <asn:2>*addr
/home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:    got void *tablep

/home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31: warning: incorrect type in argument 1 (different address spaces)
/home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:    expected void [noderef] <asn:2>*addr
/home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:    got struct efi_setup_data *[addressable] [toplevel] esdata

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 00/12] kexec kernel efi runtime support

[Dave Young]

On 11/27/13 at 12:50pm, Matt Fleming wrote:
> On Tue, 26 Nov, at 01:57:45PM, Dave Young wrote:
> > Hi,
> > 
> > Here is the V4 resend for supporting kexec kernel efi runtime.
> > Per pervious discussion I pass the 1st kernel efi runtime mapping
> > via setup_data to 2nd kernel. Besides of the runtime mapping
> > info I also pass the fw_vendor, runtime, config table, smbios
> > physical address in setup_data. EFI spec mentioned fw_vendor,
> > runtime, config table addresses will be converted to virt address
> > after entering virtual mode, but we will use it as physical address
> > in efi_init. For smbios EFI spec did not mention about the address
> > updating, but during my test on a HP workstation, the bios will
> > convert it to Virt addr, thus pass it in setup_data as well.
>  
> Dave, your commits introduce a bunch of new sparse warnings,

Hi, Matt

Will fix them, thanks.

> 
> /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15: warning: incorrect type in assignment (different address spaces)
> /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:    expected struct setup_data *sdata
> /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:    got void [noderef] <asn:2>*
> /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23: warning: incorrect type in argument 1 (different address spaces)
> /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:    expected void [noderef] <asn:2>*addr
> /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:    got struct setup_data *sdata
> /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16: warning: incorrect type in assignment (different address spaces)
> /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:    expected struct efi_setup_data *[addressable] [toplevel] esdata
> /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:    got void [noderef] <asn:2>*
> 
> /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20: warning: incorrect type in assignment (different address spaces)
> /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:    expected void *tablep
> /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:    got void [noderef] <asn:2>*
> /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23: warning: incorrect type in argument 1 (different address spaces)
> /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:    expected void [noderef] <asn:2>*addr
> /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:    got void *tablep
> 
> /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31: warning: incorrect type in argument 1 (different address spaces)
> /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:    expected void [noderef] <asn:2>*addr
> /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:    got struct efi_setup_data *[addressable] [toplevel] esdata
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v4 00/12] kexec kernel efi runtime support

[Dave Young]

On 11/28/13 at 10:08am, Dave Young wrote:
> On 11/27/13 at 12:50pm, Matt Fleming wrote:
> > On Tue, 26 Nov, at 01:57:45PM, Dave Young wrote:
> > > Hi,
> > > 
> > > Here is the V4 resend for supporting kexec kernel efi runtime.
> > > Per pervious discussion I pass the 1st kernel efi runtime mapping
> > > via setup_data to 2nd kernel. Besides of the runtime mapping
> > > info I also pass the fw_vendor, runtime, config table, smbios
> > > physical address in setup_data. EFI spec mentioned fw_vendor,
> > > runtime, config table addresses will be converted to virt address
> > > after entering virtual mode, but we will use it as physical address
> > > in efi_init. For smbios EFI spec did not mention about the address
> > > updating, but during my test on a HP workstation, the bios will
> > > convert it to Virt addr, thus pass it in setup_data as well.
> >  
> > Dave, your commits introduce a bunch of new sparse warnings,
> 
> Hi, Matt
> 
> Will fix them, thanks.
> 
> > 
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15: warning: incorrect type in assignment (different address spaces)
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:    expected struct setup_data *sdata
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:    got void [noderef] <asn:2>*
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23: warning: incorrect type in argument 1 (different address spaces)
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:    expected void [noderef] <asn:2>*addr
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:    got struct setup_data *sdata
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16: warning: incorrect type in assignment (different address spaces)
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:    expected struct efi_setup_data *[addressable] [toplevel] esdata
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:    got void [noderef] <asn:2>*
> > 
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20: warning: incorrect type in assignment (different address spaces)
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:    expected void *tablep
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:    got void [noderef] <asn:2>*
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23: warning: incorrect type in argument 1 (different address spaces)
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:    expected void [noderef] <asn:2>*addr
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:    got void *tablep
> > 
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31: warning: incorrect type in argument 1 (different address spaces)
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:    expected void [noderef] <asn:2>*addr
> > /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:    got struct efi_setup_data *[addressable] [toplevel] esdata

For the early_memremap, these warnings are safe, but it create a lot of
warnings and looks anoise.

I think for anyone who are using early_memremap should know this is a normal
kernel memory instead of real __iomem, so I'd prefer below fix, it will suppress
most of original and new warnings:

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 34f69cb..1db414f 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -325,9 +325,10 @@ extern void early_ioremap_init(void);
 extern void early_ioremap_reset(void);
 extern void __iomem *early_ioremap(resource_size_t phys_addr,
 				   unsigned long size);
-extern void __iomem *early_memremap(resource_size_t phys_addr,
+extern void *early_memremap(resource_size_t phys_addr,
 				    unsigned long size);
 extern void early_iounmap(void __iomem *addr, unsigned long size);
+extern void early_memunmap(void *addr, unsigned long size);
 extern void fixup_early_ioremap(void);
 extern bool is_early_ioremap_ptep(pte_t *ptep);
 
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 799580c..92a896652 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -562,10 +562,10 @@ early_ioremap(resource_size_t phys_addr, unsigned long size)
 }
 
 /* Remap memory */
-void __init __iomem *
+void __init *
 early_memremap(resource_size_t phys_addr, unsigned long size)
 {
-	return __early_ioremap(phys_addr, size, PAGE_KERNEL);
+	return (__force void __kernel *) __early_ioremap(phys_addr, size, PAGE_KERNEL);
 }
 
 void __init early_iounmap(void __iomem *addr, unsigned long size)
@@ -620,3 +620,8 @@ void __init early_iounmap(void __iomem *addr, unsigned long size)
 	}
 	prev_map[slot] = NULL;
 }
+
+void __init early_memunmap(void *addr, unsigned long size)
+{
+	early_iounmap((__force void __iomem *)addr, size);
+}

Re: [PATCH v4 00/12] kexec kernel efi runtime support

[Borislav Petkov]

On Fri, Nov 29, 2013 at 04:28:22PM +0800, Dave Young wrote:
> I think for anyone who are using early_memremap should know this is a
> normal kernel memory instead of real __iomem

You can never ever assume that people are using kernel interfaces
correctly.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH v4 00/12] kexec kernel efi runtime support

[Dave Young]

On 11/29/13 at 06:02pm, Borislav Petkov wrote:
> On Fri, Nov 29, 2013 at 04:28:22PM +0800, Dave Young wrote:
> > I think for anyone who are using early_memremap should know this is a
> > normal kernel memory instead of real __iomem
> 
> You can never ever assume that people are using kernel interfaces
> correctly.

But the semantic of early_memreamp should be that it will just return
a normal memory pointer instead of iomem so the fix to convert the internal
implementation to return a __force __kernel make sense to me.

For iounmap I think since we have early_memremap, we have reason to add
early_memunmap() so sparse will be happy.

Thanks
Dave