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

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

On 9/22/26 11:17 AM, Baptiste Le Duc wrote:
> On 2026-09-22 08:24 +0200, Jan Beulich wrote:
>> On 21.09.2026 19:03, Baptiste Le Duc wrote:
>>> On 2026-09-21 17:26:47+02:00, Jan Beulich wrote:
>>>> On 10.09.2026 11:34, Baptiste Le Duc wrote:
>>>>> @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
>>>>>       return false;
>>>>>   }
>>>>>   
>>>>> +/*
>>>>> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
>>>>> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
>>>>> + * that a page fault will be raised. In contrast, the Svadu extension supports
>>>>> + * hardware updating of the PTE A/D bits.
>>>>> + *
>>>>> + * There are 4 possible combinations of these extensions in the device tree.
>>>>> + * The default hardware behavior for each is:
>>>>> + *
>>>>> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
>>>>> + *    whether the platform uses Svade or Svadu. Xen should be prepared to
>>>>> + *    handle either hardware updating of the PTE A/D bits or page faults when
>>>>> + *    they need updating. In that case, Xen assumes Svade because it's
>>>>> + *    harmless if the platform is actually Svadu, while assuming Svadu on real
>>>>> + *    Svade hardware risks an unhandled page fault.
>>>>> + *
>>>>> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
>>>>> + *
>>>>> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
>>>>> + *
>>>>> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
>>>>> + *    at boot time by setting A/D bits. To use Svadu, the supervisor must
>>>>> + *    explicitly enable it using the SBI FWFT extension.
>>>>> + *
>>>>> + * The Svade extension is mandatory and the Svadu extension is optional in the
>>>>> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
>>>>> + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
>>>>> + * get the benefit of Svadu until the SBI FWFT extension is available.
>>>>> + *
>>>>> + * In other words, hardware manages the A/D bits on its own only in case 3, in
>>>>> + * all the other cases software has to preset them. Instead of open coding this
>>>>> + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
>>>>> + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
>>>>> + */
>>>>> +static void __init riscv_resolve_ad_scheme(void)
>>>>> +{
>>>>> +    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
>>>>> +    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
>>>>> +
>>>>> +    /* Case 3: leave the A/D bits management to hardware. */
>>>>> +    if ( svadu && !svade )
>>>>> +        return;
>>>>> +
>>>>> +    /* Case 4 */
>>>>> +    if ( svadu && svade ){
>>>>> +        if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
>>>>
>>>> Nit (style): Brace placement.
>>> Sorry for that. I will fix that in v3.
>>>> Furthermore this is written in a way which Misra would call "dead code". I'd
>>>> like to suggest (leaving out comments):
>>>>
>>>>      if ( svadu )
>>>>      {
>>>>          if ( !svade )
>>>>              return;
>>>>
>>>>          if ( !sbi_probe_extension(SBI_EXT_FWFT) )
>>>>              printk(...);
>>>>      }
>>> I assume you are referring to Misra C:2012 Rule 13.5 "The right operand
>>> of a logical && or || operand shall not contain persistent side effect"
>>>
>>> If yes, IMO, I think it doesn't apply here as `svade` is evaluated
>>> before the `if` so there is no side effect that wouldn't have been
>>> executed in case of svadu=false.
>>
>> No, there's nothing side-effect-ish here. With "svadu && !svade" in the
>> first if(), the rhs of "svadu && svade" in the second one is dead code:
>> Things would function the same with it dropped.
> Ok, now I understand, thanks. I'll fix it in next round.
>>
>>>>> +          printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but SBI FWFT is missing.\n"
>>>>> +                  "RISC-V: Defaulting to software A/D updates (Svade).\n"
>>>>> +                  "RISC-V: To force hardware A/D updates (Svadu), remove 'svade' from DT.\n");
>>>>
>>>> Nit (style): Indentation (in multiple ways). Furthermore XENLOG_* needs
>>>> repeating after every newline.
>>>>
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    /* Cases 1, 2: Xen assume Svade to be enabled */
>>>>> +    __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
>>>>
>>>> Isn't this a lie (to ourselves) then?
>>> If you are talking about case 1:
>>>      [1] Yes, it's technically a lie for boards shipped before
>>>      the svade/svadu extension was ratified (e.g., HiFive Premier P550).
>>>      These extensions merely formalized a mechanism that already existed in
>>>      hardware.
>>
>> Wait, how do you know this for _all_ boards anyone may ever have made?
> We don't know but based on [1] and my commit message, if neither
> Svade nor Svadu are present in DT then it is technically unknown whether
> the platform uses Svade or Svade. Hypervisor may then assume Svade to be
> present and enabled or it can discover based on mvendorid, marchid, and
> mimpid. For this patch, I choose to have the Hypervisor assumed Svade.

Can hypervisor really access this regs?

~ Oleksii

> 
> Saying that, I agree that it doesn't make sense to manually have set
> Svade extension in the isa bitfield as we could just preset A/D bits
> regardless of Svade/Svadu during the p2m_set_permission(). It's what
> kvm explains in kvm_riscv_gstage_map_page():
> 
>    /*
>     * A RISC-V implementation can choose to either:
>     * 1) Update 'A' and 'D' PTE bits in hardware
>     * 2) Generate page fault when 'A' and/or 'D' bits are not set
>     *    PTE so that software can update these bits.
>     *
>     * We support both options mentioned above. To achieve this, we
>     * always set 'A' and 'D' PTE bits at time of creating G-stage
>     * mapping. To support KVM dirty page logging with both options
>     * mentioned above, we will write-protect G-stage PTEs to track
>     * dirty pages.
>     */
> 
> 
> [1] https://lore.kernel.org/lkml/20240628093711.11716-1-yongxuan.wang@sifive.com/#t
>> And for all qemu (and alike) versions which supported RISC-V?
> 
> Concerning qemu, you're right, in case when (!svade && !svadu) they use
> by default Svadu (hw updating) for backward compatibility.
> 
>>
>>>      [2] For boards that do support svade, we could enforce DT
>>>      declaration by adding it to `required_extension` as they are
>>>      explicitly supporting it. However, doing so would cause boards
>>>      without svade/svadu support (as described above) to hit a panic
>>>      during boot.
>>>
>>>      So in both case ([1], [2]), the svade extension exist either implicitely or
>>>      explicitly. Therefore, force it doesn't compromize anything.
>>
>> If, despite my comment above, this is indeed what is wanted, I think it
>> requires a little more commentary.
>>
>> Jan
>>
>>
>>
> 
>