Re: [PATCH 1/3] riscv: Improve PTDUMP to show RSW with non-zero value

[Alexandre Ghiti]

Hi Yu,

On Wed, Sep 13, 2023 at 6:25 PM Yu Chien Peter Lin
<peterlin@xxxxxxxxxxxxx> wrote:
>
> RSW field is allowed to encode 2 bits of information, currently
> PTDUMP only prints RSW when its value is 1 or 3.
>
> To fix this issue and enhance the debug experience with PTDUMP,
> allow it to print the RSW with any non-zero value, otherwise,
> it will print a 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@xxxxxxxxxxxxx>
> ---
>  arch/riscv/include/asm/pgtable-bits.h |  2 +-
>  arch/riscv/mm/ptdump.c                | 33 ++++++++++++---------------
>  2 files changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/arch/riscv/include/asm/pgtable-bits.h b/arch/riscv/include/asm/pgtable-bits.h
> index f896708e8331..d5e828b7d7c6 100644
> --- a/arch/riscv/include/asm/pgtable-bits.h
> +++ b/arch/riscv/include/asm/pgtable-bits.h
> @@ -16,7 +16,7 @@
>  #define _PAGE_GLOBAL    (1 << 5)    /* Global */
>  #define _PAGE_ACCESSED  (1 << 6)    /* Set by hardware on any access */
>  #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 */

That makes the PAGE_SPECIAL below use the 2 software reserved bits
right? You should redefine PAGE_SPECIAL to (1 << 8).

>
>  #define _PAGE_SPECIAL   _PAGE_SOFT
>  #define _PAGE_TABLE     _PAGE_PRESENT
> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> index 20a9f991a6d7..62bbea17d475 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 = {
>  /* 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[] = {
>         {
>                 .mask = _PAGE_SOFT,
> -               .val = _PAGE_SOFT,
> -               .set = "RSW",
> -               .clear = "   ",
> +               .set = "RSW(%d)",
> +               .clear = "      ",
>         }, {
>                 .mask = _PAGE_DIRTY,
> -               .val = _PAGE_DIRTY,
>                 .set = "D",
>                 .clear = ".",
>         }, {
>                 .mask = _PAGE_ACCESSED,
> -               .val = _PAGE_ACCESSED,
>                 .set = "A",
>                 .clear = ".",
>         }, {
>                 .mask = _PAGE_GLOBAL,
> -               .val = _PAGE_GLOBAL,
>                 .set = "G",
>                 .clear = ".",
>         }, {
>                 .mask = _PAGE_USER,
> -               .val = _PAGE_USER,
>                 .set = "U",
>                 .clear = ".",
>         }, {
>                 .mask = _PAGE_EXEC,
> -               .val = _PAGE_EXEC,
>                 .set = "X",
>                 .clear = ".",
>         }, {
>                 .mask = _PAGE_WRITE,
> -               .val = _PAGE_WRITE,
>                 .set = "W",
>                 .clear = ".",
>         }, {
>                 .mask = _PAGE_READ,
> -               .val = _PAGE_READ,
>                 .set = "R",
>                 .clear = ".",
>         }, {
>                 .mask = _PAGE_PRESENT,
> -               .val = _PAGE_PRESENT,
>                 .set = "V",
>                 .clear = ".",
>         }
> @@ -208,15 +198,20 @@ static void dump_prot(struct pg_state *st)
>         unsigned int i;
>
>         for (i = 0; i < ARRAY_SIZE(pte_bits); i++) {
> -               const char *s;
> -
> -               if ((st->current_prot & pte_bits[i].mask) == pte_bits[i].val)
> -                       s = pte_bits[i].set;
> +               char s[7];
> +               unsigned long val;
> +
> +               val = st->current_prot & pte_bits[i].mask;
> +               if (val) {
> +                       if (pte_bits[i].mask == _PAGE_SOFT)
> +                               sprintf(s, pte_bits[i].set, val >> 8);
> +                       else
> +                               sprintf(s, "%s", pte_bits[i].set);
> +               }
>                 else
> -                       s = pte_bits[i].clear;
> +                       sprintf(s, "%s", pte_bits[i].clear);
>
> -               if (s)
> -                       pt_dump_seq_printf(st->seq, " %s", s);
> +               pt_dump_seq_printf(st->seq, " %s", s);
>         }
>  }
>
> --
> 2.34.1
>