[patch 0/3 v2] kexec-tools: efi runtime support

[patch 0/3 v2] kexec-tools: efi runtime support

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

This patchset is for adding efi runtime support on kexec kernel
kernel patches see below thread:
http://thread.gmane.org/gmane.linux.kernel.efi/2491

in kexec-tools, this patchset will do below:
1. retrieve efi_info from debugfs boot_params, and fill the
x86 setup header

2. collect data efi runtime needed:
/sys/firmware/efi/systab: fw_vendor, runtime, config_tables and smbios
/sys/firmware/efi/efi-runtime-map/*, the phys-virt mappings in 1st kernel

3. assemble setup_data based on data get in 2) then pass it to 2nd kernel

Tested on OVMF, dell laptop, lenovo laptop and HP workstation

Please help to review

V2 changelog:
Address comments from
Simon:
  coding style, fixed a mem leak
HPA:
  use type uint64_t instead of __uint64_t
mjg:
  do not cause regression for loading old kernel
  use xloadflags to check the bzImage support for efi.
  in case old kernel, do not pass efi_info and setup_data
  for new kernel, do not pass acpi_rsdp
Vivek:
  suggest to export a value in bzImage probe for efi support
  so it can be used to check if we should pass acpi_rsdp.

--
Thanks
dave

[patch 1/3 v2] Add function get_bootparam

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 01-kexec-add-function-get-boot-param.patch --]
[-- Type: text/plain, Size: 1737 bytes --]

Not only setup_subarch will get data from debugfs file
boot_params/data, later code for adding efi_info will
also need do same thing. Thus add a common function here
for later use.

v1->v2: make get_bootparam() static

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

--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
@@ -436,10 +436,9 @@ char *find_mnt_by_fsname(char *fsname)
 	return mntdir;
 }
 
-void setup_subarch(struct x86_linux_param_header *real_mode)
+static void get_bootparam(void *buf, off_t offset, size_t size)
 {
 	int data_file;
-	const off_t offset = offsetof(typeof(*real_mode), hardware_subarch);
 	char *debugfs_mnt;
 	char filename[PATH_MAX];
 
@@ -447,7 +446,7 @@ void setup_subarch(struct x86_linux_para
 	if (!debugfs_mnt)
 		return;
 	snprintf(filename, PATH_MAX, "%s/%s", debugfs_mnt, "boot_params/data");
-	filename[PATH_MAX-1] = 0;
+	filename[PATH_MAX - 1] = 0;
 	free(debugfs_mnt);
 
 	data_file = open(filename, O_RDONLY);
@@ -455,11 +454,18 @@ void setup_subarch(struct x86_linux_para
 		return;
 	if (lseek(data_file, offset, SEEK_SET) < 0)
 		goto close;
-	read(data_file, &real_mode->hardware_subarch, sizeof(uint32_t));
+	read(data_file, buf, size);
 close:
 	close(data_file);
 }
 
+void setup_subarch(struct x86_linux_param_header *real_mode)
+{
+	off_t offset = offsetof(typeof(*real_mode), hardware_subarch);
+
+	get_bootparam(&real_mode->hardware_subarch, offset, sizeof(uint32_t));
+}
+
 void setup_linux_system_parameters(struct kexec_info *info,
 				   struct x86_linux_param_header *real_mode)
 {

[patch 2/3 v2] Add efi_info in x86 setup header

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 02-kexec-padd-efi_info.patch --]
[-- Type: text/plain, Size: 3866 bytes --]

For supporting efi runtime on kexec kernel we need to
fill the efi_info struct in setup_header. I just get
the info in kernel exported boot_params data in debugfs.

v1->v2:
update comment for offset of reserved4_1[] in x87_linux_param_header
Address comment from mjg59: do not break old kernel when use newer kexec-tools.
  add checking for xloadflags bit 4 XLF_EFI_KEXEC. Only fill
  efi_info and pass acpi_rsdp when the kexec kernel support efi boot.
coding style fix, change internal function to be static

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 include/x86/x86-linux.h             |    3 ++-
 kexec/arch/i386/crashdump-x86.c     |    4 +++-
 kexec/arch/i386/x86-linux-setup.c   |    9 +++++++++
 kexec/arch/i386/x86-linux-setup.h   |    1 +
 kexec/arch/x86_64/kexec-bzImage64.c |    6 ++++++
 5 files changed, 21 insertions(+), 2 deletions(-)

--- kexec-tools.orig/include/x86/x86-linux.h
+++ kexec-tools/include/x86/x86-linux.h
@@ -115,7 +115,8 @@ struct x86_linux_param_header {
 	uint32_t ext_ramdisk_image;		/* 0xc0 */
 	uint32_t ext_ramdisk_size;		/* 0xc4 */
 	uint32_t ext_cmd_line_ptr;		/* 0xc8 */
-	uint8_t reserved4_1[0x1e0 - 0xcc];	/* 0xcc */
+	uint8_t reserved4_1[0x1c0 - 0xcc];	/* 0xe4 */
+	uint8_t efi_info[32];			/* 0x1c0 */
 	uint32_t alt_mem_k;			/* 0x1e0 */
 	uint8_t  reserved5[4];			/* 0x1e4 */
 	uint8_t  e820_map_nr;			/* 0x1e8 */
--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
@@ -466,6 +466,13 @@ void setup_subarch(struct x86_linux_para
 	get_bootparam(&real_mode->hardware_subarch, offset, sizeof(uint32_t));
 }
 
+static void setup_efi_info(struct x86_linux_param_header *real_mode)
+{
+	off_t offset = offsetof(typeof(*real_mode), efi_info);
+
+	get_bootparam(&real_mode->efi_info, offset, 32);
+}
+
 void setup_linux_system_parameters(struct kexec_info *info,
 				   struct x86_linux_param_header *real_mode)
 {
@@ -475,6 +482,8 @@ void setup_linux_system_parameters(struc
 
 	/* get subarch from running kernel */
 	setup_subarch(real_mode);
+	if (bzImage_support_efi_boot)
+		setup_efi_info(real_mode);
 	
 	/* Default screen size */
 	real_mode->orig_x = 0;
--- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
+++ kexec-tools/kexec/arch/i386/crashdump-x86.c
@@ -41,6 +41,7 @@
 #include "../../crashdump.h"
 #include "kexec-x86.h"
 #include "crashdump-x86.h"
+#include "x86-linux-setup.h"
 
 #ifdef HAVE_LIBXENCTRL
 #ifdef HAVE_XC_GET_MACHINE_MEMORY_MAP
@@ -1046,7 +1047,8 @@ int load_crashdump_segments(struct kexec
 	if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0)
 		return -1;
 	cmdline_add_memmap(mod_cmdline, memmap_p);
-	cmdline_add_efi(mod_cmdline);
+	if (!bzImage_support_efi_boot)
+		cmdline_add_efi(mod_cmdline);
 	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
 
 	/* Inform second kernel about the presence of ACPI tables. */
--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.h
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.h
@@ -28,5 +28,6 @@ void setup_linux_system_parameters(struc
 
 /* command line parameter may be appended by purgatory */
 #define PURGATORY_CMDLINE_SIZE 64
+extern int bzImage_support_efi_boot;
 
 #endif /* X86_LINUX_SETUP_H */
--- kexec-tools.orig/kexec/arch/x86_64/kexec-bzImage64.c
+++ kexec-tools/kexec/arch/x86_64/kexec-bzImage64.c
@@ -42,6 +42,7 @@
 #include <arch/options.h>
 
 static const int probe_debug = 0;
+int bzImage_support_efi_boot;
 
 int bzImage64_probe(const char *buf, off_t len)
 {
@@ -82,6 +83,11 @@ int bzImage64_probe(const char *buf, off
 		/* Must be KERNEL_64 and CAN_BE_LOADED_ABOVE_4G */
 		return -1;
 	}
+
+#define XLF_EFI_KEXEC   (1 << 4)
+	if ((header->xloadflags & XLF_EFI_KEXEC) == XLF_EFI_KEXEC)
+		bzImage_support_efi_boot = 1;
+
 	/* I've got a relocatable bzImage64 */
 	if (probe_debug)
 		fprintf(stderr, "It's a relocatable bzImage64\n");

[patch 3/3 v2] Passing efi related data via setup_data

[dyoung-H+wXaHxf7aLQT0dZR+AlfA]

[-- Attachment #1: 03-kexec-add-efi-setup-data.patch --]
[-- Type: text/plain, Size: 4689 bytes --]

For supporting efi runtime, several efi physical addresses
fw_vendor, runtime, config tables, smbios and the whole runtime
mapping info need to be used in kexec kernel. Thus introduce
setup_data struct for passing these data.

collect the varialbes from /sys/firmware/efi/systab and
/sys/firmware/efi/efi-runtime-map

Tested on qemu+ovmf, dell laptop, lenovo laptop and HP workstation.

v1->v2:
HPA: use uint*_t instead of __uint*_t
Simon: indention fix; fix a memory leak
move offset change update to previous patch in setup header
only passing setup_data when the bzImage support efi boot
coding style

Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 kexec/arch/i386/x86-linux-setup.c |  132 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 131 insertions(+), 1 deletion(-)

--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
@@ -473,6 +473,134 @@ static void setup_efi_info(struct x86_li
 	get_bootparam(&real_mode->efi_info, offset, 32);
 }
 
+struct efi_mem_descriptor {
+	uint32_t type;
+	uint32_t pad;
+	uint64_t phys_addr;
+	uint64_t virt_addr;
+	uint64_t num_pages;
+	uint64_t attribute;
+};
+
+struct efi_setup_data {
+	uint64_t fw_vendor;
+	uint64_t runtime;
+	uint64_t tables;
+	uint64_t smbios;
+	uint64_t reserved[8];
+	struct efi_mem_descriptor map[0];
+};
+
+struct setup_data {
+	uint64_t next;
+	uint32_t type;
+	uint32_t len;
+	uint8_t data[0];
+} __attribute__((packed));
+
+static void _get_efi_value(char *line, const char *pattern, __uint64_t *val)
+{
+	char *s, *end;
+	s = strstr(line, pattern);
+	if (s)
+		*val = strtoull(s + strlen(pattern), &end, 16);
+}
+
+static void get_efi_value(struct efi_setup_data *esd)
+{
+	FILE *fp;
+	char line[1024];
+
+	fp = fopen("/sys/firmware/efi/systab", "r");
+	if (!fp)
+		return;
+
+	while (fgets(line, sizeof(line), fp) != 0) {
+		_get_efi_value(line, "fw_vendor=0x", &esd->fw_vendor);
+		_get_efi_value(line, "runtime=0x", &esd->runtime);
+		_get_efi_value(line, "config_tables=0x", &esd->tables);
+		_get_efi_value(line, "SMBIOS=0x", &esd->smbios);
+	}
+
+	fclose(fp);
+}
+
+static int get_efi_runtime_map(struct efi_setup_data **esd)
+{
+	DIR *dirp;
+	struct dirent *entry;
+	char filename[1024];
+	struct efi_mem_descriptor md;
+	int nr_maps = 0;
+
+	dirp = opendir("/sys/firmware/efi/efi-runtime-map");
+	while ((entry = readdir(dirp)) != NULL) {
+		sprintf(filename,
+			"/sys/firmware/efi/efi-runtime-map/%s",
+			(char *)entry->d_name);
+		if (*entry->d_name == '.')
+			continue;
+		file_scanf(filename, "type", "0x%x", (unsigned int *)&md.type);
+		file_scanf(filename, "phys_addr", "0x%llx",
+			   (unsigned long long *)&md.phys_addr);
+		file_scanf(filename, "virt_addr", "0x%llx",
+			   (unsigned long long *)&md.virt_addr);
+		file_scanf(filename, "num_pages", "0x%llx",
+			   (unsigned long long *)&md.num_pages);
+		file_scanf(filename, "attribute", "0x%llx",
+			   (unsigned long long *)&md.attribute);
+		*esd = realloc(*esd, sizeof(struct efi_setup_data) +
+			   (nr_maps + 1) * sizeof(struct efi_mem_descriptor));
+		*((*esd)->map + nr_maps) = md;
+		nr_maps++;
+	}
+
+	closedir(dirp);
+	return nr_maps;
+}
+
+static void setup_efi_setup_data(struct kexec_info *info,
+			struct x86_linux_param_header *real_mode)
+{
+	int nr_maps;
+	int64_t setup_data_paddr;
+	struct setup_data *sd;
+	struct efi_setup_data *esd;
+	int size, sdsize;
+	int has_efi = 0;
+
+	has_efi = access("/sys/firmware/efi/systab", F_OK);
+	if (has_efi < 0)
+		return;
+
+	esd = malloc(sizeof(struct efi_setup_data));
+	if (!esd)
+		return;
+	memset(esd, 0, sizeof(struct efi_setup_data));
+	get_efi_value(esd);
+	nr_maps = get_efi_runtime_map(&esd);
+	size = nr_maps * sizeof(struct efi_mem_descriptor) +
+		sizeof(struct efi_setup_data);
+	sd = malloc(sizeof(struct setup_data) + size);
+	if (!sd) {
+		free(esd);
+		return;
+	}
+
+	memset(sd, 0, sizeof(struct setup_data) + size);
+	sd->next = 0;
+	sd->type = 4;
+	sd->len = size;
+	memcpy(sd->data, esd, size);
+	free(esd);
+	sdsize = sd->len + sizeof(struct setup_data);
+	setup_data_paddr = add_buffer(info, sd, sdsize, sdsize, getpagesize(),
+					0x100000, ULONG_MAX, INT_MAX);
+
+	real_mode->setup_data = setup_data_paddr;
+}
+
+
 void setup_linux_system_parameters(struct kexec_info *info,
 				   struct x86_linux_param_header *real_mode)
 {
@@ -482,8 +610,10 @@ void setup_linux_system_parameters(struc
 
 	/* get subarch from running kernel */
 	setup_subarch(real_mode);
-	if (bzImage_support_efi_boot)
+	if (bzImage_support_efi_boot) {
 		setup_efi_info(real_mode);
+		setup_efi_setup_data(info, real_mode);
+	}
 	
 	/* Default screen size */
 	real_mode->orig_x = 0;

Re: [patch 1/3 v2] Add function get_bootparam

[Toshi Kani]

On Tue, 2013-11-05 at 16:29 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> Not only setup_subarch will get data from debugfs file
> boot_params/data, later code for adding efi_info will
> also need do same thing. Thus add a common function here
> for later use.

get_bootparam() calls find_mnt_by_fsname("debugfs"), which assumes that
debugfs is mounted with device name "debugfs".  This function fails
when:
 - debugfs is not mounted, or
 - debugfs is mounted with a different device name, such as nodev [1].

This issue is not introduced by this patch.  But the original code,
which sets hardware_subarch, seems to work even if it failed to access
debugfs (which is ignored) since hardware_subarch is zero most of the
cases anyway.  With this change, however, this failure now makes the 2nd
kernel unbootable.  So, it needs to be addressed to make this code path
work reliably (or kexec should fail at least).

[1]
https://access.redhat.com/site/documentation/en-US/Red_Hat_Enterprise_MRG/2/html/Realtime_Tuning_Guide/sect-Realtime_Tuning_Guide-Realtime_Specific_Tuning-Mount_debugfs.html

Thanks,
Toshi

Re: [patch 1/3 v2] Add function get_bootparam

[Dave Young]

On 11/11/13 at 06:27pm, Toshi Kani wrote:
> On Tue, 2013-11-05 at 16:29 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> > Not only setup_subarch will get data from debugfs file
> > boot_params/data, later code for adding efi_info will
> > also need do same thing. Thus add a common function here
> > for later use.
> 
> get_bootparam() calls find_mnt_by_fsname("debugfs"), which assumes that
> debugfs is mounted with device name "debugfs".  This function fails
> when:
>  - debugfs is not mounted, or
>  - debugfs is mounted with a different device name, such as nodev [1].
> 
> This issue is not introduced by this patch.  But the original code,
> which sets hardware_subarch, seems to work even if it failed to access
> debugfs (which is ignored) since hardware_subarch is zero most of the
> cases anyway.  With this change, however, this failure now makes the 2nd
> kernel unbootable.  So, it needs to be addressed to make this code path
> work reliably (or kexec should fail at least).

Toshi, Thanks for your suggestion, it make sense. Will fix it.

> 
> [1]
> https://access.redhat.com/site/documentation/en-US/Red_Hat_Enterprise_MRG/2/html/Realtime_Tuning_Guide/sect-Realtime_Tuning_Guide-Realtime_Specific_Tuning-Mount_debugfs.html
> 
> Thanks,
> Toshi
> 

Re: [patch 1/3 v2] Add function get_bootparam

[H. Peter Anvin]

On 11/11/2013 05:27 PM, Toshi Kani wrote:
> On Tue, 2013-11-05 at 16:29 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
>> Not only setup_subarch will get data from debugfs file
>> boot_params/data, later code for adding efi_info will
>> also need do same thing. Thus add a common function here
>> for later use.
> 
> get_bootparam() calls find_mnt_by_fsname("debugfs"), which assumes that
> debugfs is mounted with device name "debugfs".  This function fails
> when:
>  - debugfs is not mounted, or
>  - debugfs is mounted with a different device name, such as nodev [1].
> 
> This issue is not introduced by this patch.  But the original code,
> which sets hardware_subarch, seems to work even if it failed to access
> debugfs (which is ignored) since hardware_subarch is zero most of the
> cases anyway.  With this change, however, this failure now makes the 2nd
> kernel unbootable.  So, it needs to be addressed to make this code path
> work reliably (or kexec should fail at least).
> 

Greg, Ingo,

Since there is now a legitimate user of this stuff, can we actually
export this in sysfs (or something else other than sploit^Wdebugfs)?

kexec-tools can have a fallback to debugfs if we really need it, but
making people mount debugfs to have some essential piece of
functionality scares the heck out of me.

	-hpa

Re: [patch 1/3 v2] Add function get_bootparam

[Ingo Molnar]

* H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:

> On 11/11/2013 05:27 PM, Toshi Kani wrote:
> > On Tue, 2013-11-05 at 16:29 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> >> Not only setup_subarch will get data from debugfs file
> >> boot_params/data, later code for adding efi_info will
> >> also need do same thing. Thus add a common function here
> >> for later use.
> > 
> > get_bootparam() calls find_mnt_by_fsname("debugfs"), which assumes that
> > debugfs is mounted with device name "debugfs".  This function fails
> > when:
> >  - debugfs is not mounted, or
> >  - debugfs is mounted with a different device name, such as nodev [1].
> > 
> > This issue is not introduced by this patch.  But the original code,
> > which sets hardware_subarch, seems to work even if it failed to access
> > debugfs (which is ignored) since hardware_subarch is zero most of the
> > cases anyway.  With this change, however, this failure now makes the 2nd
> > kernel unbootable.  So, it needs to be addressed to make this code path
> > work reliably (or kexec should fail at least).
> > 
> 
> Greg, Ingo,
> 
> Since there is now a legitimate user of this stuff, can we actually 
> export this in sysfs (or something else other than sploit^Wdebugfs)?
> 
> kexec-tools can have a fallback to debugfs if we really need it, but 
> making people mount debugfs to have some essential piece of 
> functionality scares the heck out of me.

No principial objections from me: anything that actually turns out to 
prove itself in debugfs and ends up mattering for real ought to move to 
sysfs.

Thanks,

	Ingo

Re: [patch 1/3 v2] Add function get_bootparam

[Greg KH]

On Mon, Nov 11, 2013 at 07:20:17PM -0800, H. Peter Anvin wrote:
> On 11/11/2013 05:27 PM, Toshi Kani wrote:
> > On Tue, 2013-11-05 at 16:29 +0800, dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
> >> Not only setup_subarch will get data from debugfs file
> >> boot_params/data, later code for adding efi_info will
> >> also need do same thing. Thus add a common function here
> >> for later use.
> > 
> > get_bootparam() calls find_mnt_by_fsname("debugfs"), which assumes that
> > debugfs is mounted with device name "debugfs".  This function fails
> > when:
> >  - debugfs is not mounted, or
> >  - debugfs is mounted with a different device name, such as nodev [1].
> > 
> > This issue is not introduced by this patch.  But the original code,
> > which sets hardware_subarch, seems to work even if it failed to access
> > debugfs (which is ignored) since hardware_subarch is zero most of the
> > cases anyway.  With this change, however, this failure now makes the 2nd
> > kernel unbootable.  So, it needs to be addressed to make this code path
> > work reliably (or kexec should fail at least).
> > 
> 
> Greg, Ingo,
> 
> Since there is now a legitimate user of this stuff, can we actually
> export this in sysfs (or something else other than sploit^Wdebugfs)?

Hey, after Tejun is done with the kernelfs work, I'll be porting debugfs
to it so you can't call it sploitfs anymore :)

> kexec-tools can have a fallback to debugfs if we really need it, but
> making people mount debugfs to have some essential piece of
> functionality scares the heck out of me.

I agree, that would not be good.

I'm not sure what exactly the sysfs file is wanting to look like?  The
efi section variable sysfs file isn't ok (multiple lines with multiple
values.)  What is this going to look like?

thanks,

greg k-h

Re: [patch 1/3 v2] Add function get_bootparam

[Dave Young]

On 11/11/13 at 11:58pm, Greg KH wrote:
> On Mon, Nov 11, 2013 at 07:20:17PM -0800, H. Peter Anvin wrote:
> > On 11/11/2013 05:27 PM, Toshi Kani wrote:
> > > On Tue, 2013-11-05 at 16:29 +0800, dyoung@redhat.com wrote:
> > >> Not only setup_subarch will get data from debugfs file
> > >> boot_params/data, later code for adding efi_info will
> > >> also need do same thing. Thus add a common function here
> > >> for later use.
> > > 
> > > get_bootparam() calls find_mnt_by_fsname("debugfs"), which assumes that
> > > debugfs is mounted with device name "debugfs".  This function fails
> > > when:
> > >  - debugfs is not mounted, or
> > >  - debugfs is mounted with a different device name, such as nodev [1].
> > > 
> > > This issue is not introduced by this patch.  But the original code,
> > > which sets hardware_subarch, seems to work even if it failed to access
> > > debugfs (which is ignored) since hardware_subarch is zero most of the
> > > cases anyway.  With this change, however, this failure now makes the 2nd
> > > kernel unbootable.  So, it needs to be addressed to make this code path
> > > work reliably (or kexec should fail at least).
> > > 
> > 
> > Greg, Ingo,
> > 
> > Since there is now a legitimate user of this stuff, can we actually
> > export this in sysfs (or something else other than sploit^Wdebugfs)?
> 
> Hey, after Tejun is done with the kernelfs work, I'll be porting debugfs
> to it so you can't call it sploitfs anymore :)

Good news.

> 
> > kexec-tools can have a fallback to debugfs if we really need it, but
> > making people mount debugfs to have some essential piece of
> > functionality scares the heck out of me.
> 
> I agree, that would not be good.
> 
> I'm not sure what exactly the sysfs file is wanting to look like?  The
> efi section variable sysfs file isn't ok (multiple lines with multiple
> values.)  What is this going to look like?

The current structure in debugfs is like below, export every field of setup
header as one file will be too much IMHO:
[root@darkstar debug]# tree boot_params
boot_params
├── data /* binary data of setup header */
├── setup_data /* setup_data link list nodes */
│   ├── 0
│   │   ├── data /* binary data of setup data */
│   │   └── type /* type of setup data */
│   └── 1
│       ├── data
│       └── type
└── version /* boot protocol version */

Thanks
Dave

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

Re: [patch 1/3 v2] Add function get_bootparam

[Greg KH]

On Tue, Nov 12, 2013 at 04:08:54PM +0800, Dave Young wrote:
> On 11/11/13 at 11:58pm, Greg KH wrote:
> > > kexec-tools can have a fallback to debugfs if we really need it, but
> > > making people mount debugfs to have some essential piece of
> > > functionality scares the heck out of me.
> > 
> > I agree, that would not be good.
> > 
> > I'm not sure what exactly the sysfs file is wanting to look like?  The
> > efi section variable sysfs file isn't ok (multiple lines with multiple
> > values.)  What is this going to look like?
> 
> The current structure in debugfs is like below, export every field of setup
> header as one file will be too much IMHO:
> [root@darkstar debug]# tree boot_params
> boot_params
> ├── data /* binary data of setup header */
> ├── setup_data /* setup_data link list nodes */
> │   ├── 0
> │   │   ├── data /* binary data of setup data */
> │   │   └── type /* type of setup data */
> │   └── 1
> │       ├── data
> │       └── type
> └── version /* boot protocol version */

And these binary data blobs are a "standard" somewhere, and will not
change per kernel version change?

If so, that structure is fine with me.

thanks,

greg k-h

Re: [patch 1/3 v2] Add function get_bootparam

[Dave Young]

On 11/12/13 at 12:30am, Greg KH wrote:
> On Tue, Nov 12, 2013 at 04:08:54PM +0800, Dave Young wrote:
> > On 11/11/13 at 11:58pm, Greg KH wrote:
> > > > kexec-tools can have a fallback to debugfs if we really need it, but
> > > > making people mount debugfs to have some essential piece of
> > > > functionality scares the heck out of me.
> > > 
> > > I agree, that would not be good.
> > > 
> > > I'm not sure what exactly the sysfs file is wanting to look like?  The
> > > efi section variable sysfs file isn't ok (multiple lines with multiple
> > > values.)  What is this going to look like?
> > 
> > The current structure in debugfs is like below, export every field of setup
> > header as one file will be too much IMHO:
> > [root@darkstar debug]# tree boot_params
> > boot_params
> > ├── data /* binary data of setup header */
> > ├── setup_data /* setup_data link list nodes */
> > │   ├── 0
> > │   │   ├── data /* binary data of setup data */
> > │   │   └── type /* type of setup data */
> > │   └── 1
> > │       ├── data
> > │       └── type
> > └── version /* boot protocol version */
> 
> And these binary data blobs are a "standard" somewhere, and will not
> change per kernel version change?

IMO the topology should be standard. The content could change,
for example later bumping boot protocol version, we could add
new field in setup header..

hpa should know more about this.. Peter could you answer this question?

> 
> If so, that structure is fine with me.
> 
> thanks,
> 
> greg k-h

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

Re: [patch 1/3 v2] Add function get_bootparam

[H. Peter Anvin]

On 11/12/2013 12:30 AM, Greg KH wrote:
> 
> And these binary data blobs are a "standard" somewhere, and will not
> change per kernel version change?
> 
> If so, that structure is fine with me.
> 

Correct.  The structure is documented in Documentation/x86/boot.txt, and
has been largely invariant (but extended) since the beginning of Linux.
 It has sometimes taken some serious work to keep it that way.

	-hpa

Re: [patch 1/3 v2] Add function get_bootparam

[Greg KH]

On Tue, Nov 12, 2013 at 01:37:25AM -0800, H. Peter Anvin wrote:
> On 11/12/2013 12:30 AM, Greg KH wrote:
> > 
> > And these binary data blobs are a "standard" somewhere, and will not
> > change per kernel version change?
> > 
> > If so, that structure is fine with me.
> > 
> 
> Correct.  The structure is documented in Documentation/x86/boot.txt, and
> has been largely invariant (but extended) since the beginning of Linux.
>  It has sometimes taken some serious work to keep it that way.

Ok, then use the binary sysfs file interface for these blobs, and
everyone will be happy.

Re: [patch 1/3 v2] Add function get_bootparam

[Vivek Goyal]

On Tue, Nov 12, 2013 at 04:58:50PM +1300, Matthew Garrett wrote:
> Well, there's one alternative - don't export this at all and only support UEFI kernels that have an in-kernel loader, then just pass the data internally. 

Hi Matthew,

I am still writing that in-kernel support for kexec/kdump. I guess in
next 2 weeks I might be able to post a basic RFC version. (no EFI support
in this version though).

Having said that, I think that it is good to export these values to
user space and make existing code work with UEFI. New code (if it gets
in at all) will slowly mature over a period of time and transition to it
will be slow. I think initially it will be used only if secureboot is
enabled. And in the mean time we need a good solution for kexec/kdump
with UEFI. 

Thanks
Vivek

Re: [patch 1/3 v2] Add function get_bootparam

[Dave Young]

On 11/12/13 at 06:51pm, Greg KH wrote:
> On Tue, Nov 12, 2013 at 01:37:25AM -0800, H. Peter Anvin wrote:
> > On 11/12/2013 12:30 AM, Greg KH wrote:
> > > 
> > > And these binary data blobs are a "standard" somewhere, and will not
> > > change per kernel version change?
> > > 
> > > If so, that structure is fine with me.
> > > 
> > 
> > Correct.  The structure is documented in Documentation/x86/boot.txt, and
> > has been largely invariant (but extended) since the beginning of Linux.
> >  It has sometimes taken some serious work to keep it that way.
> 
> Ok, then use the binary sysfs file interface for these blobs, and
> everyone will be happy.

Since we got an agreement I will move them to sysfs in next version.

Thanks everyone.

Re: [patch 1/3 v2] Add function get_bootparam

[Dave Young]

On 11/13/13 at 08:50am, Dave Young wrote:
> On 11/12/13 at 06:51pm, Greg KH wrote:
> > On Tue, Nov 12, 2013 at 01:37:25AM -0800, H. Peter Anvin wrote:
> > > On 11/12/2013 12:30 AM, Greg KH wrote:
> > > > 
> > > > And these binary data blobs are a "standard" somewhere, and will not
> > > > change per kernel version change?
> > > > 
> > > > If so, that structure is fine with me.
> > > > 
> > > 
> > > Correct.  The structure is documented in Documentation/x86/boot.txt, and
> > > has been largely invariant (but extended) since the beginning of Linux.
> > >  It has sometimes taken some serious work to keep it that way.
> > 
> > Ok, then use the binary sysfs file interface for these blobs, and
> > everyone will be happy.
> 
> Since we got an agreement I will move them to sysfs in next version.

I have created one patch for the sysfs part this weekend, bug I'm hesitating to
remove the debugfs part because currently there's already user, I tend to
keep them in debugfs for now.

What do you think, Peter?

--
Thanks
Dave

Re: [patch 1/3 v2] Add function get_bootparam

[H. Peter Anvin]

On 11/17/2013 06:22 PM, Dave Young wrote:
> On 11/13/13 at 08:50am, Dave Young wrote:
>> On 11/12/13 at 06:51pm, Greg KH wrote:
>>> On Tue, Nov 12, 2013 at 01:37:25AM -0800, H. Peter Anvin wrote:
>>>> On 11/12/2013 12:30 AM, Greg KH wrote:
>>>>>
>>>>> And these binary data blobs are a "standard" somewhere, and will not
>>>>> change per kernel version change?
>>>>>
>>>>> If so, that structure is fine with me.
>>>>>
>>>>
>>>> Correct.  The structure is documented in Documentation/x86/boot.txt, and
>>>> has been largely invariant (but extended) since the beginning of Linux.
>>>>  It has sometimes taken some serious work to keep it that way.
>>>
>>> Ok, then use the binary sysfs file interface for these blobs, and
>>> everyone will be happy.
>>
>> Since we got an agreement I will move them to sysfs in next version.
> 
> I have created one patch for the sysfs part this weekend, bug I'm hesitating to
> remove the debugfs part because currently there's already user, I tend to
> keep them in debugfs for now.
> 
> What do you think, Peter?
> 

Yes, it makes sense to keep them for compatibility, at least for now.
What is the specific user that already exists?

	-hpa

Re: [patch 1/3 v2] Add function get_bootparam

[Dave Young]

On 11/17/13 at 07:29pm, H. Peter Anvin wrote:
> On 11/17/2013 06:22 PM, Dave Young wrote:
> > On 11/13/13 at 08:50am, Dave Young wrote:
> >> On 11/12/13 at 06:51pm, Greg KH wrote:
> >>> On Tue, Nov 12, 2013 at 01:37:25AM -0800, H. Peter Anvin wrote:
> >>>> On 11/12/2013 12:30 AM, Greg KH wrote:
> >>>>>
> >>>>> And these binary data blobs are a "standard" somewhere, and will not
> >>>>> change per kernel version change?
> >>>>>
> >>>>> If so, that structure is fine with me.
> >>>>>
> >>>>
> >>>> Correct.  The structure is documented in Documentation/x86/boot.txt, and
> >>>> has been largely invariant (but extended) since the beginning of Linux.
> >>>>  It has sometimes taken some serious work to keep it that way.
> >>>
> >>> Ok, then use the binary sysfs file interface for these blobs, and
> >>> everyone will be happy.
> >>
> >> Since we got an agreement I will move them to sysfs in next version.
> > 
> > I have created one patch for the sysfs part this weekend, bug I'm hesitating to
> > remove the debugfs part because currently there's already user, I tend to
> > keep them in debugfs for now.
> > 
> > What do you think, Peter?
> > 
> 
> Yes, it makes sense to keep them for compatibility, at least for now.
> What is the specific user that already exists?

kexec-tools is using it to setup hardware_subarch..

--
Thanks
Dave

Re: [patch 1/3 v2] Add function get_bootparam

[H. Peter Anvin]

In other words, we didn't catch that one in time.  Oh well.

Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>On 11/17/13 at 07:29pm, H. Peter Anvin wrote:
>> On 11/17/2013 06:22 PM, Dave Young wrote:
>> > On 11/13/13 at 08:50am, Dave Young wrote:
>> >> On 11/12/13 at 06:51pm, Greg KH wrote:
>> >>> On Tue, Nov 12, 2013 at 01:37:25AM -0800, H. Peter Anvin wrote:
>> >>>> On 11/12/2013 12:30 AM, Greg KH wrote:
>> >>>>>
>> >>>>> And these binary data blobs are a "standard" somewhere, and
>will not
>> >>>>> change per kernel version change?
>> >>>>>
>> >>>>> If so, that structure is fine with me.
>> >>>>>
>> >>>>
>> >>>> Correct.  The structure is documented in
>Documentation/x86/boot.txt, and
>> >>>> has been largely invariant (but extended) since the beginning of
>Linux.
>> >>>>  It has sometimes taken some serious work to keep it that way.
>> >>>
>> >>> Ok, then use the binary sysfs file interface for these blobs, and
>> >>> everyone will be happy.
>> >>
>> >> Since we got an agreement I will move them to sysfs in next
>version.
>> > 
>> > I have created one patch for the sysfs part this weekend, bug I'm
>hesitating to
>> > remove the debugfs part because currently there's already user, I
>tend to
>> > keep them in debugfs for now.
>> > 
>> > What do you think, Peter?
>> > 
>> 
>> Yes, it makes sense to keep them for compatibility, at least for now.
>> What is the specific user that already exists?
>
>kexec-tools is using it to setup hardware_subarch..
>
>--
>Thanks
>Dave

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