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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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. 
It is unknown from DT point of view but it isn't true from h/w point of 
view. H/W knows what it supports Svade or Svadu. That is why I am not 
convinced that in p2m_set_permission() we should use DT binding 
explanation. The original comment is better as it describes all the 
possible from h/w point and not DT point of view. Also, generally 
nothing guarantee that DT is correct (someone miss to add Svade or Svadu 
in riscv,isa property) so make an explanation *only* based on it 
probably isn't the best one option and probably it will be better just 
have a comment as it was in original changes.
I think that the easiest option for us is to ...

> 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.
>
> 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
But then it means that in the case of Svadu we will have not precise 
statistics about A/D bits. I think that for p2m_set_permission() we 
still may want to have if () condition around as at the moment of 
p2m_set_permission is being called we could identify which A/D scheme is 
supported by h/w.
Therefore I think we still want to set ISA bitmap based on what I 
described below ...
> 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@xxxxxxxxxx/#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.
...to require the user to specify either Svade or Svadu in the DTS. If 
neither is mentioned in the DTS, the user should be prompted to choose 
one of the two options, since, from a hardware perspective, the hardware 
must support one of them.
Also, I think we could detect in runtime if Svade is supported but it is 
IMO overcomplication of the things instead of force use to put implicity 
Svade or Svadu in DTS.
~ Oleksii