[RFC PATCH 1/4] meminfo: add memory details for armv8

[Ilias Apalodimas]

HI Heinrich

On Thu, 30 Jan 2025 at 12:01, Heinrich Schuchardt <xypron.glpk at 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 at 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;
>