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.g...@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.apalodi...@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 ? -- Tom
signature.asc
Description: PGP signature
