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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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 <baptiste.le-duc@xxxxxxxxxx>
> ---
>   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);

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

     * 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