Re: [PATCH v7 2/6] arm64: ptdump: Expose the attribute parsing functionality

[Will Deacon <will@kernel.org>]

On Fri, Jun 21, 2024 at 12:32:26PM +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          | 37 ++---------------------------
>  2 files changed, 42 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> index 5b1701c76d1c..c550b2afcab7 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 prot_bits {
> +	u64		mask;
> +	u64		val;
> +	const char	*set;
> +	const char	*clear;
> +};
> +
> +struct pg_level {
> +	const struct 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 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;
> +};

Minor nit, but if we're moving these structure definitions into the
header then I'd be inclined to give them some more specific names (e.g.
prefix them with 'ptdump_'). Granted, this header isn't used widely, but
it's included by arch/arm64/mm/mmu.c and claiming 'struct prot_bits' is
a bit over-reaching imo!

Will