Re: [PATCH v3 4/6] xen/riscv: fix A/D bits in Xen's page-table mappings

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/29/26 6:32 PM, Baptiste Le Duc wrote:
> Xen does not handle page faults caused by clear A/D bits, so it presets
> them when creating PTEs. pt_update_entry() does so, but
> setup_initial_mapping() (the boot page tables) and arch_pmap_map() (the
> fixmap) build leaf PTEs directly without going through it. 
If I am not mistaken then not all places are mentioned here:

... does so, but three places
build leaf PTEs directly without going through it:
setup_initial_mapping() (the boot page tables), check_pgtbl_mode_support()
(the temporary root entry used to probe SATP mode support) and
arch_pmap_map() (the fixmap).

Without Svadu,
> both would fault on first access.
After this I think it makes sense to add also about 
check_pgtbl_mode_support():
For check_pgtbl_mode_support() the fault happens on the
instruction fetch right after the CSR_SATP write, before any trap
handler is set up.

> Add PTE_ACCESSED and PTE_DIRTY to PAGE_HYPERVISOR_RO, and build
> PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX on top of it. PTE_DIRTY is set in
> all cases for consistency with pt_update_entry() which sets it at runtime.
>
> 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.
... but you added PTE_ACCESSED | PTE_DIRTY to PAGE_HYPERVISOR_* so it 
isn't really "equivalent raw bit lists".
So it seems like this part should be dropped. My suggestion is ...


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

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 
check_pgtbl_mode_support() to use PAGE_HYPERVISOR_RX for its temporary 
root entry. 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, 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.
>
> pte_is_table() and pte_is_mapping() both assert that a PTE doesn't use one
> of the two reserved encodings, (V=1, W=1, R=0) and (V=1, X=1, W=1, R=0), by
> masking it with PAGE_HYPERVISOR_RW. Now that PAGE_HYPERVISOR_RW also
> carries A and D, a reserved PTE with A or D set would no longer be caught.
> Mask with the V, R and W bits explicitly instead, and factor the check out
> into pte_is_reserved().
>
> Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> ---
> Changes since v2:
> - add fixes commit ref.
> - add A/D bits to PAGE_HYPERVISOR_RO and derive PAGE_HYPERVISOR_RW/RX
>    from it for consistency with runtime.
> - introduce pte_is_reserved() to factor out the reserved-encoding assert
>    shared by pte_is_table() and pte_is_mapping().
> - reword commit title
> ---
> 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 | 33 +++++++++++++++++----------------
>   xen/arch/riscv/mm.c               |  9 ++++-----
>   2 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/riscv/include/asm/page.h 
> b/xen/arch/riscv/include/asm/page.h
> index b465a90325..7772e2f572 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_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 | PTE_DIRTY)
> +#define PAGE_HYPERVISOR_RW          (PAGE_HYPERVISOR_RO | PTE_WRITABLE)
> +#define PAGE_HYPERVISOR_RX          (PAGE_HYPERVISOR_RO | PTE_EXECUTABLE)
>   
>   #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
>   /*
> @@ -161,31 +161,32 @@ static inline bool pte_is_valid(pte_t p)
>    *      X W R Meaning
>    *      0 0 0 Pointer to next level of page table.
>    *      0 0 1 Read-only page.
> - *      0 1 0 Reserved for future use.
> + *      0 1 0 Reserved for future use. [1]
>    *      0 1 1 Read-write page.
>    *      1 0 0 Execute-only page.
>    *      1 0 1 Read-execute page.
> - *      1 1 0 Reserved for future use.
> + *      1 1 0 Reserved for future use. [2]
>    *      1 1 1 Read-write-execute page.
> + *
> + *   So if V=1 and W=1 then R also needs to be 1 as R = 0 is reserved for
> + *   future use ([1], [2]).
>    */
> +static inline bool pte_is_reserved(pte_t p)
> +{
> +    return (p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) ==
> +           (PTE_VALID | PTE_WRITABLE);
> +}
> +
Nit:

The function checks specifically for reserved R/W/X permission bit 
encodings where W=1 and R=0 (encodings 0b010 and 0b110 in Table 25 
"Encoding of PTE R/W/X fields" of the RISC-V Privileged ISA 
Specification). Per the specification, writable pages must also be 
marked readable (W=1 requires R=1 for valid leaf PTEs).
However, naming this function `pte_is_reserved()` can be ambiguous 
because the RISC-V PTE format contains several other types of reserved 
fields:
1. Bits 54–60 (and bit 63 without Svnapot) are "Reserved for future 
standard use".
2. Bits 9:8 (RSW) are "Reserved for supervisor software" and ignored by 
hardware.
3. PBMT = 0b11 is "Reserved for future standard use" under the Svpbmt 
extension.
To avoid confusion between reserved R/W/X permission encodings and 
reserved PTE bitfields/attributes, it would be much clearer to make the 
function name explicitly reflect that it checks for reserved R/W/X 
permission encodings.
My suggestion will be pte_has_reserved_rwx() or pte_has_reserved_perms().

I am not going to insist on the name change (but it would be nice to 
have) so but with commit message updated:
 Reviewed-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Thanks.

~ Oleksii