2023-09-13 16:58:17

by Yu-Chien Peter Lin

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

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 <[email protected]>
---
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 */

#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

2023-09-13 23:06:22

by Alexandre Ghiti

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

Hi Yu,

On Wed, Sep 13, 2023 at 6:25 PM Yu Chien Peter Lin
<[email protected]> 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 <[email protected]>
> ---
> 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
>

2023-09-14 01:10:16

by Yu-Chien Peter Lin

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

On Wed, Sep 13, 2023 at 09:29:11PM +0200, Alexandre Ghiti wrote:
> Hi Yu,
>
> On Wed, Sep 13, 2023 at 6:25 PM Yu Chien Peter Lin
> <[email protected]> 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 <[email protected]>
> > ---
> > 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).

Hi Alexandre,

Sure, will fix.
Thanks for the review.

Regards,
Peter Lin

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