Re: [PATCH v10 2/5] arm64: ptdump: Expose the attribute parsing functionality

[Marc Zyngier <maz@kernel.org>]

On Tue, 10 Sep 2024 10:57:18 +0100,
Will Deacon <will@kernel.org> wrote:
> 
> On Mon, Sep 09, 2024 at 12:47:18PM +0000, Sebastian Ene wrote:
> > Reuse the descriptor parsing functionality to keep the same output format
> > as the original ptdump code. In order for this to happen, move the state
> > tracking objects into a common header.
> > 
> > Signed-off-by: Sebastian Ene <sebastianene@google.com>
> > ---
> >  arch/arm64/include/asm/ptdump.h | 41 +++++++++++++++++++++++-
> >  arch/arm64/mm/ptdump.c          | 55 +++++++--------------------------
> >  2 files changed, 51 insertions(+), 45 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> > index 5b1701c76d1c..bd5d3ee3e8dc 100644
> > --- a/arch/arm64/include/asm/ptdump.h
> > +++ b/arch/arm64/include/asm/ptdump.h
> > @@ -9,6 +9,7 @@
> >  
> >  #include <linux/mm_types.h>
> >  #include <linux/seq_file.h>
> > +#include <linux/ptdump.h>
> >  
> >  struct addr_marker {
> >  	unsigned long start_address;
> > @@ -21,14 +22,52 @@ struct ptdump_info {
> >  	unsigned long			base_addr;
> >  };
> >  
> > +struct ptdump_prot_bits {
> > +	u64		mask;
> > +	u64		val;
> > +	const char	*set;
> > +	const char	*clear;
> > +};
> > +
> > +struct ptdump_pg_level {
> > +	const struct ptdump_prot_bits *bits;
> > +	char name[4];
> > +	int num;
> > +	u64 mask;
> > +};
> > +
> > +/*
> > + * The page dumper groups page table entries of the same type into a single
> > + * description. It uses pg_state to track the range information while
> > + * iterating over the pte entries. When the continuity is broken it then
> > + * dumps out a description of the range.
> > + */
> > +struct ptdump_pg_state {
> > +	struct ptdump_state ptdump;
> > +	struct seq_file *seq;
> > +	const struct addr_marker *marker;
> > +	const struct mm_struct *mm;
> > +	unsigned long start_address;
> > +	int level;
> > +	u64 current_prot;
> > +	bool check_wx;
> > +	unsigned long wx_pages;
> > +	unsigned long uxn_pages;
> > +};
> > +
> >  void ptdump_walk(struct seq_file *s, struct ptdump_info *info);
> > +void note_page(struct ptdump_state *pt_st, unsigned long addr, int level,
> > +	       u64 val);
> >  #ifdef CONFIG_PTDUMP_DEBUGFS
> >  #define EFI_RUNTIME_MAP_END	DEFAULT_MAP_WINDOW_64
> >  void __init ptdump_debugfs_register(struct ptdump_info *info, const char *name);
> >  #else
> >  static inline void ptdump_debugfs_register(struct ptdump_info *info,
> >  					   const char *name) { }
> > -#endif
> > +#endif /* CONFIG_PTDUMP_DEBUGFS */
> > +#else
> > +static inline void note_page(void *pt_st, unsigned long addr,
> > +			     int level, u64 val) { }
> 
> nit: but why isn't 'pt_st' a pointer to 'struct ptdump_state'?
> 
> Perhaps you should #include <linux/ptdump.h> before the #ifdef
> CONFIG_PTDUMP_CORE ?

Yup, that seems to do the trick. I'm folding this into the patch:

diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
index 71a7ed01153a4..6cf4aae052191 100644
--- a/arch/arm64/include/asm/ptdump.h
+++ b/arch/arm64/include/asm/ptdump.h
@@ -5,11 +5,12 @@
 #ifndef __ASM_PTDUMP_H
 #define __ASM_PTDUMP_H
 
+#include <linux/ptdump.h>
+
 #ifdef CONFIG_PTDUMP_CORE
 
 #include <linux/mm_types.h>
 #include <linux/seq_file.h>
-#include <linux/ptdump.h>
 
 struct addr_marker {
 	unsigned long start_address;
@@ -67,7 +68,7 @@ static inline void ptdump_debugfs_register(struct ptdump_info *info,
 					   const char *name) { }
 #endif /* CONFIG_PTDUMP_DEBUGFS */
 #else
-static inline void note_page(void *pt_st, unsigned long addr,
+static inline void note_page(struct ptdump_state *pt_st, unsigned long addr,
 			     int level, u64 val) { }
 #endif /* CONFIG_PTDUMP_CORE */
 
> In any case, the meat of the patch is fine:
> 
> Acked-by: Will Deacon <will@kernel.org>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.