Patchwork [v11,3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec

login
register
mail settings
Submitter Chao Fan
Date Nov. 12, 2018, 9:46 a.m.
Message ID <20181112094645.4879-4-fanc.fnst@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/654537/
State New
Headers show

Comments

Chao Fan - Nov. 12, 2018, 9:46 a.m.
Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
KEXEC writes the RSDP pointer to cmdline for EFI booting.
So if 'acpi_rsdp' found in cmdline, use it directely.

Since function kstrtoull() is needed, include it in
arch/x86/boot/string.h. To solve the definition conflict
problem, set BOOT_STRING tag to expose only kstrtoull() and
functions used by it. Other functions in lib/kstrtox.c will
be covered.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
 arch/x86/boot/string.h            |  4 ++++
 lib/kstrtox.c                     |  4 ++++
 3 files changed, 34 insertions(+)
Chao Fan - Nov. 12, 2018, 9:50 a.m.
Hi Boris,

I try to include lib/kstrtox.c in arch/x86/boot/string.c and
define the kstrtoull() function in arch/x86/boot/string.h.
But the definition problem is hard to solve, so I include
it in arch/x86/boot/string.c directely.
Then use BOOT_STRING tag to cover other functions and only
kstrtoull() is exposed.
I am not sure whether this is OK.

Thanks,
Chao Fan

On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>KEXEC writes the RSDP pointer to cmdline for EFI booting.
>So if 'acpi_rsdp' found in cmdline, use it directely.
>
>Since function kstrtoull() is needed, include it in
>arch/x86/boot/string.h. To solve the definition conflict
>problem, set BOOT_STRING tag to expose only kstrtoull() and
>functions used by it. Other functions in lib/kstrtox.c will
>be covered.
>
>Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>---
> arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
> arch/x86/boot/string.h            |  4 ++++
> lib/kstrtox.c                     |  4 ++++
> 3 files changed, 34 insertions(+)
>
>diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>index 50fa65cf824d..5cfb4efa5a19 100644
>--- a/arch/x86/boot/compressed/acpitb.c
>+++ b/arch/x86/boot/compressed/acpitb.c
>@@ -8,6 +8,12 @@
> #include <linux/numa.h>
> #include <linux/acpi.h>
> 
>+#define STATIC
>+#include <linux/decompress/mm.h>
>+
>+#define BOOT_STRING
>+#include "../string.h"
>+
> /* Search EFI table for RSDP table. */
> static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
> {
>@@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
> 		*rsdp_addr = (acpi_physical_address)address;
> 	}
> }
>+
>+static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>+{
>+#ifdef CONFIG_KEXEC
>+	unsigned long long res;
>+	int len = 0;
>+	char *val;
>+
>+	val = malloc(19);
>+	len = cmdline_find_option("acpi_rsdp", val, 19);
>+
>+	if (len == -1)
>+		return;
>+
>+	if (len > 0) {
>+		val[len] = 0;
>+		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>+	}
>+#endif
>+}
>diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>index 3d78e27077f4..0ff3edb888e4 100644
>--- a/arch/x86/boot/string.h
>+++ b/arch/x86/boot/string.h
>@@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
> 					  unsigned int base);
> 
> #endif /* BOOT_STRING_H */
>+
>+#ifdef BOOT_STRING
>+#include "../../../lib/kstrtox.c"
>+#endif
>diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>index 1006bf70bf74..3804db9eed56 100644
>--- a/lib/kstrtox.c
>+++ b/lib/kstrtox.c
>@@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
> }
> EXPORT_SYMBOL(kstrtoull);
> 
>+#ifndef BOOT_STRING
>+
> /**
>  * kstrtoll - convert a string to a long long
>  * @s: The start of the string. The string must be null-terminated, and may also
>@@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
> kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
> kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
> kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>+
>+#endif
>-- 
>2.19.1
>
Masayoshi Mizuma - Nov. 12, 2018, 5:43 p.m.
Hi Chao,

On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
> KEXEC writes the RSDP pointer to cmdline for EFI booting.
> So if 'acpi_rsdp' found in cmdline, use it directely.
> 

> Since function kstrtoull() is needed, include it in
> arch/x86/boot/string.h. To solve the definition conflict
> problem, set BOOT_STRING tag to expose only kstrtoull() and
> functions used by it. Other functions in lib/kstrtox.c will
> be covered.

How about the following get_acpi_rsdp()...? It doesn't use kstrtoull().

static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
{
#ifdef CONFIG_KEXEC
        unsigned long addr;
        char val[32];

        if (cmdline_find_option("acpi_rsdp", val, sizeof(val)) > 0) {
                char *e;

                if (!strncmp(val, "0x", 2)) {
                        addr = simple_strtoull(val + 2, &e, 16);
                        if ((addr == 0) || ((val + 2) == e))
                                return;
                        *rsdp_addr = (acpi_physical_address)addr;
                }
        }
#endif
}

Thanks,
Masa

> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>  arch/x86/boot/string.h            |  4 ++++
>  lib/kstrtox.c                     |  4 ++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 50fa65cf824d..5cfb4efa5a19 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -8,6 +8,12 @@
>  #include <linux/numa.h>
>  #include <linux/acpi.h>
>  
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#define BOOT_STRING
> +#include "../string.h"
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  		*rsdp_addr = (acpi_physical_address)address;
>  	}
>  }
> +
> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> +{
> +#ifdef CONFIG_KEXEC
> +	unsigned long long res;
> +	int len = 0;
> +	char *val;
> +
> +	val = malloc(19);
> +	len = cmdline_find_option("acpi_rsdp", val, 19);
> +
> +	if (len == -1)
> +		return;
> +
> +	if (len > 0) {
> +		val[len] = 0;
> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
> +	}
> +#endif
> +}
> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
> index 3d78e27077f4..0ff3edb888e4 100644
> --- a/arch/x86/boot/string.h
> +++ b/arch/x86/boot/string.h
> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>  					  unsigned int base);
>  
>  #endif /* BOOT_STRING_H */
> +
> +#ifdef BOOT_STRING
> +#include "../../../lib/kstrtox.c"
> +#endif
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 1006bf70bf74..3804db9eed56 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>  }
>  EXPORT_SYMBOL(kstrtoull);
>  
> +#ifndef BOOT_STRING
> +
>  /**
>   * kstrtoll - convert a string to a long long
>   * @s: The start of the string. The string must be null-terminated, and may also
> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
> +
> +#endif
> -- 
> 2.19.1
> 
> 
>
Chao Fan - Nov. 13, 2018, 2:12 a.m.
On Mon, Nov 12, 2018 at 12:43:44PM -0500, Masayoshi Mizuma wrote:
>Hi Chao,
>
>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>> So if 'acpi_rsdp' found in cmdline, use it directely.
>> 
>
>> Since function kstrtoull() is needed, include it in
>> arch/x86/boot/string.h. To solve the definition conflict
>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>> functions used by it. Other functions in lib/kstrtox.c will
>> be covered.
>
>How about the following get_acpi_rsdp()...? It doesn't use kstrtoull().
>
>static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>{
>#ifdef CONFIG_KEXEC
>        unsigned long addr;
>        char val[32];
>
>        if (cmdline_find_option("acpi_rsdp", val, sizeof(val)) > 0) {
>                char *e;
>
>                if (!strncmp(val, "0x", 2)) {
>                        addr = simple_strtoull(val + 2, &e, 16);
>                        if ((addr == 0) || ((val + 2) == e))
>                                return;
>                        *rsdp_addr = (acpi_physical_address)addr;
>                }
>        }
>#endif
>}

Thanks for the suggestion.
I used this function. In the old version, Boris said simple_strtoull()
is the old function and told me use the new kstrtoull().

Thanks,
Chao Fan

>
>Thanks,
>Masa
>
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>>  arch/x86/boot/string.h            |  4 ++++
>>  lib/kstrtox.c                     |  4 ++++
>>  3 files changed, 34 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 50fa65cf824d..5cfb4efa5a19 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -8,6 +8,12 @@
>>  #include <linux/numa.h>
>>  #include <linux/acpi.h>
>>  
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>> +#define BOOT_STRING
>> +#include "../string.h"
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  		*rsdp_addr = (acpi_physical_address)address;
>>  	}
>>  }
>> +
>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> +#ifdef CONFIG_KEXEC
>> +	unsigned long long res;
>> +	int len = 0;
>> +	char *val;
>> +
>> +	val = malloc(19);
>> +	len = cmdline_find_option("acpi_rsdp", val, 19);
>> +
>> +	if (len == -1)
>> +		return;
>> +
>> +	if (len > 0) {
>> +		val[len] = 0;
>> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>> +	}
>> +#endif
>> +}
>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>> index 3d78e27077f4..0ff3edb888e4 100644
>> --- a/arch/x86/boot/string.h
>> +++ b/arch/x86/boot/string.h
>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>>  					  unsigned int base);
>>  
>>  #endif /* BOOT_STRING_H */
>> +
>> +#ifdef BOOT_STRING
>> +#include "../../../lib/kstrtox.c"
>> +#endif
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 1006bf70bf74..3804db9eed56 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>  }
>>  EXPORT_SYMBOL(kstrtoull);
>>  
>> +#ifndef BOOT_STRING
>> +
>>  /**
>>   * kstrtoll - convert a string to a long long
>>   * @s: The start of the string. The string must be null-terminated, and may also
>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>> +
>> +#endif
>> -- 
>> 2.19.1
>> 
>> 
>> 
>
>
Masayoshi Mizuma - Nov. 13, 2018, 4:11 p.m.
Hi Chao and Boris,

On Tue, Nov 13, 2018 at 10:12:18AM +0800, Chao Fan wrote:
> On Mon, Nov 12, 2018 at 12:43:44PM -0500, Masayoshi Mizuma wrote:
> >How about the following get_acpi_rsdp()...? It doesn't use kstrtoull().
> >
> >static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> >{
> >#ifdef CONFIG_KEXEC
> >        unsigned long addr;
> >        char val[32];
> >
> >        if (cmdline_find_option("acpi_rsdp", val, sizeof(val)) > 0) {
> >                char *e;
> >
> >                if (!strncmp(val, "0x", 2)) {
> >                        addr = simple_strtoull(val + 2, &e, 16);
> >                        if ((addr == 0) || ((val + 2) == e))
> >                                return;
> >                        *rsdp_addr = (acpi_physical_address)addr;
> >                }
> >        }
> >#endif
> >}
> 
> Thanks for the suggestion.
> I used this function. In the old version, Boris said simple_strtoull()
> is the old function and told me use the new kstrtoull().

I think it's not very good idea to use kstrtoull() in
arch/x86/boot/compressed/* because some tricks are needed to
use the function, looks like Chao is trying...
It is the simple way here to use simple_strtoull() defined
in arch/x86/boot/boot.h, I think.

I know checkpatch.pl says an warning about simple_strtoull(),
however, I believe the warning is for simple_strtoull() defined
in lib/vsprintf.c.

Thanks,
Masa
Borislav Petkov - Nov. 13, 2018, 5:22 p.m.
On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote:
> I know checkpatch.pl says an warning about simple_strtoull(),
> however, I believe the warning is for simple_strtoull() defined
> in lib/vsprintf.c.

simple_strtoull is deprecated for various reasons. I'll take a look at
Chao's patch soon.
Borislav Petkov - Nov. 13, 2018, 5:51 p.m.
On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
> KEXEC writes the RSDP pointer to cmdline for EFI booting.
> So if 'acpi_rsdp' found in cmdline, use it directely.
> 
> Since function kstrtoull() is needed, include it in
> arch/x86/boot/string.h. To solve the definition conflict
> problem, set BOOT_STRING tag to expose only kstrtoull() and
> functions used by it. Other functions in lib/kstrtox.c will
> be covered.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>  arch/x86/boot/string.h            |  4 ++++
>  lib/kstrtox.c                     |  4 ++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 50fa65cf824d..5cfb4efa5a19 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -8,6 +8,12 @@
>  #include <linux/numa.h>
>  #include <linux/acpi.h>
>  
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#define BOOT_STRING
> +#include "../string.h"
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  		*rsdp_addr = (acpi_physical_address)address;
>  	}
>  }
> +
> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> +{
> +#ifdef CONFIG_KEXEC

Ok, why is that CONFIG_KEXEC dependency needed now too?

Ok, let's recap: so far, for your use case you need:

CONFIG_MEMORY_HOTREMOVE
CONFIG_RANDOMIZE_BASE
and now
CONFIG_KEXEC

So, you can clean up all that ifdeffery by defining a new config item
CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
then you can do

vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o

and get rid of the most of the ifdeffery.

Yes?

> +	unsigned long long res;
> +	int len = 0;
> +	char *val;
> +
> +	val = malloc(19);
> +	len = cmdline_find_option("acpi_rsdp", val, 19);
> +

^ Superfluous newline.

> +	if (len == -1)
> +		return;

That check is not needed since you do > 0 below.

> +
> +	if (len > 0) {
> +		val[len] = 0;
> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
> +	}
> +#endif
> +}
> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
> index 3d78e27077f4..0ff3edb888e4 100644
> --- a/arch/x86/boot/string.h
> +++ b/arch/x86/boot/string.h
> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>  					  unsigned int base);
>  
>  #endif /* BOOT_STRING_H */
> +
> +#ifdef BOOT_STRING
> +#include "../../../lib/kstrtox.c"
> +#endif
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 1006bf70bf74..3804db9eed56 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>  }
>  EXPORT_SYMBOL(kstrtoull);

This needs a comment to explain what is that guard used for.

> +#ifndef BOOT_STRING
> +
>  /**
>   * kstrtoll - convert a string to a long long
>   * @s: The start of the string. The string must be null-terminated, and may also
> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
> +
> +#endif

#endif /* BOOT_STRING */
Borislav Petkov - Nov. 13, 2018, 5:54 p.m.
On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote:
> I think it's not very good idea to use kstrtoull() in
> arch/x86/boot/compressed/* because some tricks are needed to
> use the function, looks like Chao is trying...

Ok, I had a look at the patch. And frankly, I don't see anything wrong
with the aspect of using kstrtoull() in the compressed stage too and
getting rid of simple_strtoull().

So what are your reservations?
Masayoshi Mizuma - Nov. 13, 2018, 8:06 p.m.
On Tue, Nov 13, 2018 at 06:54:13PM +0100, Borislav Petkov wrote:
> On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote:
> > I think it's not very good idea to use kstrtoull() in
> > arch/x86/boot/compressed/* because some tricks are needed to
> > use the function, looks like Chao is trying...
> 
> Ok, I had a look at the patch. And frankly, I don't see anything wrong
> with the aspect of using kstrtoull() in the compressed stage too and
> getting rid of simple_strtoull().
> 
> So what are your reservations?

Thank you for your checking.
I just felt the BOOT_STRING thing in lib/kstrtox.c confuses...
I'm OK for now if it's applied your below comment.

> > @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
> >  }
> >  EXPORT_SYMBOL(kstrtoull);
> 
> This needs a comment to explain what is that guard used for.

Thanks,
Masa
Chao Fan - Nov. 14, 2018, 1:54 a.m.
On Tue, Nov 13, 2018 at 06:51:50PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>> So if 'acpi_rsdp' found in cmdline, use it directely.
>> 
>> Since function kstrtoull() is needed, include it in
>> arch/x86/boot/string.h. To solve the definition conflict
>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>> functions used by it. Other functions in lib/kstrtox.c will
>> be covered.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>>  arch/x86/boot/string.h            |  4 ++++
>>  lib/kstrtox.c                     |  4 ++++
>>  3 files changed, 34 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 50fa65cf824d..5cfb4efa5a19 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -8,6 +8,12 @@
>>  #include <linux/numa.h>
>>  #include <linux/acpi.h>
>>  
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>> +#define BOOT_STRING
>> +#include "../string.h"
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  		*rsdp_addr = (acpi_physical_address)address;
>>  	}
>>  }
>> +
>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> +#ifdef CONFIG_KEXEC
>
>Ok, why is that CONFIG_KEXEC dependency needed now too?
>

CONFIG_KEXEC is only needed in this function.

When searching RSDP, there are three methods in order:
1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
   so it can be parsed and used. CONFIG_KEXEC is needed here.
2. When booting from EFI, parse EFI table and find RSDP.
3. When booting from BIOS, search memory for RSDP just like
   acpi_find_root_pointer() in drivers/acpi/acpica/tbxfroot.c did.

So, CONFIG_KEXEC is only needed in 1, exactly in this function
get_acpi_rsdp() of my PATCH.

Thanks,
Chao Fan

>Ok, let's recap: so far, for your use case you need:
>
>CONFIG_MEMORY_HOTREMOVE
>CONFIG_RANDOMIZE_BASE
>and now
>CONFIG_KEXEC
>
>So, you can clean up all that ifdeffery by defining a new config item
>CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
>then you can do
>
>vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
>
>and get rid of the most of the ifdeffery.
>
>Yes?
>
>> +	unsigned long long res;
>> +	int len = 0;
>> +	char *val;
>> +
>> +	val = malloc(19);
>> +	len = cmdline_find_option("acpi_rsdp", val, 19);
>> +
>
>^ Superfluous newline.
>
>> +	if (len == -1)
>> +		return;
>
>That check is not needed since you do > 0 below.
>
>> +
>> +	if (len > 0) {
>> +		val[len] = 0;
>> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>> +	}
>> +#endif
>> +}
>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>> index 3d78e27077f4..0ff3edb888e4 100644
>> --- a/arch/x86/boot/string.h
>> +++ b/arch/x86/boot/string.h
>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>>  					  unsigned int base);
>>  
>>  #endif /* BOOT_STRING_H */
>> +
>> +#ifdef BOOT_STRING
>> +#include "../../../lib/kstrtox.c"
>> +#endif
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 1006bf70bf74..3804db9eed56 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>  }
>>  EXPORT_SYMBOL(kstrtoull);
>
>This needs a comment to explain what is that guard used for.
>
>> +#ifndef BOOT_STRING
>> +
>>  /**
>>   * kstrtoll - convert a string to a long long
>>   * @s: The start of the string. The string must be null-terminated, and may also
>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>> +
>> +#endif
>
>#endif /* BOOT_STRING */
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>
Chao Fan - Nov. 14, 2018, 1:59 a.m.
On Wed, Nov 14, 2018 at 09:54:50AM +0800, Chao Fan wrote:
>On Tue, Nov 13, 2018 at 06:51:50PM +0100, Borislav Petkov wrote:
>>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>>> So if 'acpi_rsdp' found in cmdline, use it directely.
>>> 
>>> Since function kstrtoull() is needed, include it in
>>> arch/x86/boot/string.h. To solve the definition conflict
>>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>>> functions used by it. Other functions in lib/kstrtox.c will
>>> be covered.
>>> 
>>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>>> ---
>>>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>>>  arch/x86/boot/string.h            |  4 ++++
>>>  lib/kstrtox.c                     |  4 ++++
>>>  3 files changed, 34 insertions(+)
>>> 
>>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>>> index 50fa65cf824d..5cfb4efa5a19 100644
>>> --- a/arch/x86/boot/compressed/acpitb.c
>>> +++ b/arch/x86/boot/compressed/acpitb.c
>>> @@ -8,6 +8,12 @@
>>>  #include <linux/numa.h>
>>>  #include <linux/acpi.h>
>>>  
>>> +#define STATIC
>>> +#include <linux/decompress/mm.h>
>>> +
>>> +#define BOOT_STRING
>>> +#include "../string.h"
>>> +
>>>  /* Search EFI table for RSDP table. */
>>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>>  {
>>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>>  		*rsdp_addr = (acpi_physical_address)address;
>>>  	}
>>>  }
>>> +
>>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>>> +{
>>> +#ifdef CONFIG_KEXEC
>>
>>Ok, why is that CONFIG_KEXEC dependency needed now too?
>>
>
>CONFIG_KEXEC is only needed in this function.
>
>When searching RSDP, there are three methods in order:
>1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
>   so it can be parsed and used. CONFIG_KEXEC is needed here.
>2. When booting from EFI, parse EFI table and find RSDP.
>3. When booting from BIOS, search memory for RSDP just like
>   acpi_find_root_pointer() in drivers/acpi/acpica/tbxfroot.c did.
>
>So, CONFIG_KEXEC is only needed in 1, exactly in this function
>get_acpi_rsdp() of my PATCH.
>
>Thanks,
>Chao Fan
>

That means, CONFIG_KEXEC is needed by a little part of the whole PATCHSET.
Without CONFIG_KEXEC, RSDP can only be found in other methods.

Thanks,
Chao Fan

>>Ok, let's recap: so far, for your use case you need:
>>
>>CONFIG_MEMORY_HOTREMOVE
>>CONFIG_RANDOMIZE_BASE
>>and now
>>CONFIG_KEXEC
>>
>>So, you can clean up all that ifdeffery by defining a new config item
>>CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
>>then you can do
>>
>>vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
>>
>>and get rid of the most of the ifdeffery.
>>
>>Yes?
>>
>>> +	unsigned long long res;
>>> +	int len = 0;
>>> +	char *val;
>>> +
>>> +	val = malloc(19);
>>> +	len = cmdline_find_option("acpi_rsdp", val, 19);
>>> +
>>
>>^ Superfluous newline.
>>
>>> +	if (len == -1)
>>> +		return;
>>
>>That check is not needed since you do > 0 below.
>>
>>> +
>>> +	if (len > 0) {
>>> +		val[len] = 0;
>>> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>>> +	}
>>> +#endif
>>> +}
>>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>>> index 3d78e27077f4..0ff3edb888e4 100644
>>> --- a/arch/x86/boot/string.h
>>> +++ b/arch/x86/boot/string.h
>>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>>>  					  unsigned int base);
>>>  
>>>  #endif /* BOOT_STRING_H */
>>> +
>>> +#ifdef BOOT_STRING
>>> +#include "../../../lib/kstrtox.c"
>>> +#endif
>>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>>> index 1006bf70bf74..3804db9eed56 100644
>>> --- a/lib/kstrtox.c
>>> +++ b/lib/kstrtox.c
>>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>>  }
>>>  EXPORT_SYMBOL(kstrtoull);
>>
>>This needs a comment to explain what is that guard used for.
>>
>>> +#ifndef BOOT_STRING
>>> +
>>>  /**
>>>   * kstrtoll - convert a string to a long long
>>>   * @s: The start of the string. The string must be null-terminated, and may also
>>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>>>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>>>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>>>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>>> +
>>> +#endif
>>
>>#endif /* BOOT_STRING */
>>
>>-- 
>>Regards/Gruss,
>>    Boris.
>>
>>Good mailing practices for 400: avoid top-posting and trim the reply.
>>
>>
Borislav Petkov - Nov. 14, 2018, 6:33 p.m.
On Wed, Nov 14, 2018 at 09:54:50AM +0800, Chao Fan wrote:
> CONFIG_KEXEC is only needed in this function.
> 
> When searching RSDP, there are three methods in order:
> 1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
>    so it can be parsed and used. CONFIG_KEXEC is needed here.

But theoretically acpi_rsdp can be supplied by the first kernel too,
right?

So you don't need that CONFIG_KEXEC here at all?

> >Ok, let's recap: so far, for your use case you need:
> >
> >CONFIG_MEMORY_HOTREMOVE
> >CONFIG_RANDOMIZE_BASE
> >and now
> >CONFIG_KEXEC
> >
> >So, you can clean up all that ifdeffery by defining a new config item
> >CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
> >then you can do
> >
> >vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
> >
> >and get rid of the most of the ifdeffery.

Regardless of CONFIG_KEXEC - you still need to define a CONFIG_ symbol
for your use case. We won't be parsing RSDP early on !MEMORY_HOTREMOVE
machines, which is the majority.

Patch

diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
index 50fa65cf824d..5cfb4efa5a19 100644
--- a/arch/x86/boot/compressed/acpitb.c
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -8,6 +8,12 @@ 
 #include <linux/numa.h>
 #include <linux/acpi.h>
 
+#define STATIC
+#include <linux/decompress/mm.h>
+
+#define BOOT_STRING
+#include "../string.h"
+
 /* Search EFI table for RSDP table. */
 static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 {
@@ -200,3 +206,23 @@  static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 		*rsdp_addr = (acpi_physical_address)address;
 	}
 }
+
+static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
+{
+#ifdef CONFIG_KEXEC
+	unsigned long long res;
+	int len = 0;
+	char *val;
+
+	val = malloc(19);
+	len = cmdline_find_option("acpi_rsdp", val, 19);
+
+	if (len == -1)
+		return;
+
+	if (len > 0) {
+		val[len] = 0;
+		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
+	}
+#endif
+}
diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
index 3d78e27077f4..0ff3edb888e4 100644
--- a/arch/x86/boot/string.h
+++ b/arch/x86/boot/string.h
@@ -30,3 +30,7 @@  extern unsigned long long simple_strtoull(const char *cp, char **endp,
 					  unsigned int base);
 
 #endif /* BOOT_STRING_H */
+
+#ifdef BOOT_STRING
+#include "../../../lib/kstrtox.c"
+#endif
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..3804db9eed56 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -126,6 +126,8 @@  int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 }
 EXPORT_SYMBOL(kstrtoull);
 
+#ifndef BOOT_STRING
+
 /**
  * kstrtoll - convert a string to a long long
  * @s: The start of the string. The string must be null-terminated, and may also
@@ -408,3 +410,5 @@  kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
 kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
 kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
 kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
+
+#endif