[patch 0/4] kexec-tools: efi runtime support on kexec kernel

[patch 0/4] kexec-tools: efi runtime support on kexec kernel

[dyoung]

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
--
Thanks
Dave

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

[patch 1/4] Add function get_bootparam

[dyoung]

[-- Attachment #1: 01-kexec-add-function-get-boot-param.patch --]
[-- Type: text/plain, Size: 1809 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.

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


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

[patch 2/4] remove extra acpi_rsdp command line for efi

[dyoung]

[-- Attachment #1: 02-kexec-remove-cmdline-add-efi.patch --]
[-- Type: text/plain, Size: 2036 bytes --]

Removing code to pass acpi_rsdp because this
patch series will support efi runtime, it's not
necessary any more. EFI initialization code will
take the functionality.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 include/x86/x86-linux.h           |    3 ++-
 kexec/arch/i386/crashdump-x86.c   |   35 -----------------------------------
 kexec/arch/i386/x86-linux-setup.c |    8 ++++++++
 3 files changed, 10 insertions(+), 36 deletions(-)

--- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
+++ kexec-tools/kexec/arch/i386/crashdump-x86.c
@@ -848,40 +848,6 @@ static int cmdline_add_memmap_acpi(char
 	return 0;
 }
 
-/* Appends 'acpi_rsdp=' commandline for efi boot crash dump */
-static void cmdline_add_efi(char *cmdline)
-{
-	FILE *fp;
-	int cmdlen, len;
-	char line[MAX_LINE], *s;
-	const char *acpis = " acpi_rsdp=";
-
-	fp = fopen("/sys/firmware/efi/systab", "r");
-	if (!fp)
-		return;
-
-	while(fgets(line, sizeof(line), fp) != 0) {
-		/* ACPI20= always goes before ACPI= */
-		if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
-		        line[strlen(line) - 1] = '\0';
-			s = strchr(line, '=');
-			s += 1;
-			len = strlen(s) + strlen(acpis);
-			cmdlen = strlen(cmdline) + len;
-			if (cmdlen > (COMMAND_LINE_SIZE - 1))
-				die("Command line overflow\n");
-			strcat(cmdline, acpis);
-			strcat(cmdline, s);
-			dbgprintf("Command line after adding efi\n");
-			dbgprintf("%s\n", cmdline);
-
-			break;
-		}
-	}
-
-	fclose(fp);
-}
-
 static void get_backup_area(struct kexec_info *info,
 				struct memory_range *range, int ranges)
 {
@@ -1046,7 +1012,6 @@ 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);
 	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
 
 	/* Inform second kernel about the presence of ACPI tables. */


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

[patch 3/4] Add efi_info in x86 setup header

[dyoung]

[-- Attachment #1: 03-kexec-padd-efi_info.patch --]
[-- Type: text/plain, Size: 1624 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.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
--- 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];	/* 0xcc */
+	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));
 }
 
+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,7 @@ void setup_linux_system_parameters(struc
 
 	/* get subarch from running kernel */
 	setup_subarch(real_mode);
+	setup_efi_info(real_mode);
 	
 	/* Default screen size */
 	real_mode->orig_x = 0;


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

[patch 4/4] Passing efi related data via setup_data

[dyoung]

[-- Attachment #1: 05-kexec-add-efi-setup-data.patch --]
[-- Type: text/plain, Size: 5122 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.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 include/x86/x86-linux.h           |    2 
 kexec/arch/i386/x86-linux-setup.c |  120 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 120 insertions(+), 2 deletions(-)

--- kexec-tools.orig/include/x86/x86-linux.h
+++ kexec-tools/include/x86/x86-linux.h
@@ -115,7 +115,7 @@ 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[0x1c0 - 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 */
--- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
+++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
@@ -473,6 +473,124 @@ void setup_efi_info(struct x86_linux_par
 	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;
+        __u32 type;
+        __u32 len;
+        __u8 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);
+	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)
 {
@@ -483,7 +601,7 @@ void setup_linux_system_parameters(struc
 	/* get subarch from running kernel */
 	setup_subarch(real_mode);
 	setup_efi_info(real_mode);
-	
+	setup_efi_setup_data(info, real_mode);
 	/* Default screen size */
 	real_mode->orig_x = 0;
 	real_mode->orig_y = 0;


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

Re: [patch 4/4] Passing efi related data via setup_data

[Simon Horman]

On Sun, Oct 27, 2013 at 12:04:41PM +0800, dyoung@redhat.com wrote:
> 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.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  include/x86/x86-linux.h           |    2 
>  kexec/arch/i386/x86-linux-setup.c |  120 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 120 insertions(+), 2 deletions(-)
> 
> --- kexec-tools.orig/include/x86/x86-linux.h
> +++ kexec-tools/include/x86/x86-linux.h
> @@ -115,7 +115,7 @@ 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[0x1c0 - 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 */
> --- kexec-tools.orig/kexec/arch/i386/x86-linux-setup.c
> +++ kexec-tools/kexec/arch/i386/x86-linux-setup.c
> @@ -473,6 +473,124 @@ void setup_efi_info(struct x86_linux_par
>  	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;
> +        __u32 type;
> +        __u32 len;
> +        __u8 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;

The indentation of the line below is inconsistent with the line above.

> +
> +        has_efi = access("/sys/firmware/efi/systab", F_OK);
> +        if (has_efi < 0)
> +                return;
> +
> +	esd = malloc(sizeof(struct efi_setup_data));
> +	if (!esd)
> +		return;

This function appears to leak esd.

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

This function appears to leak sd.

> +
> +	memset(sd, 0, sizeof(struct setup_data) + size);
> +	sd->next = 0;
> +	sd->type = 4;
> +	sd->len = size;
> +	memcpy(sd->data, esd, size);
> +	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)
>  {
> @@ -483,7 +601,7 @@ void setup_linux_system_parameters(struc
>  	/* get subarch from running kernel */
>  	setup_subarch(real_mode);
>  	setup_efi_info(real_mode);
> -	
> +	setup_efi_setup_data(info, real_mode);
>  	/* Default screen size */
>  	real_mode->orig_x = 0;
>  	real_mode->orig_y = 0;
> 

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

Re: [patch 1/4] Add function get_bootparam

[Simon Horman]

On Sun, Oct 27, 2013 at 12:04:38PM +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.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  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)
> +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);

This change appears to be unrelated to the rest of the patch.

>  
>  	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)
>  {
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Simon Horman]

On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> Removing code to pass acpi_rsdp because this
> patch series will support efi runtime, it's not
> necessary any more. EFI initialization code will
> take the functionality.

I wonder if it would make more sense to make this the
last patch in the series or squash it into the last patch
of the series.

> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  include/x86/x86-linux.h           |    3 ++-
>  kexec/arch/i386/crashdump-x86.c   |   35 -----------------------------------
>  kexec/arch/i386/x86-linux-setup.c |    8 ++++++++
>  3 files changed, 10 insertions(+), 36 deletions(-)
> 
> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
> @@ -848,40 +848,6 @@ static int cmdline_add_memmap_acpi(char
>  	return 0;
>  }
>  
> -/* Appends 'acpi_rsdp=' commandline for efi boot crash dump */
> -static void cmdline_add_efi(char *cmdline)
> -{
> -	FILE *fp;
> -	int cmdlen, len;
> -	char line[MAX_LINE], *s;
> -	const char *acpis = " acpi_rsdp=";
> -
> -	fp = fopen("/sys/firmware/efi/systab", "r");
> -	if (!fp)
> -		return;
> -
> -	while(fgets(line, sizeof(line), fp) != 0) {
> -		/* ACPI20= always goes before ACPI= */
> -		if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
> -		        line[strlen(line) - 1] = '\0';
> -			s = strchr(line, '=');
> -			s += 1;
> -			len = strlen(s) + strlen(acpis);
> -			cmdlen = strlen(cmdline) + len;
> -			if (cmdlen > (COMMAND_LINE_SIZE - 1))
> -				die("Command line overflow\n");
> -			strcat(cmdline, acpis);
> -			strcat(cmdline, s);
> -			dbgprintf("Command line after adding efi\n");
> -			dbgprintf("%s\n", cmdline);
> -
> -			break;
> -		}
> -	}
> -
> -	fclose(fp);
> -}
> -
>  static void get_backup_area(struct kexec_info *info,
>  				struct memory_range *range, int ranges)
>  {
> @@ -1046,7 +1012,6 @@ 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);
>  	cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr);
>  
>  	/* Inform second kernel about the presence of ACPI tables. */
> 

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

Re: [patch 4/4] Passing efi related data via setup_data

[Dave Young]

Hi, Simon

Thanks for review.

> > +	int has_efi = 0;
> 
> The indentation of the line below is inconsistent with the line above.

Will fix

> 
> > +
> > +        has_efi = access("/sys/firmware/efi/systab", F_OK);
> > +        if (has_efi < 0)
> > +                return;
> > +
> > +	esd = malloc(sizeof(struct efi_setup_data));
> > +	if (!esd)
> > +		return;
> 
> This function appears to leak esd.

I left the error handling after testing but later forgot to add them.
Will fix

> 
> > +	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;
> > +	}
> 
> This function appears to leak sd.

Will fix

--
Thanks
Dave

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

Re: [patch 1/4] Add function get_bootparam

[Dave Young]

> > @@ -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);
> 
> This change appears to be unrelated to the rest of the patch.
> 

Will remove the change from the patch.

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

Re: [patch 4/4] Passing efi related data via setup_data

[Dave Young]

On 10/27/13 at 12:04pm, Dave Young wrote:
> 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.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  include/x86/x86-linux.h           |    2 
>  kexec/arch/i386/x86-linux-setup.c |  120 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 120 insertions(+), 2 deletions(-)
> 
> --- kexec-tools.orig/include/x86/x86-linux.h
> +++ kexec-tools/include/x86/x86-linux.h
> @@ -115,7 +115,7 @@ 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[0x1c0 - 0xcc];	/* 0xcc */
> +	uint8_t reserved4_1[0x1c0 - 0xcc];	/* 0xe4 */

Above change should be in previous patch. Will update in next version. 

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/28/13 at 09:39am, Simon Horman wrote:
> On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > Removing code to pass acpi_rsdp because this
> > patch series will support efi runtime, it's not
> > necessary any more. EFI initialization code will
> > take the functionality.
> 
> I wonder if it would make more sense to make this the
> last patch in the series or squash it into the last patch
> of the series.
> 

Sure, logiclly it should be the last one.

--
Thanks
Dave

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

Re: [patch 1/4] Add function get_bootparam

[Dave Young]

On 10/28/13 at 09:13am, Dave Young wrote:
> > > @@ -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);
> > 
> > This change appears to be unrelated to the rest of the patch.
> > 
> 
> Will remove the change from the patch.

Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
is in the share function get_bootparam, it's not in setup_subarch
any more so I think it's ok, what do you think?

--
Thanks
Dave

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

Re: [patch 4/4] Passing efi related data via setup_data

[Dave Young]

On 10/28/13 at 09:12am, Dave Young wrote:
> > > +	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;
> > > +	}
> > 
> > This function appears to leak sd.
> 
> Will fix

sd will be passed to add_buffer, it should be freed after kexec_load,
is it really necessary, or I misunderstand something?

> 
> --
> Thanks
> Dave

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

Re: [patch 4/4] Passing efi related data via setup_data

[Simon Horman]

On Mon, Oct 28, 2013 at 11:02:59AM +0800, Dave Young wrote:
> On 10/28/13 at 09:12am, Dave Young wrote:
> > > > +	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;
> > > > +	}
> > > 
> > > This function appears to leak sd.
> > 
> > Will fix
> 
> sd will be passed to add_buffer, it should be freed after kexec_load,
> is it really necessary, or I misunderstand something?

Thanks and sorry for missing that.
I now think the current handling of sd in this patch seems fine.

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

Re: [patch 1/4] Add function get_bootparam

[Simon Horman]

On Mon, Oct 28, 2013 at 10:30:32AM +0800, Dave Young wrote:
> On 10/28/13 at 09:13am, Dave Young wrote:
> > > > @@ -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);
> > > 
> > > This change appears to be unrelated to the rest of the patch.
> > > 
> > 
> > Will remove the change from the patch.
> 
> Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
> is in the share function get_bootparam, it's not in setup_subarch
> any more so I think it's ok, what do you think?

I'm a bit confused.

My reading of the hunk above is that it is a whitespace-only change.
If so it is not a change that I object to but also not one that
I feel belongs in this patch. If not I am somehow mistaken.

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

Re: [patch 1/4] Add function get_bootparam

[Dave Young]

On 10/28/13 at 01:20pm, Simon Horman wrote:
> On Mon, Oct 28, 2013 at 10:30:32AM +0800, Dave Young wrote:
> > On 10/28/13 at 09:13am, Dave Young wrote:
> > > > > @@ -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);
> > > > 
> > > > This change appears to be unrelated to the rest of the patch.
> > > > 
> > > 
> > > Will remove the change from the patch.
> > 
> > Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
> > is in the share function get_bootparam, it's not in setup_subarch
> > any more so I think it's ok, what do you think?
> 
> I'm a bit confused.
> 
> My reading of the hunk above is that it is a whitespace-only change.
> If so it is not a change that I object to but also not one that
> I feel belongs in this patch. If not I am somehow mistaken.

The diff context is cheating us, it might because of the context (+- 3 lines)
is same with original setup_arch, but it is really moving to a new function.

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

Re: [patch 1/4] Add function get_bootparam

[Simon Horman]

On Mon, Oct 28, 2013 at 01:06:45PM +0800, Dave Young wrote:
> On 10/28/13 at 01:20pm, Simon Horman wrote:
> > On Mon, Oct 28, 2013 at 10:30:32AM +0800, Dave Young wrote:
> > > On 10/28/13 at 09:13am, Dave Young wrote:
> > > > > > @@ -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);
> > > > > 
> > > > > This change appears to be unrelated to the rest of the patch.
> > > > > 
> > > > 
> > > > Will remove the change from the patch.
> > > 
> > > Relooking it, with this patch the line "filename[PATH_MAX - 1] = 0;"
> > > is in the share function get_bootparam, it's not in setup_subarch
> > > any more so I think it's ok, what do you think?
> > 
> > I'm a bit confused.
> > 
> > My reading of the hunk above is that it is a whitespace-only change.
> > If so it is not a change that I object to but also not one that
> > I feel belongs in this patch. If not I am somehow mistaken.
> 
> The diff context is cheating us, it might because of the context (+- 3 lines)
> is same with original setup_arch, but it is really moving to a new function.

Ok, in that case I have no objections.

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Matthew Garrett]

On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> Removing code to pass acpi_rsdp because this
> patch series will support efi runtime, it's not
> necessary any more. EFI initialization code will
> take the functionality.

Won't this break kexec of old kernels that don't have the new UEFI setup 
code?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

Hi,
Thanks for review.

On 10/28/13 at 09:42am, Matthew Garrett wrote:
> On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > Removing code to pass acpi_rsdp because this
> > patch series will support efi runtime, it's not
> > necessary any more. EFI initialization code will
> > take the functionality.
> 
> Won't this break kexec of old kernels that don't have the new UEFI setup 
> code?

For old kernels the efi setup_data esdata will be NULL, it wil switch to
old logic. Also for old kernel user need to pass acpi_rsdp in cmdline

Anyway I will do test with old/new kernel/userspace test results.

--
Thanks
Dave

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Matthew Garrett]

On Mon, Oct 28, 2013 at 05:54:59PM +0800, Dave Young wrote:
> Hi,
> Thanks for review.
> 
> On 10/28/13 at 09:42am, Matthew Garrett wrote:
> > On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > > Removing code to pass acpi_rsdp because this
> > > patch series will support efi runtime, it's not
> > > necessary any more. EFI initialization code will
> > > take the functionality.
> > 
> > Won't this break kexec of old kernels that don't have the new UEFI setup 
> > code?
> 
> For old kernels the efi setup_data esdata will be NULL, it wil switch to
> old logic. Also for old kernel user need to pass acpi_rsdp in cmdline

Right, but previously acpi_rsdp was passed automatically and now it 
won't be?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/28/13 at 10:12am, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 05:54:59PM +0800, Dave Young wrote:
> > Hi,
> > Thanks for review.
> > 
> > On 10/28/13 at 09:42am, Matthew Garrett wrote:
> > > On Sun, Oct 27, 2013 at 12:04:39PM +0800, dyoung@redhat.com wrote:
> > > > Removing code to pass acpi_rsdp because this
> > > > patch series will support efi runtime, it's not
> > > > necessary any more. EFI initialization code will
> > > > take the functionality.
> > > 
> > > Won't this break kexec of old kernels that don't have the new UEFI setup 
> > > code?
> > 
> > For old kernels the efi setup_data esdata will be NULL, it wil switch to
> > old logic. Also for old kernel user need to pass acpi_rsdp in cmdline
> 
> Right, but previously acpi_rsdp was passed automatically and now it 
> won't be?

Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.

Correct myself about esdata, for old kernels there's no setup_data type for
efi so parse_efi_setup will not be called, thus there will be no effect with
passing new setup_data because nobody use it?

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Matthew Garrett]

On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > Right, but previously acpi_rsdp was passed automatically and now it 
> > won't be?
> 
> Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.

If I upgrade kexec-tools and try to launch an old kernel, I now need to 
add an extra parameter?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

Re: [patch 4/4] Passing efi related data via setup_data

[H. Peter Anvin]

On 10/26/2013 09:04 PM, dyoung@redhat.com wrote:

> +        __uint64_t next;

WTF is __uint64_t?

	-hpa


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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/28/13 at 10:39am, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > won't be?
> > 
> > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> 
> If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> add an extra parameter?

Yes, it should work by passing the acpi_rsdp= via --append

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Matthew Garrett]

On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > won't be?
> > > 
> > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > 
> > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > add an extra parameter?
> 
> Yes, it should work by passing the acpi_rsdp= via --append

Yes, that's my point. You're breaking old configurations by requiring 
the user to pass an additional argument.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/28/13 at 10:51am, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > won't be?
> > > > 
> > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > 
> > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > add an extra parameter?
> > 
> > Yes, it should work by passing the acpi_rsdp= via --append
> 
> Yes, that's my point. You're breaking old configurations by requiring 
> the user to pass an additional argument.

Hmm, I will do some test without this patch, if it works well it's not
harmful to drop this one.

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

Re: [patch 4/4] Passing efi related data via setup_data

[Dave Young]

On 10/28/13 at 03:41am, H. Peter Anvin wrote:
> On 10/26/2013 09:04 PM, dyoung@redhat.com wrote:
> 
> > +        __uint64_t next;
> 
> WTF is __uint64_t?

bits/types.h:typedef unsigned long int __uint64_t;

I meant to use uint64_t instead of __unit64_t

Will fix.

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Vivek Goyal]

On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > won't be?
> > > > 
> > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > 
> > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > add an extra parameter?
> > 
> > Yes, it should work by passing the acpi_rsdp= via --append
> 
> Yes, that's my point. You're breaking old configurations by requiring 
> the user to pass an additional argument.

Yes this is a problem. Of course solution is easy by always passing
acpi_rsdp on command line. But in long term this is a problem. In the
sense, I am not sure how to cleanup the kexec-tools code as things improve.
Now we will support the EFI properly and still pass acpi_rsdp always in
an effort to matain backward compatibility.

For a very long time kexec-tools were not automatically appending
acpi_rsdp and user were supposed to add it on command line. We were
carrying this change in kdump scripts and pushed this change into
kexec-tools. In hindsight, it looks like that hardcoding parameters
in kexec-tools is a bad idea. It is hard to get rid of them in future.

Thanks
Vivek

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Simon Horman]

On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > > won't be?
> > > > > 
> > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > 
> > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > > add an extra parameter?
> > > 
> > > Yes, it should work by passing the acpi_rsdp= via --append
> > 
> > Yes, that's my point. You're breaking old configurations by requiring 
> > the user to pass an additional argument.
> 
> Yes this is a problem. Of course solution is easy by always passing
> acpi_rsdp on command line. But in long term this is a problem. In the
> sense, I am not sure how to cleanup the kexec-tools code as things improve.
> Now we will support the EFI properly and still pass acpi_rsdp always in
> an effort to matain backward compatibility.
> 
> For a very long time kexec-tools were not automatically appending
> acpi_rsdp and user were supposed to add it on command line. We were
> carrying this change in kdump scripts and pushed this change into
> kexec-tools. In hindsight, it looks like that hardcoding parameters
> in kexec-tools is a bad idea. It is hard to get rid of them in future.

I tend to agree.  However, if the parameters end up being hard coded
elsewhere, for example in widely used wrapper scripts, then I think that
the same problem still exists. Just outside of kexec-tools itself.

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/29/13 at 11:04am, Simon Horman wrote:
> On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> > On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > > > won't be?
> > > > > > 
> > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > 
> > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > > > add an extra parameter?
> > > > 
> > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > 
> > > Yes, that's my point. You're breaking old configurations by requiring 
> > > the user to pass an additional argument.
> > 
> > Yes this is a problem. Of course solution is easy by always passing
> > acpi_rsdp on command line. But in long term this is a problem. In the
> > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > Now we will support the EFI properly and still pass acpi_rsdp always in
> > an effort to matain backward compatibility.
> > 
> > For a very long time kexec-tools were not automatically appending
> > acpi_rsdp and user were supposed to add it on command line. We were
> > carrying this change in kdump scripts and pushed this change into
> > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > in kexec-tools is a bad idea. It is hard to get rid of them in future.
> 
> I tend to agree.  However, if the parameters end up being hard coded
> elsewhere, for example in widely used wrapper scripts, then I think that
> the same problem still exists. Just outside of kexec-tools itself.

Kernel has a rule of not breaking userspace, does kexec-tools also have the
policy? Maybe user or distribution carry that will be slight better so upstream
kexec code will be cleaner..

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/28/13 at 06:53pm, Dave Young wrote:
> On 10/28/13 at 10:51am, Matthew Garrett wrote:
> > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > > won't be?
> > > > > 
> > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > 
> > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > > add an extra parameter?
> > > 
> > > Yes, it should work by passing the acpi_rsdp= via --append
> > 
> > Yes, that's my point. You're breaking old configurations by requiring 
> > the user to pass an additional argument.
> 
> Hmm, I will do some test without this patch, if it works well it's not
> harmful to drop this one.

Removing this patch does not work because in this series efi_info is passed into
2nd kernel thus 2nd kernel will initilize efi but it does not have related code
to handle the converted talbe addresses and it will endter virtual mode again. 

I will think about how to solve this problem.


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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/29/13 at 10:36am, Dave Young wrote:
> On 10/29/13 at 11:04am, Simon Horman wrote:
> > On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> > > On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > > > > won't be?
> > > > > > > 
> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > > 
> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > > > > add an extra parameter?
> > > > > 
> > > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > > 
> > > > Yes, that's my point. You're breaking old configurations by requiring 
> > > > the user to pass an additional argument.
> > > 
> > > Yes this is a problem. Of course solution is easy by always passing
> > > acpi_rsdp on command line. But in long term this is a problem. In the
> > > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > > Now we will support the EFI properly and still pass acpi_rsdp always in
> > > an effort to matain backward compatibility.
> > > 
> > > For a very long time kexec-tools were not automatically appending
> > > acpi_rsdp and user were supposed to add it on command line. We were
> > > carrying this change in kdump scripts and pushed this change into
> > > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > > in kexec-tools is a bad idea. It is hard to get rid of them in future.
> > 
> > I tend to agree.  However, if the parameters end up being hard coded
> > elsewhere, for example in widely used wrapper scripts, then I think that
> > the same problem still exists. Just outside of kexec-tools itself.
> 
> Kernel has a rule of not breaking userspace, does kexec-tools also have the
> policy? Maybe user or distribution carry that will be slight better so upstream
> kexec code will be cleaner..

Forgot what I said above about the policy, they are different things.

I just replied mjg that even carry this acpi_rsdp, old kernel can not be used
because there's efi info in setup_header.

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/29/13 at 11:05am, Dave Young wrote:
> On 10/28/13 at 06:53pm, Dave Young wrote:
> > On 10/28/13 at 10:51am, Matthew Garrett wrote:
> > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > > > won't be?
> > > > > > 
> > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > 
> > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > > > add an extra parameter?
> > > > 
> > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > 
> > > Yes, that's my point. You're breaking old configurations by requiring 
> > > the user to pass an additional argument.
> > 
> > Hmm, I will do some test without this patch, if it works well it's not
> > harmful to drop this one.
> 
> Removing this patch does not work because in this series efi_info is passed into
> 2nd kernel thus 2nd kernel will initilize efi but it does not have related code
> to handle the converted talbe addresses and it will endter virtual mode again. 
> 
> I will think about how to solve this problem.
> 

The only way what I can think out is introduce a flag in x86 setup header.
Will try this way, the userspace code will use old logic if the kernel is old.

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Dave Young]

On 10/29/13 at 11:34am, Dave Young wrote:
> On 10/29/13 at 11:05am, Dave Young wrote:
> > On 10/28/13 at 06:53pm, Dave Young wrote:
> > > On 10/28/13 at 10:51am, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > > > > won't be?
> > > > > > > 
> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > > 
> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > > > > add an extra parameter?
> > > > > 
> > > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > > 
> > > > Yes, that's my point. You're breaking old configurations by requiring 
> > > > the user to pass an additional argument.
> > > 
> > > Hmm, I will do some test without this patch, if it works well it's not
> > > harmful to drop this one.
> > 
> > Removing this patch does not work because in this series efi_info is passed into
> > 2nd kernel thus 2nd kernel will initilize efi but it does not have related code
> > to handle the converted talbe addresses and it will endter virtual mode again. 
> > 
> > I will think about how to solve this problem.
> > 
> 
> The only way what I can think out is introduce a flag in x86 setup header.
> Will try this way, the userspace code will use old logic if the kernel is old.

Firstly I want to ask opinion from HPA, what do you think about adding a flag to
xloadflags for this purpose?

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Vivek Goyal]

On Tue, Oct 29, 2013 at 11:04:28AM +0900, Simon Horman wrote:

[..]
> > > Yes, that's my point. You're breaking old configurations by requiring 
> > > the user to pass an additional argument.
> > 
> > Yes this is a problem. Of course solution is easy by always passing
> > acpi_rsdp on command line. But in long term this is a problem. In the
> > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > Now we will support the EFI properly and still pass acpi_rsdp always in
> > an effort to matain backward compatibility.
> > 
> > For a very long time kexec-tools were not automatically appending
> > acpi_rsdp and user were supposed to add it on command line. We were
> > carrying this change in kdump scripts and pushed this change into
> > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > in kexec-tools is a bad idea. It is hard to get rid of them in future.
> 
> I tend to agree.  However, if the parameters end up being hard coded
> elsewhere, for example in widely used wrapper scripts, then I think that
> the same problem still exists. Just outside of kexec-tools itself.

I think getting rid of them in vendor scripts is easier beacause control
the range of kexec-tools and kernel version which will be used in a
particular release life cycle. So when we know that we are not going to
support other kernels than vendor shipped kernel, we can remove parameters
from scripts.

Thanks
Vivek

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[H. Peter Anvin]

Seems reasonable to me.

Dave Young <dyoung@redhat.com> wrote:
>On 10/29/13 at 11:34am, Dave Young wrote:
>> On 10/29/13 at 11:05am, Dave Young wrote:
>> > On 10/28/13 at 06:53pm, Dave Young wrote:
>> > > On 10/28/13 at 10:51am, Matthew Garrett wrote:
>> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
>> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
>> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
>> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
>> > > > > > > > Right, but previously acpi_rsdp was passed
>automatically and now it 
>> > > > > > > > won't be?
>> > > > > > > 
>> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for
>efi runtime support.
>> > > > > > 
>> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I
>now need to 
>> > > > > > add an extra parameter?
>> > > > > 
>> > > > > Yes, it should work by passing the acpi_rsdp= via --append
>> > > > 
>> > > > Yes, that's my point. You're breaking old configurations by
>requiring 
>> > > > the user to pass an additional argument.
>> > > 
>> > > Hmm, I will do some test without this patch, if it works well
>it's not
>> > > harmful to drop this one.
>> > 
>> > Removing this patch does not work because in this series efi_info
>is passed into
>> > 2nd kernel thus 2nd kernel will initilize efi but it does not have
>related code
>> > to handle the converted talbe addresses and it will endter virtual
>mode again. 
>> > 
>> > I will think about how to solve this problem.
>> > 
>> 
>> The only way what I can think out is introduce a flag in x86 setup
>header.
>> Will try this way, the userspace code will use old logic if the
>kernel is old.
>
>Firstly I want to ask opinion from HPA, what do you think about adding
>a flag to
>xloadflags for this purpose?

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

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

Re: [patch 2/4] remove extra acpi_rsdp command line for efi

[Simon Horman]

On Tue, Oct 29, 2013 at 10:36:36AM +0800, Dave Young wrote:
> On 10/29/13 at 11:04am, Simon Horman wrote:
> > On Mon, Oct 28, 2013 at 12:10:40PM -0400, Vivek Goyal wrote:
> > > On Mon, Oct 28, 2013 at 10:51:19AM +0000, Matthew Garrett wrote:
> > > > On Mon, Oct 28, 2013 at 06:45:32PM +0800, Dave Young wrote:
> > > > > On 10/28/13 at 10:39am, Matthew Garrett wrote:
> > > > > > On Mon, Oct 28, 2013 at 06:34:12PM +0800, Dave Young wrote:
> > > > > > > On 10/28/13 at 10:12am, Matthew Garrett wrote:
> > > > > > > > Right, but previously acpi_rsdp was passed automatically and now it 
> > > > > > > > won't be?
> > > > > > > 
> > > > > > > Yes, it was. I'm removing them in kexec-tools patches for efi runtime support.
> > > > > > 
> > > > > > If I upgrade kexec-tools and try to launch an old kernel, I now need to 
> > > > > > add an extra parameter?
> > > > > 
> > > > > Yes, it should work by passing the acpi_rsdp= via --append
> > > > 
> > > > Yes, that's my point. You're breaking old configurations by requiring 
> > > > the user to pass an additional argument.
> > > 
> > > Yes this is a problem. Of course solution is easy by always passing
> > > acpi_rsdp on command line. But in long term this is a problem. In the
> > > sense, I am not sure how to cleanup the kexec-tools code as things improve.
> > > Now we will support the EFI properly and still pass acpi_rsdp always in
> > > an effort to matain backward compatibility.
> > > 
> > > For a very long time kexec-tools were not automatically appending
> > > acpi_rsdp and user were supposed to add it on command line. We were
> > > carrying this change in kdump scripts and pushed this change into
> > > kexec-tools. In hindsight, it looks like that hardcoding parameters
> > > in kexec-tools is a bad idea. It is hard to get rid of them in future.
> > 
> > I tend to agree.  However, if the parameters end up being hard coded
> > elsewhere, for example in widely used wrapper scripts, then I think that
> > the same problem still exists. Just outside of kexec-tools itself.
> 
> Kernel has a rule of not breaking userspace, does kexec-tools also have
> the policy? Maybe user or distribution carry that will be slight better
> so upstream kexec code will be cleaner..

I'm not sure that we have had to make a policy decision with regards
to this. At least not recently. But yes, I think it should be a goal
of kexec-tools not to break its users.

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