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

[PATCH 0/5] 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:
- Stop requiring Zihintpause and Svpbmt at boot
    Both are already gated at some call sites that care, drop them
    from required_extensions[] so hardware without them still boots.
- Preset A/D bits in G-stage and in Xen's own page-table mappings
    Avoids an unhandled page fault on Svade/Svadu-less hardware on both
    mapping path.
- Add the missing SFENCE.VMA after enabling paging in turn_on_mmu()
  Required per the Privileged spec when ASID 0 is reused across the satp

CI pipeline:
https://gitlab.com/xen-project/people/baptleduc/xen/-/pipelines/2796673417

Baptiste Le Duc (5):
  xen/riscv: always set A/D bits at boot time
  xen/riscv: preset A/D bits in Xen's own page-table mappings
  xen/riscv: make Svpbmt no longer a required extension
  xen/riscv: make Zihintpause no longer a required extension
  xen/riscv: add SFENCE.VMA after enabling paging

 xen/arch/riscv/cpufeature.c       |  2 -
 xen/arch/riscv/include/asm/page.h | 22 ++++++----
 xen/arch/riscv/mm.c               |  7 ++-
 xen/arch/riscv/p2m.c              | 72 ++++++++++++++++++-------------
 xen/arch/riscv/riscv64/head.S     |  1 +
 5 files changed, 61 insertions(+), 43 deletions(-)

[PATCH 1/5] xen/riscv: always set A/D bits at boot time

[Baptiste Le Duc]

Always set the PTE A/D bits at boot time to avoid an unhandled page fault
on platforms that implement neither Svade nor Svadu, and on platforms that
declare both in the device tree.

Rewrite the comment to enumerate the four possible Svade/Svadu combinations
(inspired by [1]) and set A/D unconditionally, which is correct in all four
cases until Svadu is fully supported (full support requires the SBI FWFT
call to enable hardware updating of A/D bits).

[1] https://lwn.net/Articles/980016/

Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
 xen/arch/riscv/p2m.c | 70 ++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index 1cea86512c..11dc289f0f 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -591,38 +591,52 @@ static void p2m_set_permission(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.
+     * Svade and Svadu extensions represent two schemes for managing the PTE
+     * A/D bits. When the PTE A/D bits need to be set, the Svade extension
+     * indicates that a page fault will be raised. In contrast, the Svadu
+     * extension supports hardware updating of the PTE A/D bits.
      *
-     * 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.
+     * There are 4 possible combinations of these extensions in the device
+     * tree. The default hardware behavior for each is:
      *
-     *       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.
+     * 1) Neither Svade nor Svadu present in DT => It is technically unknown
+     *    whether the platform uses Svade or Svadu. Xen should be prepared to
+     *    handle either hardware updating of the PTE A/D bits or page faults
+     *    when they need updating. To support both, Xen always sets the 'A' and
+     *    'D' PTE bits at boot time.
      *
-     *       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.
+     * 2) Only Svade present in DT => Xen must assume Svade to be always
+     *    enabled.
+     *
+     * 3) Only Svadu present in DT => Xen must assume Svadu to be always
+     *    enabled.
+     *
+     * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned
+     *    off at boot time by setting A/D bits. To use Svadu, the supervisor
+     *    must explicitly enable it using the SBI FWFT extension.
+     *
+     * The Svade extension is mandatory and the Svadu extension is optional in
+     * the RVA23 profile. Platforms wanting to take advantage of Svadu can
+     * choose option 3. Platforms aware of the profile can choose option 4, and
+     * Linux won't get the benefit of Svadu until the SBI FWFT extension is
+     * available.
+     *
+     * 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.
+     *
+     * 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.
+     */
+
+    /*
+     * Preset unconditionally for all 4 cases above, harmless when Svadu
+     * manages the bits (case 3). Skipping it for case 3 requires SBI FWFT
+     * which is not yet supported.
      */
-    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
-        e->pte |= PTE_ACCESSED | PTE_DIRTY;
+    e->pte |= PTE_ACCESSED | PTE_DIRTY;
 
     switch ( t )
     {

[PATCH 2/5] xen/riscv: preset A/D bits in Xen's own page-table mappings

[Baptiste Le Duc]

The previous patch made p2m_set_permission() always set the PTE A/D bits to
map pages in G-stage, to avoid a page fault on platforms that implement
neither Svade nor Svadu, or that declare both in the device tree. Xen's own
page tables, built by setup_initial_mapping(), never go through
p2m_set_permission() and need the same fix.

Add PTE_ACCESSED to PTE_LEAF_DEFAULT and make it the minimal common leaf
permission set by dropping PTE_WRITABLE. Rebuild PAGE_HYPERVISOR_RO,
PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX from that common base, with
PAGE_HYPERVISOR_RW also adding PTE_DIRTY. Switch setup_initial_mapping() to
use these macros for its default, text, and rodata permissions instead of
the equivalent raw bit lists.

A PTE is a table entry iff PTE_VALID is set and R/W/X are all clear, so
update pte_is_table() accordingly.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
 xen/arch/riscv/include/asm/page.h | 14 ++++++++------
 xen/arch/riscv/mm.c               |  7 +++----
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index b465a90325..5c02f64a17 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_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE | PTE_ACCESSED)
 #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_LEAF_DEFAULT)
+#define PAGE_HYPERVISOR_RW          (PTE_LEAF_DEFAULT | PTE_WRITABLE | PTE_DIRTY)
+#define PAGE_HYPERVISOR_RX          (PTE_LEAF_DEFAULT | PTE_EXECUTABLE)
 
 #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
 /*
@@ -177,7 +177,8 @@ static inline bool pte_is_table(pte_t p)
      *
      * PAGE_HYPERVISOR_RW contains PTE_VALID too.
      */
-    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
+    ASSERT((p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) !=
+           (PTE_VALID | PTE_WRITABLE));
 
     return ((p.pte & (PTE_VALID | PTE_ACCESS_MASK)) == PTE_VALID);
 }
@@ -185,7 +186,8 @@ static inline bool pte_is_table(pte_t p)
 static inline bool pte_is_mapping(pte_t p)
 {
     /* See pte_is_table() */
-    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
+    ASSERT((p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) !=
+            (PTE_VALID | PTE_WRITABLE));
 
     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 4d3b8c2204..baff49cf09 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -140,7 +140,7 @@ 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 permissions = PTE_LEAF_DEFAULT;
+                unsigned int permissions = PAGE_HYPERVISOR_RW;
                 unsigned long addr = is_identity_mapping
                                      ? page_addr : virt_to_maddr(page_addr);
                 pte_t pte_to_be_written;
@@ -149,11 +149,10 @@ static void __init setup_initial_mapping(struct mmu_desc *mmu_desc,
 
                 if ( is_kernel_text(addr) ||
                      is_kernel_inittext(addr) )
-                        permissions =
-                            PTE_EXECUTABLE | PTE_READABLE | PTE_VALID;
+                    permissions = PAGE_HYPERVISOR_RX;
 
                 if ( is_kernel_rodata(addr) )
-                    permissions = PTE_READABLE | PTE_VALID;
+                    permissions = PAGE_HYPERVISOR_RO;
 
                 pte_to_be_written = paddr_to_pte(paddr, permissions);
 

[PATCH 3/5] xen/riscv: make Svpbmt no longer a required extension

[Baptiste Le Duc]

required_extensions[] panics at boot if Svpbmt is missing, which is a
problem on hardware that doesn't implement it. Xen already checks Svpbmt at
runtime in some places (vcpu_csr_init()), but not everywhere:
p2m_pte_from_mfn() and the PAGE_HYPERVISOR_NOCACHE/WC macros still set the
raw PTE_PBMT* encoding unconditionally.

Drop Svpbmt from required_extensions, and introduce pte_pbmt(), which masks
the requested PBMT encoding down to 0 when Svpbmt is unavailable, using it
in both remaining unguarded spots.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
 xen/arch/riscv/cpufeature.c       | 1 -
 xen/arch/riscv/include/asm/page.h | 8 ++++++--
 xen/arch/riscv/p2m.c              | 2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 92235fdfd5..900cb9d772 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -157,7 +157,6 @@ static const struct riscv_isa_ext_data __initconst required_extensions[] = {
     RISCV_ISA_EXT_DATA(zifencei),
     RISCV_ISA_EXT_DATA(zihintpause),
     RISCV_ISA_EXT_DATA(zbb),
-    RISCV_ISA_EXT_DATA(svpbmt),
 };
 
 static bool __init is_lowercase_extension_name(const char *str)
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index 5c02f64a17..6a3749526d 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -11,6 +11,7 @@
 #include <xen/types.h>
 
 #include <asm/atomic.h>
+#include <asm/cpufeature.h>
 #include <asm/page-bits.h>
 
 #define VPN_MASK                    (PAGETABLE_ENTRIES - 1UL)
@@ -54,6 +55,9 @@
 #define PAGE_HYPERVISOR_RX          (PTE_LEAF_DEFAULT | PTE_EXECUTABLE)
 
 #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
+
+#define pte_pbmt(pbmt) \
+    (riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) ? (pbmt) : 0UL)
 /*
  * PAGE_HYPERVISOR_NOCACHE is used for ioremap().
  *
@@ -61,8 +65,8 @@
  * is that IO is non-idempotent and strongly ordered, which makes it a good
  * candidate for mapping IOMEM.
  */
-#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | PTE_PBMT_IO)
-#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | PTE_PBMT_NOCACHE)
+#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | pte_pbmt(PTE_PBMT_IO))
+#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | pte_pbmt(PTE_PBMT_NOCACHE))
 
 /*
  * The PTE format does not contain the following bits within itself;
diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index 11dc289f0f..f6e635ec1d 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -683,7 +683,7 @@ static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t,
         switch ( t )
         {
         case p2m_mmio_direct_io:
-            e.pte |= PTE_PBMT_IO;
+            e.pte |= pte_pbmt(PTE_PBMT_IO);
             break;
 
         default:

[PATCH 4/5] xen/riscv: make Zihintpause no longer a required extension

[Baptiste Le Duc]

required_extensions[] panics at boot if Zihintpause is missing, but Xen
never actually depends on it: cpu_relax() only emits the "pause" when the
extension is implemented and otherwise falls back to the raw fence
encoding, which is a legal no-op on any hart regardless of Zihintpause
support.

Drop it from required_extensions so hardware without Zihintpause
still boots.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
 xen/arch/riscv/cpufeature.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 900cb9d772..661babc0a6 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -155,7 +155,6 @@ static const struct riscv_isa_ext_data __initconst required_extensions[] = {
     RISCV_ISA_EXT_DATA(h),
     RISCV_ISA_EXT_DATA(zicsr),
     RISCV_ISA_EXT_DATA(zifencei),
-    RISCV_ISA_EXT_DATA(zihintpause),
     RISCV_ISA_EXT_DATA(zbb),
 };
 

[PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Baptiste Le Duc]

turn_on_mmu() writes satp to switch on Sv39 paging but never fences
afterwards.

Xen never allocates a non-zero ASID, so per the Privileged spec, sec.
12.2.1 "Supervisor Memory-Management Fence Instruction":

  "If the implementation does not provide ASIDs, or software chooses
  to always use ASID 0, then after every satp write, software should
  execute SFENCE.VMA with rs1=x0."

The spec text around this rule hedges with "may be necessary", but
RISC-V spec co-author Andrew Waterman confirmed on the ISA manual
issue tracker that the fence after a satp write is not optional in
this case: "The SFENCE after the SATP write is definitely necessary
... In general, you need to SFENCE after you've recycled an ASID.
Since we don't use ASIDs in the Linux kernel yet, every context
switch is effectively an ASID reuse, hence the full TLB flush." [1]
The same reasoning applies to Xen: with ASID always 0, this satp
write is indistinguishable from an ASID reuse to the hart, so the
fence is required for correctness.

Add the missing SFENCE.VMA to order those page-table stores before
the hart's first translation under the new mapping.

[1] https://github.com/riscv/riscv-isa-manual/issues/226

Fixes: f5035d480f7a ("xen: add files needed for minimal riscv build")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
 xen/arch/riscv/riscv64/head.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 9c40512e61..7f6edc972f 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
         srli    t1, t1, PAGE_SHIFT
         or      t1, t1, t0
         csrw    CSR_SATP, t1
+        sfence.vma
 
         jr      a0
 END(turn_on_mmu)

Re: [PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Oleksii Kurochko]

On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> turn_on_mmu() writes satp to switch on Sv39 paging but never fences
> afterwards.
> 
> Xen never allocates a non-zero ASID, so per the Privileged spec, sec.
> 12.2.1 "Supervisor Memory-Management Fence Instruction":
> 
>    "If the implementation does not provide ASIDs, or software chooses
>    to always use ASID 0, then after every satp write, software should
>    execute SFENCE.VMA with rs1=x0."
> 
> The spec text around this rule hedges with "may be necessary", but
> RISC-V spec co-author Andrew Waterman confirmed on the ISA manual
> issue tracker that the fence after a satp write is not optional in
> this case: "The SFENCE after the SATP write is definitely necessary
> ... In general, you need to SFENCE after you've recycled an ASID.
> Since we don't use ASIDs in the Linux kernel yet, every context
> switch is effectively an ASID reuse, hence the full TLB flush." [1]
> The same reasoning applies to Xen: with ASID always 0, this satp
> write is indistinguishable from an ASID reuse to the hart, so the
> fence is required for correctness.

But at the moment of execution of turn_on_mmu() we don't use any ASID, 
do we? It was used in check_pgtbl_mode_support() but at the end it is done:

     csr_write(CSR_SATP, 0);

     sfence_vma();

So basically Bare mode + flush all TLBs presented before and then up to

...

> 
> Add the missing SFENCE.VMA to order those page-table stores before
> the hart's first translation under the new mapping.
> 
> [1] https://github.com/riscv/riscv-isa-manual/issues/226
> 
> Fixes: f5035d480f7a ("xen: add files needed for minimal riscv build")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
>   xen/arch/riscv/riscv64/head.S | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 9c40512e61..7f6edc972f 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
>           srli    t1, t1, PAGE_SHIFT
>           or      t1, t1, t0
>           csrw    CSR_SATP, t1

... ASID isn't used as we are in Bare mode.

What am I missing?

> +        sfence.vma

The one thing which possibly matters here, and could explain why 
sfence.vma is needed, is:
```
Implementations with virtual memory are permitted to perform address 
translations speculatively and earlier than required by an explicit 
memory access, and are permitted to cache them in address translation 
cache structures—including possibly caching the identity mappings from 
effective address to physical address used in Bare translation modes and 
M-mode.
```

So the TLB could potentially be populated with identity mappings, and I 
agree that it would be better to flush those.

I’m not entirely convinced, though, that the reason here is the ASID 
itself. Rather, it seems that we want to flush because of potentially 
cached speculative identity mappings.

If this reasoning looks correct to you, could we update the commit 
message to reflect this rationale for why sfence.vma is needed here?

Thanks.

~ Oleksii

Re: [PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Oleksii Kurochko]

On 8/27/26 6:53 PM, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
>> turn_on_mmu() writes satp to switch on Sv39 paging but never fences
>> afterwards.
>>
>> Xen never allocates a non-zero ASID, so per the Privileged spec, sec.
>> 12.2.1 "Supervisor Memory-Management Fence Instruction":
>>
>>    "If the implementation does not provide ASIDs, or software chooses
>>    to always use ASID 0, then after every satp write, software should
>>    execute SFENCE.VMA with rs1=x0."
>>
>> The spec text around this rule hedges with "may be necessary", but
>> RISC-V spec co-author Andrew Waterman confirmed on the ISA manual
>> issue tracker that the fence after a satp write is not optional in
>> this case: "The SFENCE after the SATP write is definitely necessary
>> ... In general, you need to SFENCE after you've recycled an ASID.
>> Since we don't use ASIDs in the Linux kernel yet, every context
>> switch is effectively an ASID reuse, hence the full TLB flush." [1]
>> The same reasoning applies to Xen: with ASID always 0, this satp
>> write is indistinguishable from an ASID reuse to the hart, so the
>> fence is required for correctness.
> 
> But at the moment of execution of turn_on_mmu() we don't use any ASID, 
> do we? It was used in check_pgtbl_mode_support() but at the end it is done:
> 
>      csr_write(CSR_SATP, 0);
> 
>      sfence_vma();
> 
> So basically Bare mode + flush all TLBs presented before and then up to
> 
> ...
> 
>>
>> Add the missing SFENCE.VMA to order those page-table stores before
>> the hart's first translation under the new mapping.
>>
>> [1] https://github.com/riscv/riscv-isa-manual/issues/226
>>
>> Fixes: f5035d480f7a ("xen: add files needed for minimal riscv build")
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>> ---
>>   xen/arch/riscv/riscv64/head.S | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/ 
>> head.S
>> index 9c40512e61..7f6edc972f 100644
>> --- a/xen/arch/riscv/riscv64/head.S
>> +++ b/xen/arch/riscv/riscv64/head.S
>> @@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
>>           srli    t1, t1, PAGE_SHIFT
>>           or      t1, t1, t0
>>           csrw    CSR_SATP, t1
> 
> ... ASID isn't used as we are in Bare mode.
> 
> What am I missing?
> 
>> +        sfence.vma
> 
> The one thing which possibly matters here, and could explain why 
> sfence.vma is needed, is:
> ```
> Implementations with virtual memory are permitted to perform address 
> translations speculatively and earlier than required by an explicit 
> memory access, and are permitted to cache them in address translation 
> cache structures—including possibly caching the identity mappings from 
> effective address to physical address used in Bare translation modes and 
> M-mode.
> ```
> 
> So the TLB could potentially be populated with identity mappings, and I 
> agree that it would be better to flush those.
> 
> I’m not entirely convinced, though, that the reason here is the ASID 
> itself. Rather, it seems that we want to flush because of potentially 
> cached speculative identity mappings.
> 
> If this reasoning looks correct to you, could we update the commit 
> message to reflect this rationale for why sfence.vma is needed here?

My suggestion is:

xen/riscv: add SFENCE.VMA after writing satp in turn_on_mmu()

The existing SFENCE.VMA before the satp write only orders the page
table stores from setup_initial_pagetables() against subsequent
implicit reads. It cannot invalidate translations cached after it
retires, and the Privileged spec permits an implementation to
translate speculatively and to cache the identity mappings used in
Bare mode. Such an entry would shadow the Sv39 translation once
paging is on, which matters because turn_on_mmu() jumps to a linker
address that is not identity mapped.

Does it make sense?

~ Oleksii

Re: [PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Jan Beulich]

On 27.08.2026 18:53, Oleksii Kurochko wrote:
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
>> --- a/xen/arch/riscv/riscv64/head.S
>> +++ b/xen/arch/riscv/riscv64/head.S
>> @@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
>>           srli    t1, t1, PAGE_SHIFT
>>           or      t1, t1, t0
>>           csrw    CSR_SATP, t1
> 
> ... ASID isn't used as we are in Bare mode.
> 
> What am I missing?
> 
>> +        sfence.vma
> 
> The one thing which possibly matters here, and could explain why 
> sfence.vma is needed, is:
> ```
> Implementations with virtual memory are permitted to perform address 
> translations speculatively and earlier than required by an explicit 
> memory access, and are permitted to cache them in address translation 
> cache structures—including possibly caching the identity mappings from 
> effective address to physical address used in Bare translation modes and 
> M-mode.
> ```
> 
> So the TLB could potentially be populated with identity mappings, and I 
> agree that it would be better to flush those.

First: Does (or at least may) the TLB come into play in Bare mode? If not,
there's nothing to invalidate. If so, the next question would be whether
it's indeed ASID 0 which is (or again may be) used in such TLB entries.

Jan

Re: [PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Oleksii Kurochko]

On 8/28/26 9:13 AM, Jan Beulich wrote:
> On 27.08.2026 18:53, Oleksii Kurochko wrote:
>> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
>>> --- a/xen/arch/riscv/riscv64/head.S
>>> +++ b/xen/arch/riscv/riscv64/head.S
>>> @@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
>>>            srli    t1, t1, PAGE_SHIFT
>>>            or      t1, t1, t0
>>>            csrw    CSR_SATP, t1
>>
>> ... ASID isn't used as we are in Bare mode.
>>
>> What am I missing?
>>
>>> +        sfence.vma
>>
>> The one thing which possibly matters here, and could explain why
>> sfence.vma is needed, is:
>> ```
>> Implementations with virtual memory are permitted to perform address
>> translations speculatively and earlier than required by an explicit
>> memory access, and are permitted to cache them in address translation
>> cache structures—including possibly caching the identity mappings from
>> effective address to physical address used in Bare translation modes and
>> M-mode.
>> ```
>>
>> So the TLB could potentially be populated with identity mappings, and I
>> agree that it would be better to flush those.
> 
> First: Does (or at least may) the TLB come into play in Bare mode? If not,
> there's nothing to invalidate.

In the quote from the spec I mentioned above it is written the answer is 
yes, the TLB (address-translation cache) absolutely can come into play 
in Bare mode.

> If so, the next question would be whether
> it's indeed ASID 0 which is (or again may be) used in such TLB entries.

I re-read the spec and ASID 0 will be really used even in Bare mode as 
to select MODE=Bare, software must write zero to the remaining fields of 
satp (bits 30–0 when SXLEN=32, or bits 59–0 when SXLEN=64) what 
automatically includes field ASID (so it will be zero).

And considering that idendentity mapping could be cached in TLB even in 
Bare mode they will taged with ASID = 0.

~ Oleksii

Re: [PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Oleksii Kurochko]

On 8/27/26 6:53 PM, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
>> turn_on_mmu() writes satp to switch on Sv39 paging but never fences
>> afterwards.
>>
>> Xen never allocates a non-zero ASID, so per the Privileged spec, sec.
>> 12.2.1 "Supervisor Memory-Management Fence Instruction":
>>
>>    "If the implementation does not provide ASIDs, or software chooses
>>    to always use ASID 0, then after every satp write, software should
>>    execute SFENCE.VMA with rs1=x0."
>>
>> The spec text around this rule hedges with "may be necessary", but
>> RISC-V spec co-author Andrew Waterman confirmed on the ISA manual
>> issue tracker that the fence after a satp write is not optional in
>> this case: "The SFENCE after the SATP write is definitely necessary
>> ... In general, you need to SFENCE after you've recycled an ASID.
>> Since we don't use ASIDs in the Linux kernel yet, every context
>> switch is effectively an ASID reuse, hence the full TLB flush." [1]
>> The same reasoning applies to Xen: with ASID always 0, this satp
>> write is indistinguishable from an ASID reuse to the hart, so the
>> fence is required for correctness.
> 
> But at the moment of execution of turn_on_mmu() we don't use any ASID, 
> do we? It was used in check_pgtbl_mode_support() but at the end it is done:
> 
>      csr_write(CSR_SATP, 0);
> 
>      sfence_vma();
> 
> So basically Bare mode + flush all TLBs presented before and then up to
> 
> ...
> 
>>
>> Add the missing SFENCE.VMA to order those page-table stores before
>> the hart's first translation under the new mapping.
>>
>> [1] https://github.com/riscv/riscv-isa-manual/issues/226
>>
>> Fixes: f5035d480f7a ("xen: add files needed for minimal riscv build")
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>> ---
>>   xen/arch/riscv/riscv64/head.S | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/ 
>> head.S
>> index 9c40512e61..7f6edc972f 100644
>> --- a/xen/arch/riscv/riscv64/head.S
>> +++ b/xen/arch/riscv/riscv64/head.S
>> @@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
>>           srli    t1, t1, PAGE_SHIFT
>>           or      t1, t1, t0
>>           csrw    CSR_SATP, t1
> 
> ... ASID isn't used as we are in Bare mode.
> 
> What am I missing?

After the conversation with Jan B. in the separate thread I re-read 
documentaion and found that ASID=0 will be used here too as after 
check_pgtbl_mode_support() we set Bare mode and ASID 0 will be really 
used even in Bare mode as to select MODE=Bare as software must write 
zero to the remaining fields of satp (bits 30–0 when SXLEN=32, or bits 
59–0 when SXLEN=64) what automatically includes field ASID (so it will 
be zero).

But still the full reason why we need sfence.vma here is that TLB could 
be polluted with identity mapping (even in Bare mode) and which will be 
tagged by ASID=0.

So what about to update commit message with:
```
xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu()

The existing SFENCE.VMA before the satp write only orders the page table
stores from setup_initial_pagetables() against subsequent implicit reads.
It does not prevent the CPU from speculatively caching translations 
after the fence retires.

According to the RISC-V Privileged specification, implementations are
permitted to speculatively cache Bare-mode identity mappings. Furthermore,
selecting MODE=Bare (which happens during check_pgtbl_mode_support())
requires zeroing the remaining fields of satp, causing ASID=0 to be
actively used in Bare mode. Consequently, the TLB can be polluted with Bare
identity mappings tagged with ASID=0.

Once satp is written to enable Sv39 translation, these cached identity
mappings (tagged with ASID=0) can shadow the true Sv39 translations.
This would lead to translation failures since turn_on_mmu() jumps to
a non-identity-mapped linker address.

Fix this by adding a post-satp-write SFENCE.VMA to invalidate any stale
translations (including Bare-mode identity mappings under ASID=0) before
jumping to the virtual address space.
```

~ Oleksii

Re: [PATCH 5/5] xen/riscv: add SFENCE.VMA after enabling paging

[Baptiste Le Duc]

On 2026-08-28 10:11 +0200, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 6:53 PM, Oleksii Kurochko wrote:
> > 
> > 
> > On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> >> turn_on_mmu() writes satp to switch on Sv39 paging but never fences
> >> afterwards.
> >>
> >> Xen never allocates a non-zero ASID, so per the Privileged spec, sec.
> >> 12.2.1 "Supervisor Memory-Management Fence Instruction":
> >>
> >>    "If the implementation does not provide ASIDs, or software chooses
> >>    to always use ASID 0, then after every satp write, software should
> >>    execute SFENCE.VMA with rs1=x0."
> >>
> >> The spec text around this rule hedges with "may be necessary", but
> >> RISC-V spec co-author Andrew Waterman confirmed on the ISA manual
> >> issue tracker that the fence after a satp write is not optional in
> >> this case: "The SFENCE after the SATP write is definitely necessary
> >> ... In general, you need to SFENCE after you've recycled an ASID.
> >> Since we don't use ASIDs in the Linux kernel yet, every context
> >> switch is effectively an ASID reuse, hence the full TLB flush." [1]
> >> The same reasoning applies to Xen: with ASID always 0, this satp
> >> write is indistinguishable from an ASID reuse to the hart, so the
> >> fence is required for correctness.
> > 
> > But at the moment of execution of turn_on_mmu() we don't use any ASID, 
> > do we? It was used in check_pgtbl_mode_support() but at the end it is done:
> > 
> >      csr_write(CSR_SATP, 0);
> > 
> >      sfence_vma();
> > 
> > So basically Bare mode + flush all TLBs presented before and then up to
> > 
> > ...
> > 
> >>
> >> Add the missing SFENCE.VMA to order those page-table stores before
> >> the hart's first translation under the new mapping.
> >>
> >> [1] https://github.com/riscv/riscv-isa-manual/issues/226
> >>
> >> Fixes: f5035d480f7a ("xen: add files needed for minimal riscv build")
> >> Assisted-by: Claude:claude-opus-5
> >> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> >> ---
> >>   xen/arch/riscv/riscv64/head.S | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/ 
> >> head.S
> >> index 9c40512e61..7f6edc972f 100644
> >> --- a/xen/arch/riscv/riscv64/head.S
> >> +++ b/xen/arch/riscv/riscv64/head.S
> >> @@ -98,6 +98,7 @@ FUNC(turn_on_mmu)
> >>           srli    t1, t1, PAGE_SHIFT
> >>           or      t1, t1, t0
> >>           csrw    CSR_SATP, t1
> > 
> > ... ASID isn't used as we are in Bare mode.
> > 
> > What am I missing?
> 
> After the conversation with Jan B. in the separate thread I re-read 
> documentaion and found that ASID=0 will be used here too as after 
> check_pgtbl_mode_support() we set Bare mode and ASID 0 will be really 
> used even in Bare mode as to select MODE=Bare as software must write 
> zero to the remaining fields of satp (bits 30–0 when SXLEN=32, or bits 
> 59–0 when SXLEN=64) what automatically includes field ASID (so it will 
> be zero).
> 
> But still the full reason why we need sfence.vma here is that TLB could 
> be polluted with identity mapping (even in Bare mode) and which will be 
> tagged by ASID=0.
> 
> So what about to update commit message with:
> ```
> xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu()
> 
> The existing SFENCE.VMA before the satp write only orders the page table
> stores from setup_initial_pagetables() against subsequent implicit reads.
> It does not prevent the CPU from speculatively caching translations 
> after the fence retires.
> 
> According to the RISC-V Privileged specification, implementations are
> permitted to speculatively cache Bare-mode identity mappings. Furthermore,
> selecting MODE=Bare (which happens during check_pgtbl_mode_support())
> requires zeroing the remaining fields of satp, causing ASID=0 to be
> actively used in Bare mode. Consequently, the TLB can be polluted with Bare
> identity mappings tagged with ASID=0.
> 
> Once satp is written to enable Sv39 translation, these cached identity
> mappings (tagged with ASID=0) can shadow the true Sv39 translations.
> This would lead to translation failures since turn_on_mmu() jumps to
> a non-identity-mapped linker address.
> 
> Fix this by adding a post-satp-write SFENCE.VMA to invalidate any stale
> translations (including Bare-mode identity mappings under ASID=0) before
> jumping to the virtual address space.
> ```
> 
> ~ Oleksii
> 
I read the thread and I'm ok with this suggestion.
Thanks.
> 
> 

Re: [PATCH 4/5] xen/riscv: make Zihintpause no longer a required extension

[Oleksii Kurochko]

On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> required_extensions[] panics at boot if Zihintpause is missing, but Xen
> never actually depends on it: cpu_relax() only emits the "pause" when the
> extension is implemented and otherwise falls back to the, which is a legal no-op on any hart regardless of Zihintpause
> support.

You raise a very valid point. Strictly speaking, stating that it "falls 
back to a legal no-op" can be slightly misleading because it implies the 
instruction is decoded as a literal NOP (addi x0, x0, 0).

In reality, the fallback is a fully valid FENCE instruction 
(specifically encoded as `0x0100000F`, which represents `FENCE W, 0`).

Here is why this distinction matters and why it is safe:
1. Since the FENCE instruction is a mandatory part of the RISC-V Base 
Integer Instruction Set (RV32I/RV64I), it is guaranteed to be present on 
any compliant hart. Thus, it will never trigger an "illegal instruction" 
trap.
2. When the Zihintpause extension is not implemented, the hart decodes 
and executes this instruction as a standard FENCE with a predecessor set 
of 'W' (writes) and an empty (null) successor set of '0'.
3. Because the successor set is empty, it imposes zero memory-ordering 
constraints on subsequent instructions.

Thereby I think this part of commit message will be better to re-word in 
the following way:
```
The fallback encoding `0x0100000F` is a legally valid FENCE instruction 
(`FENCE W, 0`) rather than a native NOP. Since FENCE is guaranteed by 
the RISC-V Base ISA, it will never raise an illegal instruction fault. 
In the absence of Zihintpause, it executes with an empty successor set, 
enforcing zero memory-ordering constraints and thus architecturally 
behaving as a NOP.
```


> 
> Drop it from required_extensions so hardware without Zihintpause
> still boots.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
>   xen/arch/riscv/cpufeature.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index 900cb9d772..661babc0a6 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -155,7 +155,6 @@ static const struct riscv_isa_ext_data __initconst required_extensions[] = {
>       RISCV_ISA_EXT_DATA(h),
>       RISCV_ISA_EXT_DATA(zicsr),
>       RISCV_ISA_EXT_DATA(zifencei),
> -    RISCV_ISA_EXT_DATA(zihintpause),
>       RISCV_ISA_EXT_DATA(zbb),
>   };
>   

It is also needed then to update docs/misc/riscv/booting.txt.

Generally, I agree that zihintpause should be dropped from 
required_extensions[]. One thing I would like to point out is that, once 
we do that, cpu_relax() may no longer provide a pause hint on hardware 
that doesn't implement zihintpause, even if the hardware provides its 
own pause instruction with different semantics from a fence which does 
nothing.

For example, the MIPS P8700 provides its own pause instruction with a 
different encoding from:

__asm__ __volatile__ ( ".insn r MISC_MEM, 0, 0, x0, x0, x16" );

Using fence in this case would not be power-efficient, as it behaves as 
a no-op.

For the MIPS P8700, for example:

#define MIPS_PAUSE    ASM_INSN_I("0x00501013\n\t")
#define MIPS_EHB      ASM_INSN_I("0x00301013\n\t")
#define MIPS_IHB      ASM_INSN_I("0x00101013\n\t")


I believe there are other implementations that don't use zihintpause but 
provide their own pause instruction as well.

Therefore, I suggest adding the following to riscv_fill_hw_cap():

/*
  * Zihintpause isn't mandatory: the encoding used by cpu_relax() is a
  * HINT which executes as a no-op on hardware without the extension.
  * Report it, as a platform may provide its own way to hint a spin-wait
  * loop, which then has to be wired up in cpu_relax().
  */
if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_zihintpause) )
     printk(XENLOG_WARNING
            "Zihintpause unavailable: cpu_relax() gives the CPU no hint; "
            "wire up this platform's pause equivalent in cpu_relax()\n");


Without such a check, we could easily miss updating cpu_relax() for 
platforms with their own pause mechanism. While having zihintpause as a 
required extension implicitly forces us to consider this, once it is no 
longer required, I think we should keep an explicit indication that the 
platform-specific pause mechanism may need to be wired up.

Thanks.

~ Oleksii

Re: [PATCH 4/5] xen/riscv: make Zihintpause no longer a required extension

[Baptiste Le Duc]

On 2026-08-28 10:59 +0200, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> > required_extensions[] panics at boot if Zihintpause is missing, but Xen
> > never actually depends on it: cpu_relax() only emits the "pause" when the
> > extension is implemented and otherwise falls back to the, which is a legal no-op on any hart regardless of Zihintpause
> > support.
> 
> You raise a very valid point. Strictly speaking, stating that it "falls 
> back to a legal no-op" can be slightly misleading because it implies the 
> instruction is decoded as a literal NOP (addi x0, x0, 0).
> 
> In reality, the fallback is a fully valid FENCE instruction 
> (specifically encoded as `0x0100000F`, which represents `FENCE W, 0`).
> 
> Here is why this distinction matters and why it is safe:
> 1. Since the FENCE instruction is a mandatory part of the RISC-V Base 
> Integer Instruction Set (RV32I/RV64I), it is guaranteed to be present on 
> any compliant hart. Thus, it will never trigger an "illegal instruction" 
> trap.
> 2. When the Zihintpause extension is not implemented, the hart decodes 
> and executes this instruction as a standard FENCE with a predecessor set 
> of 'W' (writes) and an empty (null) successor set of '0'.
> 3. Because the successor set is empty, it imposes zero memory-ordering 
> constraints on subsequent instructions.
> 
> Thereby I think this part of commit message will be better to re-word in 
> the following way:
> ```
> The fallback encoding `0x0100000F` is a legally valid FENCE instruction 
> (`FENCE W, 0`) rather than a native NOP. Since FENCE is guaranteed by 
> the RISC-V Base ISA, it will never raise an illegal instruction fault. 
> In the absence of Zihintpause, it executes with an empty successor set, 
> enforcing zero memory-ordering constraints and thus architecturally 
> behaving as a NOP.
> ```
I agree with this suggestion, thanks.
> 
> 
> > 
> > Drop it from required_extensions so hardware without Zihintpause
> > still boots.
> > 
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> > ---
> >   xen/arch/riscv/cpufeature.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> > index 900cb9d772..661babc0a6 100644
> > --- a/xen/arch/riscv/cpufeature.c
> > +++ b/xen/arch/riscv/cpufeature.c
> > @@ -155,7 +155,6 @@ static const struct riscv_isa_ext_data __initconst required_extensions[] = {
> >       RISCV_ISA_EXT_DATA(h),
> >       RISCV_ISA_EXT_DATA(zicsr),
> >       RISCV_ISA_EXT_DATA(zifencei),
> > -    RISCV_ISA_EXT_DATA(zihintpause),
> >       RISCV_ISA_EXT_DATA(zbb),
> >   };
> >   
> 
> It is also needed then to update docs/misc/riscv/booting.txt.
> 
> Generally, I agree that zihintpause should be dropped from 
> required_extensions[]. One thing I would like to point out is that, once 
> we do that, cpu_relax() may no longer provide a pause hint on hardware 
> that doesn't implement zihintpause, even if the hardware provides its 
> own pause instruction with different semantics from a fence which does 
> nothing.
> 
> For example, the MIPS P8700 provides its own pause instruction with a 
> different encoding from:
> 
> __asm__ __volatile__ ( ".insn r MISC_MEM, 0, 0, x0, x0, x16" );
> 
> Using fence in this case would not be power-efficient, as it behaves as 
> a no-op.
> 
> For the MIPS P8700, for example:
> 
> #define MIPS_PAUSE    ASM_INSN_I("0x00501013\n\t")
> #define MIPS_EHB      ASM_INSN_I("0x00301013\n\t")
> #define MIPS_IHB      ASM_INSN_I("0x00101013\n\t")
> 
> 
> I believe there are other implementations that don't use zihintpause but 
> provide their own pause instruction as well.
> 
> Therefore, I suggest adding the following to riscv_fill_hw_cap():
> 
> /*
>   * Zihintpause isn't mandatory: the encoding used by cpu_relax() is a
>   * HINT which executes as a no-op on hardware without the extension.
>   * Report it, as a platform may provide its own way to hint a spin-wait
>   * loop, which then has to be wired up in cpu_relax().
>   */
> if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_zihintpause) )
>      printk(XENLOG_WARNING
>             "Zihintpause unavailable: cpu_relax() gives the CPU no hint; "
>             "wire up this platform's pause equivalent in cpu_relax()\n");
> 
> 
> Without such a check, we could easily miss updating cpu_relax() for 
> platforms with their own pause mechanism. While having zihintpause as a 
> required extension implicitly forces us to consider this, once it is no 
> longer required, I think we should keep an explicit indication that the 
> platform-specific pause mechanism may need to be wired up.
> 
> Thanks.
Good catch, thanks for that. I agree with what you said to not forget
platforms that use their own pause mechanism. I will add what you
suggested in v2.
> 
> ~ Oleksii
> 
> 
Thanks.
> 
> 

Re: [PATCH 1/5] xen/riscv: always set A/D bits at boot time

[Oleksii Kurochko]

On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> Always set the PTE A/D bits at boot time to avoid an unhandled page fault
> on platforms that implement neither Svade nor Svadu, and on platforms that
> declare both in the device tree.
> 
> Rewrite the comment to enumerate the four possible Svade/Svadu combinations
> (inspired by [1]) and set A/D unconditionally, which is correct in all four
> cases until Svadu is fully supported (full support requires the SBI FWFT
> call to enable hardware updating of A/D bits).
> 
> [1] https://lwn.net/Articles/980016/
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
>   xen/arch/riscv/p2m.c | 70 ++++++++++++++++++++++++++------------------
>   1 file changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> index 1cea86512c..11dc289f0f 100644
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -591,38 +591,52 @@ static void p2m_set_permission(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.
> +     * Svade and Svadu extensions represent two schemes for managing the PTE
> +     * A/D bits. When the PTE A/D bits need to be set, the Svade extension
> +     * indicates that a page fault will be raised. In contrast, the Svadu
> +     * extension supports hardware updating of the PTE A/D bits.
>        *
> -     * 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.
> +     * There are 4 possible combinations of these extensions in the device
> +     * tree. The default hardware behavior for each is:
>        *
> -     *       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.
> +     * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> +     *    whether the platform uses Svade or Svadu. Xen should be prepared to
> +     *    handle either hardware updating of the PTE A/D bits or page faults
> +     *    when they need updating. To support both, Xen always sets the 'A' and
> +     *    'D' PTE bits at boot time.
>        *
> -     *       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.
> +     * 2) Only Svade present in DT => Xen must assume Svade to be always
> +     *    enabled.
> +     *
> +     * 3) Only Svadu present in DT => Xen must assume Svadu to be always
> +     *    enabled.
> +     *
> +     * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned
> +     *    off at boot time by setting A/D bits. To use Svadu, the supervisor
> +     *    must explicitly enable it using the SBI FWFT extension.
> +     *
> +     * The Svade extension is mandatory and the Svadu extension is optional in
> +     * the RVA23 profile. Platforms wanting to take advantage of Svadu can
> +     * choose option 3. Platforms aware of the profile can choose option 4, and
> +     * Linux won't get the benefit of Svadu until the SBI FWFT extension is
> +     * available.

I have a feeling that the DT-binding-related comment should not be 
present here, as it explains when Svadu or Svade should be considered 
enabled or disabled. We should perform this kind of detection in 
riscv_fill_hwcap() [cpufeature.c]. Then, in p2m_set_permission(), we 
should use riscv_isa_extension_available() to determine which extension 
is available and, based on that, set the A and D bits.

At this point, I think the original comment was better, as it simply 
explained what Svade and Svadu are and, therefore, provided a better 
explanation of why the A and D bits should or should not be set.

So, my suggestion is the following:

+/*
+ * Svade and Svadu extensions represent two schemes for managing the PTE
+ * A/D bits. When the PTE A/D bits need to be set, the Svade extension
+ * indicates that a page fault will be raised. In contrast, the Svadu
+ * extension supports hardware updating of the PTE A/D bits.
+ *
+ * There are 4 possible combinations of these extensions in the device 
tree.
+ * The default hardware behavior for each is:
+ *
+ * 1) Neither Svade nor Svadu present in DT => It is technically unknown
+ *    whether the platform uses Svade or Svadu. Xen should be prepared to
+ *    handle either hardware updating of the PTE A/D bits or page 
faults when
+ *    they need updating. To support both, Xen always sets the 'A' and 
'D' PTE
+ *    bits at boot time.
+ *
+ * 2) Only Svade present in DT => Xen must assume Svade to be always 
enabled.
+ *
+ * 3) Only Svadu present in DT => Xen must assume Svadu to be always 
enabled.
+ *
+ * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned
+ *    off at boot time by setting A/D bits. To use Svadu, the 
supervisor must
+ *    explicitly enable it using the SBI FWFT extension.
+ *
+ * The Svade extension is mandatory and the Svadu extension is optional 
in the
+ * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
+ * option 3. Platforms aware of the profile can choose option 4, and 
Xen won't
+ * get the benefit of Svadu until the SBI FWFT extension is available.
+ *
+ * In other words, hardware manages the A/D bits on its own only in case 3;
+ * in all the other cases software has to preset them. Instead of open 
coding
+ * this in every A/D bits user, RISCV_ISA_EXT_svade is used to mean 
"software
+ * is responsible for the A/D bits" and is set here for the cases 1, 2 
and 4.
+ */
+static void __init riscv_resolve_ad_scheme(void)
+{
+    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
+    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
+
+    /* Case 3: leave the A/D bits management to hardware. */
+    if ( svadu && !svade )
+        return;
+
+    /* Cases 1, 2 and 4: Xen has to preset the A/D bits. */
+    __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
+}
+
  void __init riscv_fill_hwcap(void)
  {
      unsigned int i;
@@ -513,6 +560,8 @@ void __init riscv_fill_hwcap(void)
          __set_bit(RISCV_ISA_EXT_sstc, riscv_isa);
      }

+    riscv_resolve_ad_scheme();
+

And then ...


> +     *
> +     * 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.
> +     *
> +     * 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.
> +     */
> +
> +    /*
> +     * Preset unconditionally for all 4 cases above, harmless when Svadu
> +     * manages the bits (case 3). Skipping it for case 3 requires SBI FWFT
> +     * which is not yet supported.
>        */
> -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
> +    e->pte |= PTE_ACCESSED | PTE_DIRTY;

... we could restore the check and the comment we originally had in 
p2m_set_permission(), but probably with some updates, something along 
the following lines:

/*
  * Xen has to preset the A/D bits unless the hardware is known to update
  * them on its own. riscv_fill_hwcap() folds all the Svade/Svadu device
  * tree combinations into RISCV_ISA_EXT_svade, which then means that
  * software is responsible for the A/D bits" (see
  * riscv_resolve_ad_scheme()).
  */

I have another comment regarding:

 > +    /*
 > +     * Preset unconditionally for all 4 cases above, harmless when Svadu
 > +     * manages the bits (case 3). Skipping it for case 3 requires 
SBI FWFT
 > +     * which is not yet supported.
 >        */
 > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
 > -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
 > +    e->pte |= PTE_ACCESSED | PTE_DIRTY;

I am not sure that this comment is correct. In case 3, we should not 
need to use the SBI FWFT extension. Case 3 means that Xen must assume 
that Svadu is enabled. Therefore, it is the responsibility of OpenSBI, 
or the pre-bootloader that loads OpenSBI, to enable it. If it fails to 
do so, then OpenSBI or the pre-bootloader is not complying with the DT 
binding documentation and it should be fixed in first place.

As further evidence, this is what OpenSBI already does [1]:
/*
  * Assume only Svadu is supported when it is the only extension
  * present in the ISA string. Svade is assumed when neither are
  * present. When both are present we must default to Svade (see
  * the zero reset value of FWFT.PTE_AD_HW_UPDATING).
  */
if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SVADE))
     __set_menvcfg_ext(SBI_HART_EXT_SVADU, ENVCFG_ADUE);

Therefore, in case 3, the original check is still valid, and there is no 
need for Xen to support the SBI FWFT extension for this case. I think 
the original check should therefore be kept as it was:

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

The SBI FWFT extension is only required for case 4. If both Svade and 
Svadu are present in the DT, Svade is selected by default. To use Svadu 
instead, SBI FWFT is required to set the ADUE bit in menvcfg, which is 
only accessible from M-mode.

Since SBI FWFT is relatively new and may not be supported by older 
OpenSBI versions, another option is to have OpenSBI hard-code ADUE=1. 
Alternatively, the DTS could specify only one of Svade or Svadu in the 
riscv,isa property. In that case, upstream OpenSBI can handle the 
configuration automatically. So specifically for our case (Svadu and 
Svade things) we don't need SBI FWFT at all.

[1] 
https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L171

~ Oleksii

Re: [PATCH 2/5] xen/riscv: preset A/D bits in Xen's own page-table mappings

[Oleksii Kurochko]

On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> The previous patch made p2m_set_permission() always set the PTE A/D bits to
> map pages in G-stage, to avoid a page fault on platforms that implement
> neither Svade nor Svadu, or that declare both in the device tree. Xen's own
> page tables, built by setup_initial_mapping(), never go through
> p2m_set_permission() and need the same fix.
> 
> Add PTE_ACCESSED to PTE_LEAF_DEFAULT and make it the minimal common leaf
> permission set by dropping PTE_WRITABLE. Rebuild PAGE_HYPERVISOR_RO,
> PAGE_HYPERVISOR_RW and PAGE_HYPERVISOR_RX from that common base, with
> PAGE_HYPERVISOR_RW also adding PTE_DIRTY. Switch setup_initial_mapping() to
> use these macros for its default, text, and rodata permissions instead of
> the equivalent raw bit lists.
> 
> A PTE is a table entry iff PTE_VALID is set and R/W/X are all clear, so
> update pte_is_table() accordingly.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
>   xen/arch/riscv/include/asm/page.h | 14 ++++++++------
>   xen/arch/riscv/mm.c               |  7 +++----
>   2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> index b465a90325..5c02f64a17 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_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE | PTE_ACCESSED)

Dropping PTE_WRITABLE here silently changes the permissions of an 
existing user of this macro that the patch doesn't touch.

check_pgtbl_mode_support() in mm.c still builds its temporary root entry as:
  index = pt_index(page_table_level, aligned_load_start);
  stage1_pgtbl_root[index] = paddr_to_pte(aligned_load_start,
                                      PTE_LEAF_DEFAULT | PTE_EXECUTABLE);

Before this patch that evaluated to V|R|W|X (RWX); afterwards it is 
V|R|A|X (RX). So the mapping loses write permission.

I believe that is harmless in practice: the entry is only alive between 
the csr_write(CSR_SATP, ...) that turns the MMU on and the 
csr_write(CSR_SATP, 0) a few lines below, it only has to make the 
current instruction stream fetchable so that the SATP mode probe can 
complete, and nothing writes through it. Arguably RX is the better 
permission set for it anyway. But it is still a behavioural change 
rather than a cosmetic one, and the commit message doesn't mention it(it 
only talks about setup_initial_mapping()).
Please call it out explicitly there.

While at it, this site should be converted too:

     stage1_pgtbl_root[index] = paddr_to_pte(aligned_load_start,
                                             PAGE_HYPERVISOR_RX);

Otherwise the patch converts three sites in setup_initial_mapping() to 
the new PAGE_HYPERVISOR_* macros while leaving a fourth one open-coding 
the redefined PTE_LEAF_DEFAULT, which is exactly the kind of asymmetry 
that makes the redefinition easy to miss on the next change.

After that conversion PTE_LEAF_DEFAULT has no users left
outside page.h itself, so it could either be dropped entirely in favour 
of PAGE_HYPERVISOR_{RO,RW,RX}, or renamed to something that reflects its 
new meaning (PTE_LEAF_COMMON or similar). "DEFAULT" now names a set that 
is not a usable permission on its own, which is misleading.

>   #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_LEAF_DEFAULT)
> +#define PAGE_HYPERVISOR_RW          (PTE_LEAF_DEFAULT | PTE_WRITABLE | PTE_DIRTY)
> +#define PAGE_HYPERVISOR_RX          (PTE_LEAF_DEFAULT | PTE_EXECUTABLE)

Adding A/D to PAGE_HYPERVISOR_RW fixes a second site beyond the ones the
commit message mentions, and I think it deserves to be spelled out.

arch_pmap_map() in asm/pmap.h writes the fixmap leaf entry directly:
     pte = pte_from_mfn(mfn, PAGE_HYPERVISOR_RW);
     write_pte(entry, pte);
i.e. it bypasses pt_update_entry(), which is the place that ORs in
PTE_ACCESSED | PTE_DIRTY for everything going through map_pages_to_xen().
So before this patch every pmap mapping was installed with A=D=0 and 
would fault on first access under Svade, in exactly the same way the 
boot page tables did.

The commit message currently frames the problem as "Xen's own page 
tables, built by setup_initial_mapping()", which undersells the fix. 
Please extend it to say that arch_pmap_map() is affected as well, and 
that it is fixed by the PAGE_HYPERVISOR_RW change rather than by the 
mm.c conversion.

FWIW I checked the remaining leaf-PTE construction sites (paddr_to_pte() 
/pte_from_mfn() callers) and with these two the series covers all of 
them: everything else either builds table entries (PTE_TABLE) or goes 
through pt_update_entry() / p2m_set_permission(), both of which set A/D 
themselves.

>   
>   #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
>   /*
> @@ -177,7 +177,8 @@ static inline bool pte_is_table(pte_t p)
>        *
>        * PAGE_HYPERVISOR_RW contains PTE_VALID too.
>        */
> -    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
> +    ASSERT((p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) !=
> +           (PTE_VALID | PTE_WRITABLE));

Please drop the last line of the comment above it:

      * PAGE_HYPERVISOR_RW contains PTE_VALID too.

That sentence existed only to explain why the old mask was written as
PAGE_HYPERVISOR_RW, i.e. that the macro is not just R|W but carries
PTE_VALID as well, which is what made the comparison against V|W work.
With the mask now written out literally, the macro is no longer 
referenced anywhere in the function, so the line dangles. It is also 
inaccurate now, since PAGE_HYPERVISOR_RW carries A and D in addition to V.

~ Oleksii

Re: [PATCH 1/5] xen/riscv: always set A/D bits at boot time

[Baptiste Le Duc]

On 2026-08-28 12:59 +0200, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> > Always set the PTE A/D bits at boot time to avoid an unhandled page fault
> > on platforms that implement neither Svade nor Svadu, and on platforms that
> > declare both in the device tree.
> > 
> > Rewrite the comment to enumerate the four possible Svade/Svadu combinations
> > (inspired by [1]) and set A/D unconditionally, which is correct in all four
> > cases until Svadu is fully supported (full support requires the SBI FWFT
> > call to enable hardware updating of A/D bits).
> > 
> > [1] https://lwn.net/Articles/980016/
> > 
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> > ---
> >   xen/arch/riscv/p2m.c | 70 ++++++++++++++++++++++++++------------------
> >   1 file changed, 42 insertions(+), 28 deletions(-)
> > 
> > diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> > index 1cea86512c..11dc289f0f 100644
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -591,38 +591,52 @@ static void p2m_set_permission(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.
> > +     * Svade and Svadu extensions represent two schemes for managing the PTE
> > +     * A/D bits. When the PTE A/D bits need to be set, the Svade extension
> > +     * indicates that a page fault will be raised. In contrast, the Svadu
> > +     * extension supports hardware updating of the PTE A/D bits.
> >        *
> > -     * 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.
> > +     * There are 4 possible combinations of these extensions in the device
> > +     * tree. The default hardware behavior for each is:
> >        *
> > -     *       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.
> > +     * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> > +     *    whether the platform uses Svade or Svadu. Xen should be prepared to
> > +     *    handle either hardware updating of the PTE A/D bits or page faults
> > +     *    when they need updating. To support both, Xen always sets the 'A' and
> > +     *    'D' PTE bits at boot time.
> >        *
> > -     *       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.
> > +     * 2) Only Svade present in DT => Xen must assume Svade to be always
> > +     *    enabled.
> > +     *
> > +     * 3) Only Svadu present in DT => Xen must assume Svadu to be always
> > +     *    enabled.
> > +     *
> > +     * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned
> > +     *    off at boot time by setting A/D bits. To use Svadu, the supervisor
> > +     *    must explicitly enable it using the SBI FWFT extension.
> > +     *
> > +     * The Svade extension is mandatory and the Svadu extension is optional in
> > +     * the RVA23 profile. Platforms wanting to take advantage of Svadu can
> > +     * choose option 3. Platforms aware of the profile can choose option 4, and
> > +     * Linux won't get the benefit of Svadu until the SBI FWFT extension is
> > +     * available.
> 
> I have a feeling that the DT-binding-related comment should not be 
> present here, as it explains when Svadu or Svade should be considered 
> enabled or disabled. We should perform this kind of detection in 
> riscv_fill_hwcap() [cpufeature.c]. Then, in p2m_set_permission(), we 
> should use riscv_isa_extension_available() to determine which extension 
> is available and, based on that, set the A and D bits.
> 
> At this point, I think the original comment was better, as it simply 
> explained what Svade and Svadu are and, therefore, provided a better 
> explanation of why the A and D bits should or should not be set.
> 
> So, my suggestion is the following:
> 
> +/*
> + * Svade and Svadu extensions represent two schemes for managing the PTE
> + * A/D bits. When the PTE A/D bits need to be set, the Svade extension
> + * indicates that a page fault will be raised. In contrast, the Svadu
> + * extension supports hardware updating of the PTE A/D bits.
> + *
> + * There are 4 possible combinations of these extensions in the device 
> tree.
> + * The default hardware behavior for each is:
> + *
> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> + *    whether the platform uses Svade or Svadu. Xen should be prepared to
> + *    handle either hardware updating of the PTE A/D bits or page 
> faults when
> + *    they need updating. To support both, Xen always sets the 'A' and 
> 'D' PTE
> + *    bits at boot time.
> + *
> + * 2) Only Svade present in DT => Xen must assume Svade to be always 
> enabled.
> + *
> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always 
> enabled.
> + *
> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned
> + *    off at boot time by setting A/D bits. To use Svadu, the 
> supervisor must
> + *    explicitly enable it using the SBI FWFT extension.
> + *
> + * The Svade extension is mandatory and the Svadu extension is optional 
> in the
> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> + * option 3. Platforms aware of the profile can choose option 4, and 
> Xen won't
> + * get the benefit of Svadu until the SBI FWFT extension is available.
> + *
> + * In other words, hardware manages the A/D bits on its own only in case 3;
> + * in all the other cases software has to preset them. Instead of open 
> coding
> + * this in every A/D bits user, RISCV_ISA_EXT_svade is used to mean 
> "software
> + * is responsible for the A/D bits" and is set here for the cases 1, 2 
> and 4.
> + */
> +static void __init riscv_resolve_ad_scheme(void)
> +{
> +    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> +    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
> +
> +    /* Case 3: leave the A/D bits management to hardware. */
> +    if ( svadu && !svade )
> +        return;
> +
> +    /* Cases 1, 2 and 4: Xen has to preset the A/D bits. */
> +    __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
> +}
> +
>   void __init riscv_fill_hwcap(void)
>   {
>       unsigned int i;
> @@ -513,6 +560,8 @@ void __init riscv_fill_hwcap(void)
>           __set_bit(RISCV_ISA_EXT_sstc, riscv_isa);
>       }
> 
> +    riscv_resolve_ad_scheme();
> +
> 
> And then ...
> 
> 
> > +     *
> > +     * 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.
> > +     *
> > +     * 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.
> > +     */
> > +
> > +    /*
> > +     * Preset unconditionally for all 4 cases above, harmless when Svadu
> > +     * manages the bits (case 3). Skipping it for case 3 requires SBI FWFT
> > +     * which is not yet supported.
> >        */
> > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> > -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
> > +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> ... we could restore the check and the comment we originally had in 

Yes it makes sense as we now manually force the svade extension in 1, 2
and 4 cases.

> p2m_set_permission(), but probably with some updates, something along 
> the following lines:
> 
> /*
>   * Xen has to preset the A/D bits unless the hardware is known to update
>   * them on its own. riscv_fill_hwcap() folds all the Svade/Svadu device
>   * tree combinations into RISCV_ISA_EXT_svade, which then means that
>   * software is responsible for the A/D bits" (see
>   * riscv_resolve_ad_scheme()).
>   */
> 
> I have another comment regarding:
> 
>  > +    /*
>  > +     * Preset unconditionally for all 4 cases above, harmless when Svadu
>  > +     * manages the bits (case 3). Skipping it for case 3 requires 
> SBI FWFT
>  > +     * which is not yet supported.
>  >        */
>  > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>  > -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
>  > +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> I am not sure that this comment is correct. In case 3, we should not 
> need to use the SBI FWFT extension. Case 3 means that Xen must assume 
> that Svadu is enabled. Therefore, it is the responsibility of OpenSBI, 
> or the pre-bootloader that loads OpenSBI, to enable it. If it fails to 
> do so, then OpenSBI or the pre-bootloader is not complying with the DT 
> binding documentation and it should be fixed in first place.

You right, thanks
> 
> As further evidence, this is what OpenSBI already does [1]:
> /*
>   * Assume only Svadu is supported when it is the only extension
>   * present in the ISA string. Svade is assumed when neither are
>   * present. When both are present we must default to Svade (see
>   * the zero reset value of FWFT.PTE_AD_HW_UPDATING).
>   */
> if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SVADE))
>      __set_menvcfg_ext(SBI_HART_EXT_SVADU, ENVCFG_ADUE);
> 
> Therefore, in case 3, the original check is still valid, and there is no 
> need for Xen to support the SBI FWFT extension for this case. I think 
> the original check should therefore be kept as it was:

Yes agree, I'll change that in v2.

> if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>      e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> The SBI FWFT extension is only required for case 4. If both Svade and 
> Svadu are present in the DT, Svade is selected by default. To use Svadu 
> instead, SBI FWFT is required to set the ADUE bit in menvcfg, which is 
> only accessible from M-mode.
> 
> Since SBI FWFT is relatively new and may not be supported by older 
> OpenSBI versions, another option is to have OpenSBI hard-code ADUE=1. 
> Alternatively, the DTS could specify only one of Svade or Svadu in the 
> riscv,isa property. In that case, upstream OpenSBI can handle the 
> configuration automatically. So specifically for our case (Svadu and 
> Svade things) we don't need SBI FWFT at all.

So if I understood correclty, you want to not let the option to change
ADUE bits in case 4 right? Therefore, I think we should document that
somewhere to clearly indicates that if someone want to use Svadu, he
should remove `svade` in the riscv,isa DT property.
> 
> [1] 
> https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L171
> 
Thanks for this very clear review.

> ~ Oleksii
> 
> 
> 
> 
> 

Re: [PATCH 3/5] xen/riscv: make Svpbmt no longer a required extension

[Oleksii Kurochko]

On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> required_extensions[] panics at boot if Svpbmt is missing, which is a
> problem on hardware that doesn't implement it.

Based only on this sentence it isn't clear why it is safe to have SvPBMT 
= n and what guarantees that if some memory for a device dma for example 
should be non-cachable and strongly ordered what will guarantee that.

So basically something like that should be added to the commit message:
```
Without the Svpbmt extension, memory attributes (such as cacheability 
and ordering) are strictly tied to physical address ranges and enforced 
by the hardware's Physical Memory Attributes (PMA) checker.

In this configuration, supervisor software relies on the platform's 
memory map: peripheral device registers (MMIO) are physically mapped 
into hardware-defined I/O regions (which are implicitly non-cacheable 
and strongly-ordered), while regular RAM is mapped as cacheable main 
memory.

S-mode paging can safely map these physical ranges without specifying 
page-based memory types in the PTEs, as the hardware MMU and PMA 
pipeline will correctly bypass caches for MMIO accesses based on the 
target physical address. Furthermore, on platforms that either feature 
fully hardware-coherent DMA or do not expose non-coherent DMA agents to 
the OS, page-level programmatic cache control via Svpbmt is not 
required, making it safe to boot and run when Svpbmt is absent.
```

  Xen already checks Svpbmt at
> runtime in some places (vcpu_csr_init()), but not everywhere:

This part sounds like there are additional places where you think the 
Svpbmt related bits should be set but I don’t see in this patch (or in 
others in this patch series_ where you are adding Svpbmt related bits to 
places where they weren’t added before. Am I missing something or did I 
misunderstand your message? If the latter then could you please re-word 
this part of the sentence.

> p2m_pte_from_mfn() and the PAGE_HYPERVISOR_NOCACHE/WC macros still set the
> raw PTE_PBMT* encoding unconditionally.
> 
> Drop Svpbmt from required_extensions, and introduce pte_pbmt(), which masks
> the requested PBMT encoding down to 0 when Svpbmt is unavailable, using it
> in both remaining unguarded spots.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
>   xen/arch/riscv/cpufeature.c       | 1 -
>   xen/arch/riscv/include/asm/page.h | 8 ++++++--
>   xen/arch/riscv/p2m.c              | 2 +-
>   3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index 92235fdfd5..900cb9d772 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -157,7 +157,6 @@ static const struct riscv_isa_ext_data __initconst required_extensions[] = {
>       RISCV_ISA_EXT_DATA(zifencei),
>       RISCV_ISA_EXT_DATA(zihintpause),
>       RISCV_ISA_EXT_DATA(zbb),
> -    RISCV_ISA_EXT_DATA(svpbmt),
>   };

Also, please update docs/misc/riscv/booting.txt.

>   
>   static bool __init is_lowercase_extension_name(const char *str)
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> index 5c02f64a17..6a3749526d 100644
> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -11,6 +11,7 @@
>   #include <xen/types.h>
>   
>   #include <asm/atomic.h>
> +#include <asm/cpufeature.h>
>   #include <asm/page-bits.h>
>   
>   #define VPN_MASK                    (PAGETABLE_ENTRIES - 1UL)
> @@ -54,6 +55,9 @@
>   #define PAGE_HYPERVISOR_RX          (PTE_LEAF_DEFAULT | PTE_EXECUTABLE)
>   
>   #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
> +
> +#define pte_pbmt(pbmt) \
> +    (riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) ? (pbmt) : 0UL)

Checking the ISA string alone isn't sufficient.

Svpbmt in HS-mode is gated by menvcfg.PBMTE. If M-mode firmware hasn't 
set it, the hardware behaves as though Svpbmt were not implemented: bits 
[62:61] become reserved again, and a non-zero encoding raises a page 
fault (even though the DT ISA string advertises svpbmt). The same 
applies to the G-stage mappings built by p2m_pte_from_mfn() below.

menvcfg isn't readable from S-mode, but the spec gives an indirect 
probe: when menvcfg.PBMTE is 0, henvcfg.PBMTE is read-only zero. Xen 
already relies on exactly this in vcpu_csr_init() (ENVCFG_PBMTE & 
csr_masks.henvcfg). So it would be more robust to compute a single flag 
in init_csr_masks():

pbmt_enabled = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) 
&& (csr_masks.henvcfg & ENVCFG_PBMTE);

and have pte_pbmt() test that instead. This makes the check reflect what 
the hardware will actually honour rather than what the DT claims, and it 
also collapses the condition in vcpu_csr_init() to a single test.

 > +
 > +#define pte_pbmt(pbmt) \
 > +    (riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) ? 
(pbmt) : 0UL)

PAGE_HYPERVISOR_NOCACHE / PAGE_HYPERVISOR_WC are no longer constant 
expressions, they are now evaluated at each use site. riscv_fill_hwcap() 
runs fairly late in start_xen(), after setup_fixmap_mappings(), 
early_fdt_map() and setup_mm(). All current ioremap() callers (aplic.c, 
kernel.c) run after it, so the code is correct today, but this is an 
implicit dependency: any ioremap introduced earlier in boot would 
silently get PBMT=0 with no diagnostic. I don't know honestly speaking 
if it is a real issue.

Worth either documenting this with a comment next to pte_pbmt(), or 
adding an ASSERT() on the initialisation state. Switching to the 
__ro_after_init flag suggested above makes the dependency explicit, 
since the flag can be set alongside csr_masks, which is also populated 
after riscv_fill_hwcap().

 > +
 > +#define pte_pbmt(pbmt) \
 > +    (riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) ? 
(pbmt) : 0UL)

pte_pbmt(pbmt) reads like a PTE accessor, i.e. something with the 
signature pte_pbmt(pte) -> enum pbmt_type, especially given that enum 
pbmt_type is declared just below in the same header. What it actually 
does is convert a requested PBMT encoding into the encoding that may 
safely be written to a PTE on this hardware.

Something like PTE_PBMT() (matching the PTE_* naming of the values it 
takes) or pbmt_encoding() would convey that better.

~ Oleksii

>   /*
>    * PAGE_HYPERVISOR_NOCACHE is used for ioremap().
>    *
> @@ -61,8 +65,8 @@
>    * is that IO is non-idempotent and strongly ordered, which makes it a good
>    * candidate for mapping IOMEM.
>    */
> -#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | PTE_PBMT_IO)
> -#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | PTE_PBMT_NOCACHE)
> +#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | pte_pbmt(PTE_PBMT_IO))
> +#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | pte_pbmt(PTE_PBMT_NOCACHE))
>   
>   /*
>    * The PTE format does not contain the following bits within itself;
> diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> index 11dc289f0f..f6e635ec1d 100644
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -683,7 +683,7 @@ static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t,
>           switch ( t )
>           {
>           case p2m_mmio_direct_io:
> -            e.pte |= PTE_PBMT_IO;
> +            e.pte |= pte_pbmt(PTE_PBMT_IO);
>               break;
>   
>           default:

Re: [PATCH 1/5] xen/riscv: always set A/D bits at boot time

[Oleksii Kurochko]

On 8/28/26 3:58 PM, Baptiste Le Duc wrote:
> On 2026-08-28 12:59 +0200, Oleksii Kurochko wrote:
>>
>>
>> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
>>> Always set the PTE A/D bits at boot time to avoid an unhandled page fault
>>> on platforms that implement neither Svade nor Svadu, and on platforms that
>>> declare both in the device tree.
>>>
>>> Rewrite the comment to enumerate the four possible Svade/Svadu combinations
>>> (inspired by [1]) and set A/D unconditionally, which is correct in all four
>>> cases until Svadu is fully supported (full support requires the SBI FWFT
>>> call to enable hardware updating of A/D bits).
>>>
>>> [1] https://lwn.net/Articles/980016/
>>>
>>> Assisted-by: Claude:claude-opus-5
>>> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>>> ---
>>>    xen/arch/riscv/p2m.c | 70 ++++++++++++++++++++++++++------------------
>>>    1 file changed, 42 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
>>> index 1cea86512c..11dc289f0f 100644
>>> --- a/xen/arch/riscv/p2m.c
>>> +++ b/xen/arch/riscv/p2m.c
>>> @@ -591,38 +591,52 @@ static void p2m_set_permission(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.
>>> +     * Svade and Svadu extensions represent two schemes for managing the PTE
>>> +     * A/D bits. When the PTE A/D bits need to be set, the Svade extension
>>> +     * indicates that a page fault will be raised. In contrast, the Svadu
>>> +     * extension supports hardware updating of the PTE A/D bits.
>>>         *
>>> -     * 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.
>>> +     * There are 4 possible combinations of these extensions in the device
>>> +     * tree. The default hardware behavior for each is:
>>>         *
>>> -     *       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.
>>> +     * 1) Neither Svade nor Svadu present in DT => It is technically unknown
>>> +     *    whether the platform uses Svade or Svadu. Xen should be prepared to
>>> +     *    handle either hardware updating of the PTE A/D bits or page faults
>>> +     *    when they need updating. To support both, Xen always sets the 'A' and
>>> +     *    'D' PTE bits at boot time.
>>>         *
>>> -     *       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.
>>> +     * 2) Only Svade present in DT => Xen must assume Svade to be always
>>> +     *    enabled.
>>> +     *
>>> +     * 3) Only Svadu present in DT => Xen must assume Svadu to be always
>>> +     *    enabled.
>>> +     *
>>> +     * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned
>>> +     *    off at boot time by setting A/D bits. To use Svadu, the supervisor
>>> +     *    must explicitly enable it using the SBI FWFT extension.
>>> +     *
>>> +     * The Svade extension is mandatory and the Svadu extension is optional in
>>> +     * the RVA23 profile. Platforms wanting to take advantage of Svadu can
>>> +     * choose option 3. Platforms aware of the profile can choose option 4, and
>>> +     * Linux won't get the benefit of Svadu until the SBI FWFT extension is
>>> +     * available.
>>
>> I have a feeling that the DT-binding-related comment should not be
>> present here, as it explains when Svadu or Svade should be considered
>> enabled or disabled. We should perform this kind of detection in
>> riscv_fill_hwcap() [cpufeature.c]. Then, in p2m_set_permission(), we
>> should use riscv_isa_extension_available() to determine which extension
>> is available and, based on that, set the A and D bits.
>>
>> At this point, I think the original comment was better, as it simply
>> explained what Svade and Svadu are and, therefore, provided a better
>> explanation of why the A and D bits should or should not be set.
>>
>> So, my suggestion is the following:
>>
>> +/*
>> + * Svade and Svadu extensions represent two schemes for managing the PTE
>> + * A/D bits. When the PTE A/D bits need to be set, the Svade extension
>> + * indicates that a page fault will be raised. In contrast, the Svadu
>> + * extension supports hardware updating of the PTE A/D bits.
>> + *
>> + * There are 4 possible combinations of these extensions in the device
>> tree.
>> + * The default hardware behavior for each is:
>> + *
>> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
>> + *    whether the platform uses Svade or Svadu. Xen should be prepared to
>> + *    handle either hardware updating of the PTE A/D bits or page
>> faults when
>> + *    they need updating. To support both, Xen always sets the 'A' and
>> 'D' PTE
>> + *    bits at boot time.
>> + *
>> + * 2) Only Svade present in DT => Xen must assume Svade to be always
>> enabled.
>> + *
>> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always
>> enabled.
>> + *
>> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned
>> + *    off at boot time by setting A/D bits. To use Svadu, the
>> supervisor must
>> + *    explicitly enable it using the SBI FWFT extension.
>> + *
>> + * The Svade extension is mandatory and the Svadu extension is optional
>> in the
>> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
>> + * option 3. Platforms aware of the profile can choose option 4, and
>> Xen won't
>> + * get the benefit of Svadu until the SBI FWFT extension is available.
>> + *
>> + * In other words, hardware manages the A/D bits on its own only in case 3;
>> + * in all the other cases software has to preset them. Instead of open
>> coding
>> + * this in every A/D bits user, RISCV_ISA_EXT_svade is used to mean
>> "software
>> + * is responsible for the A/D bits" and is set here for the cases 1, 2
>> and 4.
>> + */
>> +static void __init riscv_resolve_ad_scheme(void)
>> +{
>> +    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
>> +    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
>> +
>> +    /* Case 3: leave the A/D bits management to hardware. */
>> +    if ( svadu && !svade )
>> +        return;
>> +
>> +    /* Cases 1, 2 and 4: Xen has to preset the A/D bits. */
>> +    __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
>> +}
>> +
>>    void __init riscv_fill_hwcap(void)
>>    {
>>        unsigned int i;
>> @@ -513,6 +560,8 @@ void __init riscv_fill_hwcap(void)
>>            __set_bit(RISCV_ISA_EXT_sstc, riscv_isa);
>>        }
>>
>> +    riscv_resolve_ad_scheme();
>> +
>>
>> And then ...
>>
>>
>>> +     *
>>> +     * 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.
>>> +     *
>>> +     * 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.
>>> +     */
>>> +
>>> +    /*
>>> +     * Preset unconditionally for all 4 cases above, harmless when Svadu
>>> +     * manages the bits (case 3). Skipping it for case 3 requires SBI FWFT
>>> +     * which is not yet supported.
>>>         */
>>> -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>> -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>> +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>
>> ... we could restore the check and the comment we originally had in
> 
> Yes it makes sense as we now manually force the svade extension in 1, 2
> and 4 cases.
> 
>> p2m_set_permission(), but probably with some updates, something along
>> the following lines:
>>
>> /*
>>    * Xen has to preset the A/D bits unless the hardware is known to update
>>    * them on its own. riscv_fill_hwcap() folds all the Svade/Svadu device
>>    * tree combinations into RISCV_ISA_EXT_svade, which then means that
>>    * software is responsible for the A/D bits" (see
>>    * riscv_resolve_ad_scheme()).
>>    */
>>
>> I have another comment regarding:
>>
>>   > +    /*
>>   > +     * Preset unconditionally for all 4 cases above, harmless when Svadu
>>   > +     * manages the bits (case 3). Skipping it for case 3 requires
>> SBI FWFT
>>   > +     * which is not yet supported.
>>   >        */
>>   > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>   > -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>   > +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>
>> I am not sure that this comment is correct. In case 3, we should not
>> need to use the SBI FWFT extension. Case 3 means that Xen must assume
>> that Svadu is enabled. Therefore, it is the responsibility of OpenSBI,
>> or the pre-bootloader that loads OpenSBI, to enable it. If it fails to
>> do so, then OpenSBI or the pre-bootloader is not complying with the DT
>> binding documentation and it should be fixed in first place.
> 
> You right, thanks
>>
>> As further evidence, this is what OpenSBI already does [1]:
>> /*
>>    * Assume only Svadu is supported when it is the only extension
>>    * present in the ISA string. Svade is assumed when neither are
>>    * present. When both are present we must default to Svade (see
>>    * the zero reset value of FWFT.PTE_AD_HW_UPDATING).
>>    */
>> if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SVADE))
>>       __set_menvcfg_ext(SBI_HART_EXT_SVADU, ENVCFG_ADUE);
>>
>> Therefore, in case 3, the original check is still valid, and there is no
>> need for Xen to support the SBI FWFT extension for this case. I think
>> the original check should therefore be kept as it was:
> 
> Yes agree, I'll change that in v2.
> 
>> if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>       e->pte |= PTE_ACCESSED | PTE_DIRTY;
>>
>> The SBI FWFT extension is only required for case 4. If both Svade and
>> Svadu are present in the DT, Svade is selected by default. To use Svadu
>> instead, SBI FWFT is required to set the ADUE bit in menvcfg, which is
>> only accessible from M-mode.
>>
>> Since SBI FWFT is relatively new and may not be supported by older
>> OpenSBI versions, another option is to have OpenSBI hard-code ADUE=1.
>> Alternatively, the DTS could specify only one of Svade or Svadu in the
>> riscv,isa property. In that case, upstream OpenSBI can handle the
>> configuration automatically. So specifically for our case (Svadu and
>> Svade things) we don't need SBI FWFT at all.
> 
> So if I understood correclty, you want to not let the option to change
> ADUE bits in case 4 right? Therefore, I think we should document that
> somewhere to clearly indicates that if someone want to use Svadu, he
> should remove `svade` in the riscv,isa DT property.

Yes, that is exactly correct. Without SBI FWFT support, Xen cannot 
toggle menvcfg.ADUE in Case 4. Thus, the only viable workaround to use 
Svadu is to remove 'svade' from the riscv,isa DT property (Case 3), 
which prompts OpenSBI to enable ADUE=1 at boot time.

I agree document that somewhere will make this behavior/intention clear!

Not insisting on that:
I also think it would be a good idea to add an early printk() warning in 
the detection logic when both Svade and Svadu are present but SBI FWFT 
is missing, guiding users to drop 'svade' from their DT if they want to 
leverage Svadu. Something like:

if ( svade && svadu )
{
     /* Assuming sbi_fwft_is_supported() or similar probe is available */
     if ( !sbi_probe_extension(SBI_EXT_FWFT) )
     {
         printk(XENLOG_WARNING
                "RISC-V: Both Svade and Svadu detected, but SBI FWFT is 
missing.\n"
                "RISC-V: Defaulting to software A/D updates (Svade).\n"
                "RISC-V: To force hardware A/D updates (Svadu), remove 
'svade' from DT.\n");
     }
}

somewhere in the function (riscv_resolve_ad_scheme) I suggested above.

>>
>> [1]
>> https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L171
>>
> Thanks for this very clear review.

Welcome.

~ Oleksii