Re: [PATCH v5 18/21] arm64: Move "nokaslr" over to the early cpufeature infrastructure
On Mon, 25 Jan 2021 at 15:28, Marc Zyngier wrote: > > On 2021-01-25 14:19, Ard Biesheuvel wrote: > > On Mon, 25 Jan 2021 at 14:54, Marc Zyngier wrote: > >> > >> On 2021-01-25 12:54, Ard Biesheuvel wrote: > > [...] > > >> > This struct now takes up > >> > - ~100 bytes for the characters themselves (which btw are not emitted > >> > into __initdata or __initconst) > >> > - 6x8 bytes for the char pointers > >> > - 6x24 bytes for the RELA relocations that annotate these pointers as > >> > quantities that need to be relocated at boot (on a kernel built with > >> > KASLR) > >> > > >> > I know it's only a drop in the ocean, but in this case, where the > >> > struct is statically declared and defined only once, and in the same > >> > place, we could easily turn this into > >> > > >> > static const struct { > >> >char alias[24]; > >> >char param[20]; > >> > }; > >> > > >> > and get rid of all the overhead. The only slightly annoying thing is > >> > that the array sizes need to be kept in sync with the largest instance > >> > appearing in the array, but this is easy when the struct type is > >> > declared in the same place where its only instance is defined. > >> > >> Fair enough. I personally find the result butt-ugly, but I agree > >> that it certainly saves some memory. Does the following work for > >> you? I can even give symbolic names to the various constants (how > >> generous of me! ;-). > >> > > > > To be honest, I was anticipating more of a discussion, but this looks > > reasonable to me. > > It looked like a reasonable ask: all the strings are completely useless > once the kernel has booted, and I'm the first to moan that I can't boot > an arm64 kernel with less than 60MB of RAM (OK, it's a pretty bloated > kernel...). > > > Does 'charfeature[80];' really need 80 bytes though? > > It really needs 75 bytes, because of this: > > { "arm64.nopauth", > "id_aa64isar1.gpi=0 id_aa64isar1.gpa=0 " > "id_aa64isar1.api=0 id_aa64isar1.apa=0" }, > > 80 is a round enough number. > Fair enough. This will inflate the struct substantially, but at least it's all __initconst data now, and it's all NUL bytes so it compresses much better than the pointers and RELA entries. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 18/21] arm64: Move "nokaslr" over to the early cpufeature infrastructure
On 2021-01-25 14:19, Ard Biesheuvel wrote: On Mon, 25 Jan 2021 at 14:54, Marc Zyngier wrote: On 2021-01-25 12:54, Ard Biesheuvel wrote: [...] > This struct now takes up > - ~100 bytes for the characters themselves (which btw are not emitted > into __initdata or __initconst) > - 6x8 bytes for the char pointers > - 6x24 bytes for the RELA relocations that annotate these pointers as > quantities that need to be relocated at boot (on a kernel built with > KASLR) > > I know it's only a drop in the ocean, but in this case, where the > struct is statically declared and defined only once, and in the same > place, we could easily turn this into > > static const struct { >char alias[24]; >char param[20]; > }; > > and get rid of all the overhead. The only slightly annoying thing is > that the array sizes need to be kept in sync with the largest instance > appearing in the array, but this is easy when the struct type is > declared in the same place where its only instance is defined. Fair enough. I personally find the result butt-ugly, but I agree that it certainly saves some memory. Does the following work for you? I can even give symbolic names to the various constants (how generous of me! ;-). To be honest, I was anticipating more of a discussion, but this looks reasonable to me. It looked like a reasonable ask: all the strings are completely useless once the kernel has booted, and I'm the first to moan that I can't boot an arm64 kernel with less than 60MB of RAM (OK, it's a pretty bloated kernel...). Does 'charfeature[80];' really need 80 bytes though? It really needs 75 bytes, because of this: { "arm64.nopauth", "id_aa64isar1.gpi=0 id_aa64isar1.gpa=0 " "id_aa64isar1.api=0 id_aa64isar1.apa=0"}, 80 is a round enough number. Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 18/21] arm64: Move "nokaslr" over to the early cpufeature infrastructure
On Mon, 25 Jan 2021 at 14:54, Marc Zyngier wrote: > > On 2021-01-25 12:54, Ard Biesheuvel wrote: > > On Mon, 25 Jan 2021 at 11:53, Marc Zyngier wrote: > >> > >> Given that the early cpufeature infrastructure has borrowed quite > >> a lot of code from the kaslr implementation, let's reimplement > >> the matching of the "nokaslr" option with it. > >> > >> Signed-off-by: Marc Zyngier > >> Acked-by: Catalin Marinas > >> Acked-by: David Brazdil > >> --- > >> arch/arm64/kernel/idreg-override.c | 15 + > >> arch/arm64/kernel/kaslr.c | 36 > >> ++ > >> 2 files changed, 17 insertions(+), 34 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/idreg-override.c > >> b/arch/arm64/kernel/idreg-override.c > >> index cbb8eaa48742..3ccf51b84ba4 100644 > >> --- a/arch/arm64/kernel/idreg-override.c > >> +++ b/arch/arm64/kernel/idreg-override.c > >> @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = > >> { > >> }, > >> }; > >> > >> +extern struct arm64_ftr_override kaslr_feature_override; > >> + > >> +static const struct ftr_set_desc kaslr __initdata = { > > > > This should be __initconst not __initdata (below too) > > > >> + .name = "kaslr", > >> +#ifdef CONFIG_RANDOMIZE_BASE > >> + .override = &kaslr_feature_override, > >> +#endif > >> + .fields = { > >> + { "disabled", 0 }, > >> + {} > >> + }, > >> +}; > >> + > >> static const struct ftr_set_desc * const regs[] __initdata = { > >> &mmfr1, > >> + &kaslr, > >> }; > >> > >> static const struct { > >> @@ -41,6 +55,7 @@ static const struct { > >> } aliases[] __initdata = { > >> { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, > >> { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, > >> + { "nokaslr","kaslr.disabled=1" }, > >> }; > >> > > > > This struct now takes up > > - ~100 bytes for the characters themselves (which btw are not emitted > > into __initdata or __initconst) > > - 6x8 bytes for the char pointers > > - 6x24 bytes for the RELA relocations that annotate these pointers as > > quantities that need to be relocated at boot (on a kernel built with > > KASLR) > > > > I know it's only a drop in the ocean, but in this case, where the > > struct is statically declared and defined only once, and in the same > > place, we could easily turn this into > > > > static const struct { > >char alias[24]; > >char param[20]; > > }; > > > > and get rid of all the overhead. The only slightly annoying thing is > > that the array sizes need to be kept in sync with the largest instance > > appearing in the array, but this is easy when the struct type is > > declared in the same place where its only instance is defined. > > Fair enough. I personally find the result butt-ugly, but I agree > that it certainly saves some memory. Does the following work for > you? I can even give symbolic names to the various constants (how > generous of me! ;-). > To be honest, I was anticipating more of a discussion, but this looks reasonable to me. Does 'charfeature[80];' really need 80 bytes though? > diff --git a/arch/arm64/kernel/idreg-override.c > b/arch/arm64/kernel/idreg-override.c > index d1310438d95c..9e7043bdc808 100644 > --- a/arch/arm64/kernel/idreg-override.c > +++ b/arch/arm64/kernel/idreg-override.c > @@ -14,15 +14,15 @@ > #include > > struct ftr_set_desc { > - const char *name; > + charname[20]; > struct arm64_ftr_override *override; > struct { > - const char *name; > + charname[20]; > u8 shift; > } fields[]; > }; > > -static const struct ftr_set_desc mmfr1 __initdata = { > +static const struct ftr_set_desc mmfr1 __initconst = { > .name = "id_aa64mmfr1", > .override = &id_aa64mmfr1_override, > .fields = { > @@ -31,7 +31,7 @@ static const struct ftr_set_desc mmfr1 __initdata = { > }, > }; > > -static const struct ftr_set_desc pfr1 __initdata = { > +static const struct ftr_set_desc pfr1 __initconst = { > .name = "id_aa64pfr1", > .override = &id_aa64pfr1_override, > .fields = { > @@ -40,7 +40,7 @@ static const struct ftr_set_desc pfr1 __initdata = { > }, > }; > > -static const struct ftr_set_desc isar1 __initdata = { > +static const struct ftr_set_desc isar1 __initconst = { > .name = "id_aa64isar1", > .override = &id_aa64isar1_override, > .fields = { > @@ -54,7 +54,7 @@ static const struct ftr_set_desc isar1 __initdata = { > > extern struct arm64_ftr_override kaslr_feature_override; > > -static const struct ftr_set_desc kaslr __initdata = { > +static const struct ftr_se
Re: [PATCH v5 18/21] arm64: Move "nokaslr" over to the early cpufeature infrastructure
On 2021-01-25 12:54, Ard Biesheuvel wrote: On Mon, 25 Jan 2021 at 11:53, Marc Zyngier wrote: Given that the early cpufeature infrastructure has borrowed quite a lot of code from the kaslr implementation, let's reimplement the matching of the "nokaslr" option with it. Signed-off-by: Marc Zyngier Acked-by: Catalin Marinas Acked-by: David Brazdil --- arch/arm64/kernel/idreg-override.c | 15 + arch/arm64/kernel/kaslr.c | 36 ++ 2 files changed, 17 insertions(+), 34 deletions(-) diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index cbb8eaa48742..3ccf51b84ba4 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = { }, }; +extern struct arm64_ftr_override kaslr_feature_override; + +static const struct ftr_set_desc kaslr __initdata = { This should be __initconst not __initdata (below too) + .name = "kaslr", +#ifdef CONFIG_RANDOMIZE_BASE + .override = &kaslr_feature_override, +#endif + .fields = { + { "disabled", 0 }, + {} + }, +}; + static const struct ftr_set_desc * const regs[] __initdata = { &mmfr1, + &kaslr, }; static const struct { @@ -41,6 +55,7 @@ static const struct { } aliases[] __initdata = { { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, + { "nokaslr","kaslr.disabled=1" }, }; This struct now takes up - ~100 bytes for the characters themselves (which btw are not emitted into __initdata or __initconst) - 6x8 bytes for the char pointers - 6x24 bytes for the RELA relocations that annotate these pointers as quantities that need to be relocated at boot (on a kernel built with KASLR) I know it's only a drop in the ocean, but in this case, where the struct is statically declared and defined only once, and in the same place, we could easily turn this into static const struct { char alias[24]; char param[20]; }; and get rid of all the overhead. The only slightly annoying thing is that the array sizes need to be kept in sync with the largest instance appearing in the array, but this is easy when the struct type is declared in the same place where its only instance is defined. Fair enough. I personally find the result butt-ugly, but I agree that it certainly saves some memory. Does the following work for you? I can even give symbolic names to the various constants (how generous of me! ;-). Thanks, M. diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index d1310438d95c..9e7043bdc808 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -14,15 +14,15 @@ #include struct ftr_set_desc { - const char *name; + charname[20]; struct arm64_ftr_override *override; struct { - const char *name; + charname[20]; u8 shift; } fields[]; }; -static const struct ftr_set_desc mmfr1 __initdata = { +static const struct ftr_set_desc mmfr1 __initconst = { .name = "id_aa64mmfr1", .override = &id_aa64mmfr1_override, .fields = { @@ -31,7 +31,7 @@ static const struct ftr_set_desc mmfr1 __initdata = { }, }; -static const struct ftr_set_desc pfr1 __initdata = { +static const struct ftr_set_desc pfr1 __initconst = { .name = "id_aa64pfr1", .override = &id_aa64pfr1_override, .fields = { @@ -40,7 +40,7 @@ static const struct ftr_set_desc pfr1 __initdata = { }, }; -static const struct ftr_set_desc isar1 __initdata = { +static const struct ftr_set_desc isar1 __initconst = { .name = "id_aa64isar1", .override = &id_aa64isar1_override, .fields = { @@ -54,7 +54,7 @@ static const struct ftr_set_desc isar1 __initdata = { extern struct arm64_ftr_override kaslr_feature_override; -static const struct ftr_set_desc kaslr __initdata = { +static const struct ftr_set_desc kaslr __initconst = { .name = "kaslr", #ifdef CONFIG_RANDOMIZE_BASE .override = &kaslr_feature_override, @@ -65,7 +65,7 @@ static const struct ftr_set_desc kaslr __initdata = { }, }; -static const struct ftr_set_desc * const regs[] __initdata = { +static const struct ftr_set_desc * const regs[] __initconst = { &mmfr1, &pfr1, &isar1, @@ -73,9 +73,9 @@ static const struct ftr_set_desc * const regs[] __initdata = { }; static const struct { - const char *alias; - const char *feature; -} aliases[] __initdata = {
Re: [PATCH v5 18/21] arm64: Move "nokaslr" over to the early cpufeature infrastructure
On Mon, 25 Jan 2021 at 11:53, Marc Zyngier wrote: > > Given that the early cpufeature infrastructure has borrowed quite > a lot of code from the kaslr implementation, let's reimplement > the matching of the "nokaslr" option with it. > > Signed-off-by: Marc Zyngier > Acked-by: Catalin Marinas > Acked-by: David Brazdil > --- > arch/arm64/kernel/idreg-override.c | 15 + > arch/arm64/kernel/kaslr.c | 36 ++ > 2 files changed, 17 insertions(+), 34 deletions(-) > > diff --git a/arch/arm64/kernel/idreg-override.c > b/arch/arm64/kernel/idreg-override.c > index cbb8eaa48742..3ccf51b84ba4 100644 > --- a/arch/arm64/kernel/idreg-override.c > +++ b/arch/arm64/kernel/idreg-override.c > @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = { > }, > }; > > +extern struct arm64_ftr_override kaslr_feature_override; > + > +static const struct ftr_set_desc kaslr __initdata = { This should be __initconst not __initdata (below too) > + .name = "kaslr", > +#ifdef CONFIG_RANDOMIZE_BASE > + .override = &kaslr_feature_override, > +#endif > + .fields = { > + { "disabled", 0 }, > + {} > + }, > +}; > + > static const struct ftr_set_desc * const regs[] __initdata = { > &mmfr1, > + &kaslr, > }; > > static const struct { > @@ -41,6 +55,7 @@ static const struct { > } aliases[] __initdata = { > { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, > { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, > + { "nokaslr","kaslr.disabled=1" }, > }; > This struct now takes up - ~100 bytes for the characters themselves (which btw are not emitted into __initdata or __initconst) - 6x8 bytes for the char pointers - 6x24 bytes for the RELA relocations that annotate these pointers as quantities that need to be relocated at boot (on a kernel built with KASLR) I know it's only a drop in the ocean, but in this case, where the struct is statically declared and defined only once, and in the same place, we could easily turn this into static const struct { char alias[24]; char param[20]; }; and get rid of all the overhead. The only slightly annoying thing is that the array sizes need to be kept in sync with the largest instance appearing in the array, but this is easy when the struct type is declared in the same place where its only instance is defined. > static char *cmdline_contains_option(const char *cmdline, const char *option) > diff --git a/arch/arm64/kernel/kaslr.c b/arch/arm64/kernel/kaslr.c > index 5fc86e7d01a1..27f8939deb1b 100644 > --- a/arch/arm64/kernel/kaslr.c > +++ b/arch/arm64/kernel/kaslr.c > @@ -51,39 +51,7 @@ static __init u64 get_kaslr_seed(void *fdt) > return ret; > } > > -static __init bool cmdline_contains_nokaslr(const u8 *cmdline) > -{ > - const u8 *str; > - > - str = strstr(cmdline, "nokaslr"); > - return str == cmdline || (str > cmdline && *(str - 1) == ' '); > -} > - > -static __init bool is_kaslr_disabled_cmdline(void *fdt) > -{ > - if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) { > - int node; > - const u8 *prop; > - > - node = fdt_path_offset(fdt, "/chosen"); > - if (node < 0) > - goto out; > - > - prop = fdt_getprop(fdt, node, "bootargs", NULL); > - if (!prop) > - goto out; > - > - if (cmdline_contains_nokaslr(prop)) > - return true; > - > - if (IS_ENABLED(CONFIG_CMDLINE_EXTEND)) > - goto out; > - > - return false; > - } > -out: > - return cmdline_contains_nokaslr(CONFIG_CMDLINE); > -} > +struct arm64_ftr_override kaslr_feature_override __initdata; > > /* > * This routine will be executed with the kernel mapped at its default > virtual > @@ -126,7 +94,7 @@ u64 __init kaslr_early_init(void) > * Check if 'nokaslr' appears on the command line, and > * return 0 if that is the case. > */ > - if (is_kaslr_disabled_cmdline(fdt)) { > + if (kaslr_feature_override.val & kaslr_feature_override.mask & 0xf) { > kaslr_status = KASLR_DISABLED_CMDLINE; > return 0; > } > -- > 2.29.2 > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v5 18/21] arm64: Move "nokaslr" over to the early cpufeature infrastructure
Given that the early cpufeature infrastructure has borrowed quite a lot of code from the kaslr implementation, let's reimplement the matching of the "nokaslr" option with it. Signed-off-by: Marc Zyngier Acked-by: Catalin Marinas Acked-by: David Brazdil --- arch/arm64/kernel/idreg-override.c | 15 + arch/arm64/kernel/kaslr.c | 36 ++ 2 files changed, 17 insertions(+), 34 deletions(-) diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index cbb8eaa48742..3ccf51b84ba4 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = { }, }; +extern struct arm64_ftr_override kaslr_feature_override; + +static const struct ftr_set_desc kaslr __initdata = { + .name = "kaslr", +#ifdef CONFIG_RANDOMIZE_BASE + .override = &kaslr_feature_override, +#endif + .fields = { + { "disabled", 0 }, + {} + }, +}; + static const struct ftr_set_desc * const regs[] __initdata = { &mmfr1, + &kaslr, }; static const struct { @@ -41,6 +55,7 @@ static const struct { } aliases[] __initdata = { { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, + { "nokaslr","kaslr.disabled=1" }, }; static char *cmdline_contains_option(const char *cmdline, const char *option) diff --git a/arch/arm64/kernel/kaslr.c b/arch/arm64/kernel/kaslr.c index 5fc86e7d01a1..27f8939deb1b 100644 --- a/arch/arm64/kernel/kaslr.c +++ b/arch/arm64/kernel/kaslr.c @@ -51,39 +51,7 @@ static __init u64 get_kaslr_seed(void *fdt) return ret; } -static __init bool cmdline_contains_nokaslr(const u8 *cmdline) -{ - const u8 *str; - - str = strstr(cmdline, "nokaslr"); - return str == cmdline || (str > cmdline && *(str - 1) == ' '); -} - -static __init bool is_kaslr_disabled_cmdline(void *fdt) -{ - if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) { - int node; - const u8 *prop; - - node = fdt_path_offset(fdt, "/chosen"); - if (node < 0) - goto out; - - prop = fdt_getprop(fdt, node, "bootargs", NULL); - if (!prop) - goto out; - - if (cmdline_contains_nokaslr(prop)) - return true; - - if (IS_ENABLED(CONFIG_CMDLINE_EXTEND)) - goto out; - - return false; - } -out: - return cmdline_contains_nokaslr(CONFIG_CMDLINE); -} +struct arm64_ftr_override kaslr_feature_override __initdata; /* * This routine will be executed with the kernel mapped at its default virtual @@ -126,7 +94,7 @@ u64 __init kaslr_early_init(void) * Check if 'nokaslr' appears on the command line, and * return 0 if that is the case. */ - if (is_kaslr_disabled_cmdline(fdt)) { + if (kaslr_feature_override.val & kaslr_feature_override.mask & 0xf) { kaslr_status = KASLR_DISABLED_CMDLINE; return 0; } -- 2.29.2 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm