diff mbox series

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

Message ID 20230920035522.3180558-1-peterlin@andestech.com (mailing list archive)
State Superseded
Headers show
Series [v3,1/3] riscv: Improve PTDUMP to show RSW with non-zero value | expand

Checks

Context Check Description
conchuod/cover_letter warning Series does not have a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 0bb80ecc33a8
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 5 and now 5
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 2132 this patch: 2132
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 1386 this patch: 1386
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 29 this patch: 29
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch warning CHECK: Prefer using the BIT macro
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Yu Chien Peter Lin Sept. 20, 2023, 3:55 a.m. UTC
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 improve the debugging experience
with PTDUMP, we redefine _PAGE_SPECIAL to its original
value and use _PAGE_SOFT as the RSW mask, allow it to
print the RSW with any non-zero value.

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>
---
Changes v1 -> v2
- Redefine _PAGE_SPECIAL to (1 << 8)
Changes v2 -> v3
- Add commet for _PAGE_SPECIAL
- Add ".." when RSW field is clear
- Fix unbalanced braces warning
---
 arch/riscv/include/asm/pgtable-bits.h |  4 +--
 arch/riscv/mm/ptdump.c                | 35 ++++++++++++---------------
 2 files changed, 17 insertions(+), 22 deletions(-)

Comments

Alexandre Ghiti Sept. 20, 2023, 9:10 a.m. UTC | #1
Hi,

On 20/09/2023 05:55, 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 improve the debugging experience
> with PTDUMP, we redefine _PAGE_SPECIAL to its original
> value and use _PAGE_SOFT as the RSW mask, allow it to
> print the RSW with any non-zero value.
>
> 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>
> ---
> Changes v1 -> v2
> - Redefine _PAGE_SPECIAL to (1 << 8)
> Changes v2 -> v3
> - Add commet for _PAGE_SPECIAL
> - Add ".." when RSW field is clear
> - Fix unbalanced braces warning
> ---
>   arch/riscv/include/asm/pgtable-bits.h |  4 +--
>   arch/riscv/mm/ptdump.c                | 35 ++++++++++++---------------
>   2 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/arch/riscv/include/asm/pgtable-bits.h b/arch/riscv/include/asm/pgtable-bits.h
> index f896708e8331..179bd4afece4 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)    /* RSW: 0x1 */
>   #define _PAGE_TABLE     _PAGE_PRESENT
>   
>   /*
> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> index 20a9f991a6d7..57a0926c6627 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;
> -		else
> -			s = pte_bits[i].clear;
> +		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);
> +		}
>   
> -		if (s)
> -			pt_dump_seq_printf(st->seq, " %s", s);
> +		pt_dump_seq_printf(st->seq, " %s", s);
>   	}
>   }
>   


You can add:

Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Tested-by: Alexandre Ghiti <alexghiti@rivosinc.com>

Thanks!

Alex
Conor Dooley Sept. 20, 2023, 9:55 a.m. UTC | #2
On Wed, Sep 20, 2023 at 11:55:20AM +0800, 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 improve the debugging experience
> with PTDUMP, we redefine _PAGE_SPECIAL to its original
> value and use _PAGE_SOFT as the RSW mask, allow it to
> print the RSW with any non-zero value.
> 
> This patch also removes the val from the struct prot_bits
> as it is no longer needed.
> 

Could you please add cover letters to multi-patch patchsets?

Thanks,
Conor.
Yu Chien Peter Lin Sept. 21, 2023, 1:40 a.m. UTC | #3
Hi Conor,

On Wed, Sep 20, 2023 at 10:55:30AM +0100, Conor Dooley wrote:
> On Wed, Sep 20, 2023 at 11:55:20AM +0800, 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 improve the debugging experience
> > with PTDUMP, we redefine _PAGE_SPECIAL to its original
> > value and use _PAGE_SOFT as the RSW mask, allow it to
> > print the RSW with any non-zero value.
> > 
> > This patch also removes the val from the struct prot_bits
> > as it is no longer needed.
> > 
> 
> Could you please add cover letters to multi-patch patchsets?
> 
> Thanks,
> Conor.

Sure, will do.

Regards,
Peter Lin
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/pgtable-bits.h b/arch/riscv/include/asm/pgtable-bits.h
index f896708e8331..179bd4afece4 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)    /* RSW: 0x1 */
 #define _PAGE_TABLE     _PAGE_PRESENT
 
 /*
diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
index 20a9f991a6d7..57a0926c6627 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;
-		else
-			s = pte_bits[i].clear;
+		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);
+		}
 
-		if (s)
-			pt_dump_seq_printf(st->seq, " %s", s);
+		pt_dump_seq_printf(st->seq, " %s", s);
 	}
 }