| Message ID | 20220901151403.1735836-6-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: alternatives: improvements | expand |
Hi Mark, On Thu, 1 Sept 2022 at 17:14, Mark Rutland <mark.rutland@arm.com> wrote: > > We never alter a struct alt_region after creation, and we open-code the > bounds of the kernel alternatives region in two functions. > > This patch adds a shared struct alt_region, and marks the alt regions as > const to prevent unintentional modification. > What is the point of struct alt_region? Can we just get rid of it entirely? It seems its only purpose is to carry a <start, end> tuple that could easily be converted into start and end arguments to __apply_alternatives(). > There should be no functional change as a result of this patch. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: James Morse <james.morse@arm.com> > Cc: Joey Gouly <joey.gouly@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Will Deacon <will@kernel.org> > --- > arch/arm64/kernel/alternative.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c > index d94d97cb4a0bf..2e18c9c0f612b 100644 > --- a/arch/arm64/kernel/alternative.c > +++ b/arch/arm64/kernel/alternative.c > @@ -133,8 +133,9 @@ static void clean_dcache_range_nopatch(u64 start, u64 end) > } while (cur += d_size, cur < end); > } > > -static void __nocfi __apply_alternatives(struct alt_region *region, bool is_module, > - unsigned long *feature_mask) > +static void __nocfi __apply_alternatives(const struct alt_region *region, > + bool is_module, > + unsigned long *feature_mask) > { > struct alt_instr *alt; > __le32 *origptr, *updptr; > @@ -190,17 +191,17 @@ static void __nocfi __apply_alternatives(struct alt_region *region, bool is_modu > } > } > > +static const struct alt_region kernel_alternatives = { > + .begin = (struct alt_instr *)__alt_instructions, > + .end = (struct alt_instr *)__alt_instructions_end, > +}; > + > /* > * We might be patching the stop_machine state machine, so implement a > * really simple polling protocol here. > */ > static int __apply_alternatives_multi_stop(void *unused) > { > - struct alt_region region = { > - .begin = (struct alt_instr *)__alt_instructions, > - .end = (struct alt_instr *)__alt_instructions_end, > - }; > - > /* We always have a CPU 0 at this point (__init) */ > if (smp_processor_id()) { > while (!all_alternatives_applied) > @@ -213,7 +214,8 @@ static int __apply_alternatives_multi_stop(void *unused) > ARM64_NPATCHABLE); > > BUG_ON(all_alternatives_applied); > - __apply_alternatives(®ion, false, remaining_capabilities); > + __apply_alternatives(&kernel_alternatives, false, > + remaining_capabilities); > /* Barriers provided by the cache flushing */ > all_alternatives_applied = 1; > } > @@ -236,17 +238,13 @@ void __init apply_alternatives_all(void) > */ > void __init apply_boot_alternatives(void) > { > - struct alt_region region = { > - .begin = (struct alt_instr *)__alt_instructions, > - .end = (struct alt_instr *)__alt_instructions_end, > - }; > - > /* If called on non-boot cpu things could go wrong */ > WARN_ON(smp_processor_id() != 0); > > pr_info("applying boot alternatives\n"); > > - __apply_alternatives(®ion, false, &boot_capabilities[0]); > + __apply_alternatives(&kernel_alternatives, false, > + &boot_capabilities[0]); > } > > #ifdef CONFIG_MODULES > -- > 2.30.2 >
On Tue, Sep 06, 2022 at 05:18:54PM +0200, Ard Biesheuvel wrote: > Hi Mark, > > On Thu, 1 Sept 2022 at 17:14, Mark Rutland <mark.rutland@arm.com> wrote: > > > > We never alter a struct alt_region after creation, and we open-code the > > bounds of the kernel alternatives region in two functions. > > > > This patch adds a shared struct alt_region, and marks the alt regions as > > const to prevent unintentional modification. > > > > What is the point of struct alt_region? Can we just get rid of it > entirely? It seems its only purpose is to carry a <start, end> tuple > that could easily be converted into start and end arguments to > __apply_alternatives(). We could right now, but I'm intending to fix some noinstr issues (and make debugging easier) by splitting the alternatives sanity-checking & patching into distinct initcalls (alnog with some extra debug), and having the structure for the common definition is quite nice to avoid open-coding the start and end value in a bunch of places. So I'd prefer to keep it for now, but I can follow up and delete it if the above doesn't turn out to need it, if that's ok? Mark. > > There should be no functional change as a result of this patch. > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: James Morse <james.morse@arm.com> > > Cc: Joey Gouly <joey.gouly@arm.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: Will Deacon <will@kernel.org> > > --- > > arch/arm64/kernel/alternative.c | 26 ++++++++++++-------------- > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c > > index d94d97cb4a0bf..2e18c9c0f612b 100644 > > --- a/arch/arm64/kernel/alternative.c > > +++ b/arch/arm64/kernel/alternative.c > > @@ -133,8 +133,9 @@ static void clean_dcache_range_nopatch(u64 start, u64 end) > > } while (cur += d_size, cur < end); > > } > > > > -static void __nocfi __apply_alternatives(struct alt_region *region, bool is_module, > > - unsigned long *feature_mask) > > +static void __nocfi __apply_alternatives(const struct alt_region *region, > > + bool is_module, > > + unsigned long *feature_mask) > > { > > struct alt_instr *alt; > > __le32 *origptr, *updptr; > > @@ -190,17 +191,17 @@ static void __nocfi __apply_alternatives(struct alt_region *region, bool is_modu > > } > > } > > > > +static const struct alt_region kernel_alternatives = { > > + .begin = (struct alt_instr *)__alt_instructions, > > + .end = (struct alt_instr *)__alt_instructions_end, > > +}; > > + > > /* > > * We might be patching the stop_machine state machine, so implement a > > * really simple polling protocol here. > > */ > > static int __apply_alternatives_multi_stop(void *unused) > > { > > - struct alt_region region = { > > - .begin = (struct alt_instr *)__alt_instructions, > > - .end = (struct alt_instr *)__alt_instructions_end, > > - }; > > - > > /* We always have a CPU 0 at this point (__init) */ > > if (smp_processor_id()) { > > while (!all_alternatives_applied) > > @@ -213,7 +214,8 @@ static int __apply_alternatives_multi_stop(void *unused) > > ARM64_NPATCHABLE); > > > > BUG_ON(all_alternatives_applied); > > - __apply_alternatives(®ion, false, remaining_capabilities); > > + __apply_alternatives(&kernel_alternatives, false, > > + remaining_capabilities); > > /* Barriers provided by the cache flushing */ > > all_alternatives_applied = 1; > > } > > @@ -236,17 +238,13 @@ void __init apply_alternatives_all(void) > > */ > > void __init apply_boot_alternatives(void) > > { > > - struct alt_region region = { > > - .begin = (struct alt_instr *)__alt_instructions, > > - .end = (struct alt_instr *)__alt_instructions_end, > > - }; > > - > > /* If called on non-boot cpu things could go wrong */ > > WARN_ON(smp_processor_id() != 0); > > > > pr_info("applying boot alternatives\n"); > > > > - __apply_alternatives(®ion, false, &boot_capabilities[0]); > > + __apply_alternatives(&kernel_alternatives, false, > > + &boot_capabilities[0]); > > } > > > > #ifdef CONFIG_MODULES > > -- > > 2.30.2 > >
On Mon, 12 Sept 2022 at 10:32, Mark Rutland <mark.rutland@arm.com> wrote: > > On Tue, Sep 06, 2022 at 05:18:54PM +0200, Ard Biesheuvel wrote: > > Hi Mark, > > > > On Thu, 1 Sept 2022 at 17:14, Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > We never alter a struct alt_region after creation, and we open-code the > > > bounds of the kernel alternatives region in two functions. > > > > > > This patch adds a shared struct alt_region, and marks the alt regions as > > > const to prevent unintentional modification. > > > > > > > What is the point of struct alt_region? Can we just get rid of it > > entirely? It seems its only purpose is to carry a <start, end> tuple > > that could easily be converted into start and end arguments to > > __apply_alternatives(). > > We could right now, but I'm intending to fix some noinstr issues (and make > debugging easier) by splitting the alternatives sanity-checking & patching into > distinct initcalls (alnog with some extra debug), and having the structure for > the common definition is quite nice to avoid open-coding the start and end > value in a bunch of places. > > So I'd prefer to keep it for now, but I can follow up and delete it if the > above doesn't turn out to need it, if that's ok? > Yeah, that's fine. The above wasn't really clear to me from the context you provided.
On Mon, Sep 12, 2022 at 11:13:12AM +0100, Ard Biesheuvel wrote: > On Mon, 12 Sept 2022 at 10:32, Mark Rutland <mark.rutland@arm.com> wrote: > > > > On Tue, Sep 06, 2022 at 05:18:54PM +0200, Ard Biesheuvel wrote: > > > Hi Mark, > > > > > > On Thu, 1 Sept 2022 at 17:14, Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > > > We never alter a struct alt_region after creation, and we open-code the > > > > bounds of the kernel alternatives region in two functions. > > > > > > > > This patch adds a shared struct alt_region, and marks the alt regions as > > > > const to prevent unintentional modification. > > > > > > > > > > What is the point of struct alt_region? Can we just get rid of it > > > entirely? It seems its only purpose is to carry a <start, end> tuple > > > that could easily be converted into start and end arguments to > > > __apply_alternatives(). > > > > We could right now, but I'm intending to fix some noinstr issues (and make > > debugging easier) by splitting the alternatives sanity-checking & patching into > > distinct initcalls (alnog with some extra debug), and having the structure for > > the common definition is quite nice to avoid open-coding the start and end > > value in a bunch of places. > > > > So I'd prefer to keep it for now, but I can follow up and delete it if the > > above doesn't turn out to need it, if that's ok? > > > > Yeah, that's fine. The above wasn't really clear to me from the > context you provided. True; I'll add a note to the commit message. Thanks, Mark.
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c index d94d97cb4a0bf..2e18c9c0f612b 100644 --- a/arch/arm64/kernel/alternative.c +++ b/arch/arm64/kernel/alternative.c @@ -133,8 +133,9 @@ static void clean_dcache_range_nopatch(u64 start, u64 end) } while (cur += d_size, cur < end); } -static void __nocfi __apply_alternatives(struct alt_region *region, bool is_module, - unsigned long *feature_mask) +static void __nocfi __apply_alternatives(const struct alt_region *region, + bool is_module, + unsigned long *feature_mask) { struct alt_instr *alt; __le32 *origptr, *updptr; @@ -190,17 +191,17 @@ static void __nocfi __apply_alternatives(struct alt_region *region, bool is_modu } } +static const struct alt_region kernel_alternatives = { + .begin = (struct alt_instr *)__alt_instructions, + .end = (struct alt_instr *)__alt_instructions_end, +}; + /* * We might be patching the stop_machine state machine, so implement a * really simple polling protocol here. */ static int __apply_alternatives_multi_stop(void *unused) { - struct alt_region region = { - .begin = (struct alt_instr *)__alt_instructions, - .end = (struct alt_instr *)__alt_instructions_end, - }; - /* We always have a CPU 0 at this point (__init) */ if (smp_processor_id()) { while (!all_alternatives_applied) @@ -213,7 +214,8 @@ static int __apply_alternatives_multi_stop(void *unused) ARM64_NPATCHABLE); BUG_ON(all_alternatives_applied); - __apply_alternatives(®ion, false, remaining_capabilities); + __apply_alternatives(&kernel_alternatives, false, + remaining_capabilities); /* Barriers provided by the cache flushing */ all_alternatives_applied = 1; } @@ -236,17 +238,13 @@ void __init apply_alternatives_all(void) */ void __init apply_boot_alternatives(void) { - struct alt_region region = { - .begin = (struct alt_instr *)__alt_instructions, - .end = (struct alt_instr *)__alt_instructions_end, - }; - /* If called on non-boot cpu things could go wrong */ WARN_ON(smp_processor_id() != 0); pr_info("applying boot alternatives\n"); - __apply_alternatives(®ion, false, &boot_capabilities[0]); + __apply_alternatives(&kernel_alternatives, false, + &boot_capabilities[0]); } #ifdef CONFIG_MODULES
We never alter a struct alt_region after creation, and we open-code the bounds of the kernel alternatives region in two functions. This patch adds a shared struct alt_region, and marks the alt regions as const to prevent unintentional modification. There should be no functional change as a result of this patch. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Joey Gouly <joey.gouly@arm.com> Cc: Marc Zyngier <maz@kernel.org> Cc: Will Deacon <will@kernel.org> --- arch/arm64/kernel/alternative.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)