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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
>
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 Peter Lin,
On Sat, Sep 16, 2023 at 9:23 AM Yu-Chien Peter Lin
<[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> >
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