[PATCH v2 0/4] x86: Always to to fill acpi_rsdp_addr in boot params

[PATCH v2 0/4] x86: Always to to fill acpi_rsdp_addr in boot params

[Kairui Song]

This patch sync the behavior of user space kexec and kexec_file_load,
they will both fill the boot_params.acpi_rsdp_addr with a valid RSDP
value, to make sure second kernel can always get the RSDP consistently.

This will make it effortless to boot newer version of kernel (5.0+)
without specifying acpi_rsdp= cmdline on EFI system even with EFI
service disabled. Should not change any behavior with older kernels.

Update from V1:
  - Split into multiple patches for a cleaner structure, content is not
    changed.

Kairui Song (4):
  x86: Update boot parameters defination
  x86: Introduce helpers for getting RSDP address
  x86: Always try to fill acpi_rsdp_addr in boot params
  crashdump/x86: Use new introduce helper for getting RSDP

 include/x86/x86-linux.h            |  8 ++--
 kexec/arch/i386/crashdump-x86.c    | 34 +++++------------
 kexec/arch/i386/kexec-x86-common.c | 60 ++++++++++++++++++++++++++++++
 kexec/arch/i386/kexec-x86.h        |  1 +
 kexec/arch/i386/x86-linux-setup.c  |  6 ++-
 kexec/arch/i386/x86-linux-setup.h  |  1 +
 6 files changed, 80 insertions(+), 30 deletions(-)

-- 
2.20.1


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

[PATCH v2 1/4] x86: Update boot parameters defination

[Kairui Song]

Since kernel commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address
from boot params if available"), kernel accept a acpi_rsdp_addr param in
boot_params. Sync the x86_linux_param_header to support this param.

Signed-off-by: Kairui Song <kasong@redhat.com>
---
 include/x86/x86-linux.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/x86/x86-linux.h b/include/x86/x86-linux.h
index 352ea02..a5d8df8 100644
--- a/include/x86/x86-linux.h
+++ b/include/x86/x86-linux.h
@@ -45,8 +45,7 @@ struct apm_bios_info {
 	uint16_t cseg_len;	/* 0x4e */
 	uint16_t cseg_16_len;	/* 0x50 */
 	uint16_t dseg_len;	/* 0x52 */
-	uint8_t  reserved[44];	/* 0x54 */
-};
+} __attribute__((packed));
 
 /*
  * EDD stuff
@@ -113,12 +112,15 @@ struct x86_linux_param_header {
 	uint8_t  reserved4[2];			/* 0x3e -- 0x3f reserved for future expansion */
 
 	struct apm_bios_info apm_bios_info;	/* 0x40 */
+	uint8_t  reserved4_1[28];		/* 0x54 */
+	uint64_t acpi_rsdp_addr;		/* 0x70 */
+	uint8_t  reserved4_2[8];		/* 0x78 */
 	struct drive_info_struct drive_info;	/* 0x80 */
 	struct sys_desc_table sys_desc_table;	/* 0xa0 */
 	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];	/* 0xe4 */
+	uint8_t reserved4_3[0x1c0 - 0xcc];	/* 0xe4 */
 	uint8_t efi_info[32];			/* 0x1c0 */
 	uint32_t alt_mem_k;			/* 0x1e0 */
 	uint8_t  reserved5[4];			/* 0x1e4 */
-- 
2.20.1


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

Re: [PATCH v2 1/4] x86: Update boot parameters defination

[Dave Young]

Hi Kairui
On 05/14/19 at 01:09pm, Kairui Song wrote:
> Since kernel commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address
> from boot params if available"), kernel accept a acpi_rsdp_addr param in
> boot_params. Sync the x86_linux_param_header to support this param.
> 
> Signed-off-by: Kairui Song <kasong@redhat.com>
> ---
>  include/x86/x86-linux.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/x86/x86-linux.h b/include/x86/x86-linux.h
> index 352ea02..a5d8df8 100644
> --- a/include/x86/x86-linux.h
> +++ b/include/x86/x86-linux.h
> @@ -45,8 +45,7 @@ struct apm_bios_info {
>  	uint16_t cseg_len;	/* 0x4e */
>  	uint16_t cseg_16_len;	/* 0x50 */
>  	uint16_t dseg_len;	/* 0x52 */
> -	uint8_t  reserved[44];	/* 0x54 */
> -};
> +} __attribute__((packed));

It should be good to keep same as the kernel header without packed
attribute.

is it possible to sync the latest mainline uapi bootparam header file,
maybe after this series get solved?

>  
>  /*
>   * EDD stuff
> @@ -113,12 +112,15 @@ struct x86_linux_param_header {
>  	uint8_t  reserved4[2];			/* 0x3e -- 0x3f reserved for future expansion */
>  
>  	struct apm_bios_info apm_bios_info;	/* 0x40 */
> +	uint8_t  reserved4_1[28];		/* 0x54 */
> +	uint64_t acpi_rsdp_addr;		/* 0x70 */
> +	uint8_t  reserved4_2[8];		/* 0x78 */
>  	struct drive_info_struct drive_info;	/* 0x80 */
>  	struct sys_desc_table sys_desc_table;	/* 0xa0 */
>  	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];	/* 0xe4 */
> +	uint8_t reserved4_3[0x1c0 - 0xcc];	/* 0xe4 */
>  	uint8_t efi_info[32];			/* 0x1c0 */
>  	uint32_t alt_mem_k;			/* 0x1e0 */
>  	uint8_t  reserved5[4];			/* 0x1e4 */
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> 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 v2 1/4] x86: Update boot parameters defination

[Kairui Song]

On Thu, May 23, 2019 at 11:01 AM Dave Young <dyoung@redhat.com> wrote:
>
> Hi Kairui
> On 05/14/19 at 01:09pm, Kairui Song wrote:
> > Since kernel commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address
> > from boot params if available"), kernel accept a acpi_rsdp_addr param in
> > boot_params. Sync the x86_linux_param_header to support this param.
> >
> > Signed-off-by: Kairui Song <kasong@redhat.com>
> > ---
> >  include/x86/x86-linux.h | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/x86/x86-linux.h b/include/x86/x86-linux.h
> > index 352ea02..a5d8df8 100644
> > --- a/include/x86/x86-linux.h
> > +++ b/include/x86/x86-linux.h
> > @@ -45,8 +45,7 @@ struct apm_bios_info {
> >       uint16_t cseg_len;      /* 0x4e */
> >       uint16_t cseg_16_len;   /* 0x50 */
> >       uint16_t dseg_len;      /* 0x52 */
> > -     uint8_t  reserved[44];  /* 0x54 */
> > -};
> > +} __attribute__((packed));
>
> It should be good to keep same as the kernel header without packed
> attribute.
>
> is it possible to sync the latest mainline uapi bootparam header file,
> maybe after this series get solved?
>

Hi Dave,

I can remove the packed attr then, just ensure it won't break
anything, a bit paranoid maybe...

About sync the whole structure, the problem is kexec tools' header
structure is different so that will change a lot. Maybe could be
discussed seperately.

-- 
Best Regards,
Kairui Song

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

Re: [PATCH v2 1/4] x86: Update boot parameters defination

[Dave Young]

On 05/23/19 at 07:12pm, Kairui Song wrote:
> On Thu, May 23, 2019 at 11:01 AM Dave Young <dyoung@redhat.com> wrote:
> >
> > Hi Kairui
> > On 05/14/19 at 01:09pm, Kairui Song wrote:
> > > Since kernel commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address
> > > from boot params if available"), kernel accept a acpi_rsdp_addr param in
> > > boot_params. Sync the x86_linux_param_header to support this param.
> > >
> > > Signed-off-by: Kairui Song <kasong@redhat.com>
> > > ---
> > >  include/x86/x86-linux.h | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/x86/x86-linux.h b/include/x86/x86-linux.h
> > > index 352ea02..a5d8df8 100644
> > > --- a/include/x86/x86-linux.h
> > > +++ b/include/x86/x86-linux.h
> > > @@ -45,8 +45,7 @@ struct apm_bios_info {
> > >       uint16_t cseg_len;      /* 0x4e */
> > >       uint16_t cseg_16_len;   /* 0x50 */
> > >       uint16_t dseg_len;      /* 0x52 */
> > > -     uint8_t  reserved[44];  /* 0x54 */
> > > -};
> > > +} __attribute__((packed));
> >
> > It should be good to keep same as the kernel header without packed
> > attribute.
> >
> > is it possible to sync the latest mainline uapi bootparam header file,
> > maybe after this series get solved?
> >
> 
Hi Kairui,

> Hi Dave,
> 
> I can remove the packed attr then, just ensure it won't break
> anything, a bit paranoid maybe...
> 
> About sync the whole structure, the problem is kexec tools' header
> structure is different so that will change a lot. Maybe could be
> discussed seperately.

Yes, agreed, it is not necessary to do in this series.

> 
> -- 
> Best Regards,
> Kairui Song

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

[PATCH v2 2/4] x86: Introduce helpers for getting RSDP address

[Kairui Song]

On x86 RSDP is fundamental for booting the machine. When second kernel
is incapable of parsing the RSDP address (eg. kexec next kernel on an EFI
system with EFI service disabled), kexec should prepare the RSDP address
for second kernel.

Introduce helpers for getting RSDP from multiple sources, including boot
params, cmdline and EFI firmware.

For legacy BIOS interface, there is no better way to find the RSDP address
rather than scanning the memory region and search for it, and this will
always be done by the kernel as a fallback, so this is no need to try to
get the RSDP address for that case.

Signed-off-by: Kairui Song <kasong@redhat.com>
---
 kexec/arch/i386/kexec-x86-common.c | 60 ++++++++++++++++++++++++++++++
 kexec/arch/i386/kexec-x86.h        |  1 +
 kexec/arch/i386/x86-linux-setup.c  |  3 +-
 kexec/arch/i386/x86-linux-setup.h  |  1 +
 4 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
index de99758..4b8eb26 100644
--- a/kexec/arch/i386/kexec-x86-common.c
+++ b/kexec/arch/i386/kexec-x86-common.c
@@ -39,6 +39,7 @@
 #include "../../firmware_memmap.h"
 #include "../../crashdump.h"
 #include "kexec-x86.h"
+#include "x86-linux-setup.h"
 #include "../../kexec-xen.h"
 
 /* Used below but not present in (older?) xenctrl.h */
@@ -392,4 +393,63 @@ int get_memory_ranges(struct memory_range **range, int *ranges,
 	return ret;
 }
 
+static uint64_t cmdline_get_acpi_rsdp(void) {
+	uint64_t acpi_rsdp = 0;
+	char *tmp_cmdline, *rsdp_param;
 
+	tmp_cmdline = get_command_line();
+	rsdp_param = strstr(tmp_cmdline, "acpi_rsdp=");
+
+	if (rsdp_param)
+		sscanf(rsdp_param, "acpi_rsdp=%lx", &acpi_rsdp);
+
+	free(tmp_cmdline);
+	return acpi_rsdp;
+}
+
+static uint64_t bootparam_get_acpi_rsdp(void) {
+	uint64_t acpi_rsdp = 0;
+	off_t offset = offsetof(struct x86_linux_param_header, acpi_rsdp_addr);
+
+	if (get_bootparam(&acpi_rsdp, offset, sizeof(acpi_rsdp)))
+		return 0;
+
+	return acpi_rsdp;
+}
+
+static uint64_t efi_get_acpi_rsdp(void) {
+	FILE *fp;
+	char line[MAX_LINE], *s;
+	uint64_t acpi_rsdp = 0;
+
+	fp = fopen("/sys/firmware/efi/systab", "r");
+	if (!fp)
+		return acpi_rsdp;
+
+	while(fgets(line, sizeof(line), fp) != 0) {
+		/* ACPI20= always goes before ACPI= */
+		if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
+			s = strchr(line, '=') + 1;
+			sscanf(s, "0x%lx", &acpi_rsdp);
+			break;
+		}
+	}
+	fclose(fp);
+
+	return acpi_rsdp;
+}
+
+uint64_t get_acpi_rsdp(void)
+{
+	uint64_t acpi_rsdp = 0;
+
+	acpi_rsdp = cmdline_get_acpi_rsdp();
+
+	if (!acpi_rsdp)
+		acpi_rsdp = bootparam_get_acpi_rsdp();
+
+	if (!acpi_rsdp)
+		acpi_rsdp = efi_get_acpi_rsdp();
+
+	return acpi_rsdp;
+}
diff --git a/kexec/arch/i386/kexec-x86.h b/kexec/arch/i386/kexec-x86.h
index c2bcd37..1b58c3b 100644
--- a/kexec/arch/i386/kexec-x86.h
+++ b/kexec/arch/i386/kexec-x86.h
@@ -86,4 +86,5 @@ int nbi_load(int argc, char **argv, const char *buf, off_t len,
 void nbi_usage(void);
 
 extern unsigned xen_e820_to_kexec_type(uint32_t type);
+extern uint64_t get_acpi_rsdp(void);
 #endif /* KEXEC_X86_H */
diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
index 8fad115..5ca7c25 100644
--- a/kexec/arch/i386/x86-linux-setup.c
+++ b/kexec/arch/i386/x86-linux-setup.c
@@ -123,7 +123,6 @@ void setup_linux_bootloader_parameters_high(
 	cmdline_ptr[cmdline_len - 1] = '\0';
 }
 
-static int get_bootparam(void *buf, off_t offset, size_t size);
 static int setup_linux_vesafb(struct x86_linux_param_header *real_mode)
 {
 	struct fb_fix_screeninfo fix;
@@ -452,7 +451,7 @@ char *find_mnt_by_fsname(char *fsname)
 	return mntdir;
 }
 
-static int get_bootparam(void *buf, off_t offset, size_t size)
+int get_bootparam(void *buf, off_t offset, size_t size)
 {
 	int data_file;
 	char *debugfs_mnt, *sysfs_mnt;
diff --git a/kexec/arch/i386/x86-linux-setup.h b/kexec/arch/i386/x86-linux-setup.h
index f5d23d3..0c651e5 100644
--- a/kexec/arch/i386/x86-linux-setup.h
+++ b/kexec/arch/i386/x86-linux-setup.h
@@ -21,6 +21,7 @@ static inline void setup_linux_bootloader_parameters(
 }
 void setup_linux_system_parameters(struct kexec_info *info,
 	struct x86_linux_param_header *real_mode);
+int get_bootparam(void *buf, off_t offset, size_t size);
 
 
 #define SETUP_BASE    0x90000
-- 
2.20.1


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

Re: [PATCH v2 2/4] x86: Introduce helpers for getting RSDP address

[Dave Young]

On 05/14/19 at 01:09pm, Kairui Song wrote:
> On x86 RSDP is fundamental for booting the machine. When second kernel
> is incapable of parsing the RSDP address (eg. kexec next kernel on an EFI
> system with EFI service disabled), kexec should prepare the RSDP address
> for second kernel.
> 
> Introduce helpers for getting RSDP from multiple sources, including boot
> params, cmdline and EFI firmware.
> 
> For legacy BIOS interface, there is no better way to find the RSDP address
> rather than scanning the memory region and search for it, and this will
> always be done by the kernel as a fallback, so this is no need to try to
> get the RSDP address for that case.
> 
> Signed-off-by: Kairui Song <kasong@redhat.com>
> ---
>  kexec/arch/i386/kexec-x86-common.c | 60 ++++++++++++++++++++++++++++++
>  kexec/arch/i386/kexec-x86.h        |  1 +
>  kexec/arch/i386/x86-linux-setup.c  |  3 +-
>  kexec/arch/i386/x86-linux-setup.h  |  1 +
>  4 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
> index de99758..4b8eb26 100644
> --- a/kexec/arch/i386/kexec-x86-common.c
> +++ b/kexec/arch/i386/kexec-x86-common.c
> @@ -39,6 +39,7 @@
>  #include "../../firmware_memmap.h"
>  #include "../../crashdump.h"
>  #include "kexec-x86.h"
> +#include "x86-linux-setup.h"
>  #include "../../kexec-xen.h"
>  
>  /* Used below but not present in (older?) xenctrl.h */
> @@ -392,4 +393,63 @@ int get_memory_ranges(struct memory_range **range, int *ranges,
>  	return ret;
>  }
>  
> +static uint64_t cmdline_get_acpi_rsdp(void) {
> +	uint64_t acpi_rsdp = 0;
> +	char *tmp_cmdline, *rsdp_param;
>  
> +	tmp_cmdline = get_command_line();
> +	rsdp_param = strstr(tmp_cmdline, "acpi_rsdp=");

strstr will locate the first acpi_rsdp, what about multiple acpi_rsdp
provided?

BTW, if one provide a wrong adress in acpi_rsdp= cmdline then it is not
usable.

So not sure if adding this cmdline param is necessary, maybe only add
efi case will be reliable.

> +
> +	if (rsdp_param)
> +		sscanf(rsdp_param, "acpi_rsdp=%lx", &acpi_rsdp);
> +
> +	free(tmp_cmdline);
> +	return acpi_rsdp;
> +}
> +
> +static uint64_t bootparam_get_acpi_rsdp(void) {
> +	uint64_t acpi_rsdp = 0;
> +	off_t offset = offsetof(struct x86_linux_param_header, acpi_rsdp_addr);
> +
> +	if (get_bootparam(&acpi_rsdp, offset, sizeof(acpi_rsdp)))
> +		return 0;
> +
> +	return acpi_rsdp;
> +}
> +
> +static uint64_t efi_get_acpi_rsdp(void) {
> +	FILE *fp;
> +	char line[MAX_LINE], *s;
> +	uint64_t acpi_rsdp = 0;
> +
> +	fp = fopen("/sys/firmware/efi/systab", "r");
> +	if (!fp)
> +		return acpi_rsdp;
> +
> +	while(fgets(line, sizeof(line), fp) != 0) {
> +		/* ACPI20= always goes before ACPI= */
> +		if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
> +			s = strchr(line, '=') + 1;
> +			sscanf(s, "0x%lx", &acpi_rsdp);
> +			break;
> +		}
> +	}
> +	fclose(fp);
> +
> +	return acpi_rsdp;
> +}
> +
> +uint64_t get_acpi_rsdp(void)
> +{
> +	uint64_t acpi_rsdp = 0;
> +
> +	acpi_rsdp = cmdline_get_acpi_rsdp();
> +
> +	if (!acpi_rsdp)
> +		acpi_rsdp = bootparam_get_acpi_rsdp();
> +
> +	if (!acpi_rsdp)
> +		acpi_rsdp = efi_get_acpi_rsdp();
> +
> +	return acpi_rsdp;
> +}
> diff --git a/kexec/arch/i386/kexec-x86.h b/kexec/arch/i386/kexec-x86.h
> index c2bcd37..1b58c3b 100644
> --- a/kexec/arch/i386/kexec-x86.h
> +++ b/kexec/arch/i386/kexec-x86.h
> @@ -86,4 +86,5 @@ int nbi_load(int argc, char **argv, const char *buf, off_t len,
>  void nbi_usage(void);
>  
>  extern unsigned xen_e820_to_kexec_type(uint32_t type);
> +extern uint64_t get_acpi_rsdp(void);
>  #endif /* KEXEC_X86_H */
> diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> index 8fad115..5ca7c25 100644
> --- a/kexec/arch/i386/x86-linux-setup.c
> +++ b/kexec/arch/i386/x86-linux-setup.c
> @@ -123,7 +123,6 @@ void setup_linux_bootloader_parameters_high(
>  	cmdline_ptr[cmdline_len - 1] = '\0';
>  }
>  
> -static int get_bootparam(void *buf, off_t offset, size_t size);
>  static int setup_linux_vesafb(struct x86_linux_param_header *real_mode)
>  {
>  	struct fb_fix_screeninfo fix;
> @@ -452,7 +451,7 @@ char *find_mnt_by_fsname(char *fsname)
>  	return mntdir;
>  }
>  
> -static int get_bootparam(void *buf, off_t offset, size_t size)
> +int get_bootparam(void *buf, off_t offset, size_t size)
>  {
>  	int data_file;
>  	char *debugfs_mnt, *sysfs_mnt;
> diff --git a/kexec/arch/i386/x86-linux-setup.h b/kexec/arch/i386/x86-linux-setup.h
> index f5d23d3..0c651e5 100644
> --- a/kexec/arch/i386/x86-linux-setup.h
> +++ b/kexec/arch/i386/x86-linux-setup.h
> @@ -21,6 +21,7 @@ static inline void setup_linux_bootloader_parameters(
>  }
>  void setup_linux_system_parameters(struct kexec_info *info,
>  	struct x86_linux_param_header *real_mode);
> +int get_bootparam(void *buf, off_t offset, size_t size);
>  
>  
>  #define SETUP_BASE    0x90000
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> 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 v2 2/4] x86: Introduce helpers for getting RSDP address

[Kairui Song]

On Thu, May 23, 2019 at 11:16 AM Dave Young <dyoung@redhat.com> wrote:
>
> On 05/14/19 at 01:09pm, Kairui Song wrote:
> > On x86 RSDP is fundamental for booting the machine. When second kernel
> > is incapable of parsing the RSDP address (eg. kexec next kernel on an EFI
> > system with EFI service disabled), kexec should prepare the RSDP address
> > for second kernel.
> >
> > Introduce helpers for getting RSDP from multiple sources, including boot
> > params, cmdline and EFI firmware.
> >
> > For legacy BIOS interface, there is no better way to find the RSDP address
> > rather than scanning the memory region and search for it, and this will
> > always be done by the kernel as a fallback, so this is no need to try to
> > get the RSDP address for that case.
> >
> > Signed-off-by: Kairui Song <kasong@redhat.com>
> > ---
> >  kexec/arch/i386/kexec-x86-common.c | 60 ++++++++++++++++++++++++++++++
> >  kexec/arch/i386/kexec-x86.h        |  1 +
> >  kexec/arch/i386/x86-linux-setup.c  |  3 +-
> >  kexec/arch/i386/x86-linux-setup.h  |  1 +
> >  4 files changed, 63 insertions(+), 2 deletions(-)
> >
> > diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
> > index de99758..4b8eb26 100644
> > --- a/kexec/arch/i386/kexec-x86-common.c
> > +++ b/kexec/arch/i386/kexec-x86-common.c
> > @@ -39,6 +39,7 @@
> >  #include "../../firmware_memmap.h"
> >  #include "../../crashdump.h"
> >  #include "kexec-x86.h"
> > +#include "x86-linux-setup.h"
> >  #include "../../kexec-xen.h"
> >
> >  /* Used below but not present in (older?) xenctrl.h */
> > @@ -392,4 +393,63 @@ int get_memory_ranges(struct memory_range **range, int *ranges,
> >       return ret;
> >  }
> >
> > +static uint64_t cmdline_get_acpi_rsdp(void) {
> > +     uint64_t acpi_rsdp = 0;
> > +     char *tmp_cmdline, *rsdp_param;
> >
> > +     tmp_cmdline = get_command_line();
> > +     rsdp_param = strstr(tmp_cmdline, "acpi_rsdp=");
>
> strstr will locate the first acpi_rsdp, what about multiple acpi_rsdp
> provided?

Good catch, should always use the latest acpi_rsdp provided, will fix that.

>
> BTW, if one provide a wrong adress in acpi_rsdp= cmdline then it is not
> usable.
>

I think in that case kernel will not boot. If kexec is available then
it means a right value is given.

> So not sure if adding this cmdline param is necessary, maybe only add
> efi case will be reliable.

Adding the cmdline param ensure kexec boot loop won't fail. eg. in an
older version kernel booted with kexec, and have EFI disabled, then
cmdline is the only source for getting and storing the RSDP address.

>
> > +
> > +     if (rsdp_param)
> > +             sscanf(rsdp_param, "acpi_rsdp=%lx", &acpi_rsdp);
> > +
> > +     free(tmp_cmdline);
> > +     return acpi_rsdp;
> > +}
> > +
> > +static uint64_t bootparam_get_acpi_rsdp(void) {
> > +     uint64_t acpi_rsdp = 0;
> > +     off_t offset = offsetof(struct x86_linux_param_header, acpi_rsdp_addr);
> > +
> > +     if (get_bootparam(&acpi_rsdp, offset, sizeof(acpi_rsdp)))
> > +             return 0;
> > +
> > +     return acpi_rsdp;
> > +}
> > +
> > +static uint64_t efi_get_acpi_rsdp(void) {
> > +     FILE *fp;
> > +     char line[MAX_LINE], *s;
> > +     uint64_t acpi_rsdp = 0;
> > +
> > +     fp = fopen("/sys/firmware/efi/systab", "r");
> > +     if (!fp)
> > +             return acpi_rsdp;
> > +
> > +     while(fgets(line, sizeof(line), fp) != 0) {
> > +             /* ACPI20= always goes before ACPI= */
> > +             if ((strstr(line, "ACPI20=")) || (strstr(line, "ACPI="))) {
> > +                     s = strchr(line, '=') + 1;
> > +                     sscanf(s, "0x%lx", &acpi_rsdp);
> > +                     break;
> > +             }
> > +     }
> > +     fclose(fp);
> > +
> > +     return acpi_rsdp;
> > +}
> > +
> > +uint64_t get_acpi_rsdp(void)
> > +{
> > +     uint64_t acpi_rsdp = 0;
> > +
> > +     acpi_rsdp = cmdline_get_acpi_rsdp();
> > +
> > +     if (!acpi_rsdp)
> > +             acpi_rsdp = bootparam_get_acpi_rsdp();
> > +
> > +     if (!acpi_rsdp)
> > +             acpi_rsdp = efi_get_acpi_rsdp();
> > +
> > +     return acpi_rsdp;
> > +}
> > diff --git a/kexec/arch/i386/kexec-x86.h b/kexec/arch/i386/kexec-x86.h
> > index c2bcd37..1b58c3b 100644
> > --- a/kexec/arch/i386/kexec-x86.h
> > +++ b/kexec/arch/i386/kexec-x86.h
> > @@ -86,4 +86,5 @@ int nbi_load(int argc, char **argv, const char *buf, off_t len,
> >  void nbi_usage(void);that
> >
> >  extern unsigned xen_e820_to_kexec_type(uint32_t type);
> > +extern uint64_t get_acpi_rsdp(void);
> >  #endif /* KEXEC_X86_H */
> > diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> > index 8fad115..5ca7c25 100644
> > --- a/kexec/arch/i386/x86-linux-setup.c
> > +++ b/kexec/arch/i386/x86-linux-setup.c
> > @@ -123,7 +123,6 @@ void setup_linux_bootloader_parameters_high(
> >       cmdline_ptr[cmdline_len - 1] = '\0';
> >  }
> >
> > -static int get_bootparam(void *buf, off_t offset, size_t size);
> >  static int setup_linux_vesafb(struct x86_linux_param_header *real_mode)
> >  {
> >       struct fb_fix_screeninfo fix;
> > @@ -452,7 +451,7 @@ char *find_mnt_by_fsname(char *fsname)
> >       return mntdir;
> >  }
> >
> > -static int get_bootparam(void *buf, off_t offset, size_t size)
> > +int get_bootparam(void *buf, off_t offset, size_t size)
> >  {
> >       int data_file;
> >       char *debugfs_mnt, *sysfs_mnt;
> > diff --git a/kexec/arch/i386/x86-linux-setup.h b/kexec/arch/i386/x86-linux-setup.h
> > index f5d23d3..0c651e5 100644
> > --- a/kexec/arch/i386/x86-linux-setup.h
> > +++ b/kexec/arch/i386/x86-linux-setup.h
> > @@ -21,6 +21,7 @@ static inline void setup_linux_bootloader_parameters(
> >  }
> >  void setup_linux_system_parameters(struct kexec_info *info,
> >       struct x86_linux_param_header *real_mode);
> > +int get_bootparam(void *buf, off_t offset, size_t size);
> >
> >
> >  #define SETUP_BASE    0x90000
> > --
> > 2.20.1
> >
> >
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec



-- 
Best Regards,
Kairui Song

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

Re: [PATCH v2 2/4] x86: Introduce helpers for getting RSDP address

[Kairui Song]

On Thu, May 23, 2019 at 11:43 AM Kairui Song <kasong@redhat.com> wrote:
>
> On Thu, May 23, 2019 at 11:16 AM Dave Young <dyoung@redhat.com> wrote:
> >
> > On 05/14/19 at 01:09pm, Kairui Song wrote:
> > > On x86 RSDP is fundamental for booting the machine. When second kernel
> > > is incapable of parsing the RSDP address (eg. kexec next kernel on an EFI
> > > system with EFI service disabled), kexec should prepare the RSDP address
> > > for second kernel.
> > >
> > > Introduce helpers for getting RSDP from multiple sources, including boot
> > > params, cmdline and EFI firmware.
> > >
> > > For legacy BIOS interface, there is no better way to find the RSDP address
> > > rather than scanning the memory region and search for it, and this will
> > > always be done by the kernel as a fallback, so this is no need to try to
> > > get the RSDP address for that case.
> > >
> > > Signed-off-by: Kairui Song <kasong@redhat.com>
> > > ---
> > >  kexec/arch/i386/kexec-x86-common.c | 60 ++++++++++++++++++++++++++++++
> > >  kexec/arch/i386/kexec-x86.h        |  1 +
> > >  kexec/arch/i386/x86-linux-setup.c  |  3 +-
> > >  kexec/arch/i386/x86-linux-setup.h  |  1 +
> > >  4 files changed, 63 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
> > > index de99758..4b8eb26 100644
> > > --- a/kexec/arch/i386/kexec-x86-common.c
> > > +++ b/kexec/arch/i386/kexec-x86-common.c
> > > @@ -39,6 +39,7 @@
> > >  #include "../../firmware_memmap.h"
> > >  #include "../../crashdump.h"
> > >  #include "kexec-x86.h"
> > > +#include "x86-linux-setup.h"
> > >  #include "../../kexec-xen.h"
> > >
> > >  /* Used below but not present in (older?) xenctrl.h */
> > > @@ -392,4 +393,63 @@ int get_memory_ranges(struct memory_range **range, int *ranges,
> > >       return ret;
> > >  }
> > >
> > > +static uint64_t cmdline_get_acpi_rsdp(void) {
> > > +     uint64_t acpi_rsdp = 0;
> > > +     char *tmp_cmdline, *rsdp_param;
> > >
> > > +     tmp_cmdline = get_command_line();
> > > +     rsdp_param = strstr(tmp_cmdline, "acpi_rsdp=");
> >
> > strstr will locate the first acpi_rsdp, what about multiple acpi_rsdp
> > provided?
>
> Good catch, should always use the latest acpi_rsdp provided, will fix that.
>
> >
> > BTW, if one provide a wrong adress in acpi_rsdp= cmdline then it is not
> > usable.
> >
>
> I think in that case kernel will not boot. If kexec is available then
> it means a right value is given.
>

After double check the kernel will boot even wrong acpi_rsdp is given,
so I'll drop this part. boot_params in newer kernel will be enough to
make sure kexec loop boot with EFI disabled won't fail.

-- 
Best Regards,
Kairui Song

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

[PATCH v2 3/4] x86: Always try to fill acpi_rsdp_addr in boot params

[Kairui Song]

Since kernel commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address
from boot params if available"), kernel accept an acpi_rsdp_addr param in
boot_params. So fill in this parameter unconditionally, ensure second
kernel always get the right RSDP address consistently, and boot well on
EFI system even with EFI service disabled. User no longer need to change
the kernel cmdline to workaround the missing RSDP issue.

For older version of kernels (Before 5.0), there won't be any change of
behavior.

Signed-off-by: Kairui Song <kasong@redhat.com>
---
 kexec/arch/i386/x86-linux-setup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
index 5ca7c25..5b00b42 100644
--- a/kexec/arch/i386/x86-linux-setup.c
+++ b/kexec/arch/i386/x86-linux-setup.c
@@ -901,4 +901,7 @@ void setup_linux_system_parameters(struct kexec_info *info,
 
 	/* fill the EDD information */
 	setup_edd_info(real_mode);
+
+	/* Always try to fill acpi_rsdp_addr */
+	real_mode->acpi_rsdp_addr = get_acpi_rsdp();
 }
-- 
2.20.1


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

Re: [PATCH v2 3/4] x86: Always try to fill acpi_rsdp_addr in boot params

[Dave Young]

On 05/14/19 at 01:09pm, Kairui Song wrote:
> Since kernel commit e6e094e053af75 ("x86/acpi, x86/boot: Take RSDP address
> from boot params if available"), kernel accept an acpi_rsdp_addr param in
> boot_params. So fill in this parameter unconditionally, ensure second
> kernel always get the right RSDP address consistently, and boot well on
> EFI system even with EFI service disabled. User no longer need to change
> the kernel cmdline to workaround the missing RSDP issue.
> 
> For older version of kernels (Before 5.0), there won't be any change of
> behavior.
> 
> Signed-off-by: Kairui Song <kasong@redhat.com>
> ---
>  kexec/arch/i386/x86-linux-setup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kexec/arch/i386/x86-linux-setup.c b/kexec/arch/i386/x86-linux-setup.c
> index 5ca7c25..5b00b42 100644
> --- a/kexec/arch/i386/x86-linux-setup.c
> +++ b/kexec/arch/i386/x86-linux-setup.c
> @@ -901,4 +901,7 @@ void setup_linux_system_parameters(struct kexec_info *info,
>  
>  	/* fill the EDD information */
>  	setup_edd_info(real_mode);
> +
> +	/* Always try to fill acpi_rsdp_addr */
> +	real_mode->acpi_rsdp_addr = get_acpi_rsdp();
>  }
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Seems no better way to check old kernel, then go with this way looks
acceptable.

Thanks
Dave

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

[PATCH v2 4/4] crashdump/x86: Use new introduced helper for getting RSDP

[Kairui Song]

Use the new introduced helper for getting RSDP, this ensures RSDP is
always accessible and avoid code duplication.

Signed-off-by: Kairui Song <kasong@redhat.com>
---
 kexec/arch/i386/crashdump-x86.c | 34 +++++++++------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
index 140f45b..a2aea31 100644
--- a/kexec/arch/i386/crashdump-x86.c
+++ b/kexec/arch/i386/crashdump-x86.c
@@ -787,35 +787,19 @@ static int sysfs_efi_runtime_map_exist(void)
 /* 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=";
+	uint64_t acpi_rsdp;
+	char acpi_rsdp_buf[MAX_LINE];
 
-	fp = fopen("/sys/firmware/efi/systab", "r");
-	if (!fp)
-		return;
+	acpi_rsdp = get_acpi_rsdp();
 
-	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);
+	if (!acpi_rsdp)
+		return;
 
-			break;
-		}
-	}
+	sprintf(acpi_rsdp_buf, " acpi_rsdp=0x%lx", acpi_rsdp);
+	if (strlen(cmdline) + strlen(acpi_rsdp_buf) > (COMMAND_LINE_SIZE - 1))
+		die("Command line overflow\n");
 
-	fclose(fp);
+	strcat(cmdline, acpi_rsdp_buf);
 }
 
 static void get_backup_area(struct kexec_info *info,
-- 
2.20.1


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