Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20230914014027.273002-1-peterlin@andestech.com>
 <51f5c170-2659-d76a-afbb-632b7a55586c@ghiti.fr> <ZQVXy6I42HJM4XLM@APC323>
In-Reply-To: <ZQVXy6I42HJM4XLM@APC323>
From:   Alexandre Ghiti <alexghiti@rivosinc.com>
Date:   Mon, 18 Sep 2023 15:03:31 +0200
Message-ID: <CAHVXubjQgDbqj1Pk09odLsZ235vHyAd-XjLcOXb4xrDOgSpa9g@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] riscv: Improve PTDUMP to show RSW with non-zero value
To:     Yu-Chien Peter Lin <peterlin@andestech.com>
Cc:     Alexandre Ghiti <alex@ghiti.fr>, paul.walmsley@sifive.com,
        palmer@dabbelt.com, aou@eecs.berkeley.edu, david@redhat.com,
        akpm@linux-foundation.org, bjorn@rivosinc.com,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        ycliang@andestech.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Peter Lin,

On Sat, Sep 16, 2023 at 9:23=E2=80=AFAM Yu-Chien Peter Lin
<peterlin@andestech.com> wrote:
>
> Hi Alexandre,
>
> On Fri, Sep 15, 2023 at 01:07:04PM +0200, Alexandre Ghiti wrote:
> > On 14/09/2023 03:40, Yu Chien Peter Lin wrote:
> > > RSW field can be used to encode 2 bits of software defined
> > > information, currently PTDUMP only prints RSW when its value
> > > is 1 or 3.
> > >
> > > To fix this issue and enhance the debug experience with PTDUMP,
> > > we use _PAGE_SOFT as the RSW mask and redefine _PAGE_SPECIAL to
> > > (1 << 8), allow it to print the RSW with any non-zero value,
> > > otherwise, it will print an empty string for each row.
> > >
> > > This patch also removes the val from the struct prot_bits as
> > > it is no longer needed.
> > >
> > > Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
> > > ---
> > >   arch/riscv/include/asm/pgtable-bits.h |  4 +--
> > >   arch/riscv/mm/ptdump.c                | 36 +++++++++++-------------=
---
> > >   2 files changed, 17 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/pgtable-bits.h b/arch/riscv/inclu=
de/asm/pgtable-bits.h
> > > index f896708e8331..99e60fd3eb72 100644
> > > --- a/arch/riscv/include/asm/pgtable-bits.h
> > > +++ b/arch/riscv/include/asm/pgtable-bits.h
> > > @@ -16,9 +16,9 @@
> > >   #define _PAGE_GLOBAL    (1 << 5)    /* Global */
> > >   #define _PAGE_ACCESSED  (1 << 6)    /* Set by hardware on any acces=
s */
> > >   #define _PAGE_DIRTY     (1 << 7)    /* Set by hardware on any write=
 */
> > > -#define _PAGE_SOFT      (1 << 8)    /* Reserved for software */
> > > +#define _PAGE_SOFT      (3 << 8)    /* Reserved for software */
> > > -#define _PAGE_SPECIAL   _PAGE_SOFT
> >
> >
> > That's nit, but maybe you could have introduced a _PAGE_SOFT_1 and
> > _PAGE_SOFT_2
> >
>
> Thanks for the suggestion, maybe we just add a comment here?
>
> #define _PAGE_SPECIAL   (1 << 8) /* RSW: 0x1 */
>
> > > +#define _PAGE_SPECIAL   (1 << 8)
> >
> >
> > instead of hardcoding (1<<8) here, but that can be done when we'll use =
the
> > second bit :)
> >
> > >   #define _PAGE_TABLE     _PAGE_PRESENT
> > >   /*
> > > diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> > > index 20a9f991a6d7..85686652f342 100644
> > > --- a/arch/riscv/mm/ptdump.c
> > > +++ b/arch/riscv/mm/ptdump.c
> > > @@ -129,7 +129,6 @@ static struct ptd_mm_info efi_ptd_info =3D {
> > >   /* Page Table Entry */
> > >   struct prot_bits {
> > >     u64 mask;
> > > -   u64 val;
> > >     const char *set;
> > >     const char *clear;
> > >   };
> > > @@ -137,47 +136,38 @@ struct prot_bits {
> > >   static const struct prot_bits pte_bits[] =3D {
> > >     {
> > >             .mask =3D _PAGE_SOFT,
> > > -           .val =3D _PAGE_SOFT,
> > > -           .set =3D "RSW",
> > > -           .clear =3D "   ",
> > > +           .set =3D "RSW(%d)",
> > > +           .clear =3D "      ",
> > >     }, {
> > >             .mask =3D _PAGE_DIRTY,
> > > -           .val =3D _PAGE_DIRTY,
> > >             .set =3D "D",
> > >             .clear =3D ".",
> > >     }, {
> > >             .mask =3D _PAGE_ACCESSED,
> > > -           .val =3D _PAGE_ACCESSED,
> > >             .set =3D "A",
> > >             .clear =3D ".",
> > >     }, {
> > >             .mask =3D _PAGE_GLOBAL,
> > > -           .val =3D _PAGE_GLOBAL,
> > >             .set =3D "G",
> > >             .clear =3D ".",
> > >     }, {
> > >             .mask =3D _PAGE_USER,
> > > -           .val =3D _PAGE_USER,
> > >             .set =3D "U",
> > >             .clear =3D ".",
> > >     }, {
> > >             .mask =3D _PAGE_EXEC,
> > > -           .val =3D _PAGE_EXEC,
> > >             .set =3D "X",
> > >             .clear =3D ".",
> > >     }, {
> > >             .mask =3D _PAGE_WRITE,
> > > -           .val =3D _PAGE_WRITE,
> > >             .set =3D "W",
> > >             .clear =3D ".",
> > >     }, {
> > >             .mask =3D _PAGE_READ,
> > > -           .val =3D _PAGE_READ,
> > >             .set =3D "R",
> > >             .clear =3D ".",
> > >     }, {
> > >             .mask =3D _PAGE_PRESENT,
> > > -           .val =3D _PAGE_PRESENT,
> > >             .set =3D "V",
> > >             .clear =3D ".",
> > >     }
> > > @@ -208,15 +198,19 @@ static void dump_prot(struct pg_state *st)
> > >     unsigned int i;
> > >     for (i =3D 0; i < ARRAY_SIZE(pte_bits); i++) {
> > > -           const char *s;
> > > -
> > > -           if ((st->current_prot & pte_bits[i].mask) =3D=3D pte_bits=
[i].val)
> > > -                   s =3D pte_bits[i].set;
> > > -           else
> > > -                   s =3D pte_bits[i].clear;
> > > -
> > > -           if (s)
> > > -                   pt_dump_seq_printf(st->seq, " %s", s);
> > > +           char s[7];
> > > +           unsigned long val;
> > > +
> > > +           val =3D st->current_prot & pte_bits[i].mask;
> > > +           if (val) {
> > > +                   if (pte_bits[i].mask =3D=3D _PAGE_SOFT)
> > > +                           sprintf(s, pte_bits[i].set, val >> 8);
> > > +                   else
> > > +                           sprintf(s, "%s", pte_bits[i].set);
> > > +           } else
> > > +                   sprintf(s, "%s", pte_bits[i].clear);
> > > +
> > > +           pt_dump_seq_printf(st->seq, " %s", s);
> > >     }
> > >   }
> >
> >
> > I don't see any issue in your patch, but just the output is a bit "weir=
d"
> > now as the there is a large "hole" between the PTE type and the PTE
> > protection bits:
> >
> > Before:
> >
> > 0xffffffd800000000-0xffffffd800200000 0x0000000080000000         2M PMD
> > D A G . . W R V
> >
> > After:
> >
> > 0xffffaf8000000000-0xffffaf8000200000 0x0000000080000000         2M PMD
> > .               D A G . . W R V
> >
> > Maybe you could add the PBMT/N bits after the protections bits to void =
this
> > hole?
>
> Agreed, PBMT and RSW fields are not commonly used. How about adding ".."
> for 2-bit zero values instead of spaces? hopefully it will look better.
>
> 0xffffffc802088000-0xffffffc80208c000    0x0000000000d36000        16K PT=
E .   ..     ..   D A G . . W R V
> 0xffffffc802090000-0xffffffc802094000    0x0000000000d4d000        16K PT=
E . MT(IO)   ..   D A G . . W R V
> 0xffffffc802095000-0xffffffc8020b5000    0x0000000100d80000       128K PT=
E .   ..   RSW(2) D A G . . W R V
> 0xffffffc8020b6000-0xffffffc8020d6000    0x0000000100da0000       128K PT=
E .   ..   RSW(2) D A G . . W R V
> 0xffffffc8020d8000-0xffffffc8020dc000    0x0000000000d7b000        16K PT=
E .   ..     ..   D A G . . W R V
> 0xffffffc8020e0000-0xffffffc8020e4000    0x0000000000d7f000        16K PT=
E .   ..     ..   D A G . . W R V
>

Fine by me, I'll add my RB/TB on the next version!

Thanks,

Alex

> > Anyway, as a heavy user of this kernel page table dump, that's really
> > appreciated, thanks :)
>
> Thansk for the review :)
>
> Best regards,
> Peter Lin
>
> > Alex
> >