Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling

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

On 9/23/26 12:06 PM, Baptiste Le Duc wrote:
> On 2026-09-22 17:29 +0200, Oleksii Kurochko wrote:
>>
>>
>> On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
>>> p2m_set_permission() only presets the PTE A/D bits when the Svade extension
>>> is present in the device tree. This causes an unhandled page fault when
>>> neither Svade nor Svadu is present (the platform's actual behaviour is then
>>> unknown), and when both are present in the device tree.
>>
>> When both are present, RISCV_ISA_EXT_svade is set, so the current code
>> does preset the A/D bits and no fault happens. The only broken case is
>> when neither extension is present, so shouldn't "both present" be dropped?
> Yes i agree, I'll fix that in v3.
>>
>>>
>>> Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
>>> riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
>>> the four possible Svade/Svadu combinations (inspired by [1]), it decides
>>> whether software has to preset the A/D bits and, if so, sets
>>> RISCV_ISA_EXT_svade to record that decision:
>>> - neither present: assume Svade, since assuming Svade is harmless on real
>>>     Svadu hardware, while assuming Svadu on real Svade hardware risks an
>>>     unhandled page fault
>>> - only Svade present: assume Svade
>>> - only Svadu present: leave A/D management to hardware
>>> - both present: Svade wins until Xen supports the SBI FWFT call needed to
>>>     enable hardware updating of A/D bits, so assume Svade and warn that
>>>     dropping 'svade' from the DT is the only way to get Svadu.
>>>
>>> [1] https://lwn.net/Articles/980016/
>>>
>>> Fixes: ff14053983b0 ("xen/riscv: Implement p2m_pte_from_mfn() and support PBMT configuration")
>>> Assisted-by: Claude:claude-opus-5
>>> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>>> ---
>>> 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.
>>
>> sbi_probe_extension() is already non-static in staging, only the prototype
>> is missing. What base is this patch against?
>>
>>> - stop presetting A/D bits unconditionally in p2m_set_permission(), do it
>>>     only when Svade is present.
>>
>> What is the gain from not presetting them? Presetting A/D is correct
>> with both Svade and Svadu: with Svadu it just saves the hardware an
>> atomic PTE update on first access. Xen doesn't consume G-stage A/D bits
>> (no dirty tracking, no demand paging), and pt.c already presets A/D
>> unconditionally for Xen's own mappings. Always setting PTE_ACCESSED |
>> PTE_DIRTY in p2m_set_permission() fixes the bug in one line, with no
>> need for the resolver, the new ISA bit, FWFT probing or the ASSERT.
>> Handling A/D differently only makes sense once Xen actually wants that
>> information, and at that point FWFT support and a fault handler are
>> needed anyway.
> I agree that presetting them is the right way, it is also the way linux
> is working. If Jan agree, I will to in that way in v3.
> 
> Then, it seems there is no need to register Svade/Svadu at all in
> cpufeature.c, am I right?

If after the re-work we won't need any case of code where it is needed 
to call riscv_isa_extension_available(NULL, RISCV_ISA_EXT_{svadu,svade}) 
then it seems like we won't need it in cpufeature.c, at least, in terms 
of the current patch. (but also consider my another reply in a separate 
thread if final solution will end that we will force a user to 
explicitly tell that a user has to write Svade or Svadu in DTS then 
likely we will need to have correspondent arrays in cpufeature.c and 
emum updated).

Probably, we will need to have them mentioned in correspondent arrays in 
cpufeature.c if we want to implicitly tell for example that guest is 
supporting Svadu or Svade. But I think it isn't the case for the current 
patch.

~ Oleksii