| Message ID | 20240816123906.3683425-4-sebastianene@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ptdump: View the second stage page-tables | expand |
On Fri, 16 Aug 2024 13:39:03 +0100, Sebastian Ene <sebastianene@google.com> wrote: > > Printing the descriptor attributes requires accessing a mask which has a > different set of attributes for stage-2. In preparation for adding support > for the stage-2 pagetables dumping, use the mask from the local context > and not from the globally defined pg_level array. Store a pointer to > the pg_level array in the ptdump state structure. This will allow us to > extract the mask which is wrapped in the pg_level array and use it for > descriptor parsing in the note_page. > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > --- > arch/arm64/include/asm/ptdump.h | 1 + > arch/arm64/mm/ptdump.c | 13 ++++++++----- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h > index bd5d3ee3e8dc..71a7ed01153a 100644 > --- a/arch/arm64/include/asm/ptdump.h > +++ b/arch/arm64/include/asm/ptdump.h > @@ -44,6 +44,7 @@ struct ptdump_pg_level { > */ > struct ptdump_pg_state { > struct ptdump_state ptdump; > + struct ptdump_pg_level *pg_level; > struct seq_file *seq; > const struct addr_marker *marker; > const struct mm_struct *mm; > diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c > index 404751fd30fe..ca53ef274a8b 100644 > --- a/arch/arm64/mm/ptdump.c > +++ b/arch/arm64/mm/ptdump.c > @@ -117,7 +117,7 @@ static const struct ptdump_prot_bits pte_bits[] = { > } > }; > > -static struct ptdump_pg_level pg_level[] __ro_after_init = { > +static struct ptdump_pg_level kernel_pg_levels[] __ro_after_init = { Do you actually need this sort of renaming? Given that it is static, this looks like some slightly abusive repainting which isn't warranted here. I also didn't understand the commit message: you're not tracking any mask here, but a page table level. You are also not using it for anything yet, see below. > { /* pgd */ > .name = "PGD", > .bits = pte_bits, > @@ -192,6 +192,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level, > u64 val) > { > struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump); > + struct ptdump_pg_level *pg_level = st->pg_level; This is what I mean. What is this pg_level used for? > static const char units[] = "KMGTPE"; > u64 prot = 0; > > @@ -262,6 +263,7 @@ void ptdump_walk(struct seq_file *s, struct ptdump_info *info) > .seq = s, > .marker = info->markers, > .mm = info->mm, > + .pg_level = &kernel_pg_levels[0], > .level = -1, > .ptdump = { > .note_page = note_page, > @@ -279,10 +281,10 @@ static void __init ptdump_initialize(void) > { > unsigned i, j; > > - for (i = 0; i < ARRAY_SIZE(pg_level); i++) > - if (pg_level[i].bits) > - for (j = 0; j < pg_level[i].num; j++) > - pg_level[i].mask |= pg_level[i].bits[j].mask; > + for (i = 0; i < ARRAY_SIZE(kernel_pg_levels); i++) > + if (kernel_pg_levels[i].bits) > + for (j = 0; j < kernel_pg_levels[i].num; j++) > + kernel_pg_levels[i].mask |= kernel_pg_levels[i].bits[j].mask; > } > > static struct ptdump_info kernel_ptdump_info __ro_after_init = { > @@ -297,6 +299,7 @@ bool ptdump_check_wx(void) > { 0, NULL}, > { -1, NULL}, > }, > + .pg_level = &kernel_pg_levels[0], > .level = -1, > .check_wx = true, > .ptdump = { Thanks, M.
On Tue, Aug 20, 2024 at 02:49:04PM +0100, Marc Zyngier wrote: > On Fri, 16 Aug 2024 13:39:03 +0100, > Sebastian Ene <sebastianene@google.com> wrote: > > > > Printing the descriptor attributes requires accessing a mask which has a > > different set of attributes for stage-2. In preparation for adding support > > for the stage-2 pagetables dumping, use the mask from the local context > > and not from the globally defined pg_level array. Store a pointer to > > the pg_level array in the ptdump state structure. This will allow us to > > extract the mask which is wrapped in the pg_level array and use it for > > descriptor parsing in the note_page. > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > > --- > > arch/arm64/include/asm/ptdump.h | 1 + > > arch/arm64/mm/ptdump.c | 13 ++++++++----- > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h > > index bd5d3ee3e8dc..71a7ed01153a 100644 > > --- a/arch/arm64/include/asm/ptdump.h > > +++ b/arch/arm64/include/asm/ptdump.h > > @@ -44,6 +44,7 @@ struct ptdump_pg_level { > > */ > > struct ptdump_pg_state { > > struct ptdump_state ptdump; > > + struct ptdump_pg_level *pg_level; > > struct seq_file *seq; > > const struct addr_marker *marker; > > const struct mm_struct *mm; > > diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c > > index 404751fd30fe..ca53ef274a8b 100644 > > --- a/arch/arm64/mm/ptdump.c > > +++ b/arch/arm64/mm/ptdump.c > > @@ -117,7 +117,7 @@ static const struct ptdump_prot_bits pte_bits[] = { > > } > > }; > > > > -static struct ptdump_pg_level pg_level[] __ro_after_init = { > > +static struct ptdump_pg_level kernel_pg_levels[] __ro_after_init = { Hi Marc, > Do you actually need this sort of renaming? Given that it is static, > this looks like some slightly abusive repainting which isn't warranted > here. I applied Will's suggestion from https://lore.kernel.org/all/20240705111229.GB9231@willie-the-truck/ > > > I also didn't understand the commit message: you're not tracking any > mask here, but a page table level. You are also not using it for > anything yet, see below. and I missed updating the commit message. > > > > { /* pgd */ > > .name = "PGD", > > .bits = pte_bits, > > @@ -192,6 +192,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level, > > u64 val) > > { > > struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump); > > + struct ptdump_pg_level *pg_level = st->pg_level; > > This is what I mean. What is this pg_level used for? I make use of it to extract the name based on the level. The suggestion that Will made allowed me to keep the code with less changes. Thanks, Seb > > > static const char units[] = "KMGTPE"; > > u64 prot = 0; > > > > @@ -262,6 +263,7 @@ void ptdump_walk(struct seq_file *s, struct ptdump_info *info) > > .seq = s, > > .marker = info->markers, > > .mm = info->mm, > > + .pg_level = &kernel_pg_levels[0], > > .level = -1, > > .ptdump = { > > .note_page = note_page, > > @@ -279,10 +281,10 @@ static void __init ptdump_initialize(void) > > { > > unsigned i, j; > > > > - for (i = 0; i < ARRAY_SIZE(pg_level); i++) > > - if (pg_level[i].bits) > > - for (j = 0; j < pg_level[i].num; j++) > > - pg_level[i].mask |= pg_level[i].bits[j].mask; > > + for (i = 0; i < ARRAY_SIZE(kernel_pg_levels); i++) > > + if (kernel_pg_levels[i].bits) > > + for (j = 0; j < kernel_pg_levels[i].num; j++) > > + kernel_pg_levels[i].mask |= kernel_pg_levels[i].bits[j].mask; > > } > > > > static struct ptdump_info kernel_ptdump_info __ro_after_init = { > > @@ -297,6 +299,7 @@ bool ptdump_check_wx(void) > > { 0, NULL}, > > { -1, NULL}, > > }, > > + .pg_level = &kernel_pg_levels[0], > > .level = -1, > > .check_wx = true, > > .ptdump = { > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
On Tue, 20 Aug 2024 15:13:27 +0100, Sebastian Ene <sebastianene@google.com> wrote: > > On Tue, Aug 20, 2024 at 02:49:04PM +0100, Marc Zyngier wrote: > > On Fri, 16 Aug 2024 13:39:03 +0100, > > Sebastian Ene <sebastianene@google.com> wrote: > > > > > > Printing the descriptor attributes requires accessing a mask which has a > > > different set of attributes for stage-2. In preparation for adding support > > > for the stage-2 pagetables dumping, use the mask from the local context > > > and not from the globally defined pg_level array. Store a pointer to > > > the pg_level array in the ptdump state structure. This will allow us to > > > extract the mask which is wrapped in the pg_level array and use it for > > > descriptor parsing in the note_page. > > > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > > > --- > > > arch/arm64/include/asm/ptdump.h | 1 + > > > arch/arm64/mm/ptdump.c | 13 ++++++++----- > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h > > > index bd5d3ee3e8dc..71a7ed01153a 100644 > > > --- a/arch/arm64/include/asm/ptdump.h > > > +++ b/arch/arm64/include/asm/ptdump.h > > > @@ -44,6 +44,7 @@ struct ptdump_pg_level { > > > */ > > > struct ptdump_pg_state { > > > struct ptdump_state ptdump; > > > + struct ptdump_pg_level *pg_level; > > > struct seq_file *seq; > > > const struct addr_marker *marker; > > > const struct mm_struct *mm; > > > diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c > > > index 404751fd30fe..ca53ef274a8b 100644 > > > --- a/arch/arm64/mm/ptdump.c > > > +++ b/arch/arm64/mm/ptdump.c > > > @@ -117,7 +117,7 @@ static const struct ptdump_prot_bits pte_bits[] = { > > > } > > > }; > > > > > > -static struct ptdump_pg_level pg_level[] __ro_after_init = { > > > +static struct ptdump_pg_level kernel_pg_levels[] __ro_after_init = { > > Hi Marc, > > > > Do you actually need this sort of renaming? Given that it is static, > > this looks like some slightly abusive repainting which isn't warranted > > here. > > I applied Will's suggestion from > https://lore.kernel.org/all/20240705111229.GB9231@willie-the-truck/ > > > > > > I also didn't understand the commit message: you're not tracking any > > mask here, but a page table level. You are also not using it for > > anything yet, see below. > > and I missed updating the commit message. > > > > > > > > { /* pgd */ > > > .name = "PGD", > > > .bits = pte_bits, > > > @@ -192,6 +192,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level, > > > u64 val) > > > { > > > struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump); > > > + struct ptdump_pg_level *pg_level = st->pg_level; > > > > This is what I mean. What is this pg_level used for? > > I make use of it to extract the name based on the level. The suggestion > that Will made allowed me to keep the code with less changes. Err, I see that now. It'd be good if you described what actually happens, because it is almost impossible to understand it from reading the patch. Thanks, M.
On Tue, Aug 20, 2024 at 03:25:13PM +0100, Marc Zyngier wrote: > On Tue, 20 Aug 2024 15:13:27 +0100, > Sebastian Ene <sebastianene@google.com> wrote: > > > > On Tue, Aug 20, 2024 at 02:49:04PM +0100, Marc Zyngier wrote: > > > On Fri, 16 Aug 2024 13:39:03 +0100, > > > Sebastian Ene <sebastianene@google.com> wrote: > > > > > > > > Printing the descriptor attributes requires accessing a mask which has a > > > > different set of attributes for stage-2. In preparation for adding support > > > > for the stage-2 pagetables dumping, use the mask from the local context > > > > and not from the globally defined pg_level array. Store a pointer to > > > > the pg_level array in the ptdump state structure. This will allow us to > > > > extract the mask which is wrapped in the pg_level array and use it for > > > > descriptor parsing in the note_page. > > > > > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > > > > --- > > > > arch/arm64/include/asm/ptdump.h | 1 + > > > > arch/arm64/mm/ptdump.c | 13 ++++++++----- > > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h > > > > index bd5d3ee3e8dc..71a7ed01153a 100644 > > > > --- a/arch/arm64/include/asm/ptdump.h > > > > +++ b/arch/arm64/include/asm/ptdump.h > > > > @@ -44,6 +44,7 @@ struct ptdump_pg_level { > > > > */ > > > > struct ptdump_pg_state { > > > > struct ptdump_state ptdump; > > > > + struct ptdump_pg_level *pg_level; > > > > struct seq_file *seq; > > > > const struct addr_marker *marker; > > > > const struct mm_struct *mm; > > > > diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c > > > > index 404751fd30fe..ca53ef274a8b 100644 > > > > --- a/arch/arm64/mm/ptdump.c > > > > +++ b/arch/arm64/mm/ptdump.c > > > > @@ -117,7 +117,7 @@ static const struct ptdump_prot_bits pte_bits[] = { > > > > } > > > > }; > > > > > > > > -static struct ptdump_pg_level pg_level[] __ro_after_init = { > > > > +static struct ptdump_pg_level kernel_pg_levels[] __ro_after_init = { > > > > Hi Marc, > > > > > > > Do you actually need this sort of renaming? Given that it is static, > > > this looks like some slightly abusive repainting which isn't warranted > > > here. > > > > I applied Will's suggestion from > > https://lore.kernel.org/all/20240705111229.GB9231@willie-the-truck/ > > > > > > > > > I also didn't understand the commit message: you're not tracking any > > > mask here, but a page table level. You are also not using it for > > > anything yet, see below. > > > > and I missed updating the commit message. > > > > > > > > > > > > { /* pgd */ > > > > .name = "PGD", > > > > .bits = pte_bits, > > > > @@ -192,6 +192,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level, > > > > u64 val) > > > > { > > > > struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump); > > > > + struct ptdump_pg_level *pg_level = st->pg_level; > > > > > > This is what I mean. What is this pg_level used for? > > > > I make use of it to extract the name based on the level. The suggestion > > that Will made allowed me to keep the code with less changes. > > Err, I see that now. It'd be good if you described what actually > happens, because it is almost impossible to understand it from reading > the patch. Yes, let me re-write the commit message in the next version, Thanks, Seb > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h index bd5d3ee3e8dc..71a7ed01153a 100644 --- a/arch/arm64/include/asm/ptdump.h +++ b/arch/arm64/include/asm/ptdump.h @@ -44,6 +44,7 @@ struct ptdump_pg_level { */ struct ptdump_pg_state { struct ptdump_state ptdump; + struct ptdump_pg_level *pg_level; struct seq_file *seq; const struct addr_marker *marker; const struct mm_struct *mm; diff --git a/arch/arm64/mm/ptdump.c b/arch/arm64/mm/ptdump.c index 404751fd30fe..ca53ef274a8b 100644 --- a/arch/arm64/mm/ptdump.c +++ b/arch/arm64/mm/ptdump.c @@ -117,7 +117,7 @@ static const struct ptdump_prot_bits pte_bits[] = { } }; -static struct ptdump_pg_level pg_level[] __ro_after_init = { +static struct ptdump_pg_level kernel_pg_levels[] __ro_after_init = { { /* pgd */ .name = "PGD", .bits = pte_bits, @@ -192,6 +192,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr, int level, u64 val) { struct ptdump_pg_state *st = container_of(pt_st, struct ptdump_pg_state, ptdump); + struct ptdump_pg_level *pg_level = st->pg_level; static const char units[] = "KMGTPE"; u64 prot = 0; @@ -262,6 +263,7 @@ void ptdump_walk(struct seq_file *s, struct ptdump_info *info) .seq = s, .marker = info->markers, .mm = info->mm, + .pg_level = &kernel_pg_levels[0], .level = -1, .ptdump = { .note_page = note_page, @@ -279,10 +281,10 @@ static void __init ptdump_initialize(void) { unsigned i, j; - for (i = 0; i < ARRAY_SIZE(pg_level); i++) - if (pg_level[i].bits) - for (j = 0; j < pg_level[i].num; j++) - pg_level[i].mask |= pg_level[i].bits[j].mask; + for (i = 0; i < ARRAY_SIZE(kernel_pg_levels); i++) + if (kernel_pg_levels[i].bits) + for (j = 0; j < kernel_pg_levels[i].num; j++) + kernel_pg_levels[i].mask |= kernel_pg_levels[i].bits[j].mask; } static struct ptdump_info kernel_ptdump_info __ro_after_init = { @@ -297,6 +299,7 @@ bool ptdump_check_wx(void) { 0, NULL}, { -1, NULL}, }, + .pg_level = &kernel_pg_levels[0], .level = -1, .check_wx = true, .ptdump = {
Printing the descriptor attributes requires accessing a mask which has a different set of attributes for stage-2. In preparation for adding support for the stage-2 pagetables dumping, use the mask from the local context and not from the globally defined pg_level array. Store a pointer to the pg_level array in the ptdump state structure. This will allow us to extract the mask which is wrapped in the pg_level array and use it for descriptor parsing in the note_page. Signed-off-by: Sebastian Ene <sebastianene@google.com> --- arch/arm64/include/asm/ptdump.h | 1 + arch/arm64/mm/ptdump.c | 13 ++++++++----- 2 files changed, 9 insertions(+), 5 deletions(-)