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

[Heinrich Schuchardt]

On 1/30/25 10:35, Ilias Apalodimas wrote:
> 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.

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;
>>