| Message ID | 20181112094645.4879-4-fanc.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory | expand |
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 >
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 > > >
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 >> >> >> > >
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
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.
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 */
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?
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
On Tue, Nov 13, 2018 at 03:06:16PM -0500, Masayoshi Mizuma wrote: > I just felt the BOOT_STRING thing in lib/kstrtox.c confuses... > I'm OK for now if it's applied your below comment. Well, actually, upon a second look, I don't think that including a .c file into a header is ok: 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 Chao, why isn't this part of arch/x86/boot/compressed/misc.c ?
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. > >
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. >> >>
Hi Boris, Masa, and Baoquan, On Tue, Nov 13, 2018 at 10:51:56PM +0100, Borislav Petkov wrote: >On Tue, Nov 13, 2018 at 03:06:16PM -0500, Masayoshi Mizuma wrote: >> I just felt the BOOT_STRING thing in lib/kstrtox.c confuses... >> I'm OK for now if it's applied your below comment. > >Well, actually, upon a second look, I don't think that including a .c >file into a header is ok: > >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 > >Chao, why isn't this part of arch/x86/boot/compressed/misc.c ? > Fine, I have put it to misc.c: diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c index 8dd1d5ccae58..714b05b65a33 100644 --- a/arch/x86/boot/compressed/misc.c +++ b/arch/x86/boot/compressed/misc.c @@ -426,3 +426,7 @@ void fortify_panic(const char *name) { error("detected buffer overflow"); } + +#ifdef BOOT_STRING +#include "../../../../lib/kstrtox.c" +#endif And define it in misc.h: diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h index 4a3645fda0ed..98e28c4281ee 100644 --- a/arch/x86/boot/compressed/misc.h +++ b/arch/x86/boot/compressed/misc.h @@ -131,3 +131,5 @@ int num_immovable_mem; void get_immovable_mem(void); #endif #endif +#define BOOT_STRING +extern int kstrtoull(const char *s, unsigned int base, unsigned long long *res); But isdigit() would be redefine, so: diff --git a/include/linux/ctype.h b/include/linux/ctype.h index 363b004426db..aba01c385232 100644 --- a/include/linux/ctype.h +++ b/include/linux/ctype.h @@ -23,10 +23,12 @@ extern const unsigned char _ctype[]; #define isalnum(c) ((__ismask(c)&(_U|_L|_D)) != 0) #define isalpha(c) ((__ismask(c)&(_U|_L)) != 0) #define iscntrl(c) ((__ismask(c)&(_C)) != 0) +#ifndef BOOT_STRING static inline int isdigit(int c) { return '0' <= c && c <= '9'; } +#endif #define isgraph(c) ((__ismask(c)&(_P|_U|_L|_D)) != 0) #define islower(c) ((__ismask(c)&(_L)) != 0) #define isprint(c) ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0) Now I can make it. I wonder whether this is OK to cover isdigit() with 'BOOT_STRING' tag. Thanks, Chao Fan >-- >Regards/Gruss, > Boris. > >Good mailing practices for 400: avoid top-posting and trim the reply. > >
On Wed, Nov 14, 2018 at 02:12:16PM +0800, Chao Fan wrote: > But isdigit() would be redefine, so: > > diff --git a/include/linux/ctype.h b/include/linux/ctype.h > index 363b004426db..aba01c385232 100644 > --- a/include/linux/ctype.h > +++ b/include/linux/ctype.h > @@ -23,10 +23,12 @@ extern const unsigned char _ctype[]; > #define isalnum(c) ((__ismask(c)&(_U|_L|_D)) != 0) > #define isalpha(c) ((__ismask(c)&(_U|_L)) != 0) > #define iscntrl(c) ((__ismask(c)&(_C)) != 0) > +#ifndef BOOT_STRING > static inline int isdigit(int c) > { > return '0' <= c && c <= '9'; > } > +#endif > #define isgraph(c) ((__ismask(c)&(_P|_U|_L|_D)) != 0) > #define islower(c) ((__ismask(c)&(_L)) != 0) > #define isprint(c) ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0) > > Now I can make it. > I wonder whether this is OK to cover isdigit() with 'BOOT_STRING' tag. See the beginning of arch/x86/boot/compressed/kaslr.c for a possible way to disable boot/ctype.h
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.
On Wed, Nov 14, 2018 at 07:30:17PM +0100, Borislav Petkov wrote: >On Wed, Nov 14, 2018 at 02:12:16PM +0800, Chao Fan wrote: >> But isdigit() would be redefine, so: >> >> diff --git a/include/linux/ctype.h b/include/linux/ctype.h >> index 363b004426db..aba01c385232 100644 >> --- a/include/linux/ctype.h >> +++ b/include/linux/ctype.h >> @@ -23,10 +23,12 @@ extern const unsigned char _ctype[]; >> #define isalnum(c) ((__ismask(c)&(_U|_L|_D)) != 0) >> #define isalpha(c) ((__ismask(c)&(_U|_L)) != 0) >> #define iscntrl(c) ((__ismask(c)&(_C)) != 0) >> +#ifndef BOOT_STRING >> static inline int isdigit(int c) >> { >> return '0' <= c && c <= '9'; >> } >> +#endif >> #define isgraph(c) ((__ismask(c)&(_P|_U|_L|_D)) != 0) >> #define islower(c) ((__ismask(c)&(_L)) != 0) >> #define isprint(c) ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0) >> >> Now I can make it. >> I wonder whether this is OK to cover isdigit() with 'BOOT_STRING' tag. > >See the beginning of arch/x86/boot/compressed/kaslr.c for a possible way >to disable boot/ctype.h I have done this with BOOT_CTYPE_H. So misc.c can only use isdigit() and isxdigit() in include/linux/ctype.h. diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c index 8dd1d5ccae58..e51713fe3add 100644 --- a/arch/x86/boot/compressed/misc.c +++ b/arch/x86/boot/compressed/misc.c @@ -12,6 +12,7 @@ * High loaded stuff by Hans Lermen & Werner Almesberger, Feb. 1996 */ +#define BOOT_CTYPE_H #include "misc.h" #include "error.h" #include "pgtable.h" @@ -426,3 +427,7 @@ void fortify_panic(const char *name) { error("detected buffer overflow"); } + +#ifdef BOOT_STRING +#include "../../../../lib/kstrtox.c" +#endif This looks better than before. Thanks, Chao Fan > >-- >Regards/Gruss, > Boris. > >Good mailing practices for 400: avoid top-posting and trim the reply. > >
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
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(+)