Re: [External] Re: [PATCH v2 2/2] riscv: preserve hardware-updated A/D bits in PTE accessors

[yunhui cui]

Hi Andrew,

On Sat, May 23, 2026 at 4:02 AM Andrew Jones
<andrew.jones@xxxxxxxxxxxxxxxx> wrote:
>
> On Fri, May 22, 2026 at 10:23:58PM +0800, Yunhui Cui wrote:
> > Use cmpxchg-based merges for live RISC-V PTE permission updates so
> > software changes do not lose concurrently hardware-updated accessed and
> > dirty state. Cover ptep_set_access_flags(),
> > ptep_test_and_clear_young(), and ptep_set_wrprotect(), and extend the
> > same wrprotect handling to the PUD leaf helper used by huge mappings.
> >
> > Keep the existing Svvptc flush behaviour, but only flush when the
> > merged PTE value actually changed.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>
> > Reviewed-by: Qingwei Hu <qingwei.hu@xxxxxxxxxxxxx>
> > ---
> >  arch/riscv/include/asm/pgtable.h | 19 +++++++--
> >  arch/riscv/mm/pgtable.c          | 68 ++++++++++++++++++++++++++------
> >  2 files changed, 73 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 20663a466cf6c..984c37ca8aef7 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -668,15 +668,21 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
> >  static inline void ptep_set_wrprotect(struct mm_struct *mm,
> >                                     unsigned long address, pte_t *ptep)
> >  {
> > -     pte_t read_pte = READ_ONCE(*ptep);
> > +     pte_t old_pte;
> > +     pte_t pte;
> >       /*
> >        * ptep_set_wrprotect can be called for shadow stack ranges too.
> >        * shadow stack memory is XWR = 010 and thus clearing _PAGE_WRITE will lead to
> >        * encoding 000b which is wrong encoding with V = 1. This should lead to page fault
> >        * but we dont want this wrong configuration to be set in page tables.
> >        */
> > -     atomic_long_set((atomic_long_t *)ptep,
> > -                     ((pte_val(read_pte) & ~(unsigned long)_PAGE_WRITE) | _PAGE_READ));
> > +     pte = READ_ONCE(*ptep);
> > +     do {
> > +             old_pte = pte;
> > +             pte = pte_wrprotect(pte);
> > +             pte_val(pte) = cmpxchg_relaxed(&pte_val(*ptep), pte_val(old_pte),
> > +                                            pte_val(pte));
> > +     } while (pte_val(pte) != pte_val(old_pte));
> >  }
> >
> >  #define __HAVE_ARCH_PTEP_CLEAR_YOUNG_FLUSH
> > @@ -1030,6 +1036,13 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
> >       ptep_set_wrprotect(mm, address, (pte_t *)pmdp);
> >  }
> >
> > +#define __HAVE_ARCH_PUDP_SET_WRPROTECT
> > +static inline void pudp_set_wrprotect(struct mm_struct *mm,
> > +                                   unsigned long address, pud_t *pudp)
> > +{
> > +     ptep_set_wrprotect(mm, address, (pte_t *)pudp);
> > +}
> > +
> >  #define pmdp_establish pmdp_establish
> >  static inline pmd_t pmdp_establish(struct vm_area_struct *vma,
> >                               unsigned long address, pmd_t *pmdp, pmd_t pmd)
> > diff --git a/arch/riscv/mm/pgtable.c b/arch/riscv/mm/pgtable.c
> > index 9c4427d0b1874..b77e82362442e 100644
> > --- a/arch/riscv/mm/pgtable.c
> > +++ b/arch/riscv/mm/pgtable.c
> > @@ -5,23 +5,55 @@
> >  #include <linux/kernel.h>
> >  #include <linux/pgtable.h>
> >
> > +#define RISCV_PTE_ACCESS_FLAG_MASK   (_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC | \
> > +                                      _PAGE_ACCESSED | _PAGE_DIRTY | \
> > +                                      _PAGE_SOFT_DIRTY)
> > +
> > +static inline unsigned long riscv_pte_access_flags(unsigned long cur,
> > +                                                unsigned long entry)
> > +{
> > +     unsigned long pteval;
> > +     unsigned long hw_flags;
> > +
> > +     hw_flags = _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_SOFT_DIRTY;
>
> Why are we setting _PAGE_SOFT_DIRTY in something called hw_flags? Do we
> even need to consider _PAGE_SOFT_DIRTY here?

_PAGE_SOFT_DIRTY is not hardware-updated, so hw_flags was a misleading
name. I'll rename it in the next version.
It still needs to be preserved here, because RISC-V pte_mkdirty()
already sets it together with _PAGE_DIRTY, and the cmpxchg retry could
otherwise lose that soft-dirty state.

>
> Thanks,
> drew

Thanks,
Yunhui