Re: [PATCH v2 1/6] mm/arm64: constify pte_get*() and contpte get logic

On Mon, Aug 03, 2026 at 05:43:55PM +0100, Pedro Falcato wrote:
> None of the contpte code needs write access to the PTEs.

This seems like a very broad statement? Is that actually true? I see a bunch of
ptep's that aren't const-ified, so you should explain why those couldn't be
converted.

Also you add a new contpte_align_down() macro, you should mention that it the
commit message, explain why it was needed.

In general more needed here :) it'd be ok if it was a truly trivial change that
was all obvious but you're changing some pte_t *'s and not others so it's
clearly not.

>
> Signed-off-by: Pedro Falcato <[email protected]>
> ---
>  arch/arm64/include/asm/pgtable.h | 10 +++++-----
>  arch/arm64/mm/contpte.c          | 11 ++++++++---
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index a2681d755358..043bc0649cee 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -378,7 +378,7 @@ static inline void __set_pte(pte_t *ptep, pte_t pte)
>  	__set_pte_complete(pte);
>  }
>
> -static inline pte_t __ptep_get(pte_t *ptep)
> +static inline pte_t __ptep_get(const pte_t *ptep)
>  {
>  	return READ_ONCE(*ptep);
>  }
> @@ -1652,8 +1652,8 @@ extern void __contpte_try_fold(struct mm_struct *mm, unsigned long addr,
>  				pte_t *ptep, pte_t pte);
>  extern void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
>  				pte_t *ptep, pte_t pte);
> -extern pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte);
> -extern pte_t contpte_ptep_get_lockless(pte_t *orig_ptep);
> +extern pte_t contpte_ptep_get(const pte_t *ptep, pte_t orig_pte);
> +extern pte_t contpte_ptep_get_lockless(const pte_t *orig_ptep);

This is (very) nitty but - not sure on the policy on extern's (Will/Catalin?) -
but in mm we drop them when we touch the code since you don't need them these
days :)

>  extern void contpte_set_ptes(struct mm_struct *mm, unsigned long addr,
>  				pte_t *ptep, pte_t pte, unsigned int nr);
>  extern void contpte_clear_full_ptes(struct mm_struct *mm, unsigned long addr,
> @@ -1732,7 +1732,7 @@ static inline unsigned int pte_batch_hint(pte_t *ptep, pte_t pte)
>   */
>
>  #define ptep_get ptep_get
> -static inline pte_t ptep_get(pte_t *ptep)
> +static inline pte_t ptep_get(const pte_t *ptep)
>  {
>  	pte_t pte = __ptep_get(ptep);
>
> @@ -1743,7 +1743,7 @@ static inline pte_t ptep_get(pte_t *ptep)
>  }
>
>  #define ptep_get_lockless ptep_get_lockless
> -static inline pte_t ptep_get_lockless(pte_t *ptep)
> +static inline pte_t ptep_get_lockless(const pte_t *ptep)
>  {
>  	pte_t pte = __ptep_get(ptep);
>
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index 2de12656b4d8..3a5d6937fb51 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -26,6 +26,11 @@ static inline pte_t *contpte_align_down(pte_t *ptep)
>  	return PTR_ALIGN_DOWN(ptep, sizeof(*ptep) * CONT_PTES);
>  }
>
> +#define contpte_align_down(ptep)                                                       \
> +	_Generic((ptep),                                                               \
> +		 const pte_t *: (const pte_t *) contpte_align_down((pte_t *) (ptep)),  \
> +		 pte_t *: contpte_align_down((pte_t *) ptep))

I really hate these _Generic() helper things. So ugly. And it's a pretty horrid
cast now :(

Was it not possible to const-ify further to just be able to constify
contpte_align_down itself?

It also seems to contradict the claim that contpte doesn't need write-access to
pte's since you're going to lengths to allow non-const pte_t * here.

You should cover off why this was necessary in the commit message as above.

Anyway PTR_ALIGN_DOWN() is already const-safe so couldn't you anyway just
collapse this to:

#define contpte_align_down(ptep) \
	PTR_ALIGN_DOWN(ptep, sizeof(*(ptep)) * CONT_PTES)

Then describe in the commit message why you need to handle both cases?

> +
>  static inline pte_t *contpte_align_addr_ptep(unsigned long *start,
>  					     unsigned long *end, pte_t *ptep,
>  					     unsigned int nr)
> @@ -310,7 +315,7 @@ void __contpte_try_unfold(struct mm_struct *mm, unsigned long addr,
>  }
>  EXPORT_SYMBOL_GPL(__contpte_try_unfold);
>
> -pte_t contpte_ptep_get(pte_t *ptep, pte_t orig_pte)
> +pte_t contpte_ptep_get(const pte_t *ptep, pte_t orig_pte)
>  {
>  	/*
>  	 * Gather access/dirty bits, which may be populated in any of the ptes
> @@ -367,7 +372,7 @@ static inline bool contpte_is_consistent(pte_t pte, unsigned long pfn,
>  			pgprot_val(prot) == pgprot_val(orig_prot);
>  }
>
> -pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
> +pte_t contpte_ptep_get_lockless(const pte_t *orig_ptep)
>  {
>  	/*
>  	 * The ptep_get_lockless() API requires us to read and return *orig_ptep
> @@ -386,10 +391,10 @@ pte_t contpte_ptep_get_lockless(pte_t *orig_ptep)
>  	 * because it is not part of a contpte range.
>  	 */
>
> +	const pte_t *ptep;

Nit but this is breaking the reverse xmas tree isn't it?

>  	pgprot_t orig_prot;
>  	unsigned long pfn;
>  	pte_t orig_pte;
> -	pte_t *ptep;
>  	pte_t pte;
>  	int i;
>
> --
> 2.55.0
>

--
Cheers, Lorenzo