Re: [PATCH 2/5] xen/riscv: preset A/D bits in Xen's own page-table mappings

On 2026-08-28 15:34 +0200, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> > The previous patch made p2m_set_permission() always set the PTE A/D bits to
> > map pages in G-stage, to avoid a page fault on platforms that implement
> > neither Svade nor Svadu, or that declare both in the device tree. Xen's own
> > page tables, built by setup_initial_mapping(), never go through
> > p2m_set_permission() and need the same fix.
> > 
> > Add PTE_ACCESSED to PTE_LEAF_DEFAULT and make it the minimal common leaf
> > permission set by dropping PTE_WRITABLE. Rebuild PAGE_HYPERVISOR_RO,
> > PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX from that common base, with
> > PAGE_HYPERVISOR_RW also adding PTE_DIRTY. Switch setup_initial_mapping() to
> > use these macros for its default, text, and rodata permissions instead of
> > the equivalent raw bit lists.
> > 
> > A PTE is a table entry iff PTE_VALID is set and R/W/X are all clear, so
> > update pte_is_table() accordingly.
> > 
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Baptiste Le Duc <[email protected]>
> > ---
> >   xen/arch/riscv/include/asm/page.h | 14 ++++++++------
> >   xen/arch/riscv/mm.c               |  7 +++----
> >   2 files changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> > index b465a90325..5c02f64a17 100644
> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -46,12 +46,12 @@
> >   #define PTE_PBMT_NOCACHE            BIT(61, UL)
> >   #define PTE_PBMT_IO                 BIT(62, UL)
> >   
> > -#define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
> > +#define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE | PTE_ACCESSED)
> 
> Dropping PTE_WRITABLE here silently changes the permissions of an 
> existing user of this macro that the patch doesn't touch.
> 
> check_pgtbl_mode_support() in mm.c still builds its temporary root entry as:
>   index = pt_index(page_table_level, aligned_load_start);
>   stage1_pgtbl_root[index] = paddr_to_pte(aligned_load_start,
>                                       PTE_LEAF_DEFAULT | PTE_EXECUTABLE);
> 
> Before this patch that evaluated to V|R|W|X (RWX); afterwards it is 
> V|R|A|X (RX). So the mapping loses write permission.
> 
> I believe that is harmless in practice: the entry is only alive between 
> the csr_write(CSR_SATP, ...) that turns the MMU on and the 
> csr_write(CSR_SATP, 0) a few lines below, it only has to make the 
> current instruction stream fetchable so that the SATP mode probe can 
> complete, and nothing writes through it. Arguably RX is the better 
> permission set for it anyway. But it is still a behavioural change 
> rather than a cosmetic one, and the commit message doesn't mention it(it 
> only talks about setup_initial_mapping()).
> Please call it out explicitly there.
> 
> While at it, this site should be converted too:
> 
>      stage1_pgtbl_root[index] = paddr_to_pte(aligned_load_start,
>                                              PAGE_HYPERVISOR_RX);
> 
>
Sorry I didn't see this call site, you're right, it'll be better.
I will also explain why it goes from RWX to RX in the commit message.

> Otherwise the patch converts three sites in setup_initial_mapping() to 
> the new PAGE_HYPERVISOR_* macros while leaving a fourth one open-coding 
> the redefined PTE_LEAF_DEFAULT, which is exactly the kind of asymmetry 
> that makes the redefinition easy to miss on the next change.
> 
> After that conversion PTE_LEAF_DEFAULT has no users left
> outside page.h itself, so it could either be dropped entirely in favour 
> of PAGE_HYPERVISOR_{RO,RW,RX}, or renamed to something that reflects its 
> new meaning (PTE_LEAF_COMMON or similar). "DEFAULT" now names a set that 

I think, as every leaf pages could be read, it makes sense to rename it
to PTE_LEAF_COMMON, to make it clear it provides the minimal set of access.

> is not a usable permission on its own, which is misleading.
> 
> >   #define PTE_TABLE                   (PTE_VALID)
> >   
> > -#define PAGE_HYPERVISOR_RO          (PTE_VALID | PTE_READABLE)
> > -#define PAGE_HYPERVISOR_RW          (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
> > -#define PAGE_HYPERVISOR_RX          (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
> > +#define PAGE_HYPERVISOR_RO          (PTE_LEAF_DEFAULT)
> > +#define PAGE_HYPERVISOR_RW          (PTE_LEAF_DEFAULT | PTE_WRITABLE | PTE_DIRTY)
> > +#define PAGE_HYPERVISOR_RX          (PTE_LEAF_DEFAULT | PTE_EXECUTABLE)
> 
> Adding A/D to PAGE_HYPERVISOR_RW fixes a second site beyond the ones the
> commit message mentions, and I think it deserves to be spelled out.
> 
> arch_pmap_map() in asm/pmap.h writes the fixmap leaf entry directly:
>      pte = pte_from_mfn(mfn, PAGE_HYPERVISOR_RW);
>      write_pte(entry, pte);
> i.e. it bypasses pt_update_entry(), which is the place that ORs in
> PTE_ACCESSED | PTE_DIRTY for everything going through map_pages_to_xen().
> So before this patch every pmap mapping was installed with A=D=0 and 
> would fault on first access under Svade, in exactly the same way the 
> boot page tables did.
> 
> The commit message currently frames the problem as "Xen's own page 
> tables, built by setup_initial_mapping()", which undersells the fix. 
> Please extend it to say that arch_pmap_map() is affected as well, and 
> that it is fixed by the PAGE_HYPERVISOR_RW change rather than by the 
> mm.c conversion.
Right, I will do that in v2.
> 
> FWIW I checked the remaining leaf-PTE construction sites (paddr_to_pte() 
> /pte_from_mfn() callers) and with these two the series covers all of 
> them: everything else either builds table entries (PTE_TABLE) or goes 
> through pt_update_entry() / p2m_set_permission(), both of which set A/D 
> themselves.
> 
> >   
> >   #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
> >   /*
> > @@ -177,7 +177,8 @@ static inline bool pte_is_table(pte_t p)
> >        *
> >        * PAGE_HYPERVISOR_RW contains PTE_VALID too.
> >        */
> > -    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
> > +    ASSERT((p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) !=
> > +           (PTE_VALID | PTE_WRITABLE));
> 
> Please drop the last line of the comment above it:
Ok I'll do that.
> 
>       * PAGE_HYPERVISOR_RW contains PTE_VALID too.
> 
> That sentence existed only to explain why the old mask was written as
> PAGE_HYPERVISOR_RW, i.e. that the macro is not just R|W but carries
> PTE_VALID as well, which is what made the comparison against V|W work.
> With the mask now written out literally, the macro is no longer 
> referenced anywhere in the function, so the line dangles. It is also 
> inaccurate now, since PAGE_HYPERVISOR_RW carries A and D in addition to V.
> 
> ~ Oleksii
> 
>