[PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

In case efi booting, kdump need kernel parameter noefi and acpi_rsdp=
to disable efi reinit and retrieve the acpi root table physical address.

Add a function cmdline_add_efi to get the address from /sys/firmware/efi/systab
If there's no such file or read fail the function will just do nothing.

Tested efi boot Fedora 17 on thinkpad T420.

Signed-off-by: Dave Young <dyoung@redhat.com>
---
 kexec/arch/i386/crashdump-x86.c |   47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

--- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
+++ kexec-tools/kexec/arch/i386/crashdump-x86.c
@@ -27,6 +27,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
+#include <fcntl.h>
 #include "../../kexec.h"
 #include "../../kexec-elf.h"
 #include "../../kexec-syscall.h"
@@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char 
 	return 0;
 }
 
+/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */
+static void cmdline_add_efi(char *cmdline)
+{
+	int cmdlen, len, fd, ret, i = 0;
+	char str_efi[64], str_tmp[32];
+
+	memset(str_efi, 0x0, sizeof(str_efi));
+	memset(str_tmp, 0x0, sizeof(str_tmp));
+	snprintf(str_efi, 18, "%s", " noefi acpi_rsdp=");
+
+	fd = open("/sys/firmware/efi/systab", O_RDONLY);
+	if (fd < 0)
+		return;
+
+	while(1) {
+		ret = read(fd, &str_tmp[i], 1);
+		if (ret <= 0)
+			goto out_close;
+		if (str_tmp[i] == '\n') {
+			str_tmp[i] = '\0';
+			break;
+		}
+		i++;
+	}
+
+	if (!(strncmp(str_tmp, "ACPI20=", 7)))
+		strcat(str_efi, &str_tmp[7]);
+	else if (!(strncmp(str_tmp, "ACPI=", 5)))
+		strcat(str_efi, &str_tmp[5]);
+	else
+		goto out_close;
+
+	len = strlen(str_efi);
+	cmdlen = strlen(cmdline) + len;
+	if (cmdlen > (COMMAND_LINE_SIZE - 1))
+		die("Command line overflow\n");
+	strcat(cmdline, str_efi);
+
+	dbgprintf("Command line after adding efi\n");
+	dbgprintf("%s\n", cmdline);
+
+out_close:
+	close(fd);
+}
+
 static void get_backup_area(unsigned long *start, unsigned long *end)
 {
 	const char *iomem = proc_iomem();
@@ -830,6 +876,7 @@ 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]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Simon Horman]

On Wed, Sep 05, 2012 at 01:44:45PM +0800, Dave Young wrote:
> 
> In case efi booting, kdump need kernel parameter noefi and acpi_rsdp=
> to disable efi reinit and retrieve the acpi root table physical address.
> 
> Add a function cmdline_add_efi to get the address from /sys/firmware/efi/systab
> If there's no such file or read fail the function will just do nothing.
> 
> Tested efi boot Fedora 17 on thinkpad T420.

Hi Dave,

conceptually I am fine with this patch.
However, I do have a few nits in relation to the string handling
as explained below.

> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  kexec/arch/i386/crashdump-x86.c |   47 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
> @@ -27,6 +27,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
> +#include <fcntl.h>
>  #include "../../kexec.h"
>  #include "../../kexec-elf.h"
>  #include "../../kexec-syscall.h"
> @@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char 
>  	return 0;
>  }
>  
> +/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */
> +static void cmdline_add_efi(char *cmdline)
> +{
> +	int cmdlen, len, fd, ret, i = 0;
> +	char str_efi[64], str_tmp[32];
> +
> +	memset(str_efi, 0x0, sizeof(str_efi));
> +	memset(str_tmp, 0x0, sizeof(str_tmp));
> +	snprintf(str_efi, 18, "%s", " noefi acpi_rsdp=");

strncpy() might be be easier on the eyes.

> +
> +	fd = open("/sys/firmware/efi/systab", O_RDONLY);
> +	if (fd < 0)
> +		return;
> +
> +	while(1) {
> +		ret = read(fd, &str_tmp[i], 1);
> +		if (ret <= 0)
> +			goto out_close;
> +		if (str_tmp[i] == '\n') {
> +			str_tmp[i] = '\0';
> +			break;
> +		}
> +		i++;
> +	}

It seems to me that the above code may overflow str_tmp.

Also, I wonder if read(fd, str_tmp, sizeof str_tmp) might make
things a bit easier over all.

Finally, I have a personal preference for "a + i" over "&a[i]".
Though I'm not going to get excited about it.

> +
> +	if (!(strncmp(str_tmp, "ACPI20=", 7)))
> +		strcat(str_efi, &str_tmp[7]);
> +	else if (!(strncmp(str_tmp, "ACPI=", 5)))
> +		strcat(str_efi, &str_tmp[5]);
> +	else
> +		goto out_close;

I wonder if strncat() would be better above, though I don't think
an overflow can actually occur if str_tmp is null-terminated.

> +
> +	len = strlen(str_efi);
> +	cmdlen = strlen(cmdline) + len;
> +	if (cmdlen > (COMMAND_LINE_SIZE - 1))
> +		die("Command line overflow\n");
> +	strcat(cmdline, str_efi);
> +
> +	dbgprintf("Command line after adding efi\n");
> +	dbgprintf("%s\n", cmdline);
> +
> +out_close:
> +	close(fd);
> +}
> +
>  static void get_backup_area(unsigned long *start, unsigned long *end)
>  {
>  	const char *iomem = proc_iomem();
> @@ -830,6 +876,7 @@ 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]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Vivek Goyal]

On Wed, Sep 05, 2012 at 01:44:45PM +0800, Dave Young wrote:
> 
> In case efi booting, kdump need kernel parameter noefi and acpi_rsdp=
> to disable efi reinit and retrieve the acpi root table physical address.
> 
> Add a function cmdline_add_efi to get the address from /sys/firmware/efi/systab
> If there's no such file or read fail the function will just do nothing.
> 
> Tested efi boot Fedora 17 on thinkpad T420.
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  kexec/arch/i386/crashdump-x86.c |   47 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
> @@ -27,6 +27,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
> +#include <fcntl.h>
>  #include "../../kexec.h"
>  #include "../../kexec-elf.h"
>  #include "../../kexec-syscall.h"
> @@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char 
>  	return 0;
>  }
>  
> +/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */
> +static void cmdline_add_efi(char *cmdline)
> +{

Hi Dave,

It might be better to break it down in two functions. One for each
command line parameter.

Also, what's the corelation of efi and acpi_rsdp. Is acpi_rsdp required
only on EFI boot or in general it is required? If it is required in 
general, why kdump works currently.

Thanks
Vivek

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Khalid Aziz]

On 09/04/2012 11:44 PM, Dave Young wrote:
> In case efi booting, kdump need kernel parameter noefi and acpi_rsdp=
> to disable efi reinit and retrieve the acpi root table physical address.
>
> Add a function cmdline_add_efi to get the address from /sys/firmware/efi/systab
> If there's no such file or read fail the function will just do nothing.
>
> Tested efi boot Fedora 17 on thinkpad T420.
>
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>   kexec/arch/i386/crashdump-x86.c |   47 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
>
> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
> @@ -27,6 +27,7 @@
>   #include <sys/types.h>
>   #include <sys/stat.h>
>   #include <unistd.h>
> +#include <fcntl.h>
>   #include "../../kexec.h"
>   #include "../../kexec-elf.h"
>   #include "../../kexec-syscall.h"
> @@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char
>   	return 0;
>   }
>   
> +/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */
> +static void cmdline_add_efi(char *cmdline)
> +{
> +	int cmdlen, len, fd, ret, i = 0;
> +	char str_efi[64], str_tmp[32];
> +
> +	memset(str_efi, 0x0, sizeof(str_efi));
> +	memset(str_tmp, 0x0, sizeof(str_tmp));
> +	snprintf(str_efi, 18, "%s", " noefi acpi_rsdp=");
> +
> +	fd = open("/sys/firmware/efi/systab", O_RDONLY);
> +	if (fd < 0)
> +		return;
> +
> +	while(1) {
> +		ret = read(fd, &str_tmp[i], 1);
> +		if (ret <= 0)
> +			goto out_close;
> +		if (str_tmp[i] == '\n') {
> +			str_tmp[i] = '\0';
> +			break;
> +		}
> +		i++;
> +	}
> +
> +	if (!(strncmp(str_tmp, "ACPI20=", 7)))
> +		strcat(str_efi, &str_tmp[7]);
> +	else if (!(strncmp(str_tmp, "ACPI=", 5)))
> +		strcat(str_efi, &str_tmp[5]);
> +	else
> +		goto out_close;

This will evaluate only the first line in systab. I do not believe you 
are guaranteed to see ACPI20= or ACPI= in the first line. Here is the 
systab file from my machine:

MPS=0xf4cf0
ACPI20=0xcac3e000
ACPI=0xcac3e000
SMBIOS=0xcac4cf98

We need to parse all the lines in systab.
> +
> +	len = strlen(str_efi);
> +	cmdlen = strlen(cmdline) + len;
> +	if (cmdlen > (COMMAND_LINE_SIZE - 1))
> +		die("Command line overflow\n");
> +	strcat(cmdline, str_efi);
> +
> +	dbgprintf("Command line after adding efi\n");
> +	dbgprintf("%s\n", cmdline);
> +
> +out_close:
> +	close(fd);
> +}
> +
>   static void get_backup_area(unsigned long *start, unsigned long *end)
>   {
>   	const char *iomem = proc_iomem();
> @@ -830,6 +876,7 @@ 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

I think running a kexec/kdump kernel with "noefi" is not a good idea. 
Today kernel makes very little use of UEFI run time services but this 
might change shortly. I have already seen ideas being proposed to use 
UEFI variables to store kernel panic information which would require 
accessing UEFI runtime services. If the kdump kernel runs into a panic, 
it would be good to be able to use UEFI variables to store some sort of 
tombstone. We might also start using UEFI runtime clock services one of 
these days especially now that all PCs soon will have UEFI due to 
Windows 8 requirements. There might be other ways to deal with EFI 
virtualization issue. I have solved it on a custom ia64 kernel by 
passing the kexec'd kernel a "kexec_reboot" on command line, although I 
wouldn't recommend that approach as a general approach. Creating a new 
kernel command line parameter just to tell the kernel to not virtualize 
EFI sounds excessive. May be we can come up with a different way to tell 
a kexec/kdump kernel to not virtualize EFI, like create a new signature, 
for example "EL32_KEXEC" and "EL64_KEXEC", for 
boot_params.efi_info.efi_loader_signature which tells the kexec/kdump 
kernel to enable EFI but skip the step of virtualizing it. More work 
will be needed to make this work, for example pass the EFI runtime 
service memory map from one kernel to the next so we keep it mapped in 
exact same spot, and other similar mapping issues.

-- 
Khalid

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

On 09/05/2012 03:45 PM, Simon Horman wrote:

> On Wed, Sep 05, 2012 at 01:44:45PM +0800, Dave Young wrote:
>>
>> In case efi booting, kdump need kernel parameter noefi and acpi_rsdp=
>> to disable efi reinit and retrieve the acpi root table physical address.
>>
>> Add a function cmdline_add_efi to get the address from /sys/firmware/efi/systab
>> If there's no such file or read fail the function will just do nothing.
>>
>> Tested efi boot Fedora 17 on thinkpad T420.
> 
> Hi Dave,
> 
> conceptually I am fine with this patch.
> However, I do have a few nits in relation to the string handling
> as explained below.


Thanks for review.

> 
>>
>> Signed-off-by: Dave Young <dyoung@redhat.com>
>> ---
>>  kexec/arch/i386/crashdump-x86.c |   47 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>
>> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
>> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
>> @@ -27,6 +27,7 @@
>>  #include <sys/types.h>
>>  #include <sys/stat.h>
>>  #include <unistd.h>
>> +#include <fcntl.h>
>>  #include "../../kexec.h"
>>  #include "../../kexec-elf.h"
>>  #include "../../kexec-syscall.h"
>> @@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char 
>>  	return 0;
>>  }
>>  
>> +/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */
>> +static void cmdline_add_efi(char *cmdline)
>> +{
>> +	int cmdlen, len, fd, ret, i = 0;
>> +	char str_efi[64], str_tmp[32];
>> +
>> +	memset(str_efi, 0x0, sizeof(str_efi));
>> +	memset(str_tmp, 0x0, sizeof(str_tmp));
>> +	snprintf(str_efi, 18, "%s", " noefi acpi_rsdp=");
> 
> strncpy() might be be easier on the eyes.


Will switch to use strncpy

> 
>> +
>> +	fd = open("/sys/firmware/efi/systab", O_RDONLY);
>> +	if (fd < 0)
>> +		return;
>> +
>> +	while(1) {
>> +		ret = read(fd, &str_tmp[i], 1);
>> +		if (ret <= 0)
>> +			goto out_close;
>> +		if (str_tmp[i] == '\n') {
>> +			str_tmp[i] = '\0';
>> +			break;
>> +		}
>> +		i++;
>> +	}
> 
> It seems to me that the above code may overflow str_tmp.


Oops, will fix

> 
> Also, I wonder if read(fd, str_tmp, sizeof str_tmp) might make
> things a bit easier over all.


This is fine to me as well. Just also need to end with '\n'

As Khalid said in another reply, the ACPI20= might not be the first
line. will resolve that issue as well.

> 
> Finally, I have a personal preference for "a + i" over "&a[i]".
> Though I'm not going to get excited about it.


Both are fine to me, will convert.

> 
>> +
>> +	if (!(strncmp(str_tmp, "ACPI20=", 7)))
>> +		strcat(str_efi, &str_tmp[7]);
>> +	else if (!(strncmp(str_tmp, "ACPI=", 5)))
>> +		strcat(str_efi, &str_tmp[5]);
>> +	else
>> +		goto out_close;
> 
> I wonder if strncat() would be better above, though I don't think
> an overflow can actually occur if str_tmp is null-terminated.


Will do

> 
>> +
>> +	len = strlen(str_efi);
>> +	cmdlen = strlen(cmdline) + len;
>> +	if (cmdlen > (COMMAND_LINE_SIZE - 1))
>> +		die("Command line overflow\n");
>> +	strcat(cmdline, str_efi);
>> +
>> +	dbgprintf("Command line after adding efi\n");
>> +	dbgprintf("%s\n", cmdline);
>> +
>> +out_close:
>> +	close(fd);
>> +}
>> +
>>  static void get_backup_area(unsigned long *start, unsigned long *end)
>>  {
>>  	const char *iomem = proc_iomem();
>> @@ -830,6 +876,7 @@ 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



-- 
Thanks
Dave

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

On 09/05/2012 09:18 PM, Vivek Goyal wrote:

> On Wed, Sep 05, 2012 at 01:44:45PM +0800, Dave Young wrote:
>>
>> In case efi booting, kdump need kernel parameter noefi and acpi_rsdp=
>> to disable efi reinit and retrieve the acpi root table physical address.
>>
>> Add a function cmdline_add_efi to get the address from /sys/firmware/efi/systab
>> If there's no such file or read fail the function will just do nothing.
>>
>> Tested efi boot Fedora 17 on thinkpad T420.
>>
>> Signed-off-by: Dave Young <dyoung@redhat.com>
>> ---
>>  kexec/arch/i386/crashdump-x86.c |   47 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>
>> --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c
>> +++ kexec-tools/kexec/arch/i386/crashdump-x86.c
>> @@ -27,6 +27,7 @@
>>  #include <sys/types.h>
>>  #include <sys/stat.h>
>>  #include <unistd.h>
>> +#include <fcntl.h>
>>  #include "../../kexec.h"
>>  #include "../../kexec-elf.h"
>>  #include "../../kexec-syscall.h"
>> @@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char 
>>  	return 0;
>>  }
>>  
>> +/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */
>> +static void cmdline_add_efi(char *cmdline)
>> +{
> 
> Hi Dave,
> 
> It might be better to break it down in two functions. One for each
> command line parameter.


Ok, will do

> 
> Also, what's the corelation of efi and acpi_rsdp. Is acpi_rsdp required
> only on EFI boot or in general it is required? If it is required in 
> general, why kdump works currently.


Hi, here is the background of this issue:
http://lists.infradead.org/pipermail/kexec/2010-March/003889.html

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



-- 
Thanks
Dave

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

On 09/06/2012 07:09 AM, Khalid Aziz wrote:

> This will evaluate only the first line in systab. I do not believe you
> are guaranteed to see ACPI20= or ACPI= in the first line. Here is the
> systab file from my machine:
> 
> MPS=0xf4cf0
> ACPI20=0xcac3e000
> ACPI=0xcac3e000
> SMBIOS=0xcac4cf98
> 
> We need to parse all the lines in systab.


Thanks for the reporting, will address this in the update patch

> I think running a kexec/kdump kernel with "noefi" is not a good idea.
> Today kernel makes very little use of UEFI run time services but this
> might change shortly. I have already seen ideas being proposed to use
> UEFI variables to store kernel panic information which would require
> accessing UEFI runtime services. If the kdump kernel runs into a panic,
> it would be good to be able to use UEFI variables to store some sort of
> tombstone. We might also start using UEFI runtime clock services one of
> these days especially now that all PCs soon will have UEFI due to
> Windows 8 requirements. There might be other ways to deal with EFI
> virtualization issue. I have solved it on a custom ia64 kernel by
> passing the kexec'd kernel a "kexec_reboot" on command line, although I
> wouldn't recommend that approach as a general approach. Creating a new
> kernel command line parameter just to tell the kernel to not virtualize
> EFI sounds excessive. May be we can come up with a different way to tell
> a kexec/kdump kernel to not virtualize EFI, like create a new signature,
> for example "EL32_KEXEC" and "EL64_KEXEC", for
> boot_params.efi_info.efi_loader_signature which tells the kexec/kdump
> kernel to enable EFI but skip the step of virtualizing it. More work
> will be needed to make this work, for example pass the EFI runtime
> service memory map from one kernel to the next so we keep it mapped in
> exact same spot, and other similar mapping issues.
> 


I'm not so familiar with uefi detail, is the virtualization issue mean
"virtual mode" of efi?  From my understanding for kdump kernel I would
vote for not interacting with bios at all. We have use "noefi" for long
time by passing it to 2nd kernel while kexecing. I prefer to do this way
until we have to switch to other approach.

-- 
Thanks
Dave

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Khalid Aziz]

On 09/05/2012 09:18 PM, Dave Young wrote:
>
>> I think running a kexec/kdump kernel with "noefi" is not a good idea.
>> Today kernel makes very little use of UEFI run time services but this
>> might change shortly. I have already seen ideas being proposed to use
>> UEFI variables to store kernel panic information which would require
>> accessing UEFI runtime services. If the kdump kernel runs into a panic,
>> it would be good to be able to use UEFI variables to store some sort of
>> tombstone. We might also start using UEFI runtime clock services one of
>> these days especially now that all PCs soon will have UEFI due to
>> Windows 8 requirements. There might be other ways to deal with EFI
>> virtualization issue. I have solved it on a custom ia64 kernel by
>> passing the kexec'd kernel a "kexec_reboot" on command line, although I
>> wouldn't recommend that approach as a general approach. Creating a new
>> kernel command line parameter just to tell the kernel to not virtualize
>> EFI sounds excessive. May be we can come up with a different way to tell
>> a kexec/kdump kernel to not virtualize EFI, like create a new signature,
>> for example "EL32_KEXEC" and "EL64_KEXEC", for
>> boot_params.efi_info.efi_loader_signature which tells the kexec/kdump
>> kernel to enable EFI but skip the step of virtualizing it. More work
>> will be needed to make this work, for example pass the EFI runtime
>> service memory map from one kernel to the next so we keep it mapped in
>> exact same spot, and other similar mapping issues.
>>
>
> I'm not so familiar with uefi detail, is the virtualization issue mean
> "virtual mode" of efi?  From my understanding for kdump kernel I would
> vote for not interacting with bios at all. We have use "noefi" for long
> time by passing it to 2nd kernel while kexecing. I prefer to do this way
> until we have to switch to other approach.
>
Yes, that is what I mean by virtualizing issue. Using "noefi" for kdump 
kernel will keep it from using any runtime EFI services. That could mean 
kdump kernel may not be able to read clock or use EFI variables to store 
a tombstone in case it runs into trouble. Is that acceptable?

For the kexec'd kernel (not kdump kernel), being able to use runtime EFI 
services will be essential as we move to EFI machines. We will have to 
support enabling EFI on a kexec'd kernel. Does that sound reasonable?

-- 
Khalid

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Vivek Goyal]

On Wed, Sep 05, 2012 at 05:09:29PM -0600, Khalid Aziz wrote:
[..]
> More work will be needed to make this work, for example pass the
> EFI runtime service memory map from one kernel to the next so we
> keep it mapped in exact same spot, and other similar mapping issues.

Hi Khalid,

This sounds like significant amount of work. So till this work really
gets merged, noefi is not a bad option. Atleast it will allow us
to capture dump. And once a generic fix is in, one can remove it and
build more fancy features which make use of EFI services in second
kernel?

Thanks
Vivek

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Vivek Goyal]

On Thu, Sep 06, 2012 at 10:58:23AM +0800, Dave Young wrote:

[..]
> > Also, what's the corelation of efi and acpi_rsdp. Is acpi_rsdp required
> > only on EFI boot or in general it is required? If it is required in 
> > general, why kdump works currently.
> 
> 
> Hi, here is the background of this issue:
> http://lists.infradead.org/pipermail/kexec/2010-March/003889.html

Ok, looks like this is EFI specific. So keeping noefi and acpi_rsdp
together makes sense.

Thanks
Vivek

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

On 09/07/2012 01:57 AM, Khalid Aziz wrote:

> On 09/05/2012 09:18 PM, Dave Young wrote:
>>
>>> I think running a kexec/kdump kernel with "noefi" is not a good idea.
>>> Today kernel makes very little use of UEFI run time services but this
>>> might change shortly. I have already seen ideas being proposed to use
>>> UEFI variables to store kernel panic information which would require
>>> accessing UEFI runtime services. If the kdump kernel runs into a panic,
>>> it would be good to be able to use UEFI variables to store some sort of
>>> tombstone. We might also start using UEFI runtime clock services one of
>>> these days especially now that all PCs soon will have UEFI due to
>>> Windows 8 requirements. There might be other ways to deal with EFI
>>> virtualization issue. I have solved it on a custom ia64 kernel by
>>> passing the kexec'd kernel a "kexec_reboot" on command line, although I
>>> wouldn't recommend that approach as a general approach. Creating a new
>>> kernel command line parameter just to tell the kernel to not virtualize
>>> EFI sounds excessive. May be we can come up with a different way to tell
>>> a kexec/kdump kernel to not virtualize EFI, like create a new signature,
>>> for example "EL32_KEXEC" and "EL64_KEXEC", for
>>> boot_params.efi_info.efi_loader_signature which tells the kexec/kdump
>>> kernel to enable EFI but skip the step of virtualizing it. More work
>>> will be needed to make this work, for example pass the EFI runtime
>>> service memory map from one kernel to the next so we keep it mapped in
>>> exact same spot, and other similar mapping issues.
>>>
>>
>> I'm not so familiar with uefi detail, is the virtualization issue mean
>> "virtual mode" of efi?  From my understanding for kdump kernel I would
>> vote for not interacting with bios at all. We have use "noefi" for long
>> time by passing it to 2nd kernel while kexecing. I prefer to do this way
>> until we have to switch to other approach.
>>
> Yes, that is what I mean by virtualizing issue. Using "noefi" for kdump
> kernel will keep it from using any runtime EFI services. That could mean
> kdump kernel may not be able to read clock or use EFI variables to store
> a tombstone in case it runs into trouble. Is that acceptable?
> 
> For the kexec'd kernel (not kdump kernel), being able to use runtime EFI
> services will be essential as we move to EFI machines. We will have to
> support enabling EFI on a kexec'd kernel. Does that sound reasonable?
> 


It's probably needed, but currently relevant implementation  is not
ready especially efi init code for kdump. We can address this once it's ok.

I did some digging about the efi boot kdump in slackware with kernel
from latest linus tree. I'm surprise that kdump works even without these
"noefi" and "acpi_rsdp="

With fedora 17 3.4.3 kernel passing "acpi_rsdp=" is enough for kdump
kernel booting.

Actually in x86 setup code if bootloader do not pass correct efi_info in
boot_params efi_enabled will be set to 0 automaticlly.

So as for this patch 'noefi' is not needed.

I will do more testing to see why linus tree works without "acpi_rsdp="


-- 
Thanks
Dave

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Khalid Aziz]

On Thu, 2012-09-06 at 16:24 -0400, Vivek Goyal wrote:
> On Wed, Sep 05, 2012 at 05:09:29PM -0600, Khalid Aziz wrote:
> [..]
> > More work will be needed to make this work, for example pass the
> > EFI runtime service memory map from one kernel to the next so we
> > keep it mapped in exact same spot, and other similar mapping issues.
> 
> Hi Khalid,
> 
> This sounds like significant amount of work. So till this work really
> gets merged, noefi is not a bad option. Atleast it will allow us
> to capture dump. And once a generic fix is in, one can remove it and
> build more fancy features which make use of EFI services in second
> kernel?

Hi Vivek,

You are right, it indeed is significant amount of work. I agree noefi is
a reasonable option until EFI is fully working with kexec. As long as
noefi is not seen as long term solution, it makes perfect sense.

Thanks
-- 
Khalid Aziz <khalid.aziz@hp.com>


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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Khalid Aziz]

On Fri, 2012-09-07 at 16:56 +0800, Dave Young wrote:
> Actually in x86 setup code if bootloader do not pass correct efi_info in
> boot_params efi_enabled will be set to 0 automaticlly.
> 

When we kexec a kernel, the EFI signature is missing in the bootloader
signature in boot_params, so kexec'd kernels currently disable EFI
automatically even without noefi in this code:

arch/x86/kernel/setup.c:

733         if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
734                      "EL32", 4)) {
735                 efi_enabled = 1;
736                 efi_64bit = false;
737         } else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signatu     re,             
738                      "EL64", 4)) {
739                 efi_enabled = 1;
740                 efi_64bit = true;
741         }

efi_enabled is initialized to 0.

-- 
Khalid Aziz <khalid.aziz@hp.com>


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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

On 09/07/2012 04:56 PM, Dave Young wrote:
> 
> I did some digging about the efi boot kdump in slackware with kernel
> from latest linus tree. I'm surprise that kdump works even without these
> "noefi" and "acpi_rsdp="
> 
> With fedora 17 3.4.3 kernel passing "acpi_rsdp=" is enough for kdump
> kernel booting.
> 
> Actually in x86 setup code if bootloader do not pass correct efi_info in
> boot_params efi_enabled will be set to 0 automaticlly.
> 
> So as for this patch 'noefi' is not needed.
> 
> I will do more testing to see why linus tree works without "acpi_rsdp="
> 

There's probably some efi changes between 3.4.3 - 3.6.0-rc4 cause this
works. I have not got time to bisect it.

Because it works without explicitly passing the kernel param, I will not
update the patch anymore.

Thanks everyone for your comments.


-- 
Thanks
Dave

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Vivek Goyal]

On Fri, Sep 07, 2012 at 08:37:14AM -0600, Khalid Aziz wrote:
> On Fri, 2012-09-07 at 16:56 +0800, Dave Young wrote:
> > Actually in x86 setup code if bootloader do not pass correct efi_info in
> > boot_params efi_enabled will be set to 0 automaticlly.
> > 
> 
> When we kexec a kernel, the EFI signature is missing in the bootloader
> signature in boot_params, so kexec'd kernels currently disable EFI
> automatically even without noefi in this code:
> 
> arch/x86/kernel/setup.c:
> 
> 733         if (!strncmp((char *)&boot_params.efi_info.efi_loader_signature,
> 734                      "EL32", 4)) {
> 735                 efi_enabled = 1;
> 736                 efi_64bit = false;
> 737         } else if (!strncmp((char *)&boot_params.efi_info.efi_loader_signatu     re,             
> 738                      "EL64", 4)) {
> 739                 efi_enabled = 1;
> 740                 efi_64bit = true;
> 741         }
> 
> efi_enabled is initialized to 0.

Hi Khalid,

So what does this mean? Second kernel assumes the regular BIOS and tries
to initialize that way? That sounds broken or is it just fine given
the fact we are anyway not planning to call into any of the efi services.
But what about memory maps, ACPI tables etc.

Thanks
Vivek

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Khalid Aziz]

On 09/13/2012 07:38 AM, Vivek Goyal wrote:
> So what does this mean? Second kernel assumes the regular BIOS and tries
> to initialize that way? That sounds broken or is it just fine given
> the fact we are anyway not planning to call into any of the efi services.
> But what about memory maps, ACPI tables etc.

Hi Vivek,

My understanding on x86 side is not as clear as on ia64 side of kexec. 
 From my reading of the code, second kernel seems to assume a regular 
BIOS. It searches through EBDA or upper memory region E0000h-FFFFFh to 
find RSDP, if an RSDP pointer was not passed to the second kernel. This 
works on my EFI machine which also implements a BIOS compatibility 
layer. I wonder how it works on pure EFI machines. Do pure EFI machines 
not implement, say EBDA? If yes, acpi_find_root_pointer() should fail 
tofind RSDP.As for memory map, all I can find are reference to e820 
memory map which again assumes a BIOS but most EFI implementations I 
have come across implement a BIOS compatibility layer, so this will work 
still. I will look some more into this.

--
Khalid

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Eric W. Biederman]

Khalid Aziz <khalid.aziz@hp.com> writes:

> On 09/13/2012 07:38 AM, Vivek Goyal wrote:
>> So what does this mean? Second kernel assumes the regular BIOS and tries
>> to initialize that way? That sounds broken or is it just fine given
>> the fact we are anyway not planning to call into any of the efi services.
>> But what about memory maps, ACPI tables etc.
>
> Hi Vivek,
>
> My understanding on x86 side is not as clear as on ia64 side of kexec. From my
> reading of the code, second kernel seems to assume a regular BIOS. It searches
> through EBDA or upper memory region E0000h-FFFFFh to find RSDP, if an RSDP
> pointer was not passed to the second kernel. This works on my EFI machine which
> also implements a BIOS compatibility layer. I wonder how it works on pure EFI
> machines. Do pure EFI machines not implement, say EBDA? If yes,
> acpi_find_root_pointer() should fail tofind RSDP.As for memory map, all I can
> find are reference to e820 memory map which again assumes a BIOS but most EFI
> implementations I have come across implement a BIOS compatibility layer, so this
> will work still. I will look some more into this.

Kexec in general enters the kernel past the point where BIOS calls have
been made.  So basically the kexec path assumes no runtime services from
any firmware.  As I recall the e820 memory map can report the ACPI
location.

Last I checked we still had that useless set_virtual_memory map call in
the kernel when efi is detected and until that goes away I don't know
that we can or should depend on any efi bits in the kernel on the kdump
path.

Honestly I don't think we should make any EFI calls later than we make
BIOS calls, certainly the kdump case proves it works.

Eric

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Khalid Aziz]

On Thu, 2012-09-20 at 00:49 -0700, Eric W. Biederman wrote:
> Last I checked we still had that useless set_virtual_memory map call in
> the kernel when efi is detected and until that goes away I don't know
> that we can or should depend on any efi bits in the kernel on the kdump
> path.
> 
> Honestly I don't think we should make any EFI calls later than we make
> BIOS calls, certainly the kdump case proves it works.
> 
> Eric

I would agree for the most part about not making any EFI calls from the
kernel but there are couple of cases where we may have to rethink that.
One is accessing the hardware clock. On a pure EFI and legacy-free
machine, we will need to access hardware through EFI runtime services.
The other one being access to EFI variables. We can choose to not use
EFI variables but we might be giving up some useful functionality.

-- 
Khalid Aziz <khalid.aziz@hp.com>


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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

On 09/12/2012 10:35 AM, Dave Young wrote:

> On 09/07/2012 04:56 PM, Dave Young wrote:
>>
>> I did some digging about the efi boot kdump in slackware with kernel
>> from latest linus tree. I'm surprise that kdump works even without these
>> "noefi" and "acpi_rsdp="
>>
>> With fedora 17 3.4.3 kernel passing "acpi_rsdp=" is enough for kdump
>> kernel booting.
>>
>> Actually in x86 setup code if bootloader do not pass correct efi_info in
>> boot_params efi_enabled will be set to 0 automaticlly.
>>
>> So as for this patch 'noefi' is not needed.
>>
>> I will do more testing to see why linus tree works without "acpi_rsdp="
>>
> 
> There's probably some efi changes between 3.4.3 - 3.6.0-rc4 cause this
> works. I have not got time to bisect it.


During recent test, I found I mistakenly used wrong kernel config
without CONFIG_EFI in 1st kernel for testing. acpi_rsdp= is still needed.

Just resent the v2 patch, please help to review.

-- 
Thanks
Dave

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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Khalid Aziz]

On Thu, 2012-10-18 at 11:12 +0800, Dave Young wrote:
> On 09/12/2012 10:35 AM, Dave Young wrote:
> 
> > On 09/07/2012 04:56 PM, Dave Young wrote:
> >>
> >> I did some digging about the efi boot kdump in slackware with kernel
> >> from latest linus tree. I'm surprise that kdump works even without these
> >> "noefi" and "acpi_rsdp="
> >>
> >> With fedora 17 3.4.3 kernel passing "acpi_rsdp=" is enough for kdump
> >> kernel booting.
> >>
> >> Actually in x86 setup code if bootloader do not pass correct efi_info in
> >> boot_params efi_enabled will be set to 0 automaticlly.
> >>
> >> So as for this patch 'noefi' is not needed.
> >>
> >> I will do more testing to see why linus tree works without "acpi_rsdp="
> >>
> > 
> > There's probably some efi changes between 3.4.3 - 3.6.0-rc4 cause this
> > works. I have not got time to bisect it.
> 
> 
> During recent test, I found I mistakenly used wrong kernel config
> without CONFIG_EFI in 1st kernel for testing. acpi_rsdp= is still needed.
> 
> Just resent the v2 patch, please help to review.
> 

Hi Dave,

I suspect EFI based systems with Compatibility Support Module (CSM) are
still not going to need this, but I can see native EFI systems with no
CSM needing this. I have reviewed your patch.

BTW, I am no longer at HP, so <khalid.aziz@hp.com> is no longer valid.
<khalid@gonehiking.org> is my permanent personal email address.

--
Khalid


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

Re: [PATCH]kdump: pass noefi and acpi_rsdp= to 2nd kernel

[Dave Young]

On 10/18/2012 10:59 PM, Khalid Aziz wrote:

> On Thu, 2012-10-18 at 11:12 +0800, Dave Young wrote:
>> On 09/12/2012 10:35 AM, Dave Young wrote:
>>
>>> On 09/07/2012 04:56 PM, Dave Young wrote:
>>>>
>>>> I did some digging about the efi boot kdump in slackware with kernel
>>>> from latest linus tree. I'm surprise that kdump works even without these
>>>> "noefi" and "acpi_rsdp="
>>>>
>>>> With fedora 17 3.4.3 kernel passing "acpi_rsdp=" is enough for kdump
>>>> kernel booting.
>>>>
>>>> Actually in x86 setup code if bootloader do not pass correct efi_info in
>>>> boot_params efi_enabled will be set to 0 automaticlly.
>>>>
>>>> So as for this patch 'noefi' is not needed.
>>>>
>>>> I will do more testing to see why linus tree works without "acpi_rsdp="
>>>>
>>>
>>> There's probably some efi changes between 3.4.3 - 3.6.0-rc4 cause this
>>> works. I have not got time to bisect it.
>>
>>
>> During recent test, I found I mistakenly used wrong kernel config
>> without CONFIG_EFI in 1st kernel for testing. acpi_rsdp= is still needed.
>>
>> Just resent the v2 patch, please help to review.
>>
> 
> Hi Dave,
> 
> I suspect EFI based systems with Compatibility Support Module (CSM) are
> still not going to need this, but I can see native EFI systems with no
> CSM needing this. I have reviewed your patch.


Good to know, I have no CSM machine to test. Since Simon has applied the
patch, I think I can improve this issue later If I can find a machine
with CSM.

> 
> BTW, I am no longer at HP, so <khalid.aziz@hp.com> is no longer valid.
> <khalid@gonehiking.org> is my permanent personal email address.


Thanks for reminding.

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



-- 
Thanks
Dave

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