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

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

[Yu Chien Peter Lin]

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/include/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 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_SPECIAL   (1 << 8)
 #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 = {
 /* 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,19 @@ 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;
-		else
-			s = pte_bits[i].clear;
-
-		if (s)
-			pt_dump_seq_printf(st->seq, " %s", s);
+		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
+			sprintf(s, "%s", pte_bits[i].clear);
+
+		pt_dump_seq_printf(st->seq, " %s", s);
 	}
 }
 
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 2/3] riscv: Introduce PBMT field to PTDUMP

[Yu Chien Peter Lin]

This patch introduces the PBMT field to the PTDUMP, so it can
display the memory attributes for NC or IO.

Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
---
 arch/riscv/mm/ptdump.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
index 85686652f342..b5026fc867e8 100644
--- a/arch/riscv/mm/ptdump.c
+++ b/arch/riscv/mm/ptdump.c
@@ -135,6 +135,12 @@ struct prot_bits {
 
 static const struct prot_bits pte_bits[] = {
 	{
+#ifdef CONFIG_64BIT
+		.mask = _PAGE_MTMASK_SVPBMT,
+		.set = "MT(%s)",
+		.clear = "      ",
+	}, {
+#endif
 		.mask = _PAGE_SOFT,
 		.set = "RSW(%d)",
 		.clear = "      ",
@@ -205,6 +211,16 @@ static void dump_prot(struct pg_state *st)
 		if (val) {
 			if (pte_bits[i].mask == _PAGE_SOFT)
 				sprintf(s, pte_bits[i].set, val >> 8);
+#ifdef CONFIG_64BIT
+			else if (pte_bits[i].mask == _PAGE_MTMASK_SVPBMT) {
+				if (val == _PAGE_NOCACHE_SVPBMT)
+					sprintf(s, pte_bits[i].set, "NC");
+				else if (val == _PAGE_IO_SVPBMT)
+					sprintf(s, pte_bits[i].set, "IO");
+				else
+					sprintf(s, pte_bits[i].set, "??");
+			}
+#endif
 			else
 				sprintf(s, "%s", pte_bits[i].set);
 		} else
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 3/3] riscv: Introduce NAPOT field to PTDUMP

[Yu Chien Peter Lin]

This patch introduces the NAPOT field to PTDUMP, allowing it
to display the letter "N" for pages that have the 63rd bit set.

Signed-off-by: Yu Chien Peter Lin <peterlin@andestech.com>
---
 arch/riscv/mm/ptdump.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
index b5026fc867e8..387bed809b4f 100644
--- a/arch/riscv/mm/ptdump.c
+++ b/arch/riscv/mm/ptdump.c
@@ -136,6 +136,10 @@ struct prot_bits {
 static const struct prot_bits pte_bits[] = {
 	{
 #ifdef CONFIG_64BIT
+		.mask = _PAGE_NAPOT,
+		.set = "N",
+		.clear = ".",
+	}, {
 		.mask = _PAGE_MTMASK_SVPBMT,
 		.set = "MT(%s)",
 		.clear = "      ",
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Alexandre Ghiti]

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/include/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 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


That's nit, but maybe you could have introduced a _PAGE_SOFT_1 and 
_PAGE_SOFT_2


> +#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 = {
>   /* 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,19 @@ 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;
> -		else
> -			s = pte_bits[i].clear;
> -
> -		if (s)
> -			pt_dump_seq_printf(st->seq, " %s", s);
> +		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
> +			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 
"weird" 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?

Anyway, as a heavy user of this kernel page table dump, that's really 
appreciated, thanks :)

Alex


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yu-Chien Peter Lin]

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/include/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 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
> 
> 
> 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 = {
> >   /* 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,19 @@ 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;
> > -		else
> > -			s = pte_bits[i].clear;
> > -
> > -		if (s)
> > -			pt_dump_seq_printf(st->seq, " %s", s);
> > +		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
> > +			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 "weird"
> 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 PTE .   ..     ..   D A G . . W R V
0xffffffc802090000-0xffffffc802094000    0x0000000000d4d000        16K PTE . MT(IO)   ..   D A G . . W R V
0xffffffc802095000-0xffffffc8020b5000    0x0000000100d80000       128K PTE .   ..   RSW(2) D A G . . W R V
0xffffffc8020b6000-0xffffffc8020d6000    0x0000000100da0000       128K PTE .   ..   RSW(2) D A G . . W R V
0xffffffc8020d8000-0xffffffc8020dc000    0x0000000000d7b000        16K PTE .   ..     ..   D A G . . W R V
0xffffffc8020e0000-0xffffffc8020e4000    0x0000000000d7f000        16K PTE .   ..     ..   D A G . . W R V

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 468 bytes --]

On Thu, Sep 14, 2023 at 09:40:25AM +0800, Yu Chien Peter Lin wrote:

> +		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
> +			sprintf(s, "%s", pte_bits[i].clear);
> +

just a nit, but checkpatch in the automation is whinging that you have
forgotten to add {} around both branches if this if statement.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Alexandre Ghiti]

Hi Peter Lin,

On Sat, Sep 16, 2023 at 9:23 AM 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/include/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 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
> >
> >
> > 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 = {
> > >   /* 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,19 @@ 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;
> > > -           else
> > > -                   s = pte_bits[i].clear;
> > > -
> > > -           if (s)
> > > -                   pt_dump_seq_printf(st->seq, " %s", s);
> > > +           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
> > > +                   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 "weird"
> > 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 PTE .   ..     ..   D A G . . W R V
> 0xffffffc802090000-0xffffffc802094000    0x0000000000d4d000        16K PTE . MT(IO)   ..   D A G . . W R V
> 0xffffffc802095000-0xffffffc8020b5000    0x0000000100d80000       128K PTE .   ..   RSW(2) D A G . . W R V
> 0xffffffc8020b6000-0xffffffc8020d6000    0x0000000100da0000       128K PTE .   ..   RSW(2) D A G . . W R V
> 0xffffffc8020d8000-0xffffffc8020dc000    0x0000000000d7b000        16K PTE .   ..     ..   D A G . . W R V
> 0xffffffc8020e0000-0xffffffc8020e4000    0x0000000000d7f000        16K PTE .   ..     ..   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
> >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yu-Chien Peter Lin]

On Mon, Sep 18, 2023 at 01:53:13PM +0100, Conor Dooley wrote:
> On Thu, Sep 14, 2023 at 09:40:25AM +0800, Yu Chien Peter Lin wrote:
> 
> > +		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
> > +			sprintf(s, "%s", pte_bits[i].clear);
> > +
> 
> just a nit, but checkpatch in the automation is whinging that you have
> forgotten to add {} around both branches if this if statement.

Hi Conor,

Will fix the warning.

Thanks,
Peter Lin

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv