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

On Tue, Aug 04, 2026 at 11:55:09AM +0100, Lorenzo Stoakes (ARM) wrote:
> 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.

ACK. FTR, I think it would've been far clearer with "None of the contpte get
code". There is of course contpte code that needs write access (e.g
contpte_clear_full_ptes).

> 
> 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 :)

*nods*. For what it's worth, this is new code that never needed extern.

> 
> >  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 :(

Me too!

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

You can't do that because some callers want a pte_t* out of align_down,
others want a const pte_t* out of align_down, depending on the param.

In A More Civilized Language(TM):

template <typename T>
T contpte_align_down(T ptr);

:P

> 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.

Yep, I'll admit the commit message is confusing and crap. I'll flesh it out
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?

Yep, seems like I mistakenly broke the coding style here and in the
contpte_align_down macro above (the \ is misaligned). I'll fix it up.

(I think Andrew isn't taking more material for next cycle, and while
this should have no functional effect, it is very late and you have
pushback, so probably no rush here...)

-- 
Pedro