[PATCH v6 0/4] xen/riscv: fix boot on missing extensions and MMU setup bugs

[PATCH v6 0/4] xen/riscv: fix boot on missing extensions and MMU setup bugs

[Baptiste Le Duc]

This series introduces bugs fixes that were found while bringing up CI
support for the HiFive Premier P550 board with a basic smoke test. The
board-support series itself will follow separately as it depends on
PLIC/vPLIC and dom0less support that have not been upstreamed yet. This
series carries only the independent fixes found along the way, none of them
need the board-support series to apply.

This series:
    1: Probe the PTE A/D bits update scheme at boot
    2: rework the A/D bits comment in p2m_set_pte_flags()
    3: Fix A/D bits in Xen's page-table mappings
    4. Use pte_is_valid() in pte_is_mapping()

Test CI pipeline (uses the QTB framework to pexpect the console under
various machine svade/svadu configs):
https://gitlab.com/xen-project/people/baptleduc/xen/-/pipelines/2941839143

---
Changes since v5:
- Replace "xen/riscv: always preset A/D bits in G-stage PTEs" with a boot
  time probe of the A/D update scheme (patch 1). It checks the result
  against the Svade/Svadu DT binding and records the probed scheme when the
  DT advertises neither, instead of trusting the DT alone.
- Split the p2m_set_pte_flags() comment rework out into its own patch
  (patch 2).
- Patches 3 and 4 unchanged and are waiting to be merged.
- Link to v5:
  https://patch.msgid.link/1791215925.8631fc262581453bbf619ec5b2062170.1a10cc9f7c20003f66@vates.tech
---
Changes since v4:
- Drop patches merged upstream:
    ccf9d78c825b ("xen/riscv: rename PTE "permissions" to "pte_flags"")
- Collect trailers.
- Rebase patches on top of staging.
- Link to v4: https://patch.msgid.link/1790845467.8631fc262581453bbf619ec5b2062170.1a0f6b5397a0003f66@vates.tech
---
Changes since v3:
- Drop v3 patches 1, 6 already upstreamed:
    861e44a3ba8e ("xen/riscv: make Zihintpause no longer a required
    extension")
    469d83d42f5c ("docs/riscv: sync required ISA extensions with
    required_extensions[]")
- Add trailers (A-by and/or R-by).
- Rename patch 3 to ("xen/riscv: always preset A/D bits in G-stage PTEs")
  as it now always presets A/D bits instead of distinguish Svadu/Svade DT
  bindings cases.
- Address review comments.
- Link to v3: https://patch.msgid.link/1790699381.8631fc262581453bbf619ec5b2062170.1a0ee00218b000b504@vates.tech
---
Changes since v2:
- Drop v2 patches 5 and 6, already upstreamed:
    d8ca89f26c8a ("xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu()")
    08a898b93d21 ("xen/riscv: fix level_map_mask truncation on load_start")
- Drop "xen/riscv: make Svpbmt no longer a required extension".
  It relaxed Svpbmt so boards without it, like the HiFive P550, could boot.
  But without Svpbmt, DMA-noncoherent devices do not snoop the CPU caches
  and can read stale data from DRAM. Mainline Linux has the same problem,
  which is explained and reproducible in [1]. Fixing it needs a lot of
  machinery we do not need yet, and Svpbmt is mandatory in RVA23, so
  keeping it required is simpler and safer. An RFC will follow with a fix
  inspired by [2].
- Add three new patches:
    docs/riscv: sync required ISA extensions with required_extensions[]
    xen/riscv: rename PTE "permissions" to "pte_flags"
    xen/riscv: use pte_is_valid() in pte_is_mapping()
- Address review comments.

[1] https://github.com/davidlohr/vanilla-kernel-sifive-p550/tree/master
[2] https://lwn.net/Articles/996819/
---
Changes since v1:
- address ML comments
- rename some patchs
- add new patch-fix: 242dd1f890e4 ("xen/riscv: fix level_map_mask
  truncation on load_start") discovered when working on Spacemit K3 support
---

To: xen-devel@lists.xenproject.org
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Anthony PERARD <anthony.perard@vates.tech>
Cc: Michal Orzel <michal.orzel@amd.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Roger Pau Monné <roger@xenproject.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Alistair Francis <alistair.francis@wdc.com>
Cc: Connor Davis <connojdavis@gmail.com>
Cc: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Cc: Zheng Zhang <zhangzheng@iscas.ac.cn>

---

---
Baptiste Le Duc (4):
      xen/riscv: probe the PTE A/D bits update scheme at boot
      xen/riscv: rework the A/D bits comment in p2m_set_pte_flags()
      xen/riscv: fix A/D bits in Xen's page-table mappings
      xen/riscv: use pte_is_valid() in pte_is_mapping()

 xen/arch/riscv/cpufeature.c             | 97 +++++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/cpufeature.h |  1 +
 xen/arch/riscv/include/asm/page.h       | 35 ++++++------
 xen/arch/riscv/mm.c                     | 11 ++--
 xen/arch/riscv/p2m.c                    | 42 ++++++--------
 5 files changed, 137 insertions(+), 49 deletions(-)
---
base-commit: 2c40f10b9cc1864b2991612f44d2410e37d57ebe
change-id: 20260902-riscv-fix-boot-missing-ext-a79c23bad694

Best regards,
--  
Baptiste Le Duc <baptiste.le-duc@vates.tech>

[PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Baptiste Le Duc]

p2m_set_pte_flags() presets the A/D bits of G-stage PTEs only when
RISCV_ISA_EXT_svade is set, which today only reflects the device tree.
Platforms that fault on clear A/D bits without advertising Svade, such as
the HiFive Premier P550, then leave A/D clear and take guest-page faults
that Xen does not handle. The device tree cannot be trusted alone to tell
which scheme is in effect.

Introduce probe_hw_ad_update(): map a scratch page through the FIX_MISC
fixmap slot with a PTE whose A bit is clear, and read from it under an
exception table fixup. The read succeeds if hardware updates A/D bits and
faults otherwise.

Introduce resolve_ad_scheme(), called from riscv_fill_hwcap(), to check the
probe against what the device tree advertises, following the DT binding:
- Svade and Svadu: if the probe faults, Svade applies. Warn that hardware
  updating can be used by dropping Svade from the device tree, or through
  SBI FWFT once Xen supports it. Otherwise hardware updating applies and
  nothing is needed.
- Svadu only: firmware must have enabled hardware updating. Panic if the
  probe faults.
- Svade only: the hart must not update A/D bits. Assert the probe faults.
- Neither: record the probed scheme by setting RISCV_ISA_EXT_svadu or
  RISCV_ISA_EXT_svade.

Add Svadu to the known ISA extensions so it can be told apart from Svade.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
Changes since v5:
- new patch, probe the A/D bits update scheme instead of trusting the
  device tree alone.
---
 xen/arch/riscv/cpufeature.c             | 97 +++++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/cpufeature.h |  1 +
 2 files changed, 98 insertions(+)

diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 3d5bbd2228..60e48652a6 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -20,6 +20,8 @@
 
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
+#include <asm/fixmap.h>
+#include <asm/pmap.h>
 
 #ifdef CONFIG_ACPI
 # error "cpufeature.c functions should be updated to support ACPI"
@@ -200,6 +202,7 @@ static const struct riscv_isa_ext_entry __initconstrel riscv_isa_ext[] = {
     RISCV_ISA_EXT_ENTRY(ssaia,          ANY),
     RISCV_ISA_EXT_ENTRY(sstc,           NONE),
     RISCV_ISA_EXT_ENTRY(svade,          NONE),
+    RISCV_ISA_EXT_ENTRY(svadu,          NONE),
     RISCV_ISA_EXT_ENTRY(svpbmt,         NONE),
 };
 
@@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
         panic("Failed to build guest \"riscv,isa\" string\n");
 }
 
+static bool __init probe_hw_ad_update(void)
+{
+    bool updated = false;
+    unsigned int slot = FIX_MISC;
+    vaddr_t va = FIXMAP_ADDR(slot);
+    struct page_info *pg;
+    pte_t *entry, pte;
+    mfn_t mfn;
+    unsigned long tmp;
+
+    pg = alloc_domheap_page(NULL, 0);
+    if ( !pg )
+        return false;
+
+    mfn = page_to_mfn(pg);
+
+    arch_pmap_map(FIX_MISC, mfn);
+    entry = &xen_fixmap[slot];
+    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
+    write_pte(entry, pte);
+
+    flush_tlb_one_local(va);
+
+    asm_inline volatile (
+        "1: ld %[tmp], 0(%[va])\n"
+        "   li %[updated], 1\n"
+        "2:\n"
+        ASM_EXTABLE(1b, 2b)
+        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
+        : [va] "r" (va)
+        : "memory" );
+
+    arch_pmap_unmap(slot);
+    free_domheap_page(pg);
+
+    return updated;
+}
+
+/*
+ * The privileged spec allows exactly two schemes for the PTE A and D bits:
+ *  1) hardware sets them on access and on write.
+ *  2) accessing a page whose A and/or D bit is clear raises a page fault,
+ *     so that software can set them (ratified as Svade).
+ *
+ * Hence an implementation without Svade uses the hardware scheme (1), and one
+ * that faults on clear A/D bits implements Svade, whether or not it says so.
+ *
+ * Svadu isn't itself a scheme: it adds menvcfg.ADUE, selecting between the
+ * two for S-mode and G-stage translation, and henvcfg.ADUE, doing the same
+ * for VS-stage translation (read-only zero when menvcfg.ADUE is zero).
+ *
+ * Rather than trusting the DTS alone, probe the scheme in effect with a read
+ * through a PTE whose A bit is clear, and check it against what the DTS
+ * advertises, as described by the DT binding. Panic on a mismatch Xen cannot
+ * run with, and record the probed scheme when the DTS advertises neither.
+ */
+static void __init 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);
+    bool has_hw_update = probe_hw_ad_update();
+
+    /* Both advertised: Svade applies until FWFT enables hardware updating. */
+    if ( svade && svadu )
+    {
+        if ( !has_hw_update )
+            printk(XENLOG_WARNING
+                   "Svade and Svadu are both advertised, but software updates A/D bits. "
+                   "To use hardware updating, drop Svade from the DTS or use FWFT (not implemented yet)\n");
+        return;
+    }
+
+    /* Only Svadu advertised: firmware must have enabled hardware updating. */
+    if ( svadu )
+    {
+        if ( !has_hw_update )
+            panic("Svadu is advertised but A/D bits are not updated by hardware\n");
+        return;
+    }
+
+    /* Only Svade advertised: the hart must not update A/D bits. */
+    if ( svade )
+    {
+        ASSERT(!has_hw_update);
+        return;
+    }
+
+    /* Neither advertised: trust the probe. */
+    __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
+              riscv_isa);
+}
+
 void init_guest_isa(struct domain *d)
 {
     d->arch.isa = guest_isa;
@@ -674,6 +769,8 @@ void __init riscv_fill_hwcap(void)
                "Zihintpause unavailable: cpu_relax() gives the CPU no hint; "
                "wire up this platform's pause equivalent in cpu_relax()\n");
 
+    resolve_ad_scheme();
+
     for ( i = 0; i < req_extns_amount; i++ )
     {
         const struct riscv_isa_ext_data ext = required_extensions[i];
diff --git a/xen/arch/riscv/include/asm/cpufeature.h b/xen/arch/riscv/include/asm/cpufeature.h
index 2973eb13a5..6fb8ad4b9c 100644
--- a/xen/arch/riscv/include/asm/cpufeature.h
+++ b/xen/arch/riscv/include/asm/cpufeature.h
@@ -42,6 +42,7 @@ enum riscv_isa_ext_id {
     RISCV_ISA_EXT_sstc,
     RISCV_ISA_EXT_svade,
     RISCV_ISA_EXT_svpbmt,
+    RISCV_ISA_EXT_svadu,
     RISCV_ISA_EXT_MAX
 };
 

-- 
2.55.0

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich]

On 13.10.2026 12:07, Baptiste Le Duc wrote:
> @@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
>          panic("Failed to build guest \"riscv,isa\" string\n");
>  }
>  
> +static bool __init probe_hw_ad_update(void)
> +{
> +    bool updated = false;
> +    unsigned int slot = FIX_MISC;
> +    vaddr_t va = FIXMAP_ADDR(slot);
> +    struct page_info *pg;
> +    pte_t *entry, pte;
> +    mfn_t mfn;
> +    unsigned long tmp;
> +
> +    pg = alloc_domheap_page(NULL, 0);
> +    if ( !pg )
> +        return false;

Why would you need a fresh page here? Can't you create a secondary mapping
of a page already in use (e.g. one from Xen's .data or .bss section, or the
stack page you're running on)?

> +    mfn = page_to_mfn(pg);
> +
> +    arch_pmap_map(FIX_MISC, mfn);
> +    entry = &xen_fixmap[slot];
> +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
> +    write_pte(entry, pte);
> +
> +    flush_tlb_one_local(va);
> +
> +    asm_inline volatile (
> +        "1: ld %[tmp], 0(%[va])\n"
> +        "   li %[updated], 1\n"

If you mapped the stack page, you could preset a variable there to 1, load
from the alias mapping of that variable, and then avoid the LI altogether.

> +        "2:\n"
> +        ASM_EXTABLE(1b, 2b)
> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> +        : [va] "r" (va)
> +        : "memory" );
> +
> +    arch_pmap_unmap(slot);
> +    free_domheap_page(pg);
> +
> +    return updated;
> +}

Imo you'd better check that A also was actually set in the PTE.

> +/*
> + * The privileged spec allows exactly two schemes for the PTE A and D bits:
> + *  1) hardware sets them on access and on write.
> + *  2) accessing a page whose A and/or D bit is clear raises a page fault,
> + *     so that software can set them (ratified as Svade).
> + *
> + * Hence an implementation without Svade uses the hardware scheme (1), and one
> + * that faults on clear A/D bits implements Svade, whether or not it says so.
> + *
> + * Svadu isn't itself a scheme: it adds menvcfg.ADUE, selecting between the
> + * two for S-mode and G-stage translation, and henvcfg.ADUE, doing the same
> + * for VS-stage translation (read-only zero when menvcfg.ADUE is zero).
> + *
> + * Rather than trusting the DTS alone, probe the scheme in effect with a read
> + * through a PTE whose A bit is clear, and check it against what the DTS
> + * advertises, as described by the DT binding. Panic on a mismatch Xen cannot
> + * run with, and record the probed scheme when the DTS advertises neither.
> + */
> +static void __init 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);
> +    bool has_hw_update = probe_hw_ad_update();
> +
> +    /* Both advertised: Svade applies until FWFT enables hardware updating. */
> +    if ( svade && svadu )
> +    {
> +        if ( !has_hw_update )
> +            printk(XENLOG_WARNING
> +                   "Svade and Svadu are both advertised, but software updates A/D bits. "
> +                   "To use hardware updating, drop Svade from the DTS or use FWFT (not implemented yet)\n");
> +        return;
> +    }
> +
> +    /* Only Svadu advertised: firmware must have enabled hardware updating. */
> +    if ( svadu )
> +    {
> +        if ( !has_hw_update )
> +            panic("Svadu is advertised but A/D bits are not updated by hardware\n");
> +        return;
> +    }
> +
> +    /* Only Svade advertised: the hart must not update A/D bits. */
> +    if ( svade )
> +    {
> +        ASSERT(!has_hw_update);
> +        return;
> +    }
> +
> +    /* Neither advertised: trust the probe. */
> +    __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
> +              riscv_isa);

Hmm, this feels fragile. Other schemes are possible, e.g. A being updated
by hardware but D being left to software. Are such truly precluded by the
spec, including older versions thereof? Furthermore, probe_hw_ad_update()
can (right now) return false for unrelated reasons.

As a consequence, patch 2 imo also can't leave the A/D pre-setting unaltered.
It perhaps needs doing unless Svadu is positively identified as present.

> --- a/xen/arch/riscv/include/asm/cpufeature.h
> +++ b/xen/arch/riscv/include/asm/cpufeature.h
> @@ -42,6 +42,7 @@ enum riscv_isa_ext_id {
>      RISCV_ISA_EXT_sstc,
>      RISCV_ISA_EXT_svade,
>      RISCV_ISA_EXT_svpbmt,
> +    RISCV_ISA_EXT_svadu,

Isn't there some sort of sorting in effect here?

Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Baptiste Le Duc]

On 2026-10-13 12:36 +0200, Jan Beulich wrote:
> On 13.10.2026 12:07, Baptiste Le Duc wrote:
> > @@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
> >          panic("Failed to build guest \"riscv,isa\" string\n");
> >  }
> >  
> > +static bool __init probe_hw_ad_update(void)
> > +{
> > +    bool updated = false;
> > +    unsigned int slot = FIX_MISC;
> > +    vaddr_t va = FIXMAP_ADDR(slot);
> > +    struct page_info *pg;
> > +    pte_t *entry, pte;
> > +    mfn_t mfn;
> > +    unsigned long tmp;
> > +
> > +    pg = alloc_domheap_page(NULL, 0);
> > +    if ( !pg )
> > +        return false;
> 
> Why would you need a fresh page here? Can't you create a secondary mapping
> of a page already in use (e.g. one from Xen's .data or .bss section, or the
> stack page you're running on)?
You're right, I will map the current stack page.
> 
> > +    mfn = page_to_mfn(pg);
> > +
> > +    arch_pmap_map(FIX_MISC, mfn);
> > +    entry = &xen_fixmap[slot];
> > +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
> > +    write_pte(entry, pte);
> > +
> > +    flush_tlb_one_local(va);
> > +
> > +    asm_inline volatile (
> > +        "1: ld %[tmp], 0(%[va])\n"
> > +        "   li %[updated], 1\n"
> 
> If you mapped the stack page, you could preset a variable there to 1, load
> from the alias mapping of that variable, and then avoid the LI altogether.
Yes mapping the stack page appears to be the best way.
> 
> > +        "2:\n"
> > +        ASM_EXTABLE(1b, 2b)
> > +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> > +        : [va] "r" (va)
> > +        : "memory" );
> > +
> > +    arch_pmap_unmap(slot);
> > +    free_domheap_page(pg);
> > +
> > +    return updated;
> > +}
> 
> Imo you'd better check that A also was actually set in the PTE.
Right, I will do that.
> 
> > +
> > +    /* Neither advertised: trust the probe. */
> > +    __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
> > +              riscv_isa);
> 
> Hmm, this feels fragile. Other schemes are possible, e.g. A being updated
> by hardware but D being left to software. Are such truly precluded by the
> spec, including older versions thereof? Furthermore, probe_hw_ad_update()
> can (right now) return false for unrelated reasons.
Since 1.10 the spec lists exactly two schemes, and each one covers A and
D together: either a clear A (or a clear D on a store) raises a page
fault, or hardware sets the corresponding bit(s). A-bit being set by
hardware while D is left to software isn't one of them. Before 1.10, the
hardware must set the two bits. Therefore, there is not other schemes
possible.
> As a consequence, patch 2 imo also can't leave the A/D pre-setting unaltered.
> It perhaps needs doing unless Svadu is positively identified as present.
I agree it is more robust to preset A/D bits unless Svadu is truly
identified, i.e. advertised in the DTS (without Svade) and confirmed by
probe_hw_ad_update(). The worst case then becomes the probe failing for
an unrelated reason on hardware which does update A/D: we'd preset A/D
needlessly, which is harmless, as hardware only writes the PTE when a
bit is clear.

To sum up, and to avoid unnecessary respins, would you be fine with:
1) Keeping the probe as it is, with your remarks addressed (map stack page).
2) Presetting A/D bits unless Svadu is truly identified as above.
> 
> > --- a/xen/arch/riscv/include/asm/cpufeature.h
> > +++ b/xen/arch/riscv/include/asm/cpufeature.h
> > @@ -42,6 +42,7 @@ enum riscv_isa_ext_id {
> >      RISCV_ISA_EXT_sstc,
> >      RISCV_ISA_EXT_svade,
> >      RISCV_ISA_EXT_svpbmt,
> > +    RISCV_ISA_EXT_svadu,
> 
> Isn't there some sort of sorting in effect here?
No, as it is mentioned in the docstring that:
    * New extensions should just be added to the bottom, rather than added
    * alphabetically, in order to avoid unnecessary shuffling.

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich]

On 13.10.2026 13:57, Baptiste Le Duc wrote:
> On 2026-10-13 12:36 +0200, Jan Beulich wrote:
>> On 13.10.2026 12:07, Baptiste Le Duc wrote:
>>> @@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
>>>          panic("Failed to build guest \"riscv,isa\" string\n");
>>>  }
>>>  
>>> +static bool __init probe_hw_ad_update(void)
>>> +{
>>> +    bool updated = false;
>>> +    unsigned int slot = FIX_MISC;
>>> +    vaddr_t va = FIXMAP_ADDR(slot);
>>> +    struct page_info *pg;
>>> +    pte_t *entry, pte;
>>> +    mfn_t mfn;
>>> +    unsigned long tmp;
>>> +
>>> +    pg = alloc_domheap_page(NULL, 0);
>>> +    if ( !pg )
>>> +        return false;
>>
>> Why would you need a fresh page here? Can't you create a secondary mapping
>> of a page already in use (e.g. one from Xen's .data or .bss section, or the
>> stack page you're running on)?
> You're right, I will map the current stack page.
>>
>>> +    mfn = page_to_mfn(pg);
>>> +
>>> +    arch_pmap_map(FIX_MISC, mfn);
>>> +    entry = &xen_fixmap[slot];
>>> +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
>>> +    write_pte(entry, pte);
>>> +
>>> +    flush_tlb_one_local(va);
>>> +
>>> +    asm_inline volatile (
>>> +        "1: ld %[tmp], 0(%[va])\n"
>>> +        "   li %[updated], 1\n"
>>
>> If you mapped the stack page, you could preset a variable there to 1, load
>> from the alias mapping of that variable, and then avoid the LI altogether.
> Yes mapping the stack page appears to be the best way.
>>
>>> +        "2:\n"
>>> +        ASM_EXTABLE(1b, 2b)
>>> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
>>> +        : [va] "r" (va)
>>> +        : "memory" );
>>> +
>>> +    arch_pmap_unmap(slot);
>>> +    free_domheap_page(pg);
>>> +
>>> +    return updated;
>>> +}
>>
>> Imo you'd better check that A also was actually set in the PTE.
> Right, I will do that.
>>
>>> +
>>> +    /* Neither advertised: trust the probe. */
>>> +    __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
>>> +              riscv_isa);
>>
>> Hmm, this feels fragile. Other schemes are possible, e.g. A being updated
>> by hardware but D being left to software. Are such truly precluded by the
>> spec, including older versions thereof? Furthermore, probe_hw_ad_update()
>> can (right now) return false for unrelated reasons.
> Since 1.10 the spec lists exactly two schemes, and each one covers A and
> D together: either a clear A (or a clear D on a store) raises a page
> fault, or hardware sets the corresponding bit(s). A-bit being set by
> hardware while D is left to software isn't one of them. Before 1.10, the
> hardware must set the two bits. Therefore, there is not other schemes
> possible.
>> As a consequence, patch 2 imo also can't leave the A/D pre-setting unaltered.
>> It perhaps needs doing unless Svadu is positively identified as present.
> I agree it is more robust to preset A/D bits unless Svadu is truly
> identified, i.e. advertised in the DTS (without Svade) and confirmed by
> probe_hw_ad_update(). The worst case then becomes the probe failing for
> an unrelated reason on hardware which does update A/D: we'd preset A/D
> needlessly, which is harmless, as hardware only writes the PTE when a
> bit is clear.
> 
> To sum up, and to avoid unnecessary respins, would you be fine with:
> 1) Keeping the probe as it is, with your remarks addressed (map stack page).

(... and verify A bit was set).

> 2) Presetting A/D bits unless Svadu is truly identified as above.

Yes. Assuming Oleksii also agrees.

>>> --- a/xen/arch/riscv/include/asm/cpufeature.h
>>> +++ b/xen/arch/riscv/include/asm/cpufeature.h
>>> @@ -42,6 +42,7 @@ enum riscv_isa_ext_id {
>>>      RISCV_ISA_EXT_sstc,
>>>      RISCV_ISA_EXT_svade,
>>>      RISCV_ISA_EXT_svpbmt,
>>> +    RISCV_ISA_EXT_svadu,
>>
>> Isn't there some sort of sorting in effect here?
> No, as it is mentioned in the docstring that:
>     * New extensions should just be added to the bottom, rather than added
>     * alphabetically, in order to avoid unnecessary shuffling.

I see. Yet I wonder: What shuffling would we need to be afraid of? Always
inserting at the bottom increases the risk of patch conflicts. Furthermore
single-letter extensions can't be inserted at the bottom anyway.

Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Oleksii Kurochko]

On 10/13/26 5:24 PM, Jan Beulich wrote:
>>>> --- a/xen/arch/riscv/include/asm/cpufeature.h
>>>> +++ b/xen/arch/riscv/include/asm/cpufeature.h
>>>> @@ -42,6 +42,7 @@ enum riscv_isa_ext_id {
>>>>       RISCV_ISA_EXT_sstc,
>>>>       RISCV_ISA_EXT_svade,
>>>>       RISCV_ISA_EXT_svpbmt,
>>>> +    RISCV_ISA_EXT_svadu,
>>> Isn't there some sort of sorting in effect here?
>> No, as it is mentioned in the docstring that:
>>      * New extensions should just be added to the bottom, rather than added
>>      * alphabetically, in order to avoid unnecessary shuffling.
> I see. Yet I wonder: What shuffling would we need to be afraid of? Always
> inserting at the bottom increases the risk of patch conflicts. Furthermore
> single-letter extensions can't be inserted at the bottom anyway.

None, really. The comment was copied from Linux together with the first
version of this code, where the IDs were plain macros with explicit values:

   #define RISCV_ISA_EXT_SSTC      27
   #define RISCV_ISA_EXT_SVINVAL   28
   ...

With such definitions an alphabetical insertion means renumbering every
following line, which is the "shuffling" the comment refers to (see 
Linux commit dac8bf14bb49 "riscv: hwcap: Don't alphabetize ISA extension 
IDs"). When the IDs were converted to enum riscv_isa_ext_id, the comment 
was kept as is, although the reason for it went away: the values are 
assigned by the compiler and nothing depends on them.

So I think the comment should be updated: drop the "add to the bottom"
part (together with the leftover "macros" wording) and say that
multi-letter extensions follow the same order as riscv_isa_ext[], above
which the ordering rules are already described. Something like:

--- a/xen/arch/riscv/include/asm/cpufeature.h
+++ b/xen/arch/riscv/include/asm/cpufeature.h
@@ -8,13 +8,13 @@
  #include <xen/types.h>

  /*
- * These macros represent the logical IDs of each multi-letter RISC-V ISA
- * extension and are used in the ISA bitmap. The logical IDs start from
- * RISCV_ISA_EXT_BASE, which allows the 0-25 range to be reserved for 
single
- * letter extensions and are used in enum riscv_isa_ext_id.
+ * Logical IDs of RISC-V ISA extensions, used as bit positions in the ISA
+ * bitmap.
   *
- * New extensions should just be added to the bottom, rather than added
- * alphabetically, in order to avoid unnecessary shuffling.
+ * Single-letter extensions use IDs below RISCV_ISA_EXT_BASE and are sorted
+ * alphabetically. Multi-letter extensions start from 
RISCV_ISA_EXT_BASE and
+ * follow the same order as riscv_isa_ext[] (see the ordering rules 
described
+ * above it).
   */
  #define RISCV_ISA_EXT_BASE  26

Accordingly, svadu will go between svade and svpbmt, both in the enum 
and in riscv_isa_ext[].

~ Oleksii

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Baptiste Le Duc]

On 2026-10-13 12:36 +0200, Jan Beulich wrote:
> On 13.10.2026 12:07, Baptiste Le Duc wrote:
> > @@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
> >          panic("Failed to build guest \"riscv,isa\" string\n");
> >  }
> >  
> > +static bool __init probe_hw_ad_update(void)
> > +{
> > +    bool updated = false;
> > +    unsigned int slot = FIX_MISC;
> > +    vaddr_t va = FIXMAP_ADDR(slot);
> > +    struct page_info *pg;
> > +    pte_t *entry, pte;
> > +    mfn_t mfn;
> > +    unsigned long tmp;
> > +
> > +    pg = alloc_domheap_page(NULL, 0);
> > +    if ( !pg )
> > +        return false;
> 
> Why would you need a fresh page here? Can't you create a secondary mapping
> of a page already in use (e.g. one from Xen's .data or .bss section, or the
> stack page you're running on)?
> 
> > +    mfn = page_to_mfn(pg);
> > +
> > +    arch_pmap_map(FIX_MISC, mfn);
> > +    entry = &xen_fixmap[slot];
> > +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
> > +    write_pte(entry, pte);
> > +
> > +    flush_tlb_one_local(va);
> > +
> > +    asm_inline volatile (
> > +        "1: ld %[tmp], 0(%[va])\n"
> > +        "   li %[updated], 1\n"
> 
> If you mapped the stack page, you could preset a variable there to 1, load
> from the alias mapping of that variable, and then avoid the LI altogether.
> 
> > +        "2:\n"
> > +        ASM_EXTABLE(1b, 2b)
> > +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> > +        : [va] "r" (va)
> > +        : "memory" );
> > +
> > +    arch_pmap_unmap(slot);
> > +    free_domheap_page(pg);
> > +
> > +    return updated;
> > +}
> 
> Imo you'd better check that A also was actually set in the PTE.

If I check that A was set, can I drop the "updated" variable? The A bit
would tell us more than whether the load succeeded.

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich]

On 13.10.2026 16:32, Baptiste Le Duc wrote:
> On 2026-10-13 12:36 +0200, Jan Beulich wrote:
>> On 13.10.2026 12:07, Baptiste Le Duc wrote:
>>> @@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
>>>          panic("Failed to build guest \"riscv,isa\" string\n");
>>>  }
>>>  
>>> +static bool __init probe_hw_ad_update(void)
>>> +{
>>> +    bool updated = false;
>>> +    unsigned int slot = FIX_MISC;
>>> +    vaddr_t va = FIXMAP_ADDR(slot);
>>> +    struct page_info *pg;
>>> +    pte_t *entry, pte;
>>> +    mfn_t mfn;
>>> +    unsigned long tmp;
>>> +
>>> +    pg = alloc_domheap_page(NULL, 0);
>>> +    if ( !pg )
>>> +        return false;
>>
>> Why would you need a fresh page here? Can't you create a secondary mapping
>> of a page already in use (e.g. one from Xen's .data or .bss section, or the
>> stack page you're running on)?
>>
>>> +    mfn = page_to_mfn(pg);
>>> +
>>> +    arch_pmap_map(FIX_MISC, mfn);
>>> +    entry = &xen_fixmap[slot];
>>> +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
>>> +    write_pte(entry, pte);
>>> +
>>> +    flush_tlb_one_local(va);
>>> +
>>> +    asm_inline volatile (
>>> +        "1: ld %[tmp], 0(%[va])\n"
>>> +        "   li %[updated], 1\n"
>>
>> If you mapped the stack page, you could preset a variable there to 1, load
>> from the alias mapping of that variable, and then avoid the LI altogether.
>>
>>> +        "2:\n"
>>> +        ASM_EXTABLE(1b, 2b)
>>> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
>>> +        : [va] "r" (va)
>>> +        : "memory" );
>>> +
>>> +    arch_pmap_unmap(slot);
>>> +    free_domheap_page(pg);
>>> +
>>> +    return updated;
>>> +}
>>
>> Imo you'd better check that A also was actually set in the PTE.
> 
> If I check that A was set, can I drop the "updated" variable? The A bit
> would tell us more than whether the load succeeded.

Hmm, I'm inclined to suggest to check both.

There's one aspect to consider that I didn't think of earlier: On x86, for
example, the bit corresponding to A can also be set speculatively (the bit
corresponding to D cannot iirc). If the same is permitted on RISC-V, it
cannot be the A bit that you use for probing.

Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Baptiste Le Duc]

> >>> +        "2:\n"
> >>> +        ASM_EXTABLE(1b, 2b)
> >>> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> >>> +        : [va] "r" (va)
> >>> +        : "memory" );
> >>> +
> >>> +    arch_pmap_unmap(slot);
> >>> +    free_domheap_page(pg);
> >>> +
> >>> +    return updated;
> >>> +}
> >>
> >> Imo you'd better check that A also was actually set in the PTE.
> > 
> > If I check that A was set, can I drop the "updated" variable? The A bit
> > would tell us more than whether the load succeeded.
> 
> Hmm, I'm inclined to suggest to check both.
Then I'd rather probe D than A, since D, unlike A, cannot be set
speculatively (spec: "updates to the D bit, resulting from an explicit store,
must be exact (i.e., non-speculative)"). The result would
then be:

      return updated && d_set;

with a warning (or panic()?) on updated && !d_set, which the spec
doesn't allow (spec: "the access must not be performed before the PTE update is
globally visible").

Furthermore, if I understood your earlier suggestion correctly, you'd
like the A/D bits to be preset in p2m_set_pte_flags() unless Svadu is
in use:

    if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

However, when both Svade and Svadu are advertised, Svade is in effect
until Svadu is enabled via FWFT (perfectly legit), so this condition
would skip the preset and the first access would fault.

I see two ways to address this:

1) Keep checking Svade:

    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

    After resolve_ad_scheme(), the Svade bit is set in every case where
    software has to preset the A/D bits. The only imprecise case is both
    extensions advertised with hardware updating already enabled by
    firmware, where presetting A/D is unnecessary but harmless.

2) Record the probe result in a global, e.g.

    static bool __ro_after_init hw_ad_update;

    and use:

    if ( !hw_ad_update )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

    This is exact in all cases, at the cost of a new global that mostly
    duplicates what resolve_ad_scheme() already records in the ISA bitmap.

I discussed with Oleksii and we are fine with either. What do you prefer?

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich]

On 14.10.2026 11:00, Baptiste Le Duc wrote:
>>>>> +        "2:\n"
>>>>> +        ASM_EXTABLE(1b, 2b)
>>>>> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
>>>>> +        : [va] "r" (va)
>>>>> +        : "memory" );
>>>>> +
>>>>> +    arch_pmap_unmap(slot);
>>>>> +    free_domheap_page(pg);
>>>>> +
>>>>> +    return updated;
>>>>> +}
>>>>
>>>> Imo you'd better check that A also was actually set in the PTE.
>>>
>>> If I check that A was set, can I drop the "updated" variable? The A bit
>>> would tell us more than whether the load succeeded.
>>
>> Hmm, I'm inclined to suggest to check both.
> Then I'd rather probe D than A, since D, unlike A, cannot be set
> speculatively (spec: "updates to the D bit, resulting from an explicit store,
> must be exact (i.e., non-speculative)"). The result would
> then be:
> 
>       return updated && d_set;
> 
> with a warning (or panic()?) on updated && !d_set, which the spec
> doesn't allow (spec: "the access must not be performed before the PTE update is
> globally visible").
> 
> Furthermore, if I understood your earlier suggestion correctly, you'd
> like the A/D bits to be preset in p2m_set_pte_flags() unless Svadu is
> in use:
> 
>     if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> However, when both Svade and Svadu are advertised, Svade is in effect
> until Svadu is enabled via FWFT (perfectly legit), so this condition
> would skip the preset and the first access would fault.

In which case it would need to be

    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
         !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

albeit I have to admit that permitting two contradicting extensions to
be exposed at the same time is irritating.

> I see two ways to address this:
> 
> 1) Keep checking Svade:
> 
>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
>     After resolve_ad_scheme(), the Svade bit is set in every case where
>     software has to preset the A/D bits. The only imprecise case is both
>     extensions advertised with hardware updating already enabled by
>     firmware, where presetting A/D is unnecessary but harmless.
> 
> 2) Record the probe result in a global, e.g.
> 
>     static bool __ro_after_init hw_ad_update;
> 
>     and use:
> 
>     if ( !hw_ad_update )
>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
>     This is exact in all cases, at the cost of a new global that mostly
>     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
> 
> I discussed with Oleksii and we are fine with either. What do you prefer?

As to 1), see above. For 2), while maybe okay, I think we're better off
not deriving too much from probing. Whatever can be (reliably) derived from
what the platform description says should be preferred imo.

Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Oleksii Kurochko]

On 10/14/26 11:09 AM, Jan Beulich wrote:
> On 14.10.2026 11:00, Baptiste Le Duc wrote:
>>>>>> +        "2:\n"
>>>>>> +        ASM_EXTABLE(1b, 2b)
>>>>>> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
>>>>>> +        : [va] "r" (va)
>>>>>> +        : "memory" );
>>>>>> +
>>>>>> +    arch_pmap_unmap(slot);
>>>>>> +    free_domheap_page(pg);
>>>>>> +
>>>>>> +    return updated;
>>>>>> +}
>>>>>
>>>>> Imo you'd better check that A also was actually set in the PTE.
>>>>
>>>> If I check that A was set, can I drop the "updated" variable? The A bit
>>>> would tell us more than whether the load succeeded.
>>>
>>> Hmm, I'm inclined to suggest to check both.
>> Then I'd rather probe D than A, since D, unlike A, cannot be set
>> speculatively (spec: "updates to the D bit, resulting from an explicit store,
>> must be exact (i.e., non-speculative)"). The result would
>> then be:
>>
>>        return updated && d_set;
>>
>> with a warning (or panic()?) on updated && !d_set, which the spec
>> doesn't allow (spec: "the access must not be performed before the PTE update is
>> globally visible").
>>
>> Furthermore, if I understood your earlier suggestion correctly, you'd
>> like the A/D bits to be preset in p2m_set_pte_flags() unless Svadu is
>> in use:
>>
>>      if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>
>> However, when both Svade and Svadu are advertised, Svade is in effect
>> until Svadu is enabled via FWFT (perfectly legit), so this condition
>> would skip the preset and the first access would fault.
> 
> In which case it would need to be
> 
>      if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
>           !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> albeit I have to admit that permitting two contradicting extensions to
> be exposed at the same time is irritating.
> 
>> I see two ways to address this:
>>
>> 1) Keep checking Svade:
>>
>>      if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>
>>      After resolve_ad_scheme(), the Svade bit is set in every case where
>>      software has to preset the A/D bits. The only imprecise case is both
>>      extensions advertised with hardware updating already enabled by
>>      firmware, where presetting A/D is unnecessary but harmless.
>>
>> 2) Record the probe result in a global, e.g.
>>
>>      static bool __ro_after_init hw_ad_update;
>>
>>      and use:
>>
>>      if ( !hw_ad_update )
>>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>
>>      This is exact in all cases, at the cost of a new global that mostly
>>      duplicates what resolve_ad_scheme() already records in the ISA bitmap.
>>
>> I discussed with Oleksii and we are fine with either. What do you prefer?
> 
> As to 1), see above. For 2), while maybe okay, I think we're better off
> not deriving too much from probing. Whatever can be (reliably) derived from
> what the platform description says should be preferred imo.

Option 1 looks okay to me.

Also, it seems then we could simplify resolve_ad_scheme() to:

/*
  * p2m_set_pte_flags() presets the A/D bits unless only Svadu is 
advertised,
  * as per the DT binding that is the only case in which hardware 
updating is
  * guaranteed to be enabled (by firmware, via menvcfg.ADUE). Check that it
  * really is, and fall back to presetting A/D otherwise: a hart which 
faults
  * on clear A/D bits implements Svade, whether or not it says so.
  */
static void __init resolve_ad_scheme(void)
{
     if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) ||
          riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
         return;

     if ( !probe_hw_ad_update() )
     {
         printk(XENLOG_WARNING
                "Svadu is advertised but A/D bits aren't updated by 
hardware, presetting them\n");
         __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
     }
}

~ Oleksii

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Baptiste Le Duc]

On 2026-10-14 11:09:58+02:00, Jan Beulich wrote:
> On 14.10.2026 11:00, Baptiste Le Duc wrote:
> 
> In which case it would need to be
> 
>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
>          !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> albeit I have to admit that permitting two contradicting extensions to
> be exposed at the same time is irritating.
> 
> > I see two ways to address this:
> > 
> > 1) Keep checking Svade:
> > 
> >     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> >         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> > 
> >     After resolve_ad_scheme(), the Svade bit is set in every case where
> >     software has to preset the A/D bits. The only imprecise case is both
> >     extensions advertised with hardware updating already enabled by
> >     firmware, where presetting A/D is unnecessary but harmless.
> > 
> > 2) Record the probe result in a global, e.g.
> > 
> >     static bool __ro_after_init hw_ad_update;
> > 
> >     and use:
> > 
> >     if ( !hw_ad_update )
> >         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> > 
> >     This is exact in all cases, at the cost of a new global that mostly
> >     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
> > 
> > I discussed with Oleksii and we are fine with either. What do you prefer?
> 
> As to 1), see above. For 2), while maybe okay, I think we're better off
Sorry, I don't fully follow the issue with 1). Compared to what you proposed,
it only differs when neither Svade nor Svadu is recorded, which cannot
happen as resolve_ad_scheme() runs before and always records one of the
two.

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich]

On 14.10.2026 18:45, Baptiste Le Duc wrote:
> On 2026-10-14 11:09:58+02:00, Jan Beulich wrote:
>> On 14.10.2026 11:00, Baptiste Le Duc wrote:
>>
>> In which case it would need to be
>>
>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
>>          !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>
>> albeit I have to admit that permitting two contradicting extensions to
>> be exposed at the same time is irritating.
>>
>>> I see two ways to address this:
>>>
>>> 1) Keep checking Svade:
>>>
>>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>
>>>     After resolve_ad_scheme(), the Svade bit is set in every case where
>>>     software has to preset the A/D bits. The only imprecise case is both
>>>     extensions advertised with hardware updating already enabled by
>>>     firmware, where presetting A/D is unnecessary but harmless.
>>>
>>> 2) Record the probe result in a global, e.g.
>>>
>>>     static bool __ro_after_init hw_ad_update;
>>>
>>>     and use:
>>>
>>>     if ( !hw_ad_update )
>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>
>>>     This is exact in all cases, at the cost of a new global that mostly
>>>     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
>>>
>>> I discussed with Oleksii and we are fine with either. What do you prefer?
>>
>> As to 1), see above. For 2), while maybe okay, I think we're better off
> Sorry, I don't fully follow the issue with 1). Compared to what you proposed,
> it only differs when neither Svade nor Svadu is recorded, which cannot
> happen as resolve_ad_scheme() runs before and always records one of the
> two.

Yet previously we said that recording extensions (as such, i.e. later also
possibly exposing them to guests) when they weren't really advertised may
not be the right thing to do. I.e. imo the probing may be used to drive
Xen's internal behavior, but I'm unconvinced that it should also lead to
marking extensions as available.

Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Baptiste Le Duc]

On 2026-10-15 09:14 +0200, Jan Beulich wrote:
> On 14.10.2026 18:45, Baptiste Le Duc wrote:
> > On 2026-10-14 11:09:58+02:00, Jan Beulich wrote:
> >> On 14.10.2026 11:00, Baptiste Le Duc wrote:
> >>
> >> In which case it would need to be
> >>
> >>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
> >>          !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
> >>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >>
> >> albeit I have to admit that permitting two contradicting extensions to
> >> be exposed at the same time is irritating.
> >>
> >>> I see two ways to address this:
> >>>
> >>> 1) Keep checking Svade:
> >>>
> >>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> >>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >>>
> >>>     After resolve_ad_scheme(), the Svade bit is set in every case where
> >>>     software has to preset the A/D bits. The only imprecise case is both
> >>>     extensions advertised with hardware updating already enabled by
> >>>     firmware, where presetting A/D is unnecessary but harmless.
> >>>
> >>> 2) Record the probe result in a global, e.g.
> >>>
> >>>     static bool __ro_after_init hw_ad_update;
> >>>
> >>>     and use:
> >>>
> >>>     if ( !hw_ad_update )
> >>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >>>
> >>>     This is exact in all cases, at the cost of a new global that mostly
> >>>     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
> >>>
> >>> I discussed with Oleksii and we are fine with either. What do you prefer?
> >>
> >> As to 1), see above. For 2), while maybe okay, I think we're better off
> > Sorry, I don't fully follow the issue with 1). Compared to what you proposed,
> > it only differs when neither Svade nor Svadu is recorded, which cannot
> > happen as resolve_ad_scheme() runs before and always records one of the
> > two.
> 
> Yet previously we said that recording extensions (as such, i.e. later also
> possibly exposing them to guests) when they weren't really advertised may
> not be the right thing to do. I.e. imo the probing may be used to drive
But we also said that platforms (such as the P550) may implement an
extension without advertising it, as a RISC-V spec maintainer
confirmed in [1].

[1] https://github.com/riscv/riscv-isa-manual/issues/3450#issuecomment-6076851535
> Xen's internal behavior, but I'm unconvinced that it should also lead to
> marking extensions as available.

Therefore, as far as I understood, you're not convinced of this:

    /* Neither advertised: trust the probe. */
    __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
              riscv_isa);

If so, then, the only solution I have in mind in what I proposed in 2)
with the global variable that store the result of the real probe.
Am I correct?

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich]

On 15.10.2026 10:28, Baptiste Le Duc wrote:
> On 2026-10-15 09:14 +0200, Jan Beulich wrote:
>> On 14.10.2026 18:45, Baptiste Le Duc wrote:
>>> On 2026-10-14 11:09:58+02:00, Jan Beulich wrote:
>>>> On 14.10.2026 11:00, Baptiste Le Duc wrote:
>>>>
>>>> In which case it would need to be
>>>>
>>>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
>>>>          !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>
>>>> albeit I have to admit that permitting two contradicting extensions to
>>>> be exposed at the same time is irritating.
>>>>
>>>>> I see two ways to address this:
>>>>>
>>>>> 1) Keep checking Svade:
>>>>>
>>>>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>
>>>>>     After resolve_ad_scheme(), the Svade bit is set in every case where
>>>>>     software has to preset the A/D bits. The only imprecise case is both
>>>>>     extensions advertised with hardware updating already enabled by
>>>>>     firmware, where presetting A/D is unnecessary but harmless.
>>>>>
>>>>> 2) Record the probe result in a global, e.g.
>>>>>
>>>>>     static bool __ro_after_init hw_ad_update;
>>>>>
>>>>>     and use:
>>>>>
>>>>>     if ( !hw_ad_update )
>>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>
>>>>>     This is exact in all cases, at the cost of a new global that mostly
>>>>>     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
>>>>>
>>>>> I discussed with Oleksii and we are fine with either. What do you prefer?
>>>>
>>>> As to 1), see above. For 2), while maybe okay, I think we're better off
>>> Sorry, I don't fully follow the issue with 1). Compared to what you proposed,
>>> it only differs when neither Svade nor Svadu is recorded, which cannot
>>> happen as resolve_ad_scheme() runs before and always records one of the
>>> two.
>>
>> Yet previously we said that recording extensions (as such, i.e. later also
>> possibly exposing them to guests) when they weren't really advertised may
>> not be the right thing to do. I.e. imo the probing may be used to drive
> But we also said that platforms (such as the P550) may implement an
> extension without advertising it, as a RISC-V spec maintainer
> confirmed in [1].
> 
> [1] https://github.com/riscv/riscv-isa-manual/issues/3450#issuecomment-6076851535

What Andrew says there suggests to me that the behavior you observe on P550
is against the spec. That shouldn't really be the case though, as nothing
was said in the spec at all until Svade and Svadu were actually introduced.
An update to the spec should imo never invalidate previously conforming
implementations. I guess I'll comment there.

>> Xen's internal behavior, but I'm unconvinced that it should also lead to
>> marking extensions as available.
> 
> Therefore, as far as I understood, you're not convinced of this:
> 
>     /* Neither advertised: trust the probe. */
>     __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
>               riscv_isa);
> 
> If so, then, the only solution I have in mind in what I proposed in 2)
> with the global variable that store the result of the real probe.
> Am I correct?

Possibly. What I'm missing is some clarification towards the underlying
principle which we want to follow not just here. 

Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich]

On 15.10.2026 10:41, Jan Beulich wrote:
> On 15.10.2026 10:28, Baptiste Le Duc wrote:
>> On 2026-10-15 09:14 +0200, Jan Beulich wrote:
>>> On 14.10.2026 18:45, Baptiste Le Duc wrote:
>>>> On 2026-10-14 11:09:58+02:00, Jan Beulich wrote:
>>>>> On 14.10.2026 11:00, Baptiste Le Duc wrote:
>>>>>
>>>>> In which case it would need to be
>>>>>
>>>>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
>>>>>          !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>
>>>>> albeit I have to admit that permitting two contradicting extensions to
>>>>> be exposed at the same time is irritating.
>>>>>
>>>>>> I see two ways to address this:
>>>>>>
>>>>>> 1) Keep checking Svade:
>>>>>>
>>>>>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>>
>>>>>>     After resolve_ad_scheme(), the Svade bit is set in every case where
>>>>>>     software has to preset the A/D bits. The only imprecise case is both
>>>>>>     extensions advertised with hardware updating already enabled by
>>>>>>     firmware, where presetting A/D is unnecessary but harmless.
>>>>>>
>>>>>> 2) Record the probe result in a global, e.g.
>>>>>>
>>>>>>     static bool __ro_after_init hw_ad_update;
>>>>>>
>>>>>>     and use:
>>>>>>
>>>>>>     if ( !hw_ad_update )
>>>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>>
>>>>>>     This is exact in all cases, at the cost of a new global that mostly
>>>>>>     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
>>>>>>
>>>>>> I discussed with Oleksii and we are fine with either. What do you prefer?
>>>>>
>>>>> As to 1), see above. For 2), while maybe okay, I think we're better off
>>>> Sorry, I don't fully follow the issue with 1). Compared to what you proposed,
>>>> it only differs when neither Svade nor Svadu is recorded, which cannot
>>>> happen as resolve_ad_scheme() runs before and always records one of the
>>>> two.
>>>
>>> Yet previously we said that recording extensions (as such, i.e. later also
>>> possibly exposing them to guests) when they weren't really advertised may
>>> not be the right thing to do. I.e. imo the probing may be used to drive
>> But we also said that platforms (such as the P550) may implement an
>> extension without advertising it, as a RISC-V spec maintainer
>> confirmed in [1].
>>
>> [1] https://github.com/riscv/riscv-isa-manual/issues/3450#issuecomment-6076851535
> 
> What Andrew says there suggests to me that the behavior you observe on P550
> is against the spec.

Or wait - no, he doesn't. He said this only to the strange combination you had
indicated.

Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Baptiste Le Duc]

On 2026-10-15 10:44 +0200, Jan Beulich wrote:
> On 15.10.2026 10:41, Jan Beulich wrote:
> > On 15.10.2026 10:28, Baptiste Le Duc wrote:
> >> On 2026-10-15 09:14 +0200, Jan Beulich wrote:
> >>> On 14.10.2026 18:45, Baptiste Le Duc wrote:
> >>>> On 2026-10-14 11:09:58+02:00, Jan Beulich wrote:
> >>>>> On 14.10.2026 11:00, Baptiste Le Duc wrote:
> >>>>>
> >>>>> In which case it would need to be
> >>>>>
> >>>>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
> >>>>>          !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
> >>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >>>>>
> >>>>> albeit I have to admit that permitting two contradicting extensions to
> >>>>> be exposed at the same time is irritating.
> >>>>>
> >>>>>> I see two ways to address this:
> >>>>>>
> >>>>>> 1) Keep checking Svade:
> >>>>>>
> >>>>>>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> >>>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >>>>>>
> >>>>>>     After resolve_ad_scheme(), the Svade bit is set in every case where
> >>>>>>     software has to preset the A/D bits. The only imprecise case is both
> >>>>>>     extensions advertised with hardware updating already enabled by
> >>>>>>     firmware, where presetting A/D is unnecessary but harmless.
> >>>>>>
> >>>>>> 2) Record the probe result in a global, e.g.
> >>>>>>
> >>>>>>     static bool __ro_after_init hw_ad_update;
> >>>>>>
> >>>>>>     and use:
> >>>>>>
> >>>>>>     if ( !hw_ad_update )
> >>>>>>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >>>>>>
> >>>>>>     This is exact in all cases, at the cost of a new global that mostly
> >>>>>>     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
> >>>>>>
> >>>>>> I discussed with Oleksii and we are fine with either. What do you prefer?
> >>>>>
> >>>>> As to 1), see above. For 2), while maybe okay, I think we're better off
> >>>> Sorry, I don't fully follow the issue with 1). Compared to what you proposed,
> >>>> it only differs when neither Svade nor Svadu is recorded, which cannot
> >>>> happen as resolve_ad_scheme() runs before and always records one of the
> >>>> two.
> >>>
> >>> Yet previously we said that recording extensions (as such, i.e. later also
> >>> possibly exposing them to guests) when they weren't really advertised may
> >>> not be the right thing to do. I.e. imo the probing may be used to drive
> >> But we also said that platforms (such as the P550) may implement an
> >> extension without advertising it, as a RISC-V spec maintainer
> >> confirmed in [1].
> >>
> >> [1] https://github.com/riscv/riscv-isa-manual/issues/3450#issuecomment-6076851535
> > 
> > What Andrew says there suggests to me that the behavior you observe on P550
> > is against the spec.
> 
> Or wait - no, he doesn't. He said this only to the strange combination you had
> indicated.
For me, he clearly answered your question before:
`Implementations that trap when A/D bits are not set do implement Svade,
whether or not they explicitly say they do.`

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Oleksii Kurochko]

On 10/15/26 10:41 AM, Jan Beulich wrote:
> On 15.10.2026 10:28, Baptiste Le Duc wrote:
>> On 2026-10-15 09:14 +0200, Jan Beulich wrote:
>>> On 14.10.2026 18:45, Baptiste Le Duc wrote:
>>>> On 2026-10-14 11:09:58+02:00, Jan Beulich wrote:
>>>>> On 14.10.2026 11:00, Baptiste Le Duc wrote:
>>>>>
>>>>> In which case it would need to be
>>>>>
>>>>>      if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
>>>>>           !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>>>>>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>
>>>>> albeit I have to admit that permitting two contradicting extensions to
>>>>> be exposed at the same time is irritating.
>>>>>
>>>>>> I see two ways to address this:
>>>>>>
>>>>>> 1) Keep checking Svade:
>>>>>>
>>>>>>      if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>>>>>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>>
>>>>>>      After resolve_ad_scheme(), the Svade bit is set in every case where
>>>>>>      software has to preset the A/D bits. The only imprecise case is both
>>>>>>      extensions advertised with hardware updating already enabled by
>>>>>>      firmware, where presetting A/D is unnecessary but harmless.
>>>>>>
>>>>>> 2) Record the probe result in a global, e.g.
>>>>>>
>>>>>>      static bool __ro_after_init hw_ad_update;
>>>>>>
>>>>>>      and use:
>>>>>>
>>>>>>      if ( !hw_ad_update )
>>>>>>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>>>>>
>>>>>>      This is exact in all cases, at the cost of a new global that mostly
>>>>>>      duplicates what resolve_ad_scheme() already records in the ISA bitmap.
>>>>>>
>>>>>> I discussed with Oleksii and we are fine with either. What do you prefer?
>>>>>
>>>>> As to 1), see above. For 2), while maybe okay, I think we're better off
>>>> Sorry, I don't fully follow the issue with 1). Compared to what you proposed,
>>>> it only differs when neither Svade nor Svadu is recorded, which cannot
>>>> happen as resolve_ad_scheme() runs before and always records one of the
>>>> two.
>>>
>>> Yet previously we said that recording extensions (as such, i.e. later also
>>> possibly exposing them to guests) when they weren't really advertised may
>>> not be the right thing to do. I.e. imo the probing may be used to drive
>> But we also said that platforms (such as the P550) may implement an
>> extension without advertising it, as a RISC-V spec maintainer
>> confirmed in [1].
>>
>> [1] https://github.com/riscv/riscv-isa-manual/issues/3450#issuecomment-6076851535
> 
> What Andrew says there suggests to me that the behavior you observe on P550

What was mentioned in [1]:
```
Implementations that trap when A/D bits are not set do implement Svade, 
whether or not they explicitly say they do.
```

Sounds like a default behavior for our case and so Svade should be taken 
as a default.

> is against the spec. That shouldn't really be the case though, as nothing
> was said in the spec at all until Svade and Svadu were actually introduced.

Actually, earlier versions of the Privileged ISA specification (e.g., 
v1.11 and v1.12) already explicitly described both behaviors:
"Two schemes to manage the A and D bits are permitted:
1. When a virtual page is accessed and the A bit is clear, or is written 
and the D bit is clear, a page-fault exception is raised.
2. When a virtual page is accessed and the A bit is clear, or is written 
and the D bit is clear, the implementation sets the corresponding bit(s) 
in the PTE. The PTE update must be atomic with respect to other accesses 
to the PTE, and must atomically check that the PTE is valid and grants 
sufficient permissions. Updates of the A bit may be performed as a 
result of speculation, but updates to the D bit must be exact..."
What is now formalized as Svade corresponds to the first scheme 
(software management via page faults), while the second scheme 
(hardware-managed updates) became Svadu.

~ Oleksii

> An update to the spec should imo never invalidate previously conforming
> implementations. I guess I'll comment there.
> 
>>> Xen's internal behavior, but I'm unconvinced that it should also lead to
>>> marking extensions as available.
>>
>> Therefore, as far as I understood, you're not convinced of this:
>>
>>      /* Neither advertised: trust the probe. */
>>      __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
>>                riscv_isa);
>>
>> If so, then, the only solution I have in mind in what I proposed in 2)
>> with the global variable that store the result of the real probe.
>> Am I correct?
> 
> Possibly. What I'm missing is some clarification towards the underlying
> principle which we want to follow not just here.
> 
> Jan

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Oleksii Kurochko]

On 10/15/26 10:41 AM, Jan Beulich wrote:
>>> Xen's internal behavior, but I'm unconvinced that it should also lead to
>>> marking extensions as available.

Could we use the probe mechanism solely to verify that the A/D bits 
behavior configured by the firmware matches what is advertised in the DTS?

In other words, rather than automatically setting the corresponding bit 
in riscv_isa here ...


>> Therefore, as far as I understood, you're not convinced of this:
>>
>>      /* Neither advertised: trust the probe. */
>>      __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
>>                riscv_isa);
>>

... based on the probe result when neither extension is advertised, we 
could require riscv,isa to explicitly advertise the appropriate 
extension. If the probe result contradicts what is advertised in the 
DTS, we would fail to boot and print a message asking the user to update 
riscv,isa accordingly.

Then it seems we could cover all the cases we have and potentially will 
have.

Am I missing something?

>> If so, then, the only solution I have in mind in what I proposed in 2)
>> with the global variable that store the result of the real probe.
>> Am I correct?
> Possibly. What I'm missing is some clarification towards the underlying
> principle which we want to follow not just here.

~ Oleksii

Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Oleksii Kurochko]

On 10/13/26 12:07 PM, Baptiste Le Duc wrote:
> +static bool __init probe_hw_ad_update(void)
> +{
> +    bool updated = false;
> +    unsigned int slot = FIX_MISC;
> +    vaddr_t va = FIXMAP_ADDR(slot);
> +    struct page_info *pg;
> +    pte_t *entry, pte;
> +    mfn_t mfn;
> +    unsigned long tmp;
> +
> +    pg = alloc_domheap_page(NULL, 0);
> +    if ( !pg )
> +        return false;
> +
> +    mfn = page_to_mfn(pg);
> +
> +    arch_pmap_map(FIX_MISC, mfn);

Do we need the arch_pmap_map() call here? The PTE it writes is 
overwritten right below by write_pte() (and the TLB flush is repeated 
too), so effectively only its ASSERT(!pte_is_valid(*entry)) remains.

Also, IIUC, arch_pmap_{map,unmap}() are the arch backend of the common 
PMAP infrastructure and are meant for the FIX_PMAP_* slots handed out by
pmap_map(); FIX_MISC isn't one of them. Accessing xen_fixmap[] directly 
is fine here, as set_fixmap() can't be used (map_pages_to_xen() always 
sets A and D), but then I'd do the same for the unmapping ...:

> +    entry = &xen_fixmap[slot];
> +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
> +    write_pte(entry, pte);
> +
> +    flush_tlb_one_local(va);
> +
> +    asm_inline volatile (
> +        "1: ld %[tmp], 0(%[va])\n"
> +        "   li %[updated], 1\n"
> +        "2:\n"
> +        ASM_EXTABLE(1b, 2b)
> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> +        : [va] "r" (va)
> +        : "memory" );
> +
> +    arch_pmap_unmap(slot);

... I.e. write_pte(entry, (pte_t){}) followed by flush_tlb_one_local(va).

> +    free_domheap_page(pg);
> +
> +    return updated;
> +}


Also:

 > +    asm_inline volatile (
 > +        "1: ld %[tmp], 0(%[va])\n"
 > +        "   li %[updated], 1\n"
 > +        "2:\n"
 > +        ASM_EXTABLE(1b, 2b)
 > +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
 > +        : [va] "r" (va)
 > +        : "memory" );

Since the loaded value isn't needed, how about loading into the zero
register (the load still has to raise any exception) and using plain
asm goto? That needs no CONFIG_CC_HAS_ASM_GOTO_OUTPUT conditional (as I 
originally wanted to suggest), as there are no outputs, and both tmp and 
the LI go away. Together with accessing xen_fixmap[] directly and using 
stack page instead of allocation of new page (I assume something like 
this was suggested by Jan B.), something like this:

static bool __init probe_hw_ad_update(void)
{
     vaddr_t va = FIXMAP_ADDR(FIX_MISC);
     pte_t *entry = &xen_fixmap[FIX_MISC];
     bool updated = false;

     ASSERT(!pte_is_valid(*entry));

     /*
      * Any page in use will do, so alias the stack page this code runs on,
      * with A clear: the read below either faults or has A set by hardware.
      */
     write_pte(entry, pte_from_mfn(maddr_to_mfn(virt_to_maddr(&va)),
                                   PTE_VALID | PTE_READABLE));
     flush_tlb_one_local(va);

     asm_inline goto (
         "1: ld zero, 0(%[va])\n"
         ASM_EXTABLE(1b, %l[out])
         :
         : [va] "r" (va)
         : "memory"
         : out );

     updated = read_pte(entry).pte & PTE_ACCESSED;

  out:
     write_pte(entry, (pte_t){});
     flush_tlb_one_local(va);

     return updated;
}


One things which I also would like to note about:

 > +    asm_inline volatile (
 > +        "1: ld %[tmp], 0(%[va])\n"
 > +        "   li %[updated], 1\n"
 > +        "2:\n"
 > +        ASM_EXTABLE(1b, 2b)

Note that in staging do_trap() consults the exception table only for
CAUSE_ILLEGAL_INSTRUCTION, so on hardware which faults on a clear A bit
the load page fault ends up in do_unexpected_trap() and hence die(),
rather than in the fixup. This is addressed by "xen/riscv: look up the
exception table for any trap taken in Xen context" from the vINTC 
series, so this series depends on it; please mention that in the cover 
letter or at least something like that will be needed for now if i am 
not confusing something:

              break;
          }

+        fallthrough;
+    case CAUSE_LOAD_PAGE_FAULT:
          if ( fixup_exception(cpu_regs) )
              break;

~ Oleksii

[PATCH v6 2/4] xen/riscv: rework the A/D bits comment in p2m_set_pte_flags()

[Baptiste Le Duc]

The comment in p2m_set_pte_flags() quotes the spec at length to describe
the two A/D bits schemes, and its TODO mixes two things: why A/D are preset
today (OpenSBI delegates the resulting page faults to a lower privilege
mode, so Xen would take them with no handler) and what remains to be done.

Summarise the two schemes as a short list. The reason for presetting A/D is
not future work, so move it out of the TODO, next to the schemes. The TODO
now keeps only the later work, handling these page faults instead of
presetting A/D, which only concerns Svade as A/D page faults cannot happen
otherwise.

No functional change.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
Changes since v5:
- new patch, split out of "xen/riscv: always preset A/D bits in G-stage
  PTEs".
---
 xen/arch/riscv/p2m.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index 3d195294d5..b32472538e 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -591,35 +591,25 @@ static void p2m_set_pte_flags(pte_t *e, p2m_type_t t)
     e->pte |= PTE_USER;
 
     /*
-     * Two schemes to manage the A and D bits are defined:
-     *   • The Svade extension: when a virtual page is accessed and the A bit
-     *     is clear, or is written and the D bit is clear, a page-fault
-     *     exception is raised.
-     *   • When the Svade extension is not implemented, the following scheme
-     *     applies.
-     *     When a virtual page is accessed and the A bit is clear, the PTE is
-     *     updated to set the A bit. When the virtual page is written and the
-     *     D bit is clear, the PTE is updated to set the D bit. When G-stage
-     *     address translation is in use and is not Bare, the G-stage virtual
-     *     pages may be accessed or written by implicit accesses to VS-level
-     *     memory management data structures, such as page tables.
-     * Thereby to avoid a page-fault in case of Svade is available, it is
-     * necessary to set A and D bits.
+     * A RISC-V implementation can either:
+     * 1) Update the 'A' and 'D' PTE bits in hardware.
+     * 2) Generate a page fault when 'A' and/or 'D' is clear, so that
+     *    software can set them.
      *
-     * TODO: For now, it’s fine to simply set the A/D bits, since OpenSBI
-     *       delegates page faults to a lower privilege mode and so OpenSBI
-     *       isn't expect to handle page-faults occured in lower modes.
-     *       By setting the A/D bits here, page faults that would otherwise
-     *       be generated due to unset A/D bits will not occur in Xen.
+     * (1) needs nothing more as it is handled by hardware. For (2), set 'A'
+     * and 'D' up front: OpenSBI delegates such page faults to a lower
+     * privilege mode, so they would be raised in Xen, which has no handler
+     * for them.
      *
-     *       Currently, Xen on RISC-V does not make use of the information
-     *       that could be obtained from handling such page faults, which
-     *       could otherwise be useful for several use cases such as demand
-     *       paging, cache-flushing optimizations, memory access tracking,etc.
+     * TODO: Xen on RISC-V currently makes no use of the information that
+     *       handling such page faults could provide, which would be useful
+     *       for several use cases such as demand paging, cache-flushing
+     *       optimizations, memory access tracking, etc.
      *
-     *       To support the more general case and the optimizations mentioned
-     *       above, it would be better to stop setting the A/D bits here and
-     *       instead handle page faults that occur due to unset A/D bits.
+     *       To support the more general case and the optimizations
+     *       mentioned above, it would be better to stop setting the A/D
+     *       bits here and instead handle the page faults that occur due to
+     *       unset A/D bits.
      */
     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
         e->pte |= PTE_ACCESSED | PTE_DIRTY;

-- 
2.55.0

[PATCH v6 3/4] xen/riscv: fix A/D bits in Xen's page-table mappings

[Baptiste Le Duc]

Xen does not handle page faults caused by clear A/D bits, so it presets
them when creating PTEs. pt_update_entry() does so, but three places build
leaf PTEs directly without going through it: setup_initial_mapping() (the
boot page tables), check_pgtbl_mode_support() (the temporary root entry
used to probe SATP mode support) and arch_pmap_map() (the fixmap).

Without Svadu, all three fault on first access. For
check_pgtbl_mode_support(), the fault is raised by the instruction fetch
right after the SATP write, before any trap handler is set.

Add PTE_ACCESSED and PTE_DIRTY to PAGE_HYPERVISOR_RO, and build
PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX on top of it. PTE_DIRTY is set in
all cases for consistency with pt_update_entry() which sets it at runtime.

This fixes arch_pmap_map() for free, since it already builds its PTE from
PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
its default, text and rodata permissions, and check_pgtbl_mode_support() to
use PAGE_HYPERVISOR_RX for its temporary root entry. The latter drops
PTE_WRITABLE, going from RWX to RX, but this is harmless, as that entry
only has to make the current instruction stream fetchable between the two
CSR_SATP writes, and nothing writes through it.

Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
open-coded site above leaves it with no user outside page.h itself.

pte_is_table() and pte_is_mapping() both assert that a PTE doesn't use one
of the two reserved encodings, (V=1, W=1, R=0) and (V=1, X=1, W=1, R=0), by
masking it with PAGE_HYPERVISOR_RW. Now that PAGE_HYPERVISOR_RW also
carries A and D, a reserved PTE with A or D set would no longer be caught.
Mask with the V, R and W bits explicitly instead, and factor the check out
into pte_has_reserved_rwx().

Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes since v5:
- no changes, already reviewed and waiting to be merged.
---
Changes since v4:
- rebase on top of staging.
- add a missing changes line in "Changes since v3".
---
Changes since v3:
- rebase patch 2/6 which modified wrong indentation.
- change commit message to address review comments.
- rename pte_is_reserved() to pte_has_reserved_rwx()
---
Changes since v2:
- add fixes commit ref.
- add A/D bits to PAGE_HYPERVISOR_RO and derive PAGE_HYPERVISOR_RW/RX
  from it for consistency with runtime.
- introduce pte_is_reserved() to factor out the reserved-encoding assert
  shared by pte_is_table() and pte_is_mapping().
- reword commit title
---
Changes since v1:
- change commit title
- mention in patch message that arch_pmap_map() is fixed too, via the
  PAGE_HYPERVISOR_RW change, not just setup_initial_mapping().
- convert check_pgtbl_mode_support()'s temporary root entry to
  PAGE_HYPERVISOR_RX, as it's harmless.
- drop PTE_LEAF_DEFAULT entirely instead of keeping it, now that no site
  open-codes it anymore.
- drop the pte_is_table() comment line that referenced PAGE_HYPERVISOR_RW,
  now stale.
---
 xen/arch/riscv/include/asm/page.h | 33 +++++++++++++++++----------------
 xen/arch/riscv/mm.c               | 11 +++++------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index b465a90325..8a3e3e1e66 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -46,12 +46,12 @@
 #define PTE_PBMT_NOCACHE            BIT(61, UL)
 #define PTE_PBMT_IO                 BIT(62, UL)
 
-#define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
 #define PTE_TABLE                   (PTE_VALID)
 
-#define PAGE_HYPERVISOR_RO          (PTE_VALID | PTE_READABLE)
-#define PAGE_HYPERVISOR_RW          (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
-#define PAGE_HYPERVISOR_RX          (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
+#define PAGE_HYPERVISOR_RO          (PTE_VALID | PTE_READABLE | \
+                                     PTE_ACCESSED | PTE_DIRTY)
+#define PAGE_HYPERVISOR_RW          (PAGE_HYPERVISOR_RO | PTE_WRITABLE)
+#define PAGE_HYPERVISOR_RX          (PAGE_HYPERVISOR_RO | PTE_EXECUTABLE)
 
 #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
 /*
@@ -161,31 +161,32 @@ static inline bool pte_is_valid(pte_t p)
  *      X W R Meaning
  *      0 0 0 Pointer to next level of page table.
  *      0 0 1 Read-only page.
- *      0 1 0 Reserved for future use.
+ *      0 1 0 Reserved for future use. [1]
  *      0 1 1 Read-write page.
  *      1 0 0 Execute-only page.
  *      1 0 1 Read-execute page.
- *      1 1 0 Reserved for future use.
+ *      1 1 0 Reserved for future use. [2]
  *      1 1 1 Read-write-execute page.
+ *
+ *   So if V=1 and W=1 then R also needs to be 1 as R = 0 is reserved for
+ *   future use ([1], [2]).
  */
+static inline bool pte_has_reserved_rwx(pte_t p)
+{
+    return (p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) ==
+           (PTE_VALID | PTE_WRITABLE);
+}
+
 static inline bool pte_is_table(pte_t p)
 {
-    /*
-     * According to the spec if V=1 and W=1 then R also needs to be 1 as
-     * R = 0 is reserved for future use ( look at the Table 4.5 ) so check
-     * in ASSERT that if (V==1 && W==1) then R isn't 0.
-     *
-     * PAGE_HYPERVISOR_RW contains PTE_VALID too.
-     */
-    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
+    ASSERT(!pte_has_reserved_rwx(p));
 
     return ((p.pte & (PTE_VALID | PTE_ACCESS_MASK)) == PTE_VALID);
 }
 
 static inline bool pte_is_mapping(pte_t p)
 {
-    /* See pte_is_table() */
-    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
+    ASSERT(!pte_has_reserved_rwx(p));
 
     return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
 }
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 06f0406781..6f7dcd10b2 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -140,19 +140,18 @@ static void __init setup_initial_mapping(struct mmu_desc *mmu_desc,
         case 1: /* Level 0 */
             {
                 unsigned long paddr = (page_addr - map_start) + pa_start;
-                unsigned int pte_flags = PTE_LEAF_DEFAULT;
+                unsigned int pte_flags = PAGE_HYPERVISOR_RW;
                 unsigned long addr = is_identity_mapping
                                      ? page_addr : virt_to_maddr(page_addr);
                 pte_t pte_to_be_written;
 
                 index = pt_index(0, page_addr);
 
-                if ( is_kernel_text(addr) ||
-                     is_kernel_inittext(addr) )
-                    pte_flags = PTE_EXECUTABLE | PTE_READABLE | PTE_VALID;
+                if ( is_kernel_text(addr) || is_kernel_inittext(addr) )
+                    pte_flags = PAGE_HYPERVISOR_RX;
 
                 if ( is_kernel_rodata(addr) )
-                    pte_flags = PTE_READABLE | PTE_VALID;
+                    pte_flags = PAGE_HYPERVISOR_RO;
 
                 pte_to_be_written = paddr_to_pte(paddr, pte_flags);
 
@@ -197,7 +196,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
 
     index = pt_index(page_table_level, aligned_load_start);
     stage1_pgtbl_root[index] = paddr_to_pte(aligned_load_start,
-                                            PTE_LEAF_DEFAULT | PTE_EXECUTABLE);
+                                            PAGE_HYPERVISOR_RX);
 
     sfence_vma();
     csr_write(CSR_SATP,

-- 
2.55.0

Re: [PATCH v6 3/4] xen/riscv: fix A/D bits in Xen's page-table mappings

[Jan Beulich]

On 13.10.2026 12:07, Baptiste Le Duc wrote:
> Xen does not handle page faults caused by clear A/D bits, so it presets
> them when creating PTEs. pt_update_entry() does so, but three places build
> leaf PTEs directly without going through it: setup_initial_mapping() (the
> boot page tables), check_pgtbl_mode_support() (the temporary root entry
> used to probe SATP mode support) and arch_pmap_map() (the fixmap).
> 
> Without Svadu, all three fault on first access. For
> check_pgtbl_mode_support(), the fault is raised by the instruction fetch
> right after the SATP write, before any trap handler is set.
> 
> Add PTE_ACCESSED and PTE_DIRTY to PAGE_HYPERVISOR_RO, and build
> PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX on top of it. PTE_DIRTY is set in
> all cases for consistency with pt_update_entry() which sets it at runtime.
> 
> This fixes arch_pmap_map() for free, since it already builds its PTE from
> PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
> its default, text and rodata permissions, and check_pgtbl_mode_support() to
> use PAGE_HYPERVISOR_RX for its temporary root entry. The latter drops
> PTE_WRITABLE, going from RWX to RX, but this is harmless, as that entry
> only has to make the current instruction stream fetchable between the two
> CSR_SATP writes, and nothing writes through it.
> 
> Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
> open-coded site above leaves it with no user outside page.h itself.
> 
> pte_is_table() and pte_is_mapping() both assert that a PTE doesn't use one
> of the two reserved encodings, (V=1, W=1, R=0) and (V=1, X=1, W=1, R=0), by
> masking it with PAGE_HYPERVISOR_RW. Now that PAGE_HYPERVISOR_RW also
> carries A and D, a reserved PTE with A or D set would no longer be caught.
> Mask with the V, R and W bits explicitly instead, and factor the check out
> into pte_has_reserved_rwx().
> 
> Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes since v5:
> - no changes, already reviewed and waiting to be merged.

I'm puzzled by this, also seen in the next patch: For one, nothing is said
anywhere that patches 3 and 4 are independent of patches 1 and 2, and
hence could go in ahead of those. And then, formally, neither patch is
ready for committing - Oleksii's R-b isn't enough at this point. I would
have ack-ed the last patch if that didn't - contextually - depend on the
one here. I'm only going to ack the one here once either it is clarified
that it's independent of the first two patches, or once agreement was
reached on those earlier patches. (If this was independent, though, I
would have expected it to have got moved to the front of the series, to
disambiguate whether it may go in already.)

Jan

Re: [PATCH v6 3/4] xen/riscv: fix A/D bits in Xen's page-table mappings

[Baptiste Le Duc]

On 2026-10-13 12:43 +0200, Jan Beulich wrote:
> On 13.10.2026 12:07, Baptiste Le Duc wrote:
> > Xen does not handle page faults caused by clear A/D bits, so it presets
> > them when creating PTEs. pt_update_entry() does so, but three places build
> > leaf PTEs directly without going through it: setup_initial_mapping() (the
> > boot page tables), check_pgtbl_mode_support() (the temporary root entry
> > used to probe SATP mode support) and arch_pmap_map() (the fixmap).
> > 
> > Without Svadu, all three fault on first access. For
> > check_pgtbl_mode_support(), the fault is raised by the instruction fetch
> > right after the SATP write, before any trap handler is set.
> > 
> > Add PTE_ACCESSED and PTE_DIRTY to PAGE_HYPERVISOR_RO, and build
> > PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX on top of it. PTE_DIRTY is set in
> > all cases for consistency with pt_update_entry() which sets it at runtime.
> > 
> > This fixes arch_pmap_map() for free, since it already builds its PTE from
> > PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
> > its default, text and rodata permissions, and check_pgtbl_mode_support() to
> > use PAGE_HYPERVISOR_RX for its temporary root entry. The latter drops
> > PTE_WRITABLE, going from RWX to RX, but this is harmless, as that entry
> > only has to make the current instruction stream fetchable between the two
> > CSR_SATP writes, and nothing writes through it.
> > 
> > Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
> > open-coded site above leaves it with no user outside page.h itself.
> > 
> > pte_is_table() and pte_is_mapping() both assert that a PTE doesn't use one
> > of the two reserved encodings, (V=1, W=1, R=0) and (V=1, X=1, W=1, R=0), by
> > masking it with PAGE_HYPERVISOR_RW. Now that PAGE_HYPERVISOR_RW also
> > carries A and D, a reserved PTE with A or D set would no longer be caught.
> > Mask with the V, R and W bits explicitly instead, and factor the check out
> > into pte_has_reserved_rwx().
> > 
> > Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> > Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes since v5:
> > - no changes, already reviewed and waiting to be merged.
> 
> I'm puzzled by this, also seen in the next patch: For one, nothing is said
> anywhere that patches 3 and 4 are independent of patches 1 and 2, and
> hence could go in ahead of those. And then, formally, neither patch is
> ready for committing - Oleksii's R-b isn't enough at this point. I would
> have ack-ed the last patch if that didn't - contextually - depend on the
> one here. I'm only going to ack the one here once either it is clarified
> that it's independent of the first two patches, or once agreement was
> reached on those earlier patches. (If this was independent, though, I
> would have expected it to have got moved to the front of the series, to
> disambiguate whether it may go in already.)
Sorry I wasn't aware of that. I will put the last patch first in the
next revision as it is independent of the other ones. Thanks for
clarifying.

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

Re: [PATCH v6 3/4] xen/riscv: fix A/D bits in Xen's page-table mappings

[Oleksii Kurochko]

On 10/13/26 12:07 PM, Baptiste Le Duc wrote:
> Xen does not handle page faults caused by clear A/D bits, so it presets
> them when creating PTEs. pt_update_entry() does so, but three places build
> leaf PTEs directly without going through it: setup_initial_mapping() (the
> boot page tables), check_pgtbl_mode_support() (the temporary root entry
> used to probe SATP mode support) and arch_pmap_map() (the fixmap).
> 
> Without Svadu, all three fault on first access. For
> check_pgtbl_mode_support(), the fault is raised by the instruction fetch
> right after the SATP write, before any trap handler is set.
> 
> Add PTE_ACCESSED and PTE_DIRTY to PAGE_HYPERVISOR_RO, and build
> PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX on top of it. PTE_DIRTY is set in
> all cases for consistency with pt_update_entry() which sets it at runtime.
> 
> This fixes arch_pmap_map() for free, since it already builds its PTE from
> PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
> its default, text and rodata permissions, and check_pgtbl_mode_support() to
> use PAGE_HYPERVISOR_RX for its temporary root entry. The latter drops
> PTE_WRITABLE, going from RWX to RX, but this is harmless, as that entry
> only has to make the current instruction stream fetchable between the two
> CSR_SATP writes, and nothing writes through it.
> 
> Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
> open-coded site above leaves it with no user outside page.h itself.
> 
> pte_is_table() and pte_is_mapping() both assert that a PTE doesn't use one
> of the two reserved encodings, (V=1, W=1, R=0) and (V=1, X=1, W=1, R=0), by
> masking it with PAGE_HYPERVISOR_RW. Now that PAGE_HYPERVISOR_RW also
> carries A and D, a reserved PTE with A or D set would no longer be caught.
> Mask with the V, R and W bits explicitly instead, and factor the check out
> into pte_has_reserved_rwx().
> 
> Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
I think I missed to mentioned that before, sorry:

arch_pmap_map() was introduced by f89e1a170299 ("xen/riscv: introduce
asm/pmap.h header"), so perhaps add a 2nd Fixes: tag.

~ Oleksii

Re: [PATCH v6 3/4] xen/riscv: fix A/D bits in Xen's page-table mappings

[Baptiste Le Duc]

On 2026-10-14 11:59:43+02:00, Oleksii Kurochko wrote:
> On 10/13/26 12:07 PM, Baptiste Le Duc wrote:
> 
> > Xen does not handle page faults caused by clear A/D bits, so it presets
> > them when creating PTEs. pt_update_entry() does so, but three places build
> > leaf PTEs directly without going through it: setup_initial_mapping() (the
> > boot page tables), check_pgtbl_mode_support() (the temporary root entry
> > used to probe SATP mode support) and arch_pmap_map() (the fixmap).
> > 
> > Without Svadu, all three fault on first access. For
> > check_pgtbl_mode_support(), the fault is raised by the instruction fetch
> > right after the SATP write, before any trap handler is set.
> > 
> > Add PTE_ACCESSED and PTE_DIRTY to PAGE_HYPERVISOR_RO, and build
> > PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX on top of it. PTE_DIRTY is set in
> > all cases for consistency with pt_update_entry() which sets it at runtime.
> > 
> > This fixes arch_pmap_map() for free, since it already builds its PTE from
> > PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
> > its default, text and rodata permissions, and check_pgtbl_mode_support() to
> > use PAGE_HYPERVISOR_RX for its temporary root entry. The latter drops
> > PTE_WRITABLE, going from RWX to RX, but this is harmless, as that entry
> > only has to make the current instruction stream fetchable between the two
> > CSR_SATP writes, and nothing writes through it.
> > 
> > Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
> > open-coded site above leaves it with no user outside page.h itself.
> > 
> > pte_is_table() and pte_is_mapping() both assert that a PTE doesn't use one
> > of the two reserved encodings, (V=1, W=1, R=0) and (V=1, X=1, W=1, R=0), by
> > masking it with PAGE_HYPERVISOR_RW. Now that PAGE_HYPERVISOR_RW also
> > carries A and D, a reserved PTE with A or D set would no longer be caught.
> > Mask with the V, R and W bits explicitly instead, and factor the check out
> > into pte_has_reserved_rwx().
> > 
> > Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
> 
> I think I missed to mentioned that before, sorry:
> 
> arch_pmap_map() was introduced by f89e1a170299 ("xen/riscv: introduce
> asm/pmap.h header"), so perhaps add a 2nd Fixes: tag.
Thanks, will apply this in next version.

-- 
Baptiste Le Duc <baptiste.le-duc@vates.tech>

[PATCH v6 4/4] xen/riscv: use pte_is_valid() in pte_is_mapping()

[Baptiste Le Duc]

Use pte_is_valid() instead of open-coding the PTE_VALID check.

No functional change.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes since v5:
- no changes, already reviewed and waiting to be merged.
---
Changes since v4:
- rebase on top of staging.
---
Changes since v3:
- no changes.
- add trailers.
---
Changes since v2:
- new patch.
---
 xen/arch/riscv/include/asm/page.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index 8a3e3e1e66..f9e59e61de 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -188,7 +188,7 @@ static inline bool pte_is_mapping(pte_t p)
 {
     ASSERT(!pte_has_reserved_rwx(p));
 
-    return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
+    return pte_is_valid(p) && (p.pte & PTE_ACCESS_MASK);
 }
 
 static inline bool pte_is_superpage(pte_t p, unsigned int level)

-- 
2.55.0