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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> +          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?
And for all qemu (and alike) versions which supported RISC-V?

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