Re: [PATCH v4 2/4] xen/riscv: always preset A/D bits in G-stage PTEs

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/1/26 11:07 AM, Baptiste Le Duc wrote:
> A RISC-V implementation can manage the PTE A/D bits in one of two ways:
>      1) Update the 'A' and 'D' PTE bits in hardware (ratified as Svadu).
>      2) Generate a page fault when 'A' and/or 'D' is clear, so that software
>         can set them (ratified as Svade).
>
> p2m_set_pte_flags() presets A/D in G-stage PTEs only when the device tree
> advertises Svade. Platforms that use scheme (2) without advertising Svade,
> such as the HiFive Premier P550, then hit guest-page faults that Xen does
> not handle.
>
> Always preset A/D in G-stage PTEs so scheme (2) never faults. This is
> harmless with (1): software just does what hardware would have done on
> first access.
>
> Drop the now-unused svade entry from riscv_isa_ext[], and the TODO about
> handling A/D faults, since nothing uses that information yet.
>
> Fixes: ff14053983b0 ("xen/riscv: Implement p2m_pte_from_mfn() and support PBMT 
> configuration")
> Assisted-by: Claude:claude-opus-5
Out of curiosity, what kind of assistance did Claude provide for this patch?

> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> ---
> Changes since v3:
> - always preset A/D bits in G-stage mappings instead of keying off the DT:
>    it may not reflect what the hardware actually does.
> - change commit title/message.
> - drop svade as it is now-unused.
> ---
> Changes since v2:
> - change commit title.
> - preset A/D bits in p2m_set_pte_flags() if (!svadu || svade), instead of
> setting RISCV_ISA_EXT_svade in the riscv_isa bitmap for cases 1, 2 and 4.
> - riscv_resolve_ad_scheme() now only warns when both Svade and Svadu are
> present and SBI FWFT is missing; drop the dead 'svadu && svade' operand.
> - repeat XENLOG_WARNING on each line of the warning.
> - drop ASSERT(svade != svadu) from p2m_set_pte_flags().
> - only add the sbi_probe_extension() declaration to sbi.h.
> - rework the comment in p2m_set_pte_flags().
> ---
> Changes since v1:
> - change commit title
> - expose RISCV_ISA_EXT_svadu so the two extensions can be told apart.
> - move the Svade/Svadu resolution to a new riscv_resolve_ad_scheme(),
>    called once from riscv_fill_hwcap().
> - expose sbi_probe_extension() (was static) to probe for SBI FWFT.
> - stop presetting A/D bits unconditionally in p2m_set_permission(), do it
>    only when Svade is present.
> ---
>   xen/arch/riscv/cpufeature.c |  1 -
>   xen/arch/riscv/p2m.c        | 38 ++++++++------------------------------
>   2 files changed, 8 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index aaf544d13f..6d3c15a37f 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -199,7 +199,6 @@ static const struct riscv_isa_ext_entry __initconstrel 
> riscv_isa_ext[] = {
>       RISCV_ISA_EXT_ENTRY(smstateen,      ANY),
>       RISCV_ISA_EXT_ENTRY(ssaia,          ANY),
>       RISCV_ISA_EXT_ENTRY(sstc,           NONE),
> -    RISCV_ISA_EXT_ENTRY(svade,          NONE),
I’m not sure we should drop this from riscv_isa_ext[]. I agree that we 
currently don’t have any users in the code that check whether Svade is 
supported, but such users may appear in the future.
Since this line already existed before this patch, keeping it would also 
make the changes in this patch smaller. I also don’t see much harm in 
having the Svade extension listed in this array. If Svade is present in 
the riscv,isa string, it seems reasonable to set the corresponding bit 
in the bitmap for potential future use.
Other changes looks good to me so I just cut them.

[...]

~ Oleksii