[patch 0/7 v2] kexec kernel efi runtime support

[patch 0/7 v2] kexec kernel efi runtime support

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hi,

Here is the V2 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/systab, smbios is already in that file.

For efi runtime mapping I add a new directory /sys/firmware/efi/
efi-runtime-map/ like below
[dave@darkstar ~]$ tree /sys/firmware/efi/efi-runtime-map/
/sys/firmware/efi/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
--
Thanks
Dave


[-- Attachment #2: Type: text/plain, Size: 173 bytes --]

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

[patch 1/7 v2] Add function efi_remap_region for remapping to saved virt address

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 01-efi-map-fixed-addr.patch --]
[-- Type: text/plain, Size: 2214 bytes --]

Kexec kernel will use saved runtime virtual mapping, so add a
new function efi_remap_region to remapping it directly without
calculate the virt addr from efi_va.

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

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/include/asm/efi.h     |    1 +
 arch/x86/platform/efi/efi_32.c |    3 +++
 arch/x86/platform/efi/efi_64.c |   19 +++++++++++++++++++
 3 files changed, 23 insertions(+)

--- linux-2.6.orig/arch/x86/include/asm/efi.h
+++ linux-2.6/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);
--- linux-2.6.orig/arch/x86/platform/efi/efi_64.c
+++ linux-2.6/arch/x86/platform/efi/efi_64.c
@@ -203,6 +203,25 @@ void __init efi_map_region(efi_memory_de
 	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)
+{
+	pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
+	unsigned long pf = 0;
+
+	if (!(md->attribute & EFI_MEMORY_WB))
+		pf |= _PAGE_PCD;
+
+	if (kernel_map_pages_in_pgd(pgd, md->phys_addr, md->virt_addr,
+			md->num_pages, pf))
+		pr_warn("Error mapping PA 0x%llx -> VA 0x%llx!\n",
+			   md->phys_addr, md->virt_addr);
+}
+
 void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
 				 u32 type, u64 attribute)
 {
--- linux-2.6.orig/arch/x86/platform/efi/efi_32.c
+++ linux-2.6/arch/x86/platform/efi/efi_32.c
@@ -47,6 +47,9 @@ void __init efi_map_region(efi_memory_de
 	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;

Re: [patch 1/7 v2] Add function efi_remap_region for remapping to saved virt address

[Matt Fleming]

On Tue, 05 Nov, at 04:20:08PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> Kexec kernel will use saved runtime virtual mapping, so add a
> new function efi_remap_region to remapping it directly without
> calculate the virt addr from efi_va.
> 
> The md is passed in from 1st kernel, the virtual addr is
> saved in md->virt_addr.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/include/asm/efi.h     |    1 +
>  arch/x86/platform/efi/efi_32.c |    3 +++
>  arch/x86/platform/efi/efi_64.c |   19 +++++++++++++++++++
>  3 files changed, 23 insertions(+)

[...]

> @@ -203,6 +203,25 @@ void __init efi_map_region(efi_memory_de
>  	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)
> +{
> +	pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
> +	unsigned long pf = 0;
> +
> +	if (!(md->attribute & EFI_MEMORY_WB))
> +		pf |= _PAGE_PCD;
> +
> +	if (kernel_map_pages_in_pgd(pgd, md->phys_addr, md->virt_addr,
> +			md->num_pages, pf))
> +		pr_warn("Error mapping PA 0x%llx -> VA 0x%llx!\n",
> +			   md->phys_addr, md->virt_addr);
> +}
> +

This function is almost identical to __map_region(). Please implement
efi_map_region_fixed() in terms of __map_region(), e.g.

void __init efi_map_region_fixed(efi_memory_desc_t *md)
{
	__map_region(md, md->virt_addr);
}

> @@ -47,6 +47,9 @@ void __init efi_map_region(efi_memory_de
>  	old_map_region(md);
>  }
>  
> +void __init efi_map_region_fixed(efi_memory_desc_t *md)
> +{}
> +

The braces should be on the same line as the rest of the function
prototype. Look at how it's done elsewhere in this file.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch 1/7 v2] Add function efi_remap_region for remapping to saved virt address

[Dave Young]

On 11/13/13 at 03:50pm, Matt Fleming wrote:
> On Tue, 05 Nov, at 04:20:08PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > Kexec kernel will use saved runtime virtual mapping, so add a
> > new function efi_remap_region to remapping it directly without
> > calculate the virt addr from efi_va.
> > 
> > The md is passed in from 1st kernel, the virtual addr is
> > saved in md->virt_addr.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/include/asm/efi.h     |    1 +
> >  arch/x86/platform/efi/efi_32.c |    3 +++
> >  arch/x86/platform/efi/efi_64.c |   19 +++++++++++++++++++
> >  3 files changed, 23 insertions(+)
> 
> [...]
> 
> > @@ -203,6 +203,25 @@ void __init efi_map_region(efi_memory_de
> >  	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)
> > +{
> > +	pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
> > +	unsigned long pf = 0;
> > +
> > +	if (!(md->attribute & EFI_MEMORY_WB))
> > +		pf |= _PAGE_PCD;
> > +
> > +	if (kernel_map_pages_in_pgd(pgd, md->phys_addr, md->virt_addr,
> > +			md->num_pages, pf))
> > +		pr_warn("Error mapping PA 0x%llx -> VA 0x%llx!\n",
> > +			   md->phys_addr, md->virt_addr);
> > +}
> > +
> 
> This function is almost identical to __map_region(). Please implement
> efi_map_region_fixed() in terms of __map_region(), e.g.
> 
> void __init efi_map_region_fixed(efi_memory_desc_t *md)
> {
> 	__map_region(md, md->virt_addr);
> }

Good suggestion, will do

> 
> > @@ -47,6 +47,9 @@ void __init efi_map_region(efi_memory_de
> >  	old_map_region(md);
> >  }
> >  
> > +void __init efi_map_region_fixed(efi_memory_desc_t *md)
> > +{}
> > +
> 
> The braces should be on the same line as the rest of the function
> prototype. Look at how it's done elsewhere in this file.

Ok, will do

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

Re: [patch 1/7 v2] Add function efi_remap_region for remapping to saved virt address

[Toshi Kani]

On Tue, 2013-11-05 at 16:20 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> Kexec kernel will use saved runtime virtual mapping, so add a
> new function efi_remap_region to remapping it directly without
> calculate the virt addr from efi_va.
> 
> The md is passed in from 1st kernel, the virtual addr is
> saved in md->virt_addr.

 :

> +/*
> + * 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)

Can you simply use __map_region() instead of creating this function?
Both functions seem to do the same thing.  (In __map_region(), size and
end are not used.)

Thanks,
-Toshi

Re: [patch 1/7 v2] Add function efi_remap_region for remapping to saved virt address

[Dave Young]

On 11/15/13 at 04:02pm, Toshi Kani wrote:
> On Tue, 2013-11-05 at 16:20 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > Kexec kernel will use saved runtime virtual mapping, so add a
> > new function efi_remap_region to remapping it directly without
> > calculate the virt addr from efi_va.
> > 
> > The md is passed in from 1st kernel, the virtual addr is
> > saved in md->virt_addr.
> 
>  :
> 
> > +/*
> > + * 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)
> 
> Can you simply use __map_region() instead of creating this function?
> Both functions seem to do the same thing.  (In __map_region(), size and
> end are not used.)

Matt also suggest to reuse __map_region in another apply, will do.

--
Thanks for review
Dave

Re: [patch 1/7 v2] Add function efi_remap_region for remapping to saved virt address

[Dave Young]

On 11/18/13 at 10:09am, Dave Young wrote:
> On 11/15/13 at 04:02pm, Toshi Kani wrote:
> > On Tue, 2013-11-05 at 16:20 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > > Kexec kernel will use saved runtime virtual mapping, so add a
> > > new function efi_remap_region to remapping it directly without
> > > calculate the virt addr from efi_va.
> > > 
> > > The md is passed in from 1st kernel, the virtual addr is
> > > saved in md->virt_addr.
> > 
> >  :
> > 
> > > +/*
> > > + * 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)
> > 
> > Can you simply use __map_region() instead of creating this function?
> > Both functions seem to do the same thing.  (In __map_region(), size and
> > end are not used.)
> 
> Matt also suggest to reuse __map_region in another apply, will do.

Will remove the unused variables in another patch.

--
Thanks
Dave

[patch 2/7 v2] x86 efi: reserve boot service fix

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 02-boot-service-fix.patch --]
[-- Type: text/plain, Size: 952 bytes --]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
---
 arch/x86/platform/efi/efi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.orig/arch/x86/platform/efi/efi.c
+++ linux-2.6/arch/x86/platform/efi/efi.c
@@ -438,7 +438,7 @@ void __init efi_reserve_boot_services(vo
 		 * - 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)) {

Re: [patch 2/7 v2] x86 efi: reserve boot service fix

[Toshi Kani]

On Tue, 2013-11-05 at 16:20 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>

Acked-by: Toshi Kani <toshi.kani-VXdhtT5mjnY@public.gmane.org>

Thanks,
-Toshi

[patch 3/7 v2] Cleanup efi_enter_virtual_mode function

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 03-efi-enter-virtual-mode-cleanup-1.patch --]
[-- Type: text/plain, Size: 4786 bytes --]

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.

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

--- linux-2.6.orig/arch/x86/platform/efi/efi.c
+++ linux-2.6/arch/x86/platform/efi/efi.c
@@ -773,44 +773,12 @@ void __init old_map_region(efi_memory_de
 		       (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 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
+ * Return the new memmap with updated virtual addrresses.
+ */
+static void efi_map_regions(void **new_memmap, int *count)
+{
+	efi_memory_desc_t *md;
+	void *p;
+	unsigned long size;
+	u64 end, systab;
 
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
@@ -860,17 +840,59 @@ void __init efi_enter_virtual_mode(void)
 			efi.systab = (efi_system_table_t *) (unsigned long) systab;
 		}
 
-		new_memmap = krealloc(new_memmap,
-				      (count + 1) * memmap.desc_size,
-				      GFP_KERNEL);
-		if (!new_memmap)
+		*new_memmap = krealloc(*new_memmap,
+					(*count + 1) * memmap.desc_size,
+					GFP_KERNEL);
+		if (!*new_memmap)
 			goto err_out;
 
-		memcpy(new_memmap + (count * memmap.desc_size), md,
+		memcpy(*new_memmap + (*count * memmap.desc_size), md,
 		       memmap.desc_size);
-		count++;
+		(*count)++;
 	}
 
+err_out:
+	pr_err("Error reallocating memory, EFI runtime non-functional!\n");
+}
+
+/*
+ * 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 *p, *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();
+
+	efi_map_regions(&new_memmap, &count);
+
 	BUG_ON(!efi.systab);
 
 	efi_setup_page_tables();
@@ -922,9 +944,6 @@ void __init efi_enter_virtual_mode(void)
 			 0, NULL);
 
 	return;
-
- err_out:
-	pr_err("Error reallocating memory, EFI runtime non-functional!\n");
 }
 
 /*

Re: [patch 3/7 v2] Cleanup efi_enter_virtual_mode function

[Matt Fleming]

On Tue, 05 Nov, at 04:20:10PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/platform/efi/efi.c |  107 +++++++++++++++++++++++++-------------------
>  1 file changed, 63 insertions(+), 44 deletions(-)

[...]

> @@ -860,17 +840,59 @@ void __init efi_enter_virtual_mode(void)
>  			efi.systab = (efi_system_table_t *) (unsigned long) systab;
>  		}
>  
> -		new_memmap = krealloc(new_memmap,
> -				      (count + 1) * memmap.desc_size,
> -				      GFP_KERNEL);
> -		if (!new_memmap)
> +		*new_memmap = krealloc(*new_memmap,
> +					(*count + 1) * memmap.desc_size,
> +					GFP_KERNEL);
> +		if (!*new_memmap)
>  			goto err_out;
>  
> -		memcpy(new_memmap + (count * memmap.desc_size), md,
> +		memcpy(*new_memmap + (*count * memmap.desc_size), md,
>  		       memmap.desc_size);
> -		count++;
> +		(*count)++;
>  	}
>  
> +err_out:
> +	pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> +}

You should really return an error value here so that we don't
dereference 'new_memmap' below if it's NULL. In fact, just return the
memmap...

> +
> +	efi_merge_regions();
> +
> +	efi_map_regions(&new_memmap, &count);
> +

and make sure to check the return value here.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch 3/7 v2] Cleanup efi_enter_virtual_mode function

[Dave Young]

On 11/13/13 at 03:50pm, Matt Fleming wrote:
> On Tue, 05 Nov, at 04:20:10PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/platform/efi/efi.c |  107 +++++++++++++++++++++++++-------------------
> >  1 file changed, 63 insertions(+), 44 deletions(-)
> 
> [...]
> 
> > @@ -860,17 +840,59 @@ void __init efi_enter_virtual_mode(void)
> >  			efi.systab = (efi_system_table_t *) (unsigned long) systab;
> >  		}
> >  
> > -		new_memmap = krealloc(new_memmap,
> > -				      (count + 1) * memmap.desc_size,
> > -				      GFP_KERNEL);
> > -		if (!new_memmap)
> > +		*new_memmap = krealloc(*new_memmap,
> > +					(*count + 1) * memmap.desc_size,
> > +					GFP_KERNEL);
> > +		if (!*new_memmap)
> >  			goto err_out;
> >  
> > -		memcpy(new_memmap + (count * memmap.desc_size), md,
> > +		memcpy(*new_memmap + (*count * memmap.desc_size), md,
> >  		       memmap.desc_size);
> > -		count++;
> > +		(*count)++;
> >  	}
> >  
> > +err_out:
> > +	pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> > +}
> 
> You should really return an error value here so that we don't
> dereference 'new_memmap' below if it's NULL. In fact, just return the
> memmap...

Good catch, will do

> 
> > +
> > +	efi_merge_regions();
> > +
> > +	efi_map_regions(&new_memmap, &count);
> > +
> 
> and make sure to check the return value here.

Ok.

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

Re: [patch 3/7 v2] Cleanup efi_enter_virtual_mode function

[Toshi Kani]

On Tue, 2013-11-05 at 16:20 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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.

 :

> +void __init efi_enter_virtual_mode(void)
> +{
> +	efi_status_t status;
> +	void *p, *new_memmap = NULL;

*p is unused.


Thanks,
-Toshi

Re: [patch 3/7 v2] Cleanup efi_enter_virtual_mode function

[Dave Young]

On 11/15/13 at 04:21pm, Toshi Kani wrote:
> On Tue, 2013-11-05 at 16:20 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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.
> 
>  :
> 
> > +void __init efi_enter_virtual_mode(void)
> > +{
> > +	efi_status_t status;
> > +	void *p, *new_memmap = NULL;
> 
> *p is unused.

The *p was removed in later patch, will remove it in this patch.

Thanks for review.
Dave

[patch 4/7 v2] export more efi table variable to sysfs

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 04-export-more-efi-sysfs-vars.patch --]
[-- Type: text/plain, Size: 2319 bytes --]

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.

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c |    4 ++++
 drivers/firmware/efi/efi.c  |    9 +++++++++
 include/linux/efi.h         |    3 +++
 3 files changed, 16 insertions(+)

--- linux-2.6.orig/drivers/firmware/efi/efi.c
+++ linux-2.6/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_tables  = EFI_INVALID_TABLE_ADDR,
 };
 EXPORT_SYMBOL(efi);
 
@@ -64,6 +67,12 @@ static ssize_t systab_show(struct kobjec
 		str += sprintf(str, "BOOTINFO=0x%lx\n", efi.boot_info);
 	if (efi.uga != EFI_INVALID_TABLE_ADDR)
 		str += sprintf(str, "UGA=0x%lx\n", efi.uga);
+	if (efi.fw_vendor != EFI_INVALID_TABLE_ADDR)
+		str += sprintf(str, "fw_vendor=0x%lx\n", efi.fw_vendor);
+	if (efi.runtime != EFI_INVALID_TABLE_ADDR)
+		str += sprintf(str, "runtime=0x%lx\n", efi.runtime);
+	if (efi.config_tables != EFI_INVALID_TABLE_ADDR)
+		str += sprintf(str, "config_tables=0x%lx\n", efi.config_tables);
 
 	return str - buf;
 }
--- linux-2.6.orig/include/linux/efi.h
+++ linux-2.6/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;
+	unsigned long runtime;
+	unsigned long config_tables;
 	efi_get_time_t *get_time;
 	efi_set_time_t *set_time;
 	efi_get_wakeup_time_t *get_wakeup_time;
--- linux-2.6.orig/arch/x86/platform/efi/efi.c
+++ linux-2.6/arch/x86/platform/efi/efi.c
@@ -653,6 +653,10 @@ void __init efi_init(void)
 
 	set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility);
 
+	efi.fw_vendor = (unsigned long)efi.systab->fw_vendor;
+	efi.runtime = (unsigned long)efi.systab->runtime;
+	efi.config_tables = (unsigned long)efi.systab->tables;
+
 	/*
 	 * Show what we know for posterity
 	 */

Re: [patch 4/7 v2] export more efi table variable to sysfs

[Greg KH]

On Tue, Nov 05, 2013 at 04:20:11PM +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> Export fw_vendor, runtime and config tables physical
> addresses to /sys/firmware/efi/systab becaue kexec
> kernel will need them.

sysfs files are one-value-per-file.

Please don't abuse this and add more values to this single file, in
fact, it should be broken up into individual sysfs files as it is,
because this isn't ok now.

Sorry, you don't get to abuse this anymore :(

greg k-h

Re: [patch 4/7 v2] export more efi table variable to sysfs

[Dave Young]

On 11/11/13 at 04:40pm, Greg KH wrote:
> On Tue, Nov 05, 2013 at 04:20:11PM +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > Export fw_vendor, runtime and config tables physical
> > addresses to /sys/firmware/efi/systab becaue kexec
> > kernel will need them.
> 
> sysfs files are one-value-per-file.
> 
> Please don't abuse this and add more values to this single file, in
> fact, it should be broken up into individual sysfs files as it is,
> because this isn't ok now.
> 
> Sorry, you don't get to abuse this anymore :(

Ok, I know this, just creating this without thinking about carefully.
Will redo the patch, thanks for catching.

Thanks
Dave

Re: [patch 4/7 v2] export more efi table variable to sysfs

[Dave Young]

On 11/12/13 at 04:19pm, Dave Young wrote:
> On 11/11/13 at 04:40pm, Greg KH wrote:
> > On Tue, Nov 05, 2013 at 04:20:11PM +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > > Export fw_vendor, runtime and config tables physical
> > > addresses to /sys/firmware/efi/systab becaue kexec
> > > kernel will need them.
> > 
> > sysfs files are one-value-per-file.
> > 
> > Please don't abuse this and add more values to this single file, in
> > fact, it should be broken up into individual sysfs files as it is,
> > because this isn't ok now.
> > 
> > Sorry, you don't get to abuse this anymore :(
> 
> Ok, I know this, just creating this without thinking about carefully.
> Will redo the patch, thanks for catching.

I would like to add new files for the values used in this patchset.
And will split the original systab later in another patchset.

> 
> Thanks
> Dave

Re: [patch 4/7 v2] export more efi table variable to sysfs

[Greg KH]

On Tue, Nov 12, 2013 at 04:24:01PM +0800, Dave Young wrote:
> On 11/12/13 at 04:19pm, Dave Young wrote:
> > On 11/11/13 at 04:40pm, Greg KH wrote:
> > > On Tue, Nov 05, 2013 at 04:20:11PM +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > > > Export fw_vendor, runtime and config tables physical
> > > > addresses to /sys/firmware/efi/systab becaue kexec
> > > > kernel will need them.
> > > 
> > > sysfs files are one-value-per-file.
> > > 
> > > Please don't abuse this and add more values to this single file, in
> > > fact, it should be broken up into individual sysfs files as it is,
> > > because this isn't ok now.
> > > 
> > > Sorry, you don't get to abuse this anymore :(
> > 
> > Ok, I know this, just creating this without thinking about carefully.
> > Will redo the patch, thanks for catching.
> 
> I would like to add new files for the values used in this patchset.
> And will split the original systab later in another patchset.

That's fine with me, thanks.

greg k-h

[patch 5/7 v2] export efi runtime memory mapping to sysfs

[dyoung]

[-- Attachment #1: 05-export-efi-runtime-mapping.patch --]
[-- Type: text/plain, Size: 10360 bytes --]

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

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 Documentation/ABI/testing/sysfs-efi-runtime-map |   45 ++++++
 arch/x86/include/asm/efi.h                      |    3 
 arch/x86/platform/efi/efi.c                     |   11 +
 drivers/firmware/efi/Kconfig                    |   10 +
 drivers/firmware/efi/Makefile                   |    1 
 drivers/firmware/efi/efi-runtime-map.c          |  164 ++++++++++++++++++++++++
 drivers/firmware/efi/efi.c                      |    3 
 include/linux/efi.h                             |    6 
 8 files changed, 242 insertions(+), 1 deletion(-)

--- linux-2.6.orig/arch/x86/platform/efi/efi.c
+++ linux-2.6/arch/x86/platform/efi/efi.c
@@ -76,6 +76,9 @@ static __initdata efi_config_table_type_
 	{NULL_GUID, NULL, NULL},
 };
 
+efi_memory_desc_t *efi_runtime_map;
+int nr_efi_runtime_map;
+
 /*
  * Returns 1 if 'facility' is enabled, 0 otherwise.
  */
@@ -852,6 +855,14 @@ static void efi_map_regions(void **new_m
 
 		memcpy(*new_memmap + (*count * memmap.desc_size), md,
 		       memmap.desc_size);
+		if (md->type != EFI_BOOT_SERVICES_CODE &&
+			md->type != EFI_BOOT_SERVICES_DATA) {
+			efi_runtime_map = krealloc(efi_runtime_map,
+					(nr_efi_runtime_map + 1) *
+					sizeof(efi_memory_desc_t), GFP_KERNEL);
+			*(efi_runtime_map + nr_efi_runtime_map) = *md;
+			nr_efi_runtime_map++;
+		}
 		(*count)++;
 	}
 
--- linux-2.6.orig/drivers/firmware/efi/Kconfig
+++ linux-2.6/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 "Add efi runtime map to sysfs" if EXPERT
+	default X86 && EFI
+	help
+	  Add the efi runtime memory map 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-efi-runtime-map.
+
 endmenu
--- linux-2.6.orig/drivers/firmware/efi/Makefile
+++ linux-2.6/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)		+= efi-runtime-map.o
--- /dev/null
+++ linux-2.6/drivers/firmware/efi/efi-runtime-map.c
@@ -0,0 +1,164 @@
+/*
+ * linux/drivers/efi/efi-runtime-map.c
+ * Copyright (C) 2013 Red Hat, Inc., Dave Young <dyoung@redhat.com>
+ *
+ * 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>
+
+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 struct kobj_type __refdata map_ktype = {
+	.sysfs_ops	= &map_attr_ops,
+	.default_attrs	= def_attrs,
+};
+
+/*
+ * Add map entry on sysfs
+ */
+static int add_sysfs_fw_map_entry(struct efi_runtime_map_entry *entry)
+{
+	static int map_entries_nr;
+	static struct kset *map_kset;
+
+	if (!map_kset) {
+		map_kset = kset_create_and_add("efi-runtime-map", NULL,
+				efi_kobj);
+		if (!map_kset)
+			return -ENOMEM;
+	}
+
+	kobject_init(&entry->kobj, &map_ktype);
+	entry->kobj.kset = map_kset;
+	if (kobject_add(&entry->kobj, NULL, "%d", map_entries_nr++))
+		kobject_put(&entry->kobj);
+
+	return 0;
+}
+
+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 int __init efi_runtime_map_init(void)
+{
+	int i;
+	struct efi_runtime_map_entry *entry;
+	efi_memory_desc_t *md = efi_runtime_map;
+
+	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;
+	entry = map_entries;
+
+	for (i = 0; i < nr_efi_runtime_map; i++) {
+		entry->md = *md;
+		add_sysfs_fw_map_entry(entry);
+		entry++;
+		md++;
+	}
+
+	return 0;
+}
+late_initcall(efi_runtime_map_init);
--- linux-2.6.orig/drivers/firmware/efi/efi.c
+++ linux-2.6/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;
 
 /*
--- linux-2.6.orig/arch/x86/include/asm/efi.h
+++ linux-2.6/arch/x86/include/asm/efi.h
@@ -133,6 +133,9 @@ extern void efi_sync_low_kernel_mappings
 extern void efi_setup_page_tables(void);
 extern void __init old_map_region(efi_memory_desc_t *md);
 
+extern efi_memory_desc_t *efi_runtime_map;
+extern int nr_efi_runtime_map;
+
 #ifdef CONFIG_EFI
 
 static inline bool efi_is_native(void)
--- /dev/null
+++ linux-2.6/Documentation/ABI/testing/sysfs-efi-runtime-map
@@ -0,0 +1,45 @@
+What:		/sys/firmware/efi/efi-runtime-map/
+Date:		Oct 2013
+Contact:	Dave Young <dyoung@redhat.com>
+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/efi-runtim-map/ is what kernel export for
+		this purpose. The structure is as follows:
+
+		subdirectories are named with the number of the memory ranges:
+
+			/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
+                        ...
--- linux-2.6.orig/include/linux/efi.h
+++ linux-2.6/include/linux/efi.h
@@ -872,4 +872,10 @@ int efivars_sysfs_init(void);
 
 #endif /* CONFIG_EFI_VARS */
 
+#ifdef CONFIG_EFI_RUNTIME_MAP
+extern efi_memory_desc_t *efi_runtime_map;
+extern int nr_efi_runtime_map;
+extern struct kobject *efi_kobj;
+#endif
+
 #endif /* _LINUX_EFI_H */

Re: [patch 5/7 v2] export efi runtime memory mapping to sysfs

[Matt Fleming]

On Tue, 05 Nov, at 04:20:12PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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/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.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/ABI/testing/sysfs-efi-runtime-map |   45 ++++++
>  arch/x86/include/asm/efi.h                      |    3 
>  arch/x86/platform/efi/efi.c                     |   11 +
>  drivers/firmware/efi/Kconfig                    |   10 +
>  drivers/firmware/efi/Makefile                   |    1 
>  drivers/firmware/efi/efi-runtime-map.c          |  164 ++++++++++++++++++++++++
>  drivers/firmware/efi/efi.c                      |    3 
>  include/linux/efi.h                             |    6 
>  8 files changed, 242 insertions(+), 1 deletion(-)

[...]

> @@ -852,6 +855,14 @@ static void efi_map_regions(void **new_m
>  
>  		memcpy(*new_memmap + (*count * memmap.desc_size), md,
>  		       memmap.desc_size);
> +		if (md->type != EFI_BOOT_SERVICES_CODE &&
> +			md->type != EFI_BOOT_SERVICES_DATA) {
> +			efi_runtime_map = krealloc(efi_runtime_map,
> +					(nr_efi_runtime_map + 1) *
> +					sizeof(efi_memory_desc_t), GFP_KERNEL);
> +			*(efi_runtime_map + nr_efi_runtime_map) = *md;
> +			nr_efi_runtime_map++;
> +		}
>  		(*count)++;
>  	}

You really need to be using 'memmap.desc_size' here and not
sizeof(efi_memory_desc_t) as the two may differ. Also, you should be
checking for failure of krealloc() and using memcpy() instead of
directly dereferencing 'md'.

> --- linux-2.6.orig/drivers/firmware/efi/Makefile
> +++ linux-2.6/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)		+= efi-runtime-map.o
> --- /dev/null
> +++ linux-2.6/drivers/firmware/efi/efi-runtime-map.c

Small nit but I wouldn't bother prefixing the filename with "efi-",
since you can't build this file as a module.

> +/*
> + * 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
> +};

If the UEFI spec ever releases an update for the memory descriptor
structure, and bumps 'memmap.desc_version', how are we going to signal
the incompatibility to legacy versions of kexec tools?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch 5/7 v2] export efi runtime memory mapping to sysfs

[Dave Young]

On 11/13/13 at 03:50pm, Matt Fleming wrote:
> On Tue, 05 Nov, at 04:20:12PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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/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.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  Documentation/ABI/testing/sysfs-efi-runtime-map |   45 ++++++
> >  arch/x86/include/asm/efi.h                      |    3 
> >  arch/x86/platform/efi/efi.c                     |   11 +
> >  drivers/firmware/efi/Kconfig                    |   10 +
> >  drivers/firmware/efi/Makefile                   |    1 
> >  drivers/firmware/efi/efi-runtime-map.c          |  164 ++++++++++++++++++++++++
> >  drivers/firmware/efi/efi.c                      |    3 
> >  include/linux/efi.h                             |    6 
> >  8 files changed, 242 insertions(+), 1 deletion(-)
> 
> [...]
> 
> > @@ -852,6 +855,14 @@ static void efi_map_regions(void **new_m
> >  
> >  		memcpy(*new_memmap + (*count * memmap.desc_size), md,
> >  		       memmap.desc_size);
> > +		if (md->type != EFI_BOOT_SERVICES_CODE &&
> > +			md->type != EFI_BOOT_SERVICES_DATA) {
> > +			efi_runtime_map = krealloc(efi_runtime_map,
> > +					(nr_efi_runtime_map + 1) *
> > +					sizeof(efi_memory_desc_t), GFP_KERNEL);
> > +			*(efi_runtime_map + nr_efi_runtime_map) = *md;
> > +			nr_efi_runtime_map++;
> > +		}
> >  		(*count)++;
> >  	}
> 
> You really need to be using 'memmap.desc_size' here and not
> sizeof(efi_memory_desc_t) as the two may differ. Also, you should be
> checking for failure of krealloc() and using memcpy() instead of
> directly dereferencing 'md'.

I'm a bit confused with the desc_size and sizeof(efi_memory_desc_t),
Because I intended to same struct efi_memory_desc_t in userspace to pass the
setup_data, so I worry about if desc_size will cause problems.

Will double check it.

> 
> > --- linux-2.6.orig/drivers/firmware/efi/Makefile
> > +++ linux-2.6/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)		+= efi-runtime-map.o
> > --- /dev/null
> > +++ linux-2.6/drivers/firmware/efi/efi-runtime-map.c
> 
> Small nit but I wouldn't bother prefixing the filename with "efi-",
> since you can't build this file as a module.

Ok, will do

> 
> > +/*
> > + * 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
> > +};
> 
> If the UEFI spec ever releases an update for the memory descriptor
> structure, and bumps 'memmap.desc_version', how are we going to signal
> the incompatibility to legacy versions of kexec tools?

Hmm, that is a problem. I will consider to export memmap according to
what firmware provided with extra desc_version instead of using attrs from kernel
data structure efi_memory_desc_t

Now it's clear for previous question about desc_size and sizeof(efi_memory_desc_t)
Will switch to use desc_size.

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

Re: [patch 5/7 v2] export efi runtime memory mapping to sysfs

[Dave Young]

> > > +/*
> > > + * 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
> > > +};
> > 
> > If the UEFI spec ever releases an update for the memory descriptor
> > structure, and bumps 'memmap.desc_version', how are we going to signal
> > the incompatibility to legacy versions of kexec tools?
> 
> Hmm, that is a problem. I will consider to export memmap according to
> what firmware provided with extra desc_version instead of using attrs from kernel
> data structure efi_memory_desc_t
> 

Matt, desc_version is already in boot_params.efi_info, so kexec-tools can get
the version from there. I do not need to export it as another file.

I think for now we do not need worry much about the compatibility issue, do you
think I need add version checking in kexec-tools currently? like below?

if (desc_version != 1) /* current version is 1? */
	error out it is not supported

--
Thanks
Dave

Re: [patch 5/7 v2] export efi runtime memory mapping to sysfs

[Matt Fleming]

On Mon, 18 Nov, at 10:16:41AM, Dave Young wrote:
> Matt, desc_version is already in boot_params.efi_info, so kexec-tools
> can get the version from there. I do not need to export it as another
> file.
 
OK, cool.

> I think for now we do not need worry much about the compatibility
> issue, do you think I need add version checking in kexec-tools
> currently? like below?
> 
> if (desc_version != 1) /* current version is 1? */
> 	error out it is not supported

Yes, something like that. If you already have kexec-specific ways of
checking a version number then that's fine. Just make sure that, if the
EFI memory descriptor structure is extended in the future and more files
are exported for each descriptor, kexec-tools say "I don't know how to
build a data structure for from these files".

-- 
Matt Fleming, Intel Open Source Technology Center

[patch 6/7 v2] passing kexec necessary efi data via setup_data

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 06-use-efi-setup-data.patch --]
[-- Type: text/plain, Size: 8628 bytes --]

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.

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           |  151 ++++++++++++++++++++++++++++++----
 4 files changed, 152 insertions(+), 15 deletions(-)

--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/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;
 		}
--- linux-2.6.orig/arch/x86/platform/efi/efi.c
+++ linux-2.6/arch/x86/platform/efi/efi.c
@@ -78,6 +78,7 @@ static __initdata efi_config_table_type_
 
 efi_memory_desc_t *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
 }
 early_param("efi_no_storage_paranoia", setup_storage_paranoia);
 
+void 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);
+
+	/* setup data regions have been reserved by default */
+	esdata = phys_to_virt(esdata_phys);
+}
 
 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 *
 		}
 
 		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 *
 		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;
 
@@ -871,6 +948,39 @@ err_out:
 }
 
 /*
+ * 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.
+ */
+void efi_map_regions_fixed(void)
+{
+	int i;
+	unsigned long size;
+	efi_memory_desc_t *md;
+	u64 end, systab;
+
+	for (i = 0; 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;
+		}
+	}
+
+	efi_runtime_map = kmalloc(nr_efi_runtime_map *
+			sizeof(efi_memory_desc_t), GFP_KERNEL);
+
+	memcpy(efi_runtime_map, esdata->map,
+		nr_efi_runtime_map * sizeof(efi_memory_desc_t));
+}
+
+/*
  * 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
@@ -886,11 +996,15 @@ err_out:
  * 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)
 {
 	efi_status_t status;
-	void *p, *new_memmap = NULL;
+	void *new_memmap = NULL;
 	int count = 0;
 
 	efi.systab = NULL;
@@ -904,20 +1018,26 @@ void __init efi_enter_virtual_mode(void)
 		return;
 	}
 
-	efi_merge_regions();
-
-	efi_map_regions(&new_memmap, &count);
+	if (esdata)
+		efi_map_regions_fixed();
+	else {
+		efi_merge_regions();
+		efi_map_regions(&new_memmap, &count);
+	}
 
 	BUG_ON(!efi.systab);
 
 	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;
+	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 "
@@ -925,6 +1045,7 @@ void __init efi_enter_virtual_mode(void)
 		panic("EFI call to SetVirtualAddressMap() failed!");
 	}
 
+
 	/*
 	 * Now that EFI is in virtual mode, update the function
 	 * pointers in the runtime service table to the new virtual addresses.
--- linux-2.6.orig/arch/x86/include/uapi/asm/bootparam.h
+++ linux-2.6/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
--- linux-2.6.orig/arch/x86/include/asm/efi.h
+++ linux-2.6/arch/x86/include/asm/efi.h
@@ -136,6 +136,18 @@ extern void __init old_map_region(efi_me
 extern efi_memory_desc_t *efi_runtime_map;
 extern int nr_efi_runtime_map;
 
+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)

Re: [patch 6/7 v2] passing kexec necessary efi data via setup_data

[Matt Fleming]

On Tue, 05 Nov, at 04:20:13PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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.
 
Does that mean that on some of the platforms you tested the SMBIOS
address isn't updated?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch 6/7 v2] passing kexec necessary efi data via setup_data

[Dave Young]

On 11/13/13 at 03:50pm, Matt Fleming wrote:
> On Tue, 05 Nov, at 04:20:13PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org 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.
>  
> Does that mean that on some of the platforms you tested the SMBIOS
> address isn't updated?

Yes, on my three test machines, lenovo, dell and HP, only the HP
machine need this that means the lenovo and dell machine does not
update the SMBIOS address to virt addr.

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

[patch 7/7 v2] x86: add xloadflags bit for efi runtime support on kexec

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 07-add-xloadflags-for-kexec-efi-runtime-feature.patch --]
[-- Type: text/plain, Size: 1465 bytes --]

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.

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

--- linux-2.6.orig/arch/x86/boot/header.S
+++ linux-2.6/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)
+# 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
--- linux-2.6.orig/arch/x86/include/uapi/asm/bootparam.h
+++ linux-2.6/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__

Re: [patch 7/7 v2] x86: add xloadflags bit for efi runtime support on kexec

[Matt Fleming]

On Tue, 05 Nov, at 04:20:14PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> 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.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/boot/header.S                |    9 ++++++++-
>  arch/x86/include/uapi/asm/bootparam.h |    1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.orig/arch/x86/boot/header.S
> +++ linux-2.6/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
> +

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch 7/7 v2] x86: add xloadflags bit for efi runtime support on kexec

[Dave Young]

On 11/13/13 at 03:50pm, Matt Fleming wrote:
> On Tue, 05 Nov, at 04:20:14PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > 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.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/boot/header.S                |    9 ++++++++-
> >  arch/x86/include/uapi/asm/bootparam.h |    1 +
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > --- linux-2.6.orig/arch/x86/boot/header.S
> > +++ linux-2.6/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) ?

Will add above.

> 
> > +# define XLF4 XLF_EFI_KEXEC
> > +#else
> > +# define XLF4 0
> > +#endif
> > +
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

Re: [patch 7/7 v2] x86: add xloadflags bit for efi runtime support on kexec

[H. Peter Anvin]

On 11/05/2013 12:20 AM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> 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.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/x86/boot/header.S                |    9 ++++++++-
>  arch/x86/include/uapi/asm/bootparam.h |    1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.orig/arch/x86/boot/header.S
> +++ linux-2.6/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)
> +# 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
> --- linux-2.6.orig/arch/x86/include/uapi/asm/bootparam.h
> +++ linux-2.6/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__
>  
> 

Also needs to be added to Documentation/x86/boot.txt with the exact
semantics being exposed.

	-hpa

Re: [patch 7/7 v2] x86: add xloadflags bit for efi runtime support on kexec

[Dave Young]

On 11/13/13 at 08:20am, H. Peter Anvin wrote:
> On 11/05/2013 12:20 AM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > 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.
> > 
> > Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/x86/boot/header.S                |    9 ++++++++-
> >  arch/x86/include/uapi/asm/bootparam.h |    1 +
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > --- linux-2.6.orig/arch/x86/boot/header.S
> > +++ linux-2.6/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)
> > +# 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
> > --- linux-2.6.orig/arch/x86/include/uapi/asm/bootparam.h
> > +++ linux-2.6/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__
> >  
> > 
> 
> Also needs to be added to Documentation/x86/boot.txt with the exact
> semantics being exposed.

Will do

> 
> 	-hpa
> 

Re: [patch 0/7 v2] kexec kernel efi runtime support

[Borislav Petkov]

On Tue, Nov 05, 2013 at 04:20:07PM +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> Please help to review the patches.

Sure, but will have to wait 'til next week when I get back.

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [patch 0/7 v2] kexec kernel efi runtime support

[Matt Fleming]

On Tue, 05 Nov, at 04:20:07PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> Hi,
> 
> Here is the V2 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.

I see this in the dmesg,

[    0.000000] efi: skipping setup_data on EFI 32BIT!

despite the fact that this is on an x86-64 box. Turns out it's because
CONFIG_DEBUG_BOOT_PARAMS isn't set in my config. You may want to turn
that on automatically. After doing that things work on my ASUS box (good
work!) but the SATA controller craps out on my Tunnelmountain machine,
but that's probably unrelated and I'll debug that separately.

I see a bunch of section mismatch warnings,

WARNING: arch/x86/platform/efi/built-in.o(.text+0xd1e): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed()
The function efi_map_regions_fixed() references
the function __init efi_map_region_fixed().
This is often because efi_map_regions_fixed lacks a __init 
annotation or the annotation of efi_map_region_fixed is wrong.

WARNING: arch/x86/platform/efi/built-in.o(.text+0xd2a): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:efi_phys
The function efi_map_regions_fixed() references
the variable __initdata efi_phys.
This is often because efi_map_regions_fixed lacks a __initdata 
annotation or the annotation of efi_phys is wrong.

WARNING: arch/x86/platform/built-in.o(.text+0xd1e): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed()
The function efi_map_regions_fixed() references
the function __init efi_map_region_fixed().
This is often because efi_map_regions_fixed lacks a __init 
annotation or the annotation of efi_map_region_fixed is wrong.

WARNING: arch/x86/platform/built-in.o(.text+0xd2a): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:efi_phys
The function efi_map_regions_fixed() references
the variable __initdata efi_phys.
This is often because efi_map_regions_fixed lacks a __initdata 
annotation or the annotation of efi_phys is wrong.

WARNING: arch/x86/built-in.o(.text+0x7c357): Section mismatch in reference from the function parse_efi_setup() to the function .init.text:early_memremap()
The function parse_efi_setup() references
the function __init early_memremap().
This is often because parse_efi_setup lacks a __init 
annotation or the annotation of early_memremap is wrong.

WARNING: arch/x86/built-in.o(.text+0x7c390): Section mismatch in reference from the function parse_efi_setup() to the function .init.text:early_iounmap()
The function parse_efi_setup() references
the function __init early_iounmap().
This is often because parse_efi_setup lacks a __init 
annotation or the annotation of early_iounmap is wrong.

WARNING: arch/x86/built-in.o(.text+0x7c5ce): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed()
The function efi_map_regions_fixed() references
the function __init efi_map_region_fixed().
This is often because efi_map_regions_fixed lacks a __init 
annotation or the annotation of efi_map_region_fixed is wrong.

WARNING: arch/x86/built-in.o(.text+0x7c5da): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:vdso32_sysenter_end
The function efi_map_regions_fixed() references
the variable __initdata vdso32_sysenter_end.
This is often because efi_map_regions_fixed lacks a __initdata 
annotation or the annotation of vdso32_sysenter_end is wrong.

Also, many of your patch descriptions are missing subsystem tags. Please
fix this in your next submission.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [patch 0/7 v2] kexec kernel efi runtime support

[Dave Young]

Hi, Matt
On 11/08/13 at 02:31pm, Matt Fleming wrote:
> On Tue, 05 Nov, at 04:20:07PM, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > Hi,
> > 
> > Here is the V2 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.
> 
> I see this in the dmesg,
> 
> [    0.000000] efi: skipping setup_data on EFI 32BIT!
> 
> despite the fact that this is on an x86-64 box. Turns out it's because
> CONFIG_DEBUG_BOOT_PARAMS isn't set in my config. You may want to turn
> that on automatically. After doing that things work on my ASUS box (good
> work!) but the SATA controller craps out on my Tunnelmountain machine,
> but that's probably unrelated and I'll debug that separately.

Hmm, if CONFIG_DEBUG_BOOT_PARAMS is not set, then kexec-tools should
fail getting efi_info, so I will fix kexec-tools patch about this.

Also CONFIG_EFI_RUNTIME_MAP should select CONFIG_DEBUG_BOOT_PARAMS.
In future will try to move the boot params data out of debugfs. 

> 
> I see a bunch of section mismatch warnings,
> 
> WARNING: arch/x86/platform/efi/built-in.o(.text+0xd1e): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed()
> The function efi_map_regions_fixed() references
> the function __init efi_map_region_fixed().
> This is often because efi_map_regions_fixed lacks a __init 
> annotation or the annotation of efi_map_region_fixed is wrong.
> 
> WARNING: arch/x86/platform/efi/built-in.o(.text+0xd2a): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:efi_phys
> The function efi_map_regions_fixed() references
> the variable __initdata efi_phys.
> This is often because efi_map_regions_fixed lacks a __initdata 
> annotation or the annotation of efi_phys is wrong.
> 
> WARNING: arch/x86/platform/built-in.o(.text+0xd1e): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed()
> The function efi_map_regions_fixed() references
> the function __init efi_map_region_fixed().
> This is often because efi_map_regions_fixed lacks a __init 
> annotation or the annotation of efi_map_region_fixed is wrong.
> 
> WARNING: arch/x86/platform/built-in.o(.text+0xd2a): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:efi_phys
> The function efi_map_regions_fixed() references
> the variable __initdata efi_phys.
> This is often because efi_map_regions_fixed lacks a __initdata 
> annotation or the annotation of efi_phys is wrong.
> 
> WARNING: arch/x86/built-in.o(.text+0x7c357): Section mismatch in reference from the function parse_efi_setup() to the function .init.text:early_memremap()
> The function parse_efi_setup() references
> the function __init early_memremap().
> This is often because parse_efi_setup lacks a __init 
> annotation or the annotation of early_memremap is wrong.
> 
> WARNING: arch/x86/built-in.o(.text+0x7c390): Section mismatch in reference from the function parse_efi_setup() to the function .init.text:early_iounmap()
> The function parse_efi_setup() references
> the function __init early_iounmap().
> This is often because parse_efi_setup lacks a __init 
> annotation or the annotation of early_iounmap is wrong.
> 
> WARNING: arch/x86/built-in.o(.text+0x7c5ce): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed()
> The function efi_map_regions_fixed() references
> the function __init efi_map_region_fixed().
> This is often because efi_map_regions_fixed lacks a __init 
> annotation or the annotation of efi_map_region_fixed is wrong.
> 
> WARNING: arch/x86/built-in.o(.text+0x7c5da): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:vdso32_sysenter_end
> The function efi_map_regions_fixed() references
> the variable __initdata vdso32_sysenter_end.
> This is often because efi_map_regions_fixed lacks a __initdata 
> annotation or the annotation of vdso32_sysenter_end is wrong.

Will fix, thanks for testing.

> 
> Also, many of your patch descriptions are missing subsystem tags. Please
> fix this in your next submission.

Do you means to add "efi:" in subject?

Thanks
Dave

Re: [patch 0/7 v2] kexec kernel efi runtime support

[H. Peter Anvin]

On 11/08/2013 07:57 PM, Dave Young wrote:
> 
> Hmm, if CONFIG_DEBUG_BOOT_PARAMS is not set, then kexec-tools should
> fail getting efi_info, so I will fix kexec-tools patch about this.
> 
> Also CONFIG_EFI_RUNTIME_MAP should select CONFIG_DEBUG_BOOT_PARAMS.
> In future will try to move the boot params data out of debugfs. 
> 

Honestly, if we need debugfs to get actual functionality, it shouldn't
be in sploit^Wdebugfs...

	-hpa

Re: [patch 0/7 v2] kexec kernel efi runtime support

[Dave Young]

On 11/08/13 at 09:01pm, H. Peter Anvin wrote:
> On 11/08/2013 07:57 PM, Dave Young wrote:
> > 
> > Hmm, if CONFIG_DEBUG_BOOT_PARAMS is not set, then kexec-tools should
> > fail getting efi_info, so I will fix kexec-tools patch about this.
> > 
> > Also CONFIG_EFI_RUNTIME_MAP should select CONFIG_DEBUG_BOOT_PARAMS.
> > In future will try to move the boot params data out of debugfs. 
> > 
> 
> Honestly, if we need debugfs to get actual functionality, it shouldn't
> be in sploit^Wdebugfs...
> 

Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> created the debugfs file for boot_params.
His first version patch tried sysfs, but sysfs is not designed for such
binary blobs so finally it go to debugfs.

Any idea for this is welcome, till now I have no better idea for such kind
of data. We should have another *fs instead of using debugfs.

Thanks
Dave

Re: [patch 0/7 v2] kexec kernel efi runtime support

[H. Peter Anvin]

On 11/10/2013 06:13 PM, Dave Young wrote:
> 
> Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> created the debugfs file for boot_params.
> His first version patch tried sysfs, but sysfs is not designed for such
> binary blobs so finally it go to debugfs.
> 

That is a misunderstanding.  Binary blobs can exist in sysfs as long as
the blob is something that is inherently a blob.  This is admittedly a
corner case, but it is without any doubt a protocol-defined binary
structure.

The reason it was put in debugfs is that there was no non-debug user for
it at the time.

> Any idea for this is welcome, till now I have no better idea for such kind
> of data. We should have another *fs instead of using debugfs.

The problem with debugfs is that things go into debugfs with largely no
auditing.  As a result, mounting debugfs is very likely to mean that
your system is exploitable one way or another.

	-hpa

Re: [patch 0/7 v2] kexec kernel efi runtime support

[Dave Young]

On 11/10/13 at 06:21pm, H. Peter Anvin wrote:
> On 11/10/2013 06:13 PM, Dave Young wrote:
> > 
> > Huang Ying <ying.huang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> created the debugfs file for boot_params.
> > His first version patch tried sysfs, but sysfs is not designed for such
> > binary blobs so finally it go to debugfs.
> > 
> 
> That is a misunderstanding.  Binary blobs can exist in sysfs as long as
> the blob is something that is inherently a blob.  This is admittedly a
> corner case, but it is without any doubt a protocol-defined binary
> structure.

You are right. Greg objected that the whole structure being exported directly.

> 
> The reason it was put in debugfs is that there was no non-debug user for
> it at the time.

Ok, I did not know this background.

> 
> > Any idea for this is welcome, till now I have no better idea for such kind
> > of data. We should have another *fs instead of using debugfs.
> 
> The problem with debugfs is that things go into debugfs with largely no
> auditing.  As a result, mounting debugfs is very likely to mean that
> your system is exploitable one way or another.

Hmm, agree. Thanks for explaining about it.

Thanks
Dave