| Message ID | 20241209072020.4743-2-shijie@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: refactor the rodata=xxx | expand |
On Mon, 9 Dec 2024, Huang Shijie wrote: > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -5901,7 +5901,7 @@ > rodata= [KNL,EARLY] > on Mark read-only kernel memory as read-only (default). > off Leave read-only kernel memory writable for debugging. > - full Mark read-only kernel memory and aliases as read-only > + noalias Use more block mappings,may have better performance. > [arm64] Maybe use noalias Do not check aliases in order to allow larger kernel page sizes on a platforms without FEAT_BBM2 support[arm64]) instead? > > rockchip.usb_uart > diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h > index ba269a7a3201..b3063326b79a 100644 > --- a/arch/arm64/include/asm/setup.h > +++ b/arch/arm64/include/asm/setup.h > @@ -13,6 +13,27 @@ > extern phys_addr_t __fdt_pointer __initdata; > extern u64 __cacheline_aligned boot_args[4]; > > +/* > + * rodata=on (default): > + * Apply read-only attributes of VM areas to the linear alias of > + * the backing pages as well. This prevents code or read-only data > + * from being modified (inadvertently or intentionally) via another > + * mapping of the same memory page. > + * > + * This requires the linear region to be mapped down to pages, > + * which may adversely affect performance in some cases. > + * > + * rodata=off: > + * It provides us more block mappings and contiguous hits > + * to map the linear region which minimize the TLB footprint. > + * Leave read-only kernel memory writable for debugging. > + * > + * rodata=noalias: > + * It provides us more block mappings and contiguous hits > + * to map the linear region which minimize the TLB footprint. > + * And the linear aliases of pages belonging to read-only mappings > + * in vmalloc region are also marked as read-only. > + */ > static inline bool arch_parse_debug_rodata(char *arg) > { > extern bool rodata_enabled; > @@ -21,7 +42,7 @@ static inline bool arch_parse_debug_rodata(char *arg) > if (!arg) > return false; > > - if (!strcmp(arg, "full")) { > + if (!strcmp(arg, "on")) { > rodata_enabled = rodata_full = true; > return true; > } > @@ -31,7 +52,7 @@ static inline bool arch_parse_debug_rodata(char *arg) > return true; > } > > - if (!strcmp(arg, "on")) { > + if (!strcmp(arg, "noalias")) { > rodata_enabled = true; > rodata_full = false; > return true; >
On 2024/12/10 1:05, Christoph Lameter (Ampere) wrote: >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -5901,7 +5901,7 @@ >> rodata= [KNL,EARLY] >> on Mark read-only kernel memory as read-only (default). >> off Leave read-only kernel memory writable for debugging. >> - full Mark read-only kernel memory and aliases as read-only >> + noalias Use more block mappings,may have better performance. >> [arm64] > Maybe use > noalias Do not check aliases in order to allow larger kernel page > sizes on a platforms without FEAT_BBM2 support[arm64]) > > instead? The "rodata=noalias" here is equal to original "rodata=on". But Yang Shi's patch set works with original "rodata=full" : https://lists.infradead.org/pipermail/linux-arm-kernel/2024-November/979770.html IMHO, it's not proper to mention the FEAT_BBM in "rodata=noalias". Thanks Huang Shijie
Hello Huang, On 12/9/24 12:50, Huang Shijie wrote: >>From Documentation/admin-guide/kernel-parameters.txt: > rodata= [KNL,EARLY] > on Mark read-only kernel memory as read-only (default). > > So "rodata=on" is the default. > > But the current code does not follow the document, it makes "rodata=full" > as the default. > > This patch refactors the "rodata=xxx" : > 1.) Make rodata=on behaviour be the original rodata=full. > (Drop the original rodata=full.) > 2.) Introduce "rodata=noalias" which is the original "rodata=on" > 3.) Add more comment for arch_parse_debug_rodata(), > update kernel-parameters.txt. > > After this patch, the rodata=on will be the default. Could this commit message be reworded/reformatted something like the following instead ? Although feel free to improve this as required. ----------------------------------------------------------------------- As per admin guide documentation, "rodata=on" should be the default on platforms. Documentation/admin-guide/kernel-parameters.txt describes these options as rodata= [KNL,EARLY] on Mark read-only kernel memory as read-only (default). off Leave read-only kernel memory writable for debugging. full Mark read-only kernel memory and aliases as read-only [arm64] But on arm64 platform, "rodata=full" is the default instead. This patch implements the following changes. - Make "rodata=on" behaviour same as the original "rodata=full" - Make "rodata=noalias" (new) behaviour same as the original "rodata=on" - Drop the original "rodata=full" - Add comment for arch_parse_debug_rodata() - Update kernel-parameters.txt as required After this patch, the "rodata=on" will be the default on arm64 platform as well. ----------------------------------------------------------------------- > > Signed-off-by: Huang Shijie <shijie@os.amperecomputing.com> > --- > .../admin-guide/kernel-parameters.txt | 2 +- > arch/arm64/include/asm/setup.h | 25 +++++++++++++++++-- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index a22b7e621007..51bce7b9d805 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -5901,7 +5901,7 @@ > rodata= [KNL,EARLY] > on Mark read-only kernel memory as read-only (default). > off Leave read-only kernel memory writable for debugging. > - full Mark read-only kernel memory and aliases as read-only > + noalias Use more block mappings,may have better performance. > [arm64] > > rockchip.usb_uart > diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h > index ba269a7a3201..b3063326b79a 100644 > --- a/arch/arm64/include/asm/setup.h > +++ b/arch/arm64/include/asm/setup.h > @@ -13,6 +13,27 @@ > extern phys_addr_t __fdt_pointer __initdata; > extern u64 __cacheline_aligned boot_args[4]; > > +/* > + * rodata=on (default): > + * Apply read-only attributes of VM areas to the linear alias of > + * the backing pages as well. This prevents code or read-only data > + * from being modified (inadvertently or intentionally) via another > + * mapping of the same memory page. > + * > + * This requires the linear region to be mapped down to pages, > + * which may adversely affect performance in some cases. > + * > + * rodata=off: > + * It provides us more block mappings and contiguous hits > + * to map the linear region which minimize the TLB footprint. > + * Leave read-only kernel memory writable for debugging. > + * > + * rodata=noalias: > + * It provides us more block mappings and contiguous hits > + * to map the linear region which minimize the TLB footprint. > + * And the linear aliases of pages belonging to read-only mappings > + * in vmalloc region are also marked as read-only. > + */ Reformatted and cleaned up the above comment a bit but feel free to improve it further. /* * rodata=on (default) * * This applies read-only attributes to VM areas and to the linear * alias of the backing pages as well. This prevents code or read- * only data from being modified (inadvertently or intentionally), * via another mapping for the same memory page. * * But this might cause linear map region to be mapped down to base * pages, which may adversely affect performance in some cases. * * rodata=off * * This provides more block mappings and contiguous hints for linear * map region which would minimize TLB footprint. This also leaves * read-only kernel memory writable for debugging. * * rodata=noalias * * This provides more block mappings and contiguous hints for linear * map region which would minimize TLB footprint. Linear aliases of * pages belonging to read-only mappings in vmalloc region are also * marked as read-only. > static inline bool arch_parse_debug_rodata(char *arg) > { > extern bool rodata_enabled; > @@ -21,7 +42,7 @@ static inline bool arch_parse_debug_rodata(char *arg) > if (!arg) > return false; > > - if (!strcmp(arg, "full")) { > + if (!strcmp(arg, "on")) { > rodata_enabled = rodata_full = true; > return true; > } > @@ -31,7 +52,7 @@ static inline bool arch_parse_debug_rodata(char *arg) > return true; > } > > - if (!strcmp(arg, "on")) { > + if (!strcmp(arg, "noalias")) { > rodata_enabled = true; > rodata_full = false; > return true;
On Tue, 10 Dec 2024 at 08:17, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > ... > > Reformatted and cleaned up the above comment a bit but feel free to > improve it further. > > /* > * rodata=on (default) > * > * This applies read-only attributes to VM areas and to the linear > * alias of the backing pages as well. This prevents code or read- > * only data from being modified (inadvertently or intentionally), > * via another mapping for the same memory page. > * > * But this might cause linear map region to be mapped down to base > * pages, which may adversely affect performance in some cases. > * > * rodata=off > * > * This provides more block mappings and contiguous hints for linear > * map region which would minimize TLB footprint. This also leaves > * read-only kernel memory writable for debugging. > * > * rodata=noalias > * > * This provides more block mappings and contiguous hints for linear > * map region which would minimize TLB footprint. Linear aliases of > * pages belonging to read-only mappings in vmalloc region are also > * marked as read-only. > If linear aliases are marked as read-only, how does 'noalias' differ from 'on'?
On 12/10/24 12:51, Ard Biesheuvel wrote: > On Tue, 10 Dec 2024 at 08:17, Anshuman Khandual > <anshuman.khandual@arm.com> wrote: >> > ... >> >> Reformatted and cleaned up the above comment a bit but feel free to >> improve it further. >> >> /* >> * rodata=on (default) >> * >> * This applies read-only attributes to VM areas and to the linear >> * alias of the backing pages as well. This prevents code or read- >> * only data from being modified (inadvertently or intentionally), >> * via another mapping for the same memory page. >> * >> * But this might cause linear map region to be mapped down to base >> * pages, which may adversely affect performance in some cases. >> * >> * rodata=off >> * >> * This provides more block mappings and contiguous hints for linear >> * map region which would minimize TLB footprint. This also leaves >> * read-only kernel memory writable for debugging. >> * >> * rodata=noalias >> * >> * This provides more block mappings and contiguous hints for linear >> * map region which would minimize TLB footprint. Linear aliases of >> * pages belonging to read-only mappings in vmalloc region are also >> * marked as read-only. >> > > If linear aliases are marked as read-only, how does 'noalias' differ from 'on'? Right, the last sentence can be can dropped.
On 2024/12/10 16:38, Anshuman Khandual wrote: > > On 12/10/24 12:51, Ard Biesheuvel wrote: >> On Tue, 10 Dec 2024 at 08:17, Anshuman Khandual >> <anshuman.khandual@arm.com> wrote: >> ... >>> Reformatted and cleaned up the above comment a bit but feel free to >>> improve it further. >>> >>> /* >>> * rodata=on (default) >>> * >>> * This applies read-only attributes to VM areas and to the linear >>> * alias of the backing pages as well. This prevents code or read- >>> * only data from being modified (inadvertently or intentionally), >>> * via another mapping for the same memory page. >>> * >>> * But this might cause linear map region to be mapped down to base >>> * pages, which may adversely affect performance in some cases. >>> * >>> * rodata=off >>> * >>> * This provides more block mappings and contiguous hints for linear >>> * map region which would minimize TLB footprint. This also leaves >>> * read-only kernel memory writable for debugging. >>> * >>> * rodata=noalias >>> * >>> * This provides more block mappings and contiguous hints for linear >>> * map region which would minimize TLB footprint. Linear aliases of >>> * pages belonging to read-only mappings in vmalloc region are also >>> * marked as read-only. >>> >> If linear aliases are marked as read-only, how does 'noalias' differ from 'on'? > Right, the last sentence can be can dropped. Okay, no problem. Thanks Huang Shijie
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index a22b7e621007..51bce7b9d805 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -5901,7 +5901,7 @@ rodata= [KNL,EARLY] on Mark read-only kernel memory as read-only (default). off Leave read-only kernel memory writable for debugging. - full Mark read-only kernel memory and aliases as read-only + noalias Use more block mappings,may have better performance. [arm64] rockchip.usb_uart diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h index ba269a7a3201..b3063326b79a 100644 --- a/arch/arm64/include/asm/setup.h +++ b/arch/arm64/include/asm/setup.h @@ -13,6 +13,27 @@ extern phys_addr_t __fdt_pointer __initdata; extern u64 __cacheline_aligned boot_args[4]; +/* + * rodata=on (default): + * Apply read-only attributes of VM areas to the linear alias of + * the backing pages as well. This prevents code or read-only data + * from being modified (inadvertently or intentionally) via another + * mapping of the same memory page. + * + * This requires the linear region to be mapped down to pages, + * which may adversely affect performance in some cases. + * + * rodata=off: + * It provides us more block mappings and contiguous hits + * to map the linear region which minimize the TLB footprint. + * Leave read-only kernel memory writable for debugging. + * + * rodata=noalias: + * It provides us more block mappings and contiguous hits + * to map the linear region which minimize the TLB footprint. + * And the linear aliases of pages belonging to read-only mappings + * in vmalloc region are also marked as read-only. + */ static inline bool arch_parse_debug_rodata(char *arg) { extern bool rodata_enabled; @@ -21,7 +42,7 @@ static inline bool arch_parse_debug_rodata(char *arg) if (!arg) return false; - if (!strcmp(arg, "full")) { + if (!strcmp(arg, "on")) { rodata_enabled = rodata_full = true; return true; } @@ -31,7 +52,7 @@ static inline bool arch_parse_debug_rodata(char *arg) return true; } - if (!strcmp(arg, "on")) { + if (!strcmp(arg, "noalias")) { rodata_enabled = true; rodata_full = false; return true;
From Documentation/admin-guide/kernel-parameters.txt: rodata= [KNL,EARLY] on Mark read-only kernel memory as read-only (default). So "rodata=on" is the default. But the current code does not follow the document, it makes "rodata=full" as the default. This patch refactors the "rodata=xxx" : 1.) Make rodata=on behaviour be the original rodata=full. (Drop the original rodata=full.) 2.) Introduce "rodata=noalias" which is the original "rodata=on" 3.) Add more comment for arch_parse_debug_rodata(), update kernel-parameters.txt. After this patch, the rodata=on will be the default. Signed-off-by: Huang Shijie <shijie@os.amperecomputing.com> --- .../admin-guide/kernel-parameters.txt | 2 +- arch/arm64/include/asm/setup.h | 25 +++++++++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-)