Re: [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> The previous patch set A/D bits in case of the Svade extension for G-stage
> mappings. Xen's own S-stage mappings need the same fix as both
> setup_initial_mapping() (the boot page tables) and arch_pmap_map() (the
> fixmap) build leaf PTEs directly instead of going through
> pt_update_entry(), which is what adds A/D bits. So with Svade, both would
> fault on first access.

Please don't refer to "the previous patch": once applied, the commit
message should stand on its own. Just state the fact instead, e.g.:

   With Svade, hardware doesn't update the A/D bits; instead it raises a
   page fault when A is clear (or D is clear on a write).
   pt_update_entry() already sets them, but ...

> 
> Add PTE_ACCESSED to all PAGE_HYPERVISOR_* and also PTE_DIRTY to
> PAGE_HYPERVISOR_RW as it needs to be set during a write to avoid a fault.
> This fixes arch_pmap_map() for free, since it already builds its PTE from
> PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
> its default, text and rodata permissions, and for the temporary root entry
> built by check_pgtbl_mode_support(), instead of the equivalent raw bit
> lists. The latter drops PTE_WRITABLE, going from RWX to RX, but this is
> harmless, as that entry only has to make the current instruction stream
> fetchable between the two CSR_SATP writes used to probe SATP mode support,
> and nothing writes through it.
> 
> Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
> open-coded site above leaves it with no user outside page.h itself.
> 
> A PTE is a table entry iff PTE_VALID is set and R/W/X are all clear, so
> update pte_is_table() and pte_is_mapping() accordingly.

This doesn't describe what the patch actually changes: the return
expressions of pte_is_table()/pte_is_mapping() are untouched, only the
ASSERT()s change. The real reason is that the ASSERT()s masked the PTE
with PAGE_HYPERVISOR_RW, which now contains A|D, so for the reserved
encoding V|W|A we would compare V|W|A != V|W and the ASSERT() would
silently stop firing. Something like:

   The ASSERT()s in pte_is_table() and pte_is_mapping() mask the PTE
   with PAGE_HYPERVISOR_RW to detect the reserved W=1,R=0 encoding. Now
   that PAGE_HYPERVISOR_RW includes A/D, the check would no longer
   trigger for a PTE with A or D set, so use an explicit V|R|W mask.

> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
> Changes since v1:
> - change commit title
> - mention in patch message that arch_pmap_map() is fixed too, via the
>    PAGE_HYPERVISOR_RW change, not just setup_initial_mapping().
> - convert check_pgtbl_mode_support()'s temporary root entry to
>    PAGE_HYPERVISOR_RX, as it's harmless.
> - drop PTE_LEAF_DEFAULT entirely instead of keeping it, now that no site
>    open-codes it anymore.
> - drop the pte_is_table() comment line that referenced PAGE_HYPERVISOR_RW,
>    now stale.
> ---
>   xen/arch/riscv/include/asm/page.h | 15 +++++++--------
>   xen/arch/riscv/mm.c               |  9 ++++-----
>   2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> index b465a90325..1977634efc 100644
> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -46,12 +46,11 @@
>   #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_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_VALID | PTE_READABLE | PTE_ACCESSED)
> +#define PAGE_HYPERVISOR_RW          (PTE_VALID | PTE_READABLE | PTE_WRITABLE | PTE_ACCESSED | PTE_DIRTY)
> +#define PAGE_HYPERVISOR_RX          (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE | PTE_ACCESSED)

These two lines exceed 80 columns, please wrap them, e.g.:

   #define PAGE_HYPERVISOR_RW          (PTE_VALID | PTE_READABLE | 
PTE_WRITABLE | \
                                        PTE_ACCESSED | PTE_DIRTY)

Also, pt_update_entry() sets D on every leaf, while here RO/RX get only
A. Both are valid per the spec, but it means boot-time and runtime
mappings of the same kind of page differ in D. Either set D on RO/RX
too for consistency, or say in the commit message why it isn't done.

>   
>   #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
>   /*
> @@ -174,10 +173,9 @@ static inline bool pte_is_table(pte_t p)
>        * According to the spec if V=1 and W=1 then R also needs to be 1 as
>        * R = 0 is reserved for future use ( look at the Table 4.5 ) so check
>        * in ASSERT that if (V==1 && W==1) then R isn't 0.
> -     *
> -     * 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));
>   
>       return ((p.pte & (PTE_VALID | PTE_ACCESS_MASK)) == PTE_VALID);
>   }
> @@ -185,7 +183,8 @@ static inline bool pte_is_table(pte_t p)
>   static inline bool pte_is_mapping(pte_t p)
>   {
>       /* See pte_is_table() */
> -    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
> +    ASSERT((p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) !=
> +            (PTE_VALID | PTE_WRITABLE));

Nit: the indentation differs from the one in pte_is_table() (one extra
space here). As the same expression is now open-coded twice, maybe it
is worth introducing a small helper (e.g. pte_is_reserved_wr()) and
using it in both ASSERT()s?

Thanks.

~ Oleksii