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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

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.

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@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.
> > 
> >     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
> 
> 
>