| Message ID | 20250130072100.27297-2-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix page permission on arm64 architectures | expand |
Hi Ilias, On 1/30/25 08:20, Ilias Apalodimas wrote: > Upcoming patches are mapping memory with RO, RW^X etc permsissions. > Fix the meminfo command to display them properly > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/arm/cpu/armv8/cache_v8.c | 18 +++++++++++++++++- > arch/arm/include/asm/armv8/mmu.h | 2 ++ > cmd/meminfo.c | 5 +++++ > 3 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > index 5d6953ffedd1..a4ca56c8ed42 100644 > --- a/arch/arm/cpu/armv8/cache_v8.c > +++ b/arch/arm/cpu/armv8/cache_v8.c > @@ -421,7 +421,7 @@ static int count_ranges(void) > return count; > } > > -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK) > +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK) > #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3) > > enum walker_state { > @@ -568,6 +568,17 @@ static void pretty_print_table_attrs(u64 pte) > static void pretty_print_block_attrs(u64 pte) > { > u64 attrs = pte & PMD_ATTRINDX_MASK; > + u64 perm_attrs = pte & PMD_ATTRMASK; > + > + if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN) && > + perm_attrs & PTE_BLOCK_RO) > + printf("%-5s", " | RO"); > + else if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN)) > + printf("%-5s", " | RW"); > + else if (perm_attrs & PTE_BLOCK_RO) > + printf("%-5s", " | RX"); > + else > + printf("%-5s", " | RWX"); We could also print out the unprivileged and privileged permissions explicitly, for example rwxRWX or rw-RW- etc. but maybe the format you're proposing is more to the point since it is not detailed debugging info that we're after. > > switch (attrs) { > case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE): > @@ -1112,3 +1123,8 @@ void __weak enable_caches(void) > icache_enable(); > dcache_enable(); > } > + > +void arch_dump_mem_attrs(void) > +{ > + dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL)); > +} > diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h > index 0ab681c893d3..6af8cd111a44 100644 > --- a/arch/arm/include/asm/armv8/mmu.h > +++ b/arch/arm/include/asm/armv8/mmu.h > @@ -66,6 +66,7 @@ > #define PTE_BLOCK_NG (1 << 11) > #define PTE_BLOCK_PXN (UL(1) << 53) > #define PTE_BLOCK_UXN (UL(1) << 54) > +#define PTE_BLOCK_RO (UL(1) << 7) > > /* > * AttrIndx[2:0] > @@ -75,6 +76,7 @@ > #define PMD_ATTRMASK (PTE_BLOCK_PXN | \ > PTE_BLOCK_UXN | \ > PMD_ATTRINDX_MASK | \ > + PTE_BLOCK_RO | \ > PTE_TYPE_VALID) > > /* > diff --git a/cmd/meminfo.c b/cmd/meminfo.c > index 5e83d61c2dd3..3915e2bbb268 100644 > --- a/cmd/meminfo.c > +++ b/cmd/meminfo.c > @@ -15,6 +15,10 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +void __weak arch_dump_mem_attrs(void) > +{ > +} > + > static void print_region(const char *name, ulong base, ulong size, ulong *uptop) > { > ulong end = base + size; > @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc, > > puts("DRAM: "); > print_size(gd->ram_size, "\n"); > + arch_dump_mem_attrs(); > > if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP)) > return 0; Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Hi Jerome, On Thu, 30 Jan 2025 at 11:09, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > Hi Ilias, > > On 1/30/25 08:20, Ilias Apalodimas wrote: > > Upcoming patches are mapping memory with RO, RW^X etc permsissions. > > Fix the meminfo command to display them properly > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > arch/arm/cpu/armv8/cache_v8.c | 18 +++++++++++++++++- > > arch/arm/include/asm/armv8/mmu.h | 2 ++ > > cmd/meminfo.c | 5 +++++ > > 3 files changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > > index 5d6953ffedd1..a4ca56c8ed42 100644 > > --- a/arch/arm/cpu/armv8/cache_v8.c > > +++ b/arch/arm/cpu/armv8/cache_v8.c > > @@ -421,7 +421,7 @@ static int count_ranges(void) > > return count; > > } > > > > -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK) > > +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK) > > #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3) > > > > enum walker_state { > > @@ -568,6 +568,17 @@ static void pretty_print_table_attrs(u64 pte) > > static void pretty_print_block_attrs(u64 pte) > > { > > u64 attrs = pte & PMD_ATTRINDX_MASK; > > + u64 perm_attrs = pte & PMD_ATTRMASK; > > + > > + if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN) && > > + perm_attrs & PTE_BLOCK_RO) > > + printf("%-5s", " | RO"); > > + else if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN)) > > + printf("%-5s", " | RW"); > > + else if (perm_attrs & PTE_BLOCK_RO) > > + printf("%-5s", " | RX"); > > + else > > + printf("%-5s", " | RWX"); > > We could also print out the unprivileged and privileged permissions > explicitly, for example rwxRWX or rw-RW- etc. but maybe the format > you're proposing is more to the point since it is not detailed > debugging info that we're after. In a previous version I was printing PXN/UXN etc. But I thought it would be easier to read with RO/RW/RX. Cheers /Ilias > > > > > switch (attrs) { > > case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE): > > @@ -1112,3 +1123,8 @@ void __weak enable_caches(void) > > icache_enable(); > > dcache_enable(); > > } > > + > > +void arch_dump_mem_attrs(void) > > +{ > > + dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL)); > > +} > > diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h > > index 0ab681c893d3..6af8cd111a44 100644 > > --- a/arch/arm/include/asm/armv8/mmu.h > > +++ b/arch/arm/include/asm/armv8/mmu.h > > @@ -66,6 +66,7 @@ > > #define PTE_BLOCK_NG (1 << 11) > > #define PTE_BLOCK_PXN (UL(1) << 53) > > #define PTE_BLOCK_UXN (UL(1) << 54) > > +#define PTE_BLOCK_RO (UL(1) << 7) > > > > /* > > * AttrIndx[2:0] > > @@ -75,6 +76,7 @@ > > #define PMD_ATTRMASK (PTE_BLOCK_PXN | \ > > PTE_BLOCK_UXN | \ > > PMD_ATTRINDX_MASK | \ > > + PTE_BLOCK_RO | \ > > PTE_TYPE_VALID) > > > > /* > > diff --git a/cmd/meminfo.c b/cmd/meminfo.c > > index 5e83d61c2dd3..3915e2bbb268 100644 > > --- a/cmd/meminfo.c > > +++ b/cmd/meminfo.c > > @@ -15,6 +15,10 @@ > > > > DECLARE_GLOBAL_DATA_PTR; > > > > +void __weak arch_dump_mem_attrs(void) > > +{ > > +} > > + > > static void print_region(const char *name, ulong base, ulong size, ulong *uptop) > > { > > ulong end = base + size; > > @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc, > > > > puts("DRAM: "); > > print_size(gd->ram_size, "\n"); > > + arch_dump_mem_attrs(); > > > > if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP)) > > return 0; > > Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Thanks! If we can sort out the slight misalignment, this is one of the patches that can go in without the rest /Ilias
On 1/30/25 07:20, Ilias Apalodimas wrote: > Upcoming patches are mapping memory with RO, RW^X etc permsissions. > Fix the meminfo command to display them properly > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/arm/cpu/armv8/cache_v8.c | 18 +++++++++++++++++- > arch/arm/include/asm/armv8/mmu.h | 2 ++ > cmd/meminfo.c | 5 +++++ > 3 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > index 5d6953ffedd1..a4ca56c8ed42 100644 > --- a/arch/arm/cpu/armv8/cache_v8.c > +++ b/arch/arm/cpu/armv8/cache_v8.c > @@ -421,7 +421,7 @@ static int count_ranges(void) > return count; > } > > -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK) > +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK) > #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3) > > enum walker_state { > @@ -568,6 +568,17 @@ static void pretty_print_table_attrs(u64 pte) > static void pretty_print_block_attrs(u64 pte) > { > u64 attrs = pte & PMD_ATTRINDX_MASK; > + u64 perm_attrs = pte & PMD_ATTRMASK; The variable perm_attrs is not needed. You could use pte directly. > + > + if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN) && > + perm_attrs & PTE_BLOCK_RO) > + printf("%-5s", " | RO"); > + else if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN)) > + printf("%-5s", " | RW"); > + else if (perm_attrs & PTE_BLOCK_RO) > + printf("%-5s", " | RX"); > + else > + printf("%-5s", " | RWX"); The string has 6 characters. "%-5s" does not match! As the string length is known using a length identifier is superfluous. It would be helpful to add descriptions to the flags in arch/arm/include/asm/armv8/mmu.h. https://documentation-service.arm.com/static/5efa1d23dbdee951c1ccdec5 describes: "The Unprivileged Execute Never (UXN) and Privileged Execute Never (PXN) attributes can be set separately. This is used to prevent, for example, application code running with kernel privilege, or attempts to execute kernel code while in an unprivileged state." Shouldn't we show the values of UXN and PXN separately? E.g: printf("%s%s%s\", pte & PTE_BLOCK_RO ? " | RO " : "", pte & PTE_BLOCK_PXN ? " | PXN" : "", pte & PTE_BLOCK_UXN ? " | UXN" : ""); Best regards Heinrich > > switch (attrs) { > case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE): > @@ -1112,3 +1123,8 @@ void __weak enable_caches(void) > icache_enable(); > dcache_enable(); > } > + > +void arch_dump_mem_attrs(void) > +{ > + dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL)); > +} > diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h > index 0ab681c893d3..6af8cd111a44 100644 > --- a/arch/arm/include/asm/armv8/mmu.h > +++ b/arch/arm/include/asm/armv8/mmu.h > @@ -66,6 +66,7 @@ > #define PTE_BLOCK_NG (1 << 11) > #define PTE_BLOCK_PXN (UL(1) << 53) > #define PTE_BLOCK_UXN (UL(1) << 54) > +#define PTE_BLOCK_RO (UL(1) << 7) > > /* > * AttrIndx[2:0] > @@ -75,6 +76,7 @@ > #define PMD_ATTRMASK (PTE_BLOCK_PXN | \ > PTE_BLOCK_UXN | \ > PMD_ATTRINDX_MASK | \ > + PTE_BLOCK_RO | \ > PTE_TYPE_VALID) > > /* > diff --git a/cmd/meminfo.c b/cmd/meminfo.c > index 5e83d61c2dd3..3915e2bbb268 100644 > --- a/cmd/meminfo.c > +++ b/cmd/meminfo.c > @@ -15,6 +15,10 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +void __weak arch_dump_mem_attrs(void) > +{ > +} > + > static void print_region(const char *name, ulong base, ulong size, ulong *uptop) > { > ulong end = base + size; > @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc, > > puts("DRAM: "); > print_size(gd->ram_size, "\n"); > + arch_dump_mem_attrs(); > > if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP)) > return 0;
HI Heinrich On Thu, 30 Jan 2025 at 12:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 1/30/25 07:20, Ilias Apalodimas wrote: > > Upcoming patches are mapping memory with RO, RW^X etc permsissions. > > Fix the meminfo command to display them properly > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > arch/arm/cpu/armv8/cache_v8.c | 18 +++++++++++++++++- > > arch/arm/include/asm/armv8/mmu.h | 2 ++ > > cmd/meminfo.c | 5 +++++ > > 3 files changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > > index 5d6953ffedd1..a4ca56c8ed42 100644 > > --- a/arch/arm/cpu/armv8/cache_v8.c > > +++ b/arch/arm/cpu/armv8/cache_v8.c > > @@ -421,7 +421,7 @@ static int count_ranges(void) > > return count; > > } > > > > -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK) > > +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK) > > #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3) > > > > enum walker_state { > > @@ -568,6 +568,17 @@ static void pretty_print_table_attrs(u64 pte) > > static void pretty_print_block_attrs(u64 pte) > > { > > u64 attrs = pte & PMD_ATTRINDX_MASK; > > + u64 perm_attrs = pte & PMD_ATTRMASK; > > The variable perm_attrs is not needed. You could use pte directly. > > > + > > + if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN) && > > + perm_attrs & PTE_BLOCK_RO) > > + printf("%-5s", " | RO"); > > + else if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN)) > > + printf("%-5s", " | RW"); > > + else if (perm_attrs & PTE_BLOCK_RO) > > + printf("%-5s", " | RX"); > > + else > > + printf("%-5s", " | RWX"); > > The string has 6 characters. "%-5s" does not match! Ah thanks! That's the misalignment i was chasing > As the string length is known using a length identifier is superfluous. > > It would be helpful to add descriptions to the flags in > arch/arm/include/asm/armv8/mmu.h. > > https://documentation-service.arm.com/static/5efa1d23dbdee951c1ccdec5 > describes: > > "The Unprivileged Execute Never (UXN) and Privileged Execute Never (PXN) > attributes can be set separately. This is used to prevent, for example, > application code running with kernel privilege, or attempts to execute > kernel code while in an unprivileged state." > > Shouldn't we show the values of UXN and PXN separately? I had that on my initial patches. But then I decided it's too Arm specific to print it. I was hoping more archs would fix that so I preferred RO, RW, RX since it's arch agnosticI don't mind re-adding it. Thanks for taking a look! Cheers /Ilias > > E.g: > > printf("%s%s%s\", > pte & PTE_BLOCK_RO ? " | RO " : "", > pte & PTE_BLOCK_PXN ? " | PXN" : "", > pte & PTE_BLOCK_UXN ? " | UXN" : ""); > > Best regards > > Heinrich > > > > > switch (attrs) { > > case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE): > > @@ -1112,3 +1123,8 @@ void __weak enable_caches(void) > > icache_enable(); > > dcache_enable(); > > } > > + > > +void arch_dump_mem_attrs(void) > > +{ > > + dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL)); > > +} > > diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h > > index 0ab681c893d3..6af8cd111a44 100644 > > --- a/arch/arm/include/asm/armv8/mmu.h > > +++ b/arch/arm/include/asm/armv8/mmu.h > > @@ -66,6 +66,7 @@ > > #define PTE_BLOCK_NG (1 << 11) > > #define PTE_BLOCK_PXN (UL(1) << 53) > > #define PTE_BLOCK_UXN (UL(1) << 54) > > +#define PTE_BLOCK_RO (UL(1) << 7) > > > > /* > > * AttrIndx[2:0] > > @@ -75,6 +76,7 @@ > > #define PMD_ATTRMASK (PTE_BLOCK_PXN | \ > > PTE_BLOCK_UXN | \ > > PMD_ATTRINDX_MASK | \ > > + PTE_BLOCK_RO | \ > > PTE_TYPE_VALID) > > > > /* > > diff --git a/cmd/meminfo.c b/cmd/meminfo.c > > index 5e83d61c2dd3..3915e2bbb268 100644 > > --- a/cmd/meminfo.c > > +++ b/cmd/meminfo.c > > @@ -15,6 +15,10 @@ > > > > DECLARE_GLOBAL_DATA_PTR; > > > > +void __weak arch_dump_mem_attrs(void) > > +{ > > +} > > + > > static void print_region(const char *name, ulong base, ulong size, ulong *uptop) > > { > > ulong end = base + size; > > @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc, > > > > puts("DRAM: "); > > print_size(gd->ram_size, "\n"); > > + arch_dump_mem_attrs(); > > > > if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP)) > > return 0; >
On 1/30/25 10:35, Ilias Apalodimas wrote: > HI Heinrich > > On Thu, 30 Jan 2025 at 12:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 1/30/25 07:20, Ilias Apalodimas wrote: >>> Upcoming patches are mapping memory with RO, RW^X etc permsissions. >>> Fix the meminfo command to display them properly >>> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >>> --- >>> arch/arm/cpu/armv8/cache_v8.c | 18 +++++++++++++++++- >>> arch/arm/include/asm/armv8/mmu.h | 2 ++ >>> cmd/meminfo.c | 5 +++++ >>> 3 files changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c >>> index 5d6953ffedd1..a4ca56c8ed42 100644 >>> --- a/arch/arm/cpu/armv8/cache_v8.c >>> +++ b/arch/arm/cpu/armv8/cache_v8.c >>> @@ -421,7 +421,7 @@ static int count_ranges(void) >>> return count; >>> } >>> >>> -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK) >>> +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK) >>> #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3) >>> >>> enum walker_state { >>> @@ -568,6 +568,17 @@ static void pretty_print_table_attrs(u64 pte) >>> static void pretty_print_block_attrs(u64 pte) >>> { >>> u64 attrs = pte & PMD_ATTRINDX_MASK; >>> + u64 perm_attrs = pte & PMD_ATTRMASK; >> >> The variable perm_attrs is not needed. You could use pte directly. >> >>> + >>> + if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN) && >>> + perm_attrs & PTE_BLOCK_RO) >>> + printf("%-5s", " | RO"); >>> + else if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN)) >>> + printf("%-5s", " | RW"); >>> + else if (perm_attrs & PTE_BLOCK_RO) >>> + printf("%-5s", " | RX"); >>> + else >>> + printf("%-5s", " | RWX"); >> >> The string has 6 characters. "%-5s" does not match! > > Ah thanks! That's the misalignment i was chasing > >> As the string length is known using a length identifier is superfluous. >> >> It would be helpful to add descriptions to the flags in >> arch/arm/include/asm/armv8/mmu.h. >> >> https://documentation-service.arm.com/static/5efa1d23dbdee951c1ccdec5 >> describes: >> >> "The Unprivileged Execute Never (UXN) and Privileged Execute Never (PXN) >> attributes can be set separately. This is used to prevent, for example, >> application code running with kernel privilege, or attempts to execute >> kernel code while in an unprivileged state." >> >> Shouldn't we show the values of UXN and PXN separately? > > I had that on my initial patches. But then I decided it's too Arm > specific to print it. > I was hoping more archs would fix that so I preferred RO, RW, RX since > it's arch agnosticI don't mind re-adding it. This code in arch/arm/cpu/armv8/cache_v8.c is architecture specific. RISC-V has flags R, W, X, U where U controls access by user mode. When printing the flags I guess we should use an architecture specific notation. Best regards Heinrich > > Thanks for taking a look! > > Cheers > /Ilias >> >> E.g: >> >> printf("%s%s%s\", >> pte & PTE_BLOCK_RO ? " | RO " : "", >> pte & PTE_BLOCK_PXN ? " | PXN" : "", >> pte & PTE_BLOCK_UXN ? " | UXN" : ""); >> >> Best regards >> >> Heinrich >> >>> >>> switch (attrs) { >>> case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE): >>> @@ -1112,3 +1123,8 @@ void __weak enable_caches(void) >>> icache_enable(); >>> dcache_enable(); >>> } >>> + >>> +void arch_dump_mem_attrs(void) >>> +{ >>> + dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL)); >>> +} >>> diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h >>> index 0ab681c893d3..6af8cd111a44 100644 >>> --- a/arch/arm/include/asm/armv8/mmu.h >>> +++ b/arch/arm/include/asm/armv8/mmu.h >>> @@ -66,6 +66,7 @@ >>> #define PTE_BLOCK_NG (1 << 11) >>> #define PTE_BLOCK_PXN (UL(1) << 53) >>> #define PTE_BLOCK_UXN (UL(1) << 54) >>> +#define PTE_BLOCK_RO (UL(1) << 7) >>> >>> /* >>> * AttrIndx[2:0] >>> @@ -75,6 +76,7 @@ >>> #define PMD_ATTRMASK (PTE_BLOCK_PXN | \ >>> PTE_BLOCK_UXN | \ >>> PMD_ATTRINDX_MASK | \ >>> + PTE_BLOCK_RO | \ >>> PTE_TYPE_VALID) >>> >>> /* >>> diff --git a/cmd/meminfo.c b/cmd/meminfo.c >>> index 5e83d61c2dd3..3915e2bbb268 100644 >>> --- a/cmd/meminfo.c >>> +++ b/cmd/meminfo.c >>> @@ -15,6 +15,10 @@ >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> +void __weak arch_dump_mem_attrs(void) >>> +{ >>> +} >>> + >>> static void print_region(const char *name, ulong base, ulong size, ulong *uptop) >>> { >>> ulong end = base + size; >>> @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc, >>> >>> puts("DRAM: "); >>> print_size(gd->ram_size, "\n"); >>> + arch_dump_mem_attrs(); >>> >>> if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP)) >>> return 0; >>
On Thu, Jan 30, 2025 at 02:36:40PM +0100, Heinrich Schuchardt wrote: > On 1/30/25 10:35, Ilias Apalodimas wrote: > > HI Heinrich > > > > On Thu, 30 Jan 2025 at 12:01, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 1/30/25 07:20, Ilias Apalodimas wrote: > > > > Upcoming patches are mapping memory with RO, RW^X etc permsissions. > > > > Fix the meminfo command to display them properly > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > --- > > > > arch/arm/cpu/armv8/cache_v8.c | 18 +++++++++++++++++- > > > > arch/arm/include/asm/armv8/mmu.h | 2 ++ > > > > cmd/meminfo.c | 5 +++++ > > > > 3 files changed, 24 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > > > > index 5d6953ffedd1..a4ca56c8ed42 100644 > > > > --- a/arch/arm/cpu/armv8/cache_v8.c > > > > +++ b/arch/arm/cpu/armv8/cache_v8.c > > > > @@ -421,7 +421,7 @@ static int count_ranges(void) > > > > return count; > > > > } > > > > > > > > -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK) > > > > +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK) > > > > #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3) > > > > > > > > enum walker_state { > > > > @@ -568,6 +568,17 @@ static void pretty_print_table_attrs(u64 pte) > > > > static void pretty_print_block_attrs(u64 pte) > > > > { > > > > u64 attrs = pte & PMD_ATTRINDX_MASK; > > > > + u64 perm_attrs = pte & PMD_ATTRMASK; > > > > > > The variable perm_attrs is not needed. You could use pte directly. > > > > > > > + > > > > + if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN) && > > > > + perm_attrs & PTE_BLOCK_RO) > > > > + printf("%-5s", " | RO"); > > > > + else if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN)) > > > > + printf("%-5s", " | RW"); > > > > + else if (perm_attrs & PTE_BLOCK_RO) > > > > + printf("%-5s", " | RX"); > > > > + else > > > > + printf("%-5s", " | RWX"); > > > > > > The string has 6 characters. "%-5s" does not match! > > > > Ah thanks! That's the misalignment i was chasing > > > > > As the string length is known using a length identifier is superfluous. > > > > > > It would be helpful to add descriptions to the flags in > > > arch/arm/include/asm/armv8/mmu.h. > > > > > > https://documentation-service.arm.com/static/5efa1d23dbdee951c1ccdec5 > > > describes: > > > > > > "The Unprivileged Execute Never (UXN) and Privileged Execute Never (PXN) > > > attributes can be set separately. This is used to prevent, for example, > > > application code running with kernel privilege, or attempts to execute > > > kernel code while in an unprivileged state." > > > > > > Shouldn't we show the values of UXN and PXN separately? > > > > I had that on my initial patches. But then I decided it's too Arm > > specific to print it. > > I was hoping more archs would fix that so I preferred RO, RW, RX since > > it's arch agnosticI don't mind re-adding it. > > This code in arch/arm/cpu/armv8/cache_v8.c is architecture specific. > > RISC-V has flags R, W, X, U where U controls access by user mode. > > When printing the flags I guess we should use an architecture specific > notation. ... and update doc/usage/cmd/meminfo.rst to note that it's printing arch-specific flags here, perhaps with an arm64 subsection explaining any shorthand and links to appropriate documentation ?
diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c index 5d6953ffedd1..a4ca56c8ed42 100644 --- a/arch/arm/cpu/armv8/cache_v8.c +++ b/arch/arm/cpu/armv8/cache_v8.c @@ -421,7 +421,7 @@ static int count_ranges(void) return count; } -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK) +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK) #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3) enum walker_state { @@ -568,6 +568,17 @@ static void pretty_print_table_attrs(u64 pte) static void pretty_print_block_attrs(u64 pte) { u64 attrs = pte & PMD_ATTRINDX_MASK; + u64 perm_attrs = pte & PMD_ATTRMASK; + + if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN) && + perm_attrs & PTE_BLOCK_RO) + printf("%-5s", " | RO"); + else if ((perm_attrs & PTE_BLOCK_PXN) && (perm_attrs & PTE_BLOCK_UXN)) + printf("%-5s", " | RW"); + else if (perm_attrs & PTE_BLOCK_RO) + printf("%-5s", " | RX"); + else + printf("%-5s", " | RWX"); switch (attrs) { case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE): @@ -1112,3 +1123,8 @@ void __weak enable_caches(void) icache_enable(); dcache_enable(); } + +void arch_dump_mem_attrs(void) +{ + dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL)); +} diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h index 0ab681c893d3..6af8cd111a44 100644 --- a/arch/arm/include/asm/armv8/mmu.h +++ b/arch/arm/include/asm/armv8/mmu.h @@ -66,6 +66,7 @@ #define PTE_BLOCK_NG (1 << 11) #define PTE_BLOCK_PXN (UL(1) << 53) #define PTE_BLOCK_UXN (UL(1) << 54) +#define PTE_BLOCK_RO (UL(1) << 7) /* * AttrIndx[2:0] @@ -75,6 +76,7 @@ #define PMD_ATTRMASK (PTE_BLOCK_PXN | \ PTE_BLOCK_UXN | \ PMD_ATTRINDX_MASK | \ + PTE_BLOCK_RO | \ PTE_TYPE_VALID) /* diff --git a/cmd/meminfo.c b/cmd/meminfo.c index 5e83d61c2dd3..3915e2bbb268 100644 --- a/cmd/meminfo.c +++ b/cmd/meminfo.c @@ -15,6 +15,10 @@ DECLARE_GLOBAL_DATA_PTR; +void __weak arch_dump_mem_attrs(void) +{ +} + static void print_region(const char *name, ulong base, ulong size, ulong *uptop) { ulong end = base + size; @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc, puts("DRAM: "); print_size(gd->ram_size, "\n"); + arch_dump_mem_attrs(); if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP)) return 0;
Upcoming patches are mapping memory with RO, RW^X etc permsissions. Fix the meminfo command to display them properly Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- arch/arm/cpu/armv8/cache_v8.c | 18 +++++++++++++++++- arch/arm/include/asm/armv8/mmu.h | 2 ++ cmd/meminfo.c | 5 +++++ 3 files changed, 24 insertions(+), 1 deletion(-)