[PATCH v3 3/6] xen/riscv: fix A/D bit handling in G-stage mappings

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

There are two schemes for managing the PTE A/D bits: either a page fault is
raised when an access requires A or D to be set (ratified as Svade), or
hardware updates the bits itself (ratified as Svadu).

p2m_set_pte_flags() presets the A/D bits in G-stage PTEs only when Svade is
present in the device tree. When neither Svade nor Svadu is present, this
causes an unhandled page fault.

The four possible Svade/Svadu combinations in the device tree mean (see
[1]):
- neither present: the scheme is unknown, so the A/D bits have to be
  preset. This is harmless if hardware actually updates them itself.
- only Svade present: page faults, so the A/D bits have to be preset.
- only Svadu present: hardware updates the A/D bits.
- both present: hardware updating is off at boot and has to be enabled
  through the SBI FWFT extension, which Xen doesn't support yet, so the A/D
  bits have to be preset.

Hence preset the A/D bits in p2m_set_pte_flags() unless only Svadu is
present, i.e. when (!svadu || svade). Add riscv_resolve_ad_scheme(), called
once from riscv_fill_hwcap(), to warn when both are present but SBI FWFT is
missing, as dropping 'svade' from the DT is then the only way to get Svadu.

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

Fixes: ff14053983b0 ("xen/riscv: Implement p2m_pte_from_mfn() and support PBMT 
configuration")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
---
Changes since v2:
- change commit title.
- preset A/D bits in p2m_set_pte_flags() if (!svadu || svade), instead of
setting RISCV_ISA_EXT_svade in the riscv_isa bitmap for cases 1, 2 and 4.
- riscv_resolve_ad_scheme() now only warns when both Svade and Svadu are
present and SBI FWFT is missing; drop the dead 'svadu && svade' operand.
- repeat XENLOG_WARNING on each line of the warning.
- drop ASSERT(svade != svadu) from p2m_set_pte_flags().
- only add the sbi_probe_extension() declaration to sbi.h.
- rework the comment in p2m_set_pte_flags().
---
Changes since v1:
- change commit title
- expose RISCV_ISA_EXT_svadu so the two extensions can be told apart.
- move the Svade/Svadu resolution to a new riscv_resolve_ad_scheme(),
  called once from riscv_fill_hwcap().
- expose sbi_probe_extension() (was static) to probe for SBI FWFT.
- stop presetting A/D bits unconditionally in p2m_set_permission(), do it
  only when Svade is present.
---
 xen/arch/riscv/cpufeature.c             | 43 +++++++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/cpufeature.h |  1 +
 xen/arch/riscv/include/asm/sbi.h        |  8 ++++++
 xen/arch/riscv/p2m.c                    | 40 +++++++++---------------------
 4 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 2d12dffae7..ee35c7a17c 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -20,6 +20,7 @@
 
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
+#include <asm/sbi.h>
 
 #ifdef CONFIG_ACPI
 # error "cpufeature.c functions should be updated to support ACPI"
@@ -200,6 +201,7 @@ static const struct riscv_isa_ext_entry __initconstrel 
riscv_isa_ext[] = {
     RISCV_ISA_EXT_ENTRY(ssaia,          ANY),
     RISCV_ISA_EXT_ENTRY(sstc,           NONE),
     RISCV_ISA_EXT_ENTRY(svade,          NONE),
+    RISCV_ISA_EXT_ENTRY(svadu,          NONE),
     RISCV_ISA_EXT_ENTRY(svpbmt,         NONE),
 };
 
@@ -500,6 +502,45 @@ static void __init riscv_fill_hwcap_from_isa_string(void)
     }
 }
 
+/*
+ * 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.
+ *
+ * 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, so it presets the 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.
+ */
+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);
+
+    if ( svadu && svade )
+        if ( sbi_probe_extension(SBI_EXT_FWFT) <= 0 )
+            printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but 
SBI FWFT is missing.\n"
+                XENLOG_WARNING "RISC-V: Defaulting to software A/D updates 
(Svade).\n"
+                XENLOG_WARNING "RISC-V: To force hardware A/D updates (Svadu), 
remove 'svade' from DT.\n");
+}
+
 static bool __init has_isa_extensions_property(void)
 {
     const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
@@ -664,6 +705,8 @@ void __init riscv_fill_hwcap(void)
         __set_bit(RISCV_ISA_EXT_sstc, riscv_isa);
     }
 
+    riscv_resolve_ad_scheme();
+
     for ( i = 0; i < req_extns_amount; i++ )
     {
         const struct riscv_isa_ext_data ext = required_extensions[i];
diff --git a/xen/arch/riscv/include/asm/cpufeature.h 
b/xen/arch/riscv/include/asm/cpufeature.h
index 2973eb13a5..7a448d6111 100644
--- a/xen/arch/riscv/include/asm/cpufeature.h
+++ b/xen/arch/riscv/include/asm/cpufeature.h
@@ -41,6 +41,7 @@ enum riscv_isa_ext_id {
     RISCV_ISA_EXT_ssaia,
     RISCV_ISA_EXT_sstc,
     RISCV_ISA_EXT_svade,
+    RISCV_ISA_EXT_svadu,
     RISCV_ISA_EXT_svpbmt,
     RISCV_ISA_EXT_MAX
 };
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index 1952868e96..4efa166603 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -30,6 +30,7 @@
 #define SBI_EXT_BASE                    0x10
 #define SBI_EXT_RFENCE                  0x52464E43
 #define SBI_EXT_TIME                    0x54494D45
+#define SBI_EXT_FWFT                    0x46574654
 
 /* SBI function IDs for BASE extension */
 #define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
@@ -138,6 +139,13 @@ int sbi_remote_hfence_gvma(const cpumask_t *cpu_mask, 
vaddr_t start,
 int sbi_remote_hfence_gvma_vmid(const cpumask_t *cpu_mask, vaddr_t start,
                                 size_t size, unsigned long vmid);
 
+/*
+ * Check if an SBI extension ID is supported or not.
+ *
+ * @return: > 0 if supported, 0 if not, negative errno on SBI error.
+ */
+int sbi_probe_extension(long extid);
+
 /*
  * Initialize SBI library
  *
diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index f7b380b90a..5c8e480050 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -586,42 +586,24 @@ static inline void p2m_clean_pte(pte_t *p, bool 
clean_cache)
 
 static void p2m_set_pte_flags(pte_t *e, p2m_type_t t)
 {
+    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
+    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
+
     e->pte &= ~PTE_ACCESS_MASK;
 
     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.
-     *
-     * 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.
-     *
-     *       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.
+     * A RISC-V implementation can choose to either:
+     * 1) Update 'A' and 'D' PTE bits in hardware.
+     * 2) Generate page fault when 'A' and/or 'D' PTE bits are not set so that
+     *    software can update these bits.
      *
-     *       To support the more general case and the optimizations mentioned
-     *       above, it would be better to stop setting the A/D bits here and
-     *       instead handle page faults that occur due to unset A/D bits.
+     * Xen supports both options mentioned above: unless the platform 
guarantees
+     * (1), i.e. only Svadu is present, set 'A' and 'D' so that (2) never
+     * faults.
      */
-    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
+    if ( !svadu || svade )
         e->pte |= PTE_ACCESSED | PTE_DIRTY;
 
     switch ( t )

-- 
2.55.0