* [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-10 9:30 [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Baptiste Le Duc
@ 2026-09-10 9:34 ` Baptiste Le Duc
2026-09-21 15:26 ` Jan Beulich
2026-09-22 15:29 ` Oleksii Kurochko
2026-09-10 9:34 ` [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade Baptiste Le Duc
` (5 subsequent siblings)
6 siblings, 2 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-10 9:34 UTC (permalink / raw)
To: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: Baptiste Le Duc, xen-devel
p2m_set_permission() only presets the PTE A/D bits when the Svade extension
is present in the device tree. This causes an unhandled page fault when
neither Svade nor Svadu is present (the platform's actual behaviour is then
unknown), and when both are present in the device tree.
Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
the four possible Svade/Svadu combinations (inspired by [1]), it decides
whether software has to preset the A/D bits and, if so, sets
RISCV_ISA_EXT_svade to record that decision:
- neither present: assume Svade, since assuming Svade is harmless on real
Svadu hardware, while assuming Svadu on real Svade hardware risks an
unhandled page fault
- only Svade present: assume Svade
- only Svadu present: leave A/D management to hardware
- both present: Svade wins until Xen supports the SBI FWFT call needed to
enable hardware updating of A/D bits, so assume Svade and warn that
dropping 'svade' from the DT is 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@vates.tech>
---
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 | 59 +++++++++++++++++++++++++++++++++
xen/arch/riscv/include/asm/cpufeature.h | 1 +
xen/arch/riscv/include/asm/sbi.h | 8 +++++
xen/arch/riscv/p2m.c | 47 ++++++++++----------------
4 files changed, 86 insertions(+), 29 deletions(-)
diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 92235fdfd5..19454544a7 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -18,6 +18,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"
@@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
return false;
}
+/*
+ * Svade and Svadu extensions represent two schemes for managing the PTE A/D
+ * bits. When the PTE A/D bits need to be set, the Svade extension indicates
+ * that a page fault will be raised. In contrast, the Svadu extension supports
+ * hardware updating of the PTE A/D bits.
+ *
+ * There are 4 possible combinations of these extensions in the device tree.
+ * The default hardware behavior for each is:
+ *
+ * 1) Neither Svade nor Svadu present in DT => It is technically unknown
+ * whether the platform uses Svade or Svadu. Xen should be prepared to
+ * handle either hardware updating of the PTE A/D bits or page faults when
+ * they need updating. In that case, Xen assumes Svade because it's
+ * harmless if the platform is actually Svadu, while assuming Svadu on real
+ * Svade hardware risks an unhandled page fault.
+ *
+ * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
+ *
+ * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
+ *
+ * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
+ * at boot time by setting A/D bits. To use Svadu, the supervisor must
+ * explicitly enable it using the SBI FWFT extension.
+ *
+ * The Svade extension is mandatory and the Svadu extension is optional in the
+ * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
+ * option 3. Platforms aware of the profile can choose option 4, and Xen won't
+ * get the benefit of Svadu until the SBI FWFT extension is available.
+ *
+ * In other words, hardware manages the A/D bits on its own only in case 3, in
+ * all the other cases software has to preset them. Instead of open coding this
+ * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
+ * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
+ */
+static void __init riscv_resolve_ad_scheme(void)
+{
+ bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
+ bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
+
+ /* Case 3: leave the A/D bits management to hardware. */
+ if ( svadu && !svade )
+ return;
+
+ /* Case 4 */
+ if ( svadu && svade ){
+ if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
+ 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");
+ }
+ }
+
+ /* Cases 1, 2: Xen assume Svade to be enabled */
+ __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
+}
+
bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
enum riscv_isa_ext_id id)
{
@@ -513,6 +570,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 0c48d57a03..74200ce7c9 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_sstc,
RISCV_ISA_EXT_svade,
RISCV_ISA_EXT_svpbmt,
+ RISCV_ISA_EXT_svadu,
RISCV_ISA_EXT_MAX
};
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index 1952868e96..4f13e8c7a0 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.
+ * @extid: The extension ID to be probed.
+ *
+ * @return: 1 or an extension specific nonzero value if yes, 0 otherwise.
+ */
+int sbi_probe_extension(long extid);
/*
* Initialize SBI library
*
diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index 1cea86512c..22ad4a2aee 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -586,42 +586,31 @@ static inline void p2m_clean_pte(pte_t *p, bool clean_cache)
static void p2m_set_permission(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.
+ * riscv_fill_hwcap() sets either RISCV_ISA_EXT_svade or
+ * RISCV_ISA_EXT_svadu (mutually exclusive) depending on the Svade/Svadu
+ * device tree combination (see riscv_resolve_ad_scheme()):
+ * - RISCV_ISA_EXT_svade means that software is responsible for the A/D
+ * bits.
+ * - RISCV_ISA_EXT_svadu means the hardware is responsible for the A/D
+ * bits.
*
- * To support the more general case and the optimizations mentioned
- * above, it would be better to stop setting the A/D bits here and
- * instead handle page faults that occur due to unset A/D bits.
+ * Currently, when RISCV_ISA_EXT_svade is set, Xen doesn't track A/D
+ * bits, so it does not make use of the information that could be
+ * obtained from handling the resulting page faults, which could
+ * otherwise be useful for several use cases such as demand paging,
+ * cache-flushing optimizations, memory access tracking, etc. To avoid
+ * such a page fault, Xen presets the A and D bits instead.
*/
- if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
+ ASSERT(svade != svadu); /* exactly one of svade/svadu must be set by riscv_fill_hwcap() */
+ if ( svade )
e->pte |= PTE_ACCESSED | PTE_DIRTY;
switch ( t )
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-10 9:34 ` [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling Baptiste Le Duc
@ 2026-09-21 15:26 ` Jan Beulich
2026-09-21 17:03 ` Baptiste Le Duc
2026-09-22 15:29 ` Oleksii Kurochko
1 sibling, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2026-09-21 15:26 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Oleksii Kurochko,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini
On 10.09.2026 11:34, Baptiste Le Duc wrote:
> p2m_set_permission() only presets the PTE A/D bits when the Svade extension
> is present in the device tree. This causes an unhandled page fault when
> neither Svade nor Svadu is present (the platform's actual behaviour is then
> unknown), and when both are present in the device tree.
>
> Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
> riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
> the four possible Svade/Svadu combinations (inspired by [1]), it decides
> whether software has to preset the A/D bits and, if so, sets
> RISCV_ISA_EXT_svade to record that decision:
> - neither present: assume Svade, since assuming Svade is harmless on real
> Svadu hardware, while assuming Svadu on real Svade hardware risks an
> unhandled page fault
> - only Svade present: assume Svade
> - only Svadu present: leave A/D management to hardware
> - both present: Svade wins until Xen supports the SBI FWFT call needed to
> enable hardware updating of A/D bits, so assume Svade and warn that
> dropping 'svade' from the DT is 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@vates.tech>
> ---
> 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 | 59 +++++++++++++++++++++++++++++++++
> xen/arch/riscv/include/asm/cpufeature.h | 1 +
> xen/arch/riscv/include/asm/sbi.h | 8 +++++
> xen/arch/riscv/p2m.c | 47 ++++++++++----------------
> 4 files changed, 86 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index 92235fdfd5..19454544a7 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -18,6 +18,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"
> @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
> return false;
> }
>
> +/*
> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
> + * that a page fault will be raised. In contrast, the Svadu extension supports
> + * hardware updating of the PTE A/D bits.
> + *
> + * There are 4 possible combinations of these extensions in the device tree.
> + * The default hardware behavior for each is:
> + *
> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> + * whether the platform uses Svade or Svadu. Xen should be prepared to
> + * handle either hardware updating of the PTE A/D bits or page faults when
> + * they need updating. In that case, Xen assumes Svade because it's
> + * harmless if the platform is actually Svadu, while assuming Svadu on real
> + * Svade hardware risks an unhandled page fault.
> + *
> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
> + *
> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
> + *
> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
> + * at boot time by setting A/D bits. To use Svadu, the supervisor must
> + * explicitly enable it using the SBI FWFT extension.
> + *
> + * The Svade extension is mandatory and the Svadu extension is optional in the
> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
> + * get the benefit of Svadu until the SBI FWFT extension is available.
> + *
> + * In other words, hardware manages the A/D bits on its own only in case 3, in
> + * all the other cases software has to preset them. Instead of open coding this
> + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
> + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
> + */
> +static void __init riscv_resolve_ad_scheme(void)
> +{
> + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
> +
> + /* Case 3: leave the A/D bits management to hardware. */
> + if ( svadu && !svade )
> + return;
> +
> + /* Case 4 */
> + if ( svadu && svade ){
> + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
Nit (style): Brace placement.
Furthermore this is written in a way which Misra would call "dead code". I'd
like to suggest (leaving out comments):
if ( svadu )
{
if ( !svade )
return;
if ( !sbi_probe_extension(SBI_EXT_FWFT) )
printk(...);
}
> + printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but SBI FWFT is missing.\n"
> + "RISC-V: Defaulting to software A/D updates (Svade).\n"
> + "RISC-V: To force hardware A/D updates (Svadu), remove 'svade' from DT.\n");
Nit (style): Indentation (in multiple ways). Furthermore XENLOG_* needs
repeating after every newline.
> + }
> + }
> +
> + /* Cases 1, 2: Xen assume Svade to be enabled */
> + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
Isn't this a lie (to ourselves) then?
> --- 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.
> + * @extid: The extension ID to be probed.
> + *
> + * @return: 1 or an extension specific nonzero value if yes, 0 otherwise.
> + */
> +int sbi_probe_extension(long extid);
> /*
Nit (style): Also add a blank line.
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -586,42 +586,31 @@ static inline void p2m_clean_pte(pte_t *p, bool clean_cache)
>
> static void p2m_set_permission(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.
> + * riscv_fill_hwcap() sets either RISCV_ISA_EXT_svade or
> + * RISCV_ISA_EXT_svadu (mutually exclusive) depending on the Svade/Svadu
> + * device tree combination (see riscv_resolve_ad_scheme()):
> + * - RISCV_ISA_EXT_svade means that software is responsible for the A/D
> + * bits.
> + * - RISCV_ISA_EXT_svadu means the hardware is responsible for the A/D
> + * bits.
> *
> - * To support the more general case and the optimizations mentioned
> - * above, it would be better to stop setting the A/D bits here and
> - * instead handle page faults that occur due to unset A/D bits.
> + * Currently, when RISCV_ISA_EXT_svade is set, Xen doesn't track A/D
> + * bits, so it does not make use of the information that could be
> + * obtained from handling the resulting page faults, which could
> + * otherwise be useful for several use cases such as demand paging,
> + * cache-flushing optimizations, memory access tracking, etc. To avoid
> + * such a page fault, Xen presets the A and D bits instead.
> */
> - if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> + ASSERT(svade != svadu); /* exactly one of svade/svadu must be set by riscv_fill_hwcap() */
Here I'm lost: riscv_resolve_ad_scheme() specifically handles the "both set"
case. How can you then assert that exactly one of them is set?
Apart from this the line is also too long and the comment doesn't match our
style.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-21 15:26 ` Jan Beulich
@ 2026-09-21 17:03 ` Baptiste Le Duc
2026-09-22 6:24 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-21 17:03 UTC (permalink / raw)
To: Jan Beulich
Cc: Baptiste Le Duc, xen-devel, Alistair Francis, Connor Davis,
Oleksii Kurochko, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini
On 2026-09-21 17:26:47+02:00, Jan Beulich wrote:
> On 10.09.2026 11:34, Baptiste Le Duc wrote:
>
> > p2m_set_permission() only presets the PTE A/D bits when the Svade extension
> > is present in the device tree. This causes an unhandled page fault when
> > neither Svade nor Svadu is present (the platform's actual behaviour is then
> > unknown), and when both are present in the device tree.
> >
> > Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
> > riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
> > the four possible Svade/Svadu combinations (inspired by [1]), it decides
> > whether software has to preset the A/D bits and, if so, sets
> > RISCV_ISA_EXT_svade to record that decision:
> > - neither present: assume Svade, since assuming Svade is harmless on real
> > Svadu hardware, while assuming Svadu on real Svade hardware risks an
> > unhandled page fault
> > - only Svade present: assume Svade
> > - only Svadu present: leave A/D management to hardware
> > - both present: Svade wins until Xen supports the SBI FWFT call needed to
> > enable hardware updating of A/D bits, so assume Svade and warn that
> > dropping 'svade' from the DT is 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@vates.tech>
> > ---
> > 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 | 59 +++++++++++++++++++++++++++++++++
> > xen/arch/riscv/include/asm/cpufeature.h | 1 +
> > xen/arch/riscv/include/asm/sbi.h | 8 +++++
> > xen/arch/riscv/p2m.c | 47 ++++++++++----------------
> > 4 files changed, 86 insertions(+), 29 deletions(-)
> >
> > diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> > index 92235fdfd5..19454544a7 100644
> > --- a/xen/arch/riscv/cpufeature.c
> > +++ b/xen/arch/riscv/cpufeature.c
> > @@ -18,6 +18,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"
> > @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
> > return false;
> > }
> >
> > +/*
> > + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
> > + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
> > + * that a page fault will be raised. In contrast, the Svadu extension supports
> > + * hardware updating of the PTE A/D bits.
> > + *
> > + * There are 4 possible combinations of these extensions in the device tree.
> > + * The default hardware behavior for each is:
> > + *
> > + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> > + * whether the platform uses Svade or Svadu. Xen should be prepared to
> > + * handle either hardware updating of the PTE A/D bits or page faults when
> > + * they need updating. In that case, Xen assumes Svade because it's
> > + * harmless if the platform is actually Svadu, while assuming Svadu on real
> > + * Svade hardware risks an unhandled page fault.
> > + *
> > + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
> > + *
> > + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
> > + *
> > + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
> > + * at boot time by setting A/D bits. To use Svadu, the supervisor must
> > + * explicitly enable it using the SBI FWFT extension.
> > + *
> > + * The Svade extension is mandatory and the Svadu extension is optional in the
> > + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> > + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
> > + * get the benefit of Svadu until the SBI FWFT extension is available.
> > + *
> > + * In other words, hardware manages the A/D bits on its own only in case 3, in
> > + * all the other cases software has to preset them. Instead of open coding this
> > + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
> > + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
> > + */
> > +static void __init riscv_resolve_ad_scheme(void)
> > +{
> > + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> > + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
> > +
> > + /* Case 3: leave the A/D bits management to hardware. */
> > + if ( svadu && !svade )
> > + return;
> > +
> > + /* Case 4 */
> > + if ( svadu && svade ){
> > + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
>
> Nit (style): Brace placement.
Sorry for that. I will fix that in v3.
> Furthermore this is written in a way which Misra would call "dead code". I'd
> like to suggest (leaving out comments):
>
> if ( svadu )
> {
> if ( !svade )
> return;
>
> if ( !sbi_probe_extension(SBI_EXT_FWFT) )
> printk(...);
> }
I assume you are referring to Misra C:2012 Rule 13.5 "The right operand
of a logical && or || operand shall not contain persistent side effect"
If yes, IMO, I think it doesn't apply here as `svade` is evaluated
before the `if` so there is no side effect that wouldn't have been
executed in case of svadu=false.
>
> > + printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but SBI FWFT is missing.\n"
> > + "RISC-V: Defaulting to software A/D updates (Svade).\n"
> > + "RISC-V: To force hardware A/D updates (Svadu), remove 'svade' from DT.\n");
>
> Nit (style): Indentation (in multiple ways). Furthermore XENLOG_* needs
> repeating after every newline.
>
> > + }
> > + }
> > +
> > + /* Cases 1, 2: Xen assume Svade to be enabled */
> > + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
>
> Isn't this a lie (to ourselves) then?
If you are talking about case 1:
[1] Yes, it's technically a lie for boards shipped before
the svade/svadu extension was ratified (e.g., HiFive Premier P550).
These extensions merely formalized a mechanism that already existed in
hardware.
[2] For boards that do support svade, we could enforce DT
declaration by adding it to `required_extension` as they are
explicitly supporting it. However, doing so would cause boards
without svade/svadu support (as described above) to hit a panic
during boot.
So in both case ([1], [2]), the svade extension exist either implicitely or
explicitly. Therefore, force it doesn't compromize anything.
>
> > --- 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.
> > + * @extid: The extension ID to be probed.
> > + *
> > + * @return: 1 or an extension specific nonzero value if yes, 0 otherwise.
> > + */
> > +int sbi_probe_extension(long extid);
> > /*
>
> Nit (style): Also add a blank line.
>
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -586,42 +586,31 @@ static inline void p2m_clean_pte(pte_t *p, bool clean_cache)
> >
> > static void p2m_set_permission(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.
> > + * riscv_fill_hwcap() sets either RISCV_ISA_EXT_svade or
> > + * RISCV_ISA_EXT_svadu (mutually exclusive) depending on the Svade/Svadu
> > + * device tree combination (see riscv_resolve_ad_scheme()):
> > + * - RISCV_ISA_EXT_svade means that software is responsible for the A/D
> > + * bits.
> > + * - RISCV_ISA_EXT_svadu means the hardware is responsible for the A/D
> > + * bits.
> > *
> > - * To support the more general case and the optimizations mentioned
> > - * above, it would be better to stop setting the A/D bits here and
> > - * instead handle page faults that occur due to unset A/D bits.
> > + * Currently, when RISCV_ISA_EXT_svade is set, Xen doesn't track A/D
> > + * bits, so it does not make use of the information that could be
> > + * obtained from handling the resulting page faults, which could
> > + * otherwise be useful for several use cases such as demand paging,
> > + * cache-flushing optimizations, memory access tracking, etc. To avoid
> > + * such a page fault, Xen presets the A and D bits instead.
> > */
> > - if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> > + ASSERT(svade != svadu); /* exactly one of svade/svadu must be set by riscv_fill_hwcap() */
>
> Here I'm lost: riscv_resolve_ad_scheme() specifically handles the "both set"
> case. How can you then assert that exactly one of them is set?
Because in the future, with SBI FWFT support, both extensions could be
supported by the hardware and listed in the DT. Xen could still assume
that only Svadu is turned-off at boot time but then, to use Svadu, it
should explicitly enable it by using SBI FWFT extension.
I agree it's not needed right now. I'll drop it until then.
>
> Apart from this the line is also too long and the comment doesn't match our
> style.
>
> Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-21 17:03 ` Baptiste Le Duc
@ 2026-09-22 6:24 ` Jan Beulich
2026-09-22 9:17 ` Baptiste Le Duc
0 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2026-09-22 6:24 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Oleksii Kurochko,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini
On 21.09.2026 19:03, Baptiste Le Duc wrote:
> On 2026-09-21 17:26:47+02:00, Jan Beulich wrote:
>> On 10.09.2026 11:34, Baptiste Le Duc wrote:
>>> @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
>>> return false;
>>> }
>>>
>>> +/*
>>> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
>>> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
>>> + * that a page fault will be raised. In contrast, the Svadu extension supports
>>> + * hardware updating of the PTE A/D bits.
>>> + *
>>> + * There are 4 possible combinations of these extensions in the device tree.
>>> + * The default hardware behavior for each is:
>>> + *
>>> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
>>> + * whether the platform uses Svade or Svadu. Xen should be prepared to
>>> + * handle either hardware updating of the PTE A/D bits or page faults when
>>> + * they need updating. In that case, Xen assumes Svade because it's
>>> + * harmless if the platform is actually Svadu, while assuming Svadu on real
>>> + * Svade hardware risks an unhandled page fault.
>>> + *
>>> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
>>> + *
>>> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
>>> + *
>>> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
>>> + * at boot time by setting A/D bits. To use Svadu, the supervisor must
>>> + * explicitly enable it using the SBI FWFT extension.
>>> + *
>>> + * The Svade extension is mandatory and the Svadu extension is optional in the
>>> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
>>> + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
>>> + * get the benefit of Svadu until the SBI FWFT extension is available.
>>> + *
>>> + * In other words, hardware manages the A/D bits on its own only in case 3, in
>>> + * all the other cases software has to preset them. Instead of open coding this
>>> + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
>>> + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
>>> + */
>>> +static void __init riscv_resolve_ad_scheme(void)
>>> +{
>>> + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
>>> + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
>>> +
>>> + /* Case 3: leave the A/D bits management to hardware. */
>>> + if ( svadu && !svade )
>>> + return;
>>> +
>>> + /* Case 4 */
>>> + if ( svadu && svade ){
>>> + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
>>
>> Nit (style): Brace placement.
> Sorry for that. I will fix that in v3.
>> Furthermore this is written in a way which Misra would call "dead code". I'd
>> like to suggest (leaving out comments):
>>
>> if ( svadu )
>> {
>> if ( !svade )
>> return;
>>
>> if ( !sbi_probe_extension(SBI_EXT_FWFT) )
>> printk(...);
>> }
> I assume you are referring to Misra C:2012 Rule 13.5 "The right operand
> of a logical && or || operand shall not contain persistent side effect"
>
> If yes, IMO, I think it doesn't apply here as `svade` is evaluated
> before the `if` so there is no side effect that wouldn't have been
> executed in case of svadu=false.
No, there's nothing side-effect-ish here. With "svadu && !svade" in the
first if(), the rhs of "svadu && svade" in the second one is dead code:
Things would function the same with it dropped.
>>> + printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but SBI FWFT is missing.\n"
>>> + "RISC-V: Defaulting to software A/D updates (Svade).\n"
>>> + "RISC-V: To force hardware A/D updates (Svadu), remove 'svade' from DT.\n");
>>
>> Nit (style): Indentation (in multiple ways). Furthermore XENLOG_* needs
>> repeating after every newline.
>>
>>> + }
>>> + }
>>> +
>>> + /* Cases 1, 2: Xen assume Svade to be enabled */
>>> + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
>>
>> Isn't this a lie (to ourselves) then?
> If you are talking about case 1:
> [1] Yes, it's technically a lie for boards shipped before
> the svade/svadu extension was ratified (e.g., HiFive Premier P550).
> These extensions merely formalized a mechanism that already existed in
> hardware.
Wait, how do you know this for _all_ boards anyone may ever have made?
And for all qemu (and alike) versions which supported RISC-V?
> [2] For boards that do support svade, we could enforce DT
> declaration by adding it to `required_extension` as they are
> explicitly supporting it. However, doing so would cause boards
> without svade/svadu support (as described above) to hit a panic
> during boot.
>
> So in both case ([1], [2]), the svade extension exist either implicitely or
> explicitly. Therefore, force it doesn't compromize anything.
If, despite my comment above, this is indeed what is wanted, I think it
requires a little more commentary.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-22 6:24 ` Jan Beulich
@ 2026-09-22 9:17 ` Baptiste Le Duc
2026-09-22 15:14 ` Oleksii Kurochko
2026-09-23 7:31 ` Oleksii Kurochko
0 siblings, 2 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-22 9:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Baptiste Le Duc, xen-devel, Alistair Francis, Connor Davis,
Oleksii Kurochko, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini
On 2026-09-22 08:24 +0200, Jan Beulich wrote:
> On 21.09.2026 19:03, Baptiste Le Duc wrote:
> > On 2026-09-21 17:26:47+02:00, Jan Beulich wrote:
> >> On 10.09.2026 11:34, Baptiste Le Duc wrote:
> >>> @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
> >>> return false;
> >>> }
> >>>
> >>> +/*
> >>> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
> >>> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
> >>> + * that a page fault will be raised. In contrast, the Svadu extension supports
> >>> + * hardware updating of the PTE A/D bits.
> >>> + *
> >>> + * There are 4 possible combinations of these extensions in the device tree.
> >>> + * The default hardware behavior for each is:
> >>> + *
> >>> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> >>> + * whether the platform uses Svade or Svadu. Xen should be prepared to
> >>> + * handle either hardware updating of the PTE A/D bits or page faults when
> >>> + * they need updating. In that case, Xen assumes Svade because it's
> >>> + * harmless if the platform is actually Svadu, while assuming Svadu on real
> >>> + * Svade hardware risks an unhandled page fault.
> >>> + *
> >>> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
> >>> + *
> >>> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
> >>> + *
> >>> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
> >>> + * at boot time by setting A/D bits. To use Svadu, the supervisor must
> >>> + * explicitly enable it using the SBI FWFT extension.
> >>> + *
> >>> + * The Svade extension is mandatory and the Svadu extension is optional in the
> >>> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> >>> + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
> >>> + * get the benefit of Svadu until the SBI FWFT extension is available.
> >>> + *
> >>> + * In other words, hardware manages the A/D bits on its own only in case 3, in
> >>> + * all the other cases software has to preset them. Instead of open coding this
> >>> + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
> >>> + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
> >>> + */
> >>> +static void __init riscv_resolve_ad_scheme(void)
> >>> +{
> >>> + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> >>> + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
> >>> +
> >>> + /* Case 3: leave the A/D bits management to hardware. */
> >>> + if ( svadu && !svade )
> >>> + return;
> >>> +
> >>> + /* Case 4 */
> >>> + if ( svadu && svade ){
> >>> + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
> >>
> >> Nit (style): Brace placement.
> > Sorry for that. I will fix that in v3.
> >> Furthermore this is written in a way which Misra would call "dead code". I'd
> >> like to suggest (leaving out comments):
> >>
> >> if ( svadu )
> >> {
> >> if ( !svade )
> >> return;
> >>
> >> if ( !sbi_probe_extension(SBI_EXT_FWFT) )
> >> printk(...);
> >> }
> > I assume you are referring to Misra C:2012 Rule 13.5 "The right operand
> > of a logical && or || operand shall not contain persistent side effect"
> >
> > If yes, IMO, I think it doesn't apply here as `svade` is evaluated
> > before the `if` so there is no side effect that wouldn't have been
> > executed in case of svadu=false.
>
> No, there's nothing side-effect-ish here. With "svadu && !svade" in the
> first if(), the rhs of "svadu && svade" in the second one is dead code:
> Things would function the same with it dropped.
Ok, now I understand, thanks. I'll fix it in next round.
>
> >>> + printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but SBI FWFT is missing.\n"
> >>> + "RISC-V: Defaulting to software A/D updates (Svade).\n"
> >>> + "RISC-V: To force hardware A/D updates (Svadu), remove 'svade' from DT.\n");
> >>
> >> Nit (style): Indentation (in multiple ways). Furthermore XENLOG_* needs
> >> repeating after every newline.
> >>
> >>> + }
> >>> + }
> >>> +
> >>> + /* Cases 1, 2: Xen assume Svade to be enabled */
> >>> + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
> >>
> >> Isn't this a lie (to ourselves) then?
> > If you are talking about case 1:
> > [1] Yes, it's technically a lie for boards shipped before
> > the svade/svadu extension was ratified (e.g., HiFive Premier P550).
> > These extensions merely formalized a mechanism that already existed in
> > hardware.
>
> Wait, how do you know this for _all_ boards anyone may ever have made?
We don't know but based on [1] and my commit message, if neither
Svade nor Svadu are present in DT then it is technically unknown whether
the platform uses Svade or Svade. Hypervisor may then assume Svade to be
present and enabled or it can discover based on mvendorid, marchid, and
mimpid. For this patch, I choose to have the Hypervisor assumed Svade.
Saying that, I agree that it doesn't make sense to manually have set
Svade extension in the isa bitfield as we could just preset A/D bits
regardless of Svade/Svadu during the p2m_set_permission(). It's what
kvm explains in kvm_riscv_gstage_map_page():
/*
* A RISC-V implementation can choose to either:
* 1) Update 'A' and 'D' PTE bits in hardware
* 2) Generate page fault when 'A' and/or 'D' bits are not set
* PTE so that software can update these bits.
*
* We support both options mentioned above. To achieve this, we
* always set 'A' and 'D' PTE bits at time of creating G-stage
* mapping. To support KVM dirty page logging with both options
* mentioned above, we will write-protect G-stage PTEs to track
* dirty pages.
*/
[1] https://lore.kernel.org/lkml/20240628093711.11716-1-yongxuan.wang@sifive.com/#t
> And for all qemu (and alike) versions which supported RISC-V?
Concerning qemu, you're right, in case when (!svade && !svadu) they use
by default Svadu (hw updating) for backward compatibility.
>
> > [2] For boards that do support svade, we could enforce DT
> > declaration by adding it to `required_extension` as they are
> > explicitly supporting it. However, doing so would cause boards
> > without svade/svadu support (as described above) to hit a panic
> > during boot.
> >
> > So in both case ([1], [2]), the svade extension exist either implicitely or
> > explicitly. Therefore, force it doesn't compromize anything.
>
> If, despite my comment above, this is indeed what is wanted, I think it
> requires a little more commentary.
>
> Jan
>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-22 9:17 ` Baptiste Le Duc
@ 2026-09-22 15:14 ` Oleksii Kurochko
2026-09-22 15:18 ` Baptiste Le Duc
2026-09-23 7:31 ` Oleksii Kurochko
1 sibling, 1 reply; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 15:14 UTC (permalink / raw)
To: Baptiste Le Duc, Jan Beulich
Cc: xen-devel, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On 9/22/26 11:17 AM, Baptiste Le Duc wrote:
> On 2026-09-22 08:24 +0200, Jan Beulich wrote:
>> On 21.09.2026 19:03, Baptiste Le Duc wrote:
>>> On 2026-09-21 17:26:47+02:00, Jan Beulich wrote:
>>>> On 10.09.2026 11:34, Baptiste Le Duc wrote:
>>>>> @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
>>>>> return false;
>>>>> }
>>>>>
>>>>> +/*
>>>>> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
>>>>> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
>>>>> + * that a page fault will be raised. In contrast, the Svadu extension supports
>>>>> + * hardware updating of the PTE A/D bits.
>>>>> + *
>>>>> + * There are 4 possible combinations of these extensions in the device tree.
>>>>> + * The default hardware behavior for each is:
>>>>> + *
>>>>> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
>>>>> + * whether the platform uses Svade or Svadu. Xen should be prepared to
>>>>> + * handle either hardware updating of the PTE A/D bits or page faults when
>>>>> + * they need updating. In that case, Xen assumes Svade because it's
>>>>> + * harmless if the platform is actually Svadu, while assuming Svadu on real
>>>>> + * Svade hardware risks an unhandled page fault.
>>>>> + *
>>>>> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
>>>>> + *
>>>>> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
>>>>> + *
>>>>> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
>>>>> + * at boot time by setting A/D bits. To use Svadu, the supervisor must
>>>>> + * explicitly enable it using the SBI FWFT extension.
>>>>> + *
>>>>> + * The Svade extension is mandatory and the Svadu extension is optional in the
>>>>> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
>>>>> + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
>>>>> + * get the benefit of Svadu until the SBI FWFT extension is available.
>>>>> + *
>>>>> + * In other words, hardware manages the A/D bits on its own only in case 3, in
>>>>> + * all the other cases software has to preset them. Instead of open coding this
>>>>> + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
>>>>> + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
>>>>> + */
>>>>> +static void __init riscv_resolve_ad_scheme(void)
>>>>> +{
>>>>> + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
>>>>> + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
>>>>> +
>>>>> + /* Case 3: leave the A/D bits management to hardware. */
>>>>> + if ( svadu && !svade )
>>>>> + return;
>>>>> +
>>>>> + /* Case 4 */
>>>>> + if ( svadu && svade ){
>>>>> + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
>>>>
>>>> Nit (style): Brace placement.
>>> Sorry for that. I will fix that in v3.
>>>> Furthermore this is written in a way which Misra would call "dead code". I'd
>>>> like to suggest (leaving out comments):
>>>>
>>>> if ( svadu )
>>>> {
>>>> if ( !svade )
>>>> return;
>>>>
>>>> if ( !sbi_probe_extension(SBI_EXT_FWFT) )
>>>> printk(...);
>>>> }
>>> I assume you are referring to Misra C:2012 Rule 13.5 "The right operand
>>> of a logical && or || operand shall not contain persistent side effect"
>>>
>>> If yes, IMO, I think it doesn't apply here as `svade` is evaluated
>>> before the `if` so there is no side effect that wouldn't have been
>>> executed in case of svadu=false.
>>
>> No, there's nothing side-effect-ish here. With "svadu && !svade" in the
>> first if(), the rhs of "svadu && svade" in the second one is dead code:
>> Things would function the same with it dropped.
> Ok, now I understand, thanks. I'll fix it in next round.
>>
>>>>> + printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but SBI FWFT is missing.\n"
>>>>> + "RISC-V: Defaulting to software A/D updates (Svade).\n"
>>>>> + "RISC-V: To force hardware A/D updates (Svadu), remove 'svade' from DT.\n");
>>>>
>>>> Nit (style): Indentation (in multiple ways). Furthermore XENLOG_* needs
>>>> repeating after every newline.
>>>>
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /* Cases 1, 2: Xen assume Svade to be enabled */
>>>>> + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
>>>>
>>>> Isn't this a lie (to ourselves) then?
>>> If you are talking about case 1:
>>> [1] Yes, it's technically a lie for boards shipped before
>>> the svade/svadu extension was ratified (e.g., HiFive Premier P550).
>>> These extensions merely formalized a mechanism that already existed in
>>> hardware.
>>
>> Wait, how do you know this for _all_ boards anyone may ever have made?
> We don't know but based on [1] and my commit message, if neither
> Svade nor Svadu are present in DT then it is technically unknown whether
> the platform uses Svade or Svade. Hypervisor may then assume Svade to be
> present and enabled or it can discover based on mvendorid, marchid, and
> mimpid. For this patch, I choose to have the Hypervisor assumed Svade.
Can hypervisor really access this regs?
~ Oleksii
>
> Saying that, I agree that it doesn't make sense to manually have set
> Svade extension in the isa bitfield as we could just preset A/D bits
> regardless of Svade/Svadu during the p2m_set_permission(). It's what
> kvm explains in kvm_riscv_gstage_map_page():
>
> /*
> * A RISC-V implementation can choose to either:
> * 1) Update 'A' and 'D' PTE bits in hardware
> * 2) Generate page fault when 'A' and/or 'D' bits are not set
> * PTE so that software can update these bits.
> *
> * We support both options mentioned above. To achieve this, we
> * always set 'A' and 'D' PTE bits at time of creating G-stage
> * mapping. To support KVM dirty page logging with both options
> * mentioned above, we will write-protect G-stage PTEs to track
> * dirty pages.
> */
>
>
> [1] https://lore.kernel.org/lkml/20240628093711.11716-1-yongxuan.wang@sifive.com/#t
>> And for all qemu (and alike) versions which supported RISC-V?
>
> Concerning qemu, you're right, in case when (!svade && !svadu) they use
> by default Svadu (hw updating) for backward compatibility.
>
>>
>>> [2] For boards that do support svade, we could enforce DT
>>> declaration by adding it to `required_extension` as they are
>>> explicitly supporting it. However, doing so would cause boards
>>> without svade/svadu support (as described above) to hit a panic
>>> during boot.
>>>
>>> So in both case ([1], [2]), the svade extension exist either implicitely or
>>> explicitly. Therefore, force it doesn't compromize anything.
>>
>> If, despite my comment above, this is indeed what is wanted, I think it
>> requires a little more commentary.
>>
>> Jan
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-22 15:14 ` Oleksii Kurochko
@ 2026-09-22 15:18 ` Baptiste Le Duc
0 siblings, 0 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-22 15:18 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Baptiste Le Duc, Jan Beulich, xen-devel, Alistair Francis,
Connor Davis, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini
On 2026-09-22 17:14:23+02:00, Oleksii Kurochko wrote:
> On 9/22/26 11:17 AM, Baptiste Le Duc wrote:
>
> > On 2026-09-22 08:24 +0200, Jan Beulich wrote:
> > Ok, now I understand, thanks. I'll fix it in next round.
> > We don't know but based on [1] and my commit message, if neither
> > Svade nor Svadu are present in DT then it is technically unknown whether
> > the platform uses Svade or Svade. Hypervisor may then assume Svade to be
> > present and enabled or it can discover based on mvendorid, marchid, and
> > mimpid. For this patch, I choose to have the Hypervisor assumed Svade.
>
> Can hypervisor really access this regs?
No it can't as they are M-mode only, it's why I choose to have the Hypervisor assumed Svade.
>
> ~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-22 9:17 ` Baptiste Le Duc
2026-09-22 15:14 ` Oleksii Kurochko
@ 2026-09-23 7:31 ` Oleksii Kurochko
1 sibling, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-23 7:31 UTC (permalink / raw)
To: Baptiste Le Duc, Jan Beulich
Cc: xen-devel, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On 9/22/26 11:17 AM, Baptiste Le Duc wrote:
> On 2026-09-22 08:24 +0200, Jan Beulich wrote:
>> On 21.09.2026 19:03, Baptiste Le Duc wrote:
>>> On 2026-09-21 17:26:47+02:00, Jan Beulich wrote:
>>>> On 10.09.2026 11:34, Baptiste Le Duc wrote:
>>>>> @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
>>>>> return false;
>>>>> }
>>>>>
>>>>> +/*
>>>>> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
>>>>> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
>>>>> + * that a page fault will be raised. In contrast, the Svadu extension supports
>>>>> + * hardware updating of the PTE A/D bits.
>>>>> + *
>>>>> + * There are 4 possible combinations of these extensions in the device tree.
>>>>> + * The default hardware behavior for each is:
>>>>> + *
>>>>> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
>>>>> + * whether the platform uses Svade or Svadu. Xen should be prepared to
>>>>> + * handle either hardware updating of the PTE A/D bits or page faults when
>>>>> + * they need updating. In that case, Xen assumes Svade because it's
>>>>> + * harmless if the platform is actually Svadu, while assuming Svadu on real
>>>>> + * Svade hardware risks an unhandled page fault.
>>>>> + *
>>>>> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
>>>>> + *
>>>>> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
>>>>> + *
>>>>> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
>>>>> + * at boot time by setting A/D bits. To use Svadu, the supervisor must
>>>>> + * explicitly enable it using the SBI FWFT extension.
>>>>> + *
>>>>> + * The Svade extension is mandatory and the Svadu extension is optional in the
>>>>> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
>>>>> + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
>>>>> + * get the benefit of Svadu until the SBI FWFT extension is available.
>>>>> + *
>>>>> + * In other words, hardware manages the A/D bits on its own only in case 3, in
>>>>> + * all the other cases software has to preset them. Instead of open coding this
>>>>> + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
>>>>> + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
>>>>> + */
>>>>> +static void __init riscv_resolve_ad_scheme(void)
>>>>> +{
>>>>> + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
>>>>> + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
>>>>> +
>>>>> + /* Case 3: leave the A/D bits management to hardware. */
>>>>> + if ( svadu && !svade )
>>>>> + return;
>>>>> +
>>>>> + /* Case 4 */
>>>>> + if ( svadu && svade ){
>>>>> + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
>>>>
>>>> Nit (style): Brace placement.
>>> Sorry for that. I will fix that in v3.
>>>> Furthermore this is written in a way which Misra would call "dead code". I'd
>>>> like to suggest (leaving out comments):
>>>>
>>>> if ( svadu )
>>>> {
>>>> if ( !svade )
>>>> return;
>>>>
>>>> if ( !sbi_probe_extension(SBI_EXT_FWFT) )
>>>> printk(...);
>>>> }
>>> I assume you are referring to Misra C:2012 Rule 13.5 "The right operand
>>> of a logical && or || operand shall not contain persistent side effect"
>>>
>>> If yes, IMO, I think it doesn't apply here as `svade` is evaluated
>>> before the `if` so there is no side effect that wouldn't have been
>>> executed in case of svadu=false.
>>
>> No, there's nothing side-effect-ish here. With "svadu && !svade" in the
>> first if(), the rhs of "svadu && svade" in the second one is dead code:
>> Things would function the same with it dropped.
> Ok, now I understand, thanks. I'll fix it in next round.
>>
>>>>> + printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, but SBI FWFT is missing.\n"
>>>>> + "RISC-V: Defaulting to software A/D updates (Svade).\n"
>>>>> + "RISC-V: To force hardware A/D updates (Svadu), remove 'svade' from DT.\n");
>>>>
>>>> Nit (style): Indentation (in multiple ways). Furthermore XENLOG_* needs
>>>> repeating after every newline.
>>>>
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /* Cases 1, 2: Xen assume Svade to be enabled */
>>>>> + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
>>>>
>>>> Isn't this a lie (to ourselves) then?
>>> If you are talking about case 1:
>>> [1] Yes, it's technically a lie for boards shipped before
>>> the svade/svadu extension was ratified (e.g., HiFive Premier P550).
>>> These extensions merely formalized a mechanism that already existed in
>>> hardware.
>>
>> Wait, how do you know this for _all_ boards anyone may ever have made?
> We don't know but based on [1] and my commit message, if neither
> Svade nor Svadu are present in DT then it is technically unknown whether
> the platform uses Svade or Svade.
It is unknown from DT point of view but it isn't true from h/w point of
view. H/W knows what it supports Svade or Svadu. That is why I am not
convinced that in p2m_set_permission() we should use DT binding
explanation. The original comment is better as it describes all the
possible from h/w point and not DT point of view. Also, generally
nothing guarantee that DT is correct (someone miss to add Svade or Svadu
in riscv,isa property) so make an explanation *only* based on it
probably isn't the best one option and probably it will be better just
have a comment as it was in original changes.
I think that the easiest option for us is to ...
> Hypervisor may then assume Svade to be
> present and enabled or it can discover based on mvendorid, marchid, and
> mimpid. For this patch, I choose to have the Hypervisor assumed Svade.
>
> Saying that, I agree that it doesn't make sense to manually have set
> Svade extension in the isa bitfield as we could just preset A/D bits
> regardless of Svade/Svadu during the p2m_set_permission(). It's what
But then it means that in the case of Svadu we will have not precise
statistics about A/D bits. I think that for p2m_set_permission() we
still may want to have if () condition around as at the moment of
p2m_set_permission is being called we could identify which A/D scheme is
supported by h/w.
Therefore I think we still want to set ISA bitmap based on what I
described below ...
> kvm explains in kvm_riscv_gstage_map_page():
>
> /*
> * A RISC-V implementation can choose to either:
> * 1) Update 'A' and 'D' PTE bits in hardware
> * 2) Generate page fault when 'A' and/or 'D' bits are not set
> * PTE so that software can update these bits.
> *
> * We support both options mentioned above. To achieve this, we
> * always set 'A' and 'D' PTE bits at time of creating G-stage
> * mapping. To support KVM dirty page logging with both options
> * mentioned above, we will write-protect G-stage PTEs to track
> * dirty pages.
> */
>
>
> [1] https://lore.kernel.org/lkml/20240628093711.11716-1-yongxuan.wang@sifive.com/#t
>> And for all qemu (and alike) versions which supported RISC-V?
>
> Concerning qemu, you're right, in case when (!svade && !svadu) they use
> by default Svadu (hw updating) for backward compatibility.
>
>>
>>> [2] For boards that do support svade, we could enforce DT
>>> declaration by adding it to `required_extension` as they are
>>> explicitly supporting it. However, doing so would cause boards
>>> without svade/svadu support (as described above) to hit a panic
>>> during boot.
...to require the user to specify either Svade or Svadu in the DTS. If
neither is mentioned in the DTS, the user should be prompted to choose
one of the two options, since, from a hardware perspective, the hardware
must support one of them.
Also, I think we could detect in runtime if Svade is supported but it is
IMO overcomplication of the things instead of force use to put implicity
Svade or Svadu in DTS.
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-10 9:34 ` [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling Baptiste Le Duc
2026-09-21 15:26 ` Jan Beulich
@ 2026-09-22 15:29 ` Oleksii Kurochko
2026-09-23 10:06 ` Baptiste Le Duc
1 sibling, 1 reply; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 15:29 UTC (permalink / raw)
To: Baptiste Le Duc, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: xen-devel
On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> p2m_set_permission() only presets the PTE A/D bits when the Svade extension
> is present in the device tree. This causes an unhandled page fault when
> neither Svade nor Svadu is present (the platform's actual behaviour is then
> unknown), and when both are present in the device tree.
When both are present, RISCV_ISA_EXT_svade is set, so the current code
does preset the A/D bits and no fault happens. The only broken case is
when neither extension is present, so shouldn't "both present" be dropped?
>
> Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
> riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
> the four possible Svade/Svadu combinations (inspired by [1]), it decides
> whether software has to preset the A/D bits and, if so, sets
> RISCV_ISA_EXT_svade to record that decision:
> - neither present: assume Svade, since assuming Svade is harmless on real
> Svadu hardware, while assuming Svadu on real Svade hardware risks an
> unhandled page fault
> - only Svade present: assume Svade
> - only Svadu present: leave A/D management to hardware
> - both present: Svade wins until Xen supports the SBI FWFT call needed to
> enable hardware updating of A/D bits, so assume Svade and warn that
> dropping 'svade' from the DT is 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@vates.tech>
> ---
> 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.
sbi_probe_extension() is already non-static in staging, only the prototype
is missing. What base is this patch against?
> - stop presetting A/D bits unconditionally in p2m_set_permission(), do it
> only when Svade is present.
What is the gain from not presetting them? Presetting A/D is correct
with both Svade and Svadu: with Svadu it just saves the hardware an
atomic PTE update on first access. Xen doesn't consume G-stage A/D bits
(no dirty tracking, no demand paging), and pt.c already presets A/D
unconditionally for Xen's own mappings. Always setting PTE_ACCESSED |
PTE_DIRTY in p2m_set_permission() fixes the bug in one line, with no
need for the resolver, the new ISA bit, FWFT probing or the ASSERT.
Handling A/D differently only makes sense once Xen actually wants that
information, and at that point FWFT support and a fault handler are
needed anyway.
> ---
> xen/arch/riscv/cpufeature.c | 59 +++++++++++++++++++++++++++++++++
> xen/arch/riscv/include/asm/cpufeature.h | 1 +
> xen/arch/riscv/include/asm/sbi.h | 8 +++++
> xen/arch/riscv/p2m.c | 47 ++++++++++----------------
> 4 files changed, 86 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index 92235fdfd5..19454544a7 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -18,6 +18,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"
> @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
> return false;
> }
>
> +/*
> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
> + * that a page fault will be raised. In contrast, the Svadu extension supports
> + * hardware updating of the PTE A/D bits.
> + *
> + * There are 4 possible combinations of these extensions in the device tree.
> + * The default hardware behavior for each is:
> + *
> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> + * whether the platform uses Svade or Svadu. Xen should be prepared to
> + * handle either hardware updating of the PTE A/D bits or page faults when
> + * they need updating. In that case, Xen assumes Svade because it's
> + * harmless if the platform is actually Svadu, while assuming Svadu on real
> + * Svade hardware risks an unhandled page fault.
> + *
> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
> + *
> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
> + *
> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
> + * at boot time by setting A/D bits. To use Svadu, the supervisor must
> + * explicitly enable it using the SBI FWFT extension.
> + *
> + * The Svade extension is mandatory and the Svadu extension is optional in the
> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
> + * get the benefit of Svadu until the SBI FWFT extension is available.
> + *
> + * In other words, hardware manages the A/D bits on its own only in case 3, in
> + * all the other cases software has to preset them. Instead of open coding this
> + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
> + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
> + */
> +static void __init riscv_resolve_ad_scheme(void)
> +{
> + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
svadu is always false here: the patch doesn't add
RISCV_ISA_EXT_ENTRY(svadu, NONE) to riscv_isa_ext[], so match_isa_ext()
never sets this bit. Cases 3 and 4 are dead code. Am I missing something?
> +
> + /* Case 3: leave the A/D bits management to hardware. */
> + if ( svadu && !svade )
> + return;
> +
> + /* Case 4 */
> + if ( svadu && svade ){
> + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
> + 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");
> + }
> + }
sbi_probe_extension() returns a negative errno on SBI failure, so
!sbi_probe_extension() is false in that case and an error is treated as
"FWFT present". The existing callers check "> 0", so this should be
"<= 0".
> +
> + /* Cases 1, 2: Xen assume Svade to be enabled */
s/assume/assumes.
> + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
In case 4 RISCV_ISA_EXT_svadu stays set, so both bits are set and the
ASSERT() in p2m_set_permission() fires (once svadu is actually parsed).
This contradicts the "mutually exclusive" statement there.
> +}
> +
> bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
> enum riscv_isa_ext_id id)
> {
> @@ -513,6 +570,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 0c48d57a03..74200ce7c9 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_sstc,
> RISCV_ISA_EXT_svade,
> RISCV_ISA_EXT_svpbmt,
> + RISCV_ISA_EXT_svadu,
Please keep the same order as riscv_isa_ext[], i.e. between svade and
svpbmt, and add the matching riscv_isa_ext[] entry there as well.
> RISCV_ISA_EXT_MAX
> };
>
> diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
> index 1952868e96..4f13e8c7a0 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.
> + * @extid: The extension ID to be probed.
> + *
> + * @return: 1 or an extension specific nonzero value if yes, 0 otherwise.
> + */
This is incorrect: on failure the function returns a negative errno, not
0. Also, the rest of the file uses /* */ and not kernel-doc /**.
> +int sbi_probe_extension(long extid);
A blank line is missing before the next comment block.
> /*
> * Initialize SBI library
> *
> diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> index 1cea86512c..22ad4a2aee 100644
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -586,42 +586,31 @@ static inline void p2m_clean_pte(pte_t *p, bool clean_cache)
>
> static void p2m_set_permission(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);
This runs for every p2m PTE. The second test_bit() exists only for the
ASSERT().
> +
> 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.
> + * riscv_fill_hwcap() sets either RISCV_ISA_EXT_svade or
> + * RISCV_ISA_EXT_svadu (mutually exclusive) depending on the Svade/Svadu
> + * device tree combination (see riscv_resolve_ad_scheme()):
This isn't true, see the comment on riscv_resolve_ad_scheme(): in case 4
both bits end up set.
> + * - RISCV_ISA_EXT_svade means that software is responsible for the A/D
> + * bits.
> + * - RISCV_ISA_EXT_svadu means the hardware is responsible for the A/D
> + * bits.
> *
> - * To support the more general case and the optimizations mentioned
> - * above, it would be better to stop setting the A/D bits here and
> - * instead handle page faults that occur due to unset A/D bits.
> + * Currently, when RISCV_ISA_EXT_svade is set, Xen doesn't track A/D
> + * bits, so it does not make use of the information that could be
> + * obtained from handling the resulting page faults, which could
> + * otherwise be useful for several use cases such as demand paging,
> + * cache-flushing optimizations, memory access tracking, etc. To avoid
> + * such a page fault, Xen presets the A and D bits instead.
> */
> - if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> + ASSERT(svade != svadu); /* exactly one of svade/svadu must be set by riscv_fill_hwcap() */
The line is over 80 columns, and the trailing comment just repeats the
block comment above.
> + if ( svade )
> e->pte |= PTE_ACCESSED | PTE_DIRTY;
>
> switch ( t )
>
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-22 15:29 ` Oleksii Kurochko
@ 2026-09-23 10:06 ` Baptiste Le Duc
2026-09-23 10:41 ` Oleksii Kurochko
0 siblings, 1 reply; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-23 10:06 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: Baptiste Le Duc, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini, xen-devel
On 2026-09-22 17:29 +0200, Oleksii Kurochko wrote:
>
>
> On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> > p2m_set_permission() only presets the PTE A/D bits when the Svade extension
> > is present in the device tree. This causes an unhandled page fault when
> > neither Svade nor Svadu is present (the platform's actual behaviour is then
> > unknown), and when both are present in the device tree.
>
> When both are present, RISCV_ISA_EXT_svade is set, so the current code
> does preset the A/D bits and no fault happens. The only broken case is
> when neither extension is present, so shouldn't "both present" be dropped?
Yes i agree, I'll fix that in v3.
>
> >
> > Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
> > riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
> > the four possible Svade/Svadu combinations (inspired by [1]), it decides
> > whether software has to preset the A/D bits and, if so, sets
> > RISCV_ISA_EXT_svade to record that decision:
> > - neither present: assume Svade, since assuming Svade is harmless on real
> > Svadu hardware, while assuming Svadu on real Svade hardware risks an
> > unhandled page fault
> > - only Svade present: assume Svade
> > - only Svadu present: leave A/D management to hardware
> > - both present: Svade wins until Xen supports the SBI FWFT call needed to
> > enable hardware updating of A/D bits, so assume Svade and warn that
> > dropping 'svade' from the DT is 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@vates.tech>
> > ---
> > 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.
>
> sbi_probe_extension() is already non-static in staging, only the prototype
> is missing. What base is this patch against?
>
> > - stop presetting A/D bits unconditionally in p2m_set_permission(), do it
> > only when Svade is present.
>
> What is the gain from not presetting them? Presetting A/D is correct
> with both Svade and Svadu: with Svadu it just saves the hardware an
> atomic PTE update on first access. Xen doesn't consume G-stage A/D bits
> (no dirty tracking, no demand paging), and pt.c already presets A/D
> unconditionally for Xen's own mappings. Always setting PTE_ACCESSED |
> PTE_DIRTY in p2m_set_permission() fixes the bug in one line, with no
> need for the resolver, the new ISA bit, FWFT probing or the ASSERT.
> Handling A/D differently only makes sense once Xen actually wants that
> information, and at that point FWFT support and a fault handler are
> needed anyway.
I agree that presetting them is the right way, it is also the way linux
is working. If Jan agree, I will to in that way in v3.
Then, it seems there is no need to register Svade/Svadu at all in
cpufeature.c, am I right?
> > ---
> > xen/arch/riscv/cpufeature.c | 59 +++++++++++++++++++++++++++++++++
> > xen/arch/riscv/include/asm/cpufeature.h | 1 +
> > xen/arch/riscv/include/asm/sbi.h | 8 +++++
> > xen/arch/riscv/p2m.c | 47 ++++++++++----------------
> > 4 files changed, 86 insertions(+), 29 deletions(-)
> >
> > diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> > index 92235fdfd5..19454544a7 100644
> > --- a/xen/arch/riscv/cpufeature.c
> > +++ b/xen/arch/riscv/cpufeature.c
> > @@ -18,6 +18,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"
> > @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
> > return false;
> > }
> >
> > +/*
> > + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
> > + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
> > + * that a page fault will be raised. In contrast, the Svadu extension supports
> > + * hardware updating of the PTE A/D bits.
> > + *
> > + * There are 4 possible combinations of these extensions in the device tree.
> > + * The default hardware behavior for each is:
> > + *
> > + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> > + * whether the platform uses Svade or Svadu. Xen should be prepared to
> > + * handle either hardware updating of the PTE A/D bits or page faults when
> > + * they need updating. In that case, Xen assumes Svade because it's
> > + * harmless if the platform is actually Svadu, while assuming Svadu on real
> > + * Svade hardware risks an unhandled page fault.
> > + *
> > + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
> > + *
> > + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
> > + *
> > + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned off
> > + * at boot time by setting A/D bits. To use Svadu, the supervisor must
> > + * explicitly enable it using the SBI FWFT extension.
> > + *
> > + * The Svade extension is mandatory and the Svadu extension is optional in the
> > + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> > + * option 3. Platforms aware of the profile can choose option 4, and Xen won't
> > + * get the benefit of Svadu until the SBI FWFT extension is available.
> > + *
> > + * In other words, hardware manages the A/D bits on its own only in case 3, in
> > + * all the other cases software has to preset them. Instead of open coding this
> > + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
> > + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
> > + */
> > +static void __init riscv_resolve_ad_scheme(void)
> > +{
> > + bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> > + bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
>
> svadu is always false here: the patch doesn't add
> RISCV_ISA_EXT_ENTRY(svadu, NONE) to riscv_isa_ext[], so match_isa_ext()
> never sets this bit. Cases 3 and 4 are dead code. Am I missing something?
>
> > +
> > + /* Case 3: leave the A/D bits management to hardware. */
> > + if ( svadu && !svade )
> > + return;
> > +
> > + /* Case 4 */
> > + if ( svadu && svade ){
> > + if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
> > + 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");
> > + }
> > + }
>
> sbi_probe_extension() returns a negative errno on SBI failure, so
> !sbi_probe_extension() is false in that case and an error is treated as
> "FWFT present". The existing callers check "> 0", so this should be
> "<= 0".
>
> > +
> > + /* Cases 1, 2: Xen assume Svade to be enabled */
>
> s/assume/assumes.
>
> > + __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
>
> In case 4 RISCV_ISA_EXT_svadu stays set, so both bits are set and the
> ASSERT() in p2m_set_permission() fires (once svadu is actually parsed).
> This contradicts the "mutually exclusive" statement there.
>
> > +}
> > +
> > bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
> > enum riscv_isa_ext_id id)
> > {
> > @@ -513,6 +570,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 0c48d57a03..74200ce7c9 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_sstc,
> > RISCV_ISA_EXT_svade,
> > RISCV_ISA_EXT_svpbmt,
> > + RISCV_ISA_EXT_svadu,
>
> Please keep the same order as riscv_isa_ext[], i.e. between svade and
> svpbmt, and add the matching riscv_isa_ext[] entry there as well.
>
> > RISCV_ISA_EXT_MAX
> > };
> >
> > diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
> > index 1952868e96..4f13e8c7a0 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.
> > + * @extid: The extension ID to be probed.
> > + *
> > + * @return: 1 or an extension specific nonzero value if yes, 0 otherwise.
> > + */
>
> This is incorrect: on failure the function returns a negative errno, not
> 0. Also, the rest of the file uses /* */ and not kernel-doc /**.
>
> > +int sbi_probe_extension(long extid);
>
> A blank line is missing before the next comment block.
>
> > /*
> > * Initialize SBI library
> > *
> > diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> > index 1cea86512c..22ad4a2aee 100644
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -586,42 +586,31 @@ static inline void p2m_clean_pte(pte_t *p, bool clean_cache)
> >
> > static void p2m_set_permission(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);
>
> This runs for every p2m PTE. The second test_bit() exists only for the
> ASSERT().
>
> > +
> > 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.
> > + * riscv_fill_hwcap() sets either RISCV_ISA_EXT_svade or
> > + * RISCV_ISA_EXT_svadu (mutually exclusive) depending on the Svade/Svadu
> > + * device tree combination (see riscv_resolve_ad_scheme()):
>
> This isn't true, see the comment on riscv_resolve_ad_scheme(): in case 4
> both bits end up set.
>
> > + * - RISCV_ISA_EXT_svade means that software is responsible for the A/D
> > + * bits.
> > + * - RISCV_ISA_EXT_svadu means the hardware is responsible for the A/D
> > + * bits.
> > *
> > - * To support the more general case and the optimizations mentioned
> > - * above, it would be better to stop setting the A/D bits here and
> > - * instead handle page faults that occur due to unset A/D bits.
> > + * Currently, when RISCV_ISA_EXT_svade is set, Xen doesn't track A/D
> > + * bits, so it does not make use of the information that could be
> > + * obtained from handling the resulting page faults, which could
> > + * otherwise be useful for several use cases such as demand paging,
> > + * cache-flushing optimizations, memory access tracking, etc. To avoid
> > + * such a page fault, Xen presets the A and D bits instead.
> > */
> > - if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> > + ASSERT(svade != svadu); /* exactly one of svade/svadu must be set by riscv_fill_hwcap() */
>
> The line is over 80 columns, and the trailing comment just repeats the
> block comment above.
>
> > + if ( svade )
> > e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >
> > switch ( t )
> >
>
> ~ Oleksii
>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling
2026-09-23 10:06 ` Baptiste Le Duc
@ 2026-09-23 10:41 ` Oleksii Kurochko
0 siblings, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-23 10:41 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: Alistair Francis, Connor Davis, Andrew Cooper, Anthony PERARD,
Michal Orzel, Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, xen-devel
On 9/23/26 12:06 PM, Baptiste Le Duc wrote:
> On 2026-09-22 17:29 +0200, Oleksii Kurochko wrote:
>>
>>
>> On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
>>> p2m_set_permission() only presets the PTE A/D bits when the Svade extension
>>> is present in the device tree. This causes an unhandled page fault when
>>> neither Svade nor Svadu is present (the platform's actual behaviour is then
>>> unknown), and when both are present in the device tree.
>>
>> When both are present, RISCV_ISA_EXT_svade is set, so the current code
>> does preset the A/D bits and no fault happens. The only broken case is
>> when neither extension is present, so shouldn't "both present" be dropped?
> Yes i agree, I'll fix that in v3.
>>
>>>
>>> Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
>>> riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
>>> the four possible Svade/Svadu combinations (inspired by [1]), it decides
>>> whether software has to preset the A/D bits and, if so, sets
>>> RISCV_ISA_EXT_svade to record that decision:
>>> - neither present: assume Svade, since assuming Svade is harmless on real
>>> Svadu hardware, while assuming Svadu on real Svade hardware risks an
>>> unhandled page fault
>>> - only Svade present: assume Svade
>>> - only Svadu present: leave A/D management to hardware
>>> - both present: Svade wins until Xen supports the SBI FWFT call needed to
>>> enable hardware updating of A/D bits, so assume Svade and warn that
>>> dropping 'svade' from the DT is 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@vates.tech>
>>> ---
>>> 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.
>>
>> sbi_probe_extension() is already non-static in staging, only the prototype
>> is missing. What base is this patch against?
>>
>>> - stop presetting A/D bits unconditionally in p2m_set_permission(), do it
>>> only when Svade is present.
>>
>> What is the gain from not presetting them? Presetting A/D is correct
>> with both Svade and Svadu: with Svadu it just saves the hardware an
>> atomic PTE update on first access. Xen doesn't consume G-stage A/D bits
>> (no dirty tracking, no demand paging), and pt.c already presets A/D
>> unconditionally for Xen's own mappings. Always setting PTE_ACCESSED |
>> PTE_DIRTY in p2m_set_permission() fixes the bug in one line, with no
>> need for the resolver, the new ISA bit, FWFT probing or the ASSERT.
>> Handling A/D differently only makes sense once Xen actually wants that
>> information, and at that point FWFT support and a fault handler are
>> needed anyway.
> I agree that presetting them is the right way, it is also the way linux
> is working. If Jan agree, I will to in that way in v3.
>
> Then, it seems there is no need to register Svade/Svadu at all in
> cpufeature.c, am I right?
If after the re-work we won't need any case of code where it is needed
to call riscv_isa_extension_available(NULL, RISCV_ISA_EXT_{svadu,svade})
then it seems like we won't need it in cpufeature.c, at least, in terms
of the current patch. (but also consider my another reply in a separate
thread if final solution will end that we will force a user to
explicitly tell that a user has to write Svade or Svadu in DTS then
likely we will need to have correspondent arrays in cpufeature.c and
emum updated).
Probably, we will need to have them mentioned in correspondent arrays in
cpufeature.c if we want to implicitly tell for example that guest is
supporting Svadu or Svade. But I think it isn't the case for the current
patch.
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade
2026-09-10 9:30 [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Baptiste Le Duc
2026-09-10 9:34 ` [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling Baptiste Le Duc
@ 2026-09-10 9:34 ` Baptiste Le Duc
2026-09-16 8:54 ` Zhang Zheng
` (2 more replies)
2026-09-10 9:34 ` [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension Baptiste Le Duc
` (4 subsequent siblings)
6 siblings, 3 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-10 9:34 UTC (permalink / raw)
To: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: Baptiste Le Duc, xen-devel
The previous patch set A/D bits in case of the Svade extension for G-stage
mappings. Xen's own S-stage mappings need the same fix as both
setup_initial_mapping() (the boot page tables) and arch_pmap_map() (the
fixmap) build leaf PTEs directly instead of going through
pt_update_entry(), which is what adds A/D bits. So with Svade, both would
fault on first access.
Add PTE_ACCESSED to all PAGE_HYPERVISOR_* and also PTE_DIRTY to
PAGE_HYPERVISOR_RW as it needs to be set during a write to avoid a fault.
This fixes arch_pmap_map() for free, since it already builds its PTE from
PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
its default, text and rodata permissions, and for the temporary root entry
built by check_pgtbl_mode_support(), instead of the equivalent raw bit
lists. The latter drops PTE_WRITABLE, going from RWX to RX, but this is
harmless, as that entry only has to make the current instruction stream
fetchable between the two CSR_SATP writes used to probe SATP mode support,
and nothing writes through it.
Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
open-coded site above leaves it with no user outside page.h itself.
A PTE is a table entry iff PTE_VALID is set and R/W/X are all clear, so
update pte_is_table() and pte_is_mapping() accordingly.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
Changes since v1:
- change commit title
- mention in patch message that arch_pmap_map() is fixed too, via the
PAGE_HYPERVISOR_RW change, not just setup_initial_mapping().
- convert check_pgtbl_mode_support()'s temporary root entry to
PAGE_HYPERVISOR_RX, as it's harmless.
- drop PTE_LEAF_DEFAULT entirely instead of keeping it, now that no site
open-codes it anymore.
- drop the pte_is_table() comment line that referenced PAGE_HYPERVISOR_RW,
now stale.
---
xen/arch/riscv/include/asm/page.h | 15 +++++++--------
xen/arch/riscv/mm.c | 9 ++++-----
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index b465a90325..1977634efc 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -46,12 +46,11 @@
#define PTE_PBMT_NOCACHE BIT(61, UL)
#define PTE_PBMT_IO BIT(62, UL)
-#define PTE_LEAF_DEFAULT (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
#define PTE_TABLE (PTE_VALID)
-#define PAGE_HYPERVISOR_RO (PTE_VALID | PTE_READABLE)
-#define PAGE_HYPERVISOR_RW (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
-#define PAGE_HYPERVISOR_RX (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
+#define PAGE_HYPERVISOR_RO (PTE_VALID | PTE_READABLE | PTE_ACCESSED)
+#define PAGE_HYPERVISOR_RW (PTE_VALID | PTE_READABLE | PTE_WRITABLE | PTE_ACCESSED | PTE_DIRTY)
+#define PAGE_HYPERVISOR_RX (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE | PTE_ACCESSED)
#define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
/*
@@ -174,10 +173,9 @@ static inline bool pte_is_table(pte_t p)
* According to the spec if V=1 and W=1 then R also needs to be 1 as
* R = 0 is reserved for future use ( look at the Table 4.5 ) so check
* in ASSERT that if (V==1 && W==1) then R isn't 0.
- *
- * PAGE_HYPERVISOR_RW contains PTE_VALID too.
*/
- ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
+ ASSERT((p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) !=
+ (PTE_VALID | PTE_WRITABLE));
return ((p.pte & (PTE_VALID | PTE_ACCESS_MASK)) == PTE_VALID);
}
@@ -185,7 +183,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..53bebbcabf 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);
@@ -198,7 +197,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
index = pt_index(page_table_level, aligned_load_start);
stage1_pgtbl_root[index] = paddr_to_pte(aligned_load_start,
- PTE_LEAF_DEFAULT | PTE_EXECUTABLE);
+ PAGE_HYPERVISOR_RX);
sfence_vma();
csr_write(CSR_SATP,
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade
2026-09-10 9:34 ` [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade Baptiste Le Duc
@ 2026-09-16 8:54 ` Zhang Zheng
2026-09-21 15:35 ` Jan Beulich
2026-09-22 15:05 ` Oleksii Kurochko
2 siblings, 0 replies; 35+ messages in thread
From: Zhang Zheng @ 2026-09-16 8:54 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini, xen-devel
# Add your code comments below. There is no need to trim or delete
# any existing content -- just insert your comments under the relevant
# lines of code. Lines starting with "> " are quoted diff context and
# lines starting with "| " are comments from other reviewers.
# The final email will be reformatted automatically to include only
# the sections that have your comments.
#
> The previous patch set A/D bits in case of the Svade extension for G-stage
> mappings. Xen's own S-stage mappings need the same fix as both
> setup_initial_mapping() (the boot page tables) and arch_pmap_map() (the
> fixmap) build leaf PTEs directly instead of going through
> pt_update_entry(), which is what adds A/D bits. So with Svade, both would
> fault on first access.
>
> Add PTE_ACCESSED to all PAGE_HYPERVISOR_* and also PTE_DIRTY to
> PAGE_HYPERVISOR_RW as it needs to be set during a write to avoid a fault.
> This fixes arch_pmap_map() for free, since it already builds its PTE from
> PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
> its default, text and rodata permissions, and for the temporary root entry
> built by check_pgtbl_mode_support(), instead of the equivalent raw bit
> lists. The latter drops PTE_WRITABLE, going from RWX to RX, but this is
> harmless, as that entry only has to make the current instruction stream
> fetchable between the two CSR_SATP writes used to probe SATP mode support,
> and nothing writes through it.
>
> Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
> open-coded site above leaves it with no user outside page.h itself.
>
> A PTE is a table entry iff PTE_VALID is set and R/W/X are all clear, so
> update pte_is_table() and pte_is_mapping() accordingly.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
Tested-by: Zheng Zhang <zhangzheng@iscas.ac.cn>
--
Zhang Zheng <zhangzheng@iscas.ac.cn>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade
2026-09-10 9:34 ` [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade Baptiste Le Duc
2026-09-16 8:54 ` Zhang Zheng
@ 2026-09-21 15:35 ` Jan Beulich
2026-09-22 15:05 ` Oleksii Kurochko
2 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2026-09-21 15:35 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Oleksii Kurochko,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini
On 10.09.2026 11:34, Baptiste Le Duc wrote:
> The previous patch set A/D bits in case of the Svade extension for G-stage
As I think I have said before - no "the previous patch" or anything alike
please in descriptions.
> --- 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;
The variable was already not named entirely adequately, as PTE_VALID is
only somewhat a "permission". Now it clearly holds more than just
permission bits, so want to be given a better name.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade
2026-09-10 9:34 ` [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade Baptiste Le Duc
2026-09-16 8:54 ` Zhang Zheng
2026-09-21 15:35 ` Jan Beulich
@ 2026-09-22 15:05 ` Oleksii Kurochko
2 siblings, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 15:05 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Julien Grall, Connor Davis, Alistair Francis,
Anthony PERARD, Andrew Cooper, Stefano Stabellini,
Roger Pau Monné, Michal Orzel, Jan Beulich
On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> The previous patch set A/D bits in case of the Svade extension for G-stage
> mappings. Xen's own S-stage mappings need the same fix as both
> setup_initial_mapping() (the boot page tables) and arch_pmap_map() (the
> fixmap) build leaf PTEs directly instead of going through
> pt_update_entry(), which is what adds A/D bits. So with Svade, both would
> fault on first access.
Please don't refer to "the previous patch": once applied, the commit
message should stand on its own. Just state the fact instead, e.g.:
With Svade, hardware doesn't update the A/D bits; instead it raises a
page fault when A is clear (or D is clear on a write).
pt_update_entry() already sets them, but ...
>
> Add PTE_ACCESSED to all PAGE_HYPERVISOR_* and also PTE_DIRTY to
> PAGE_HYPERVISOR_RW as it needs to be set during a write to avoid a fault.
> This fixes arch_pmap_map() for free, since it already builds its PTE from
> PAGE_HYPERVISOR_RW. Switch setup_initial_mapping() to use these macros for
> its default, text and rodata permissions, and for the temporary root entry
> built by check_pgtbl_mode_support(), instead of the equivalent raw bit
> lists. The latter drops PTE_WRITABLE, going from RWX to RX, but this is
> harmless, as that entry only has to make the current instruction stream
> fetchable between the two CSR_SATP writes used to probe SATP mode support,
> and nothing writes through it.
>
> Drop the now-redundant PTE_LEAF_DEFAULT, since converting the last
> open-coded site above leaves it with no user outside page.h itself.
>
> A PTE is a table entry iff PTE_VALID is set and R/W/X are all clear, so
> update pte_is_table() and pte_is_mapping() accordingly.
This doesn't describe what the patch actually changes: the return
expressions of pte_is_table()/pte_is_mapping() are untouched, only the
ASSERT()s change. The real reason is that the ASSERT()s masked the PTE
with PAGE_HYPERVISOR_RW, which now contains A|D, so for the reserved
encoding V|W|A we would compare V|W|A != V|W and the ASSERT() would
silently stop firing. Something like:
The ASSERT()s in pte_is_table() and pte_is_mapping() mask the PTE
with PAGE_HYPERVISOR_RW to detect the reserved W=1,R=0 encoding. Now
that PAGE_HYPERVISOR_RW includes A/D, the check would no longer
trigger for a PTE with A or D set, so use an explicit V|R|W mask.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
> Changes since v1:
> - change commit title
> - mention in patch message that arch_pmap_map() is fixed too, via the
> PAGE_HYPERVISOR_RW change, not just setup_initial_mapping().
> - convert check_pgtbl_mode_support()'s temporary root entry to
> PAGE_HYPERVISOR_RX, as it's harmless.
> - drop PTE_LEAF_DEFAULT entirely instead of keeping it, now that no site
> open-codes it anymore.
> - drop the pte_is_table() comment line that referenced PAGE_HYPERVISOR_RW,
> now stale.
> ---
> xen/arch/riscv/include/asm/page.h | 15 +++++++--------
> xen/arch/riscv/mm.c | 9 ++++-----
> 2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> index b465a90325..1977634efc 100644
> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -46,12 +46,11 @@
> #define PTE_PBMT_NOCACHE BIT(61, UL)
> #define PTE_PBMT_IO BIT(62, UL)
>
> -#define PTE_LEAF_DEFAULT (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
> #define PTE_TABLE (PTE_VALID)
>
> -#define PAGE_HYPERVISOR_RO (PTE_VALID | PTE_READABLE)
> -#define PAGE_HYPERVISOR_RW (PTE_VALID | PTE_READABLE | PTE_WRITABLE)
> -#define PAGE_HYPERVISOR_RX (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
> +#define PAGE_HYPERVISOR_RO (PTE_VALID | PTE_READABLE | PTE_ACCESSED)
> +#define PAGE_HYPERVISOR_RW (PTE_VALID | PTE_READABLE | PTE_WRITABLE | PTE_ACCESSED | PTE_DIRTY)
> +#define PAGE_HYPERVISOR_RX (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE | PTE_ACCESSED)
These two lines exceed 80 columns, please wrap them, e.g.:
#define PAGE_HYPERVISOR_RW (PTE_VALID | PTE_READABLE |
PTE_WRITABLE | \
PTE_ACCESSED | PTE_DIRTY)
Also, pt_update_entry() sets D on every leaf, while here RO/RX get only
A. Both are valid per the spec, but it means boot-time and runtime
mappings of the same kind of page differ in D. Either set D on RO/RX
too for consistency, or say in the commit message why it isn't done.
>
> #define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
> /*
> @@ -174,10 +173,9 @@ static inline bool pte_is_table(pte_t p)
> * According to the spec if V=1 and W=1 then R also needs to be 1 as
> * R = 0 is reserved for future use ( look at the Table 4.5 ) so check
> * in ASSERT that if (V==1 && W==1) then R isn't 0.
> - *
> - * PAGE_HYPERVISOR_RW contains PTE_VALID too.
> */
> - ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID | PTE_WRITABLE)));
> + ASSERT((p.pte & (PTE_VALID | PTE_READABLE | PTE_WRITABLE)) !=
> + (PTE_VALID | PTE_WRITABLE));
>
> return ((p.pte & (PTE_VALID | PTE_ACCESS_MASK)) == PTE_VALID);
> }
> @@ -185,7 +183,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));
Nit: the indentation differs from the one in pte_is_table() (one extra
space here). As the same expression is now open-coded twice, maybe it
is worth introducing a small helper (e.g. pte_is_reserved_wr()) and
using it in both ASSERT()s?
Thanks.
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension
2026-09-10 9:30 [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Baptiste Le Duc
2026-09-10 9:34 ` [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling Baptiste Le Duc
2026-09-10 9:34 ` [PATCH v2 2/6] xen/riscv: set A/D bits in Xen's page-table mappings under Svade Baptiste Le Duc
@ 2026-09-10 9:34 ` Baptiste Le Duc
2026-09-21 15:57 ` Jan Beulich
2026-09-22 14:48 ` Oleksii Kurochko
2026-09-10 9:34 ` [PATCH v2 4/6] xen/riscv: make Zihintpause " Baptiste Le Duc
` (3 subsequent siblings)
6 siblings, 2 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-10 9:34 UTC (permalink / raw)
To: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: Baptiste Le Duc, xen-devel
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)
- 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 and use caches for RAM
accesses, based on the target physical address.
Furthermore, on platforms that either feature fully hardware-coherent DMA
or don't 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.
Drop Svpbmt from required_extensions. Introduce svpbmt_enabled, a
__ro_after_init flag computed once in init_csr_masks() from ISA
availability and the henvcfg.PBMTE bit. Xen cannot read menvcfg.PBMTE
directly, since menvcfg is M-mode-only and unreadable from HS-mode, but the
spec guarantees henvcfg.PBMTE reads as zero whenever menvcfg.PBMTE is zero,
so checking henvcfg.PBMTE alone is sufficient. Use svpbmt_enabled in
pte_pbmt_nocache()/pte_pbmt_io(), two new inline helpers that mask the PBMT
encoding down to 0 when Svpbmt is unavailable.
Also switch vcpu_csr_init() branch to determine if Svpbmt was enabled to
svpbmt_enabled as it does the same logic.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
Changes since v1:
- Replace the pte_pbmt() macro, which re-checked
riscv_isa_extension_available() on every call, with a svpbmt_enabled
flag cached once in init_csr_masks().
- Add pte_pbmt_nocache()/pte_pbmt_io() inline helpers instead, used by
PAGE_HYPERVISOR_NOCACHE/WC and p2m_pte_from_mfn().
- Switch vcpu_csr_init() to the same cached svpbmt_enabled flag instead
of re-deriving Svpbmt availability itself.
---
xen/arch/riscv/cpufeature.c | 1 -
xen/arch/riscv/domain.c | 10 ++++++++--
xen/arch/riscv/include/asm/page.h | 22 +++++++++++++++++++---
xen/arch/riscv/p2m.c | 2 +-
4 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 19454544a7..986a6dec78 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -158,7 +158,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/domain.c b/xen/arch/riscv/domain.c
index 2819ff4e7c..f6f20824e3 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -47,6 +47,8 @@ static struct csr_masks __ro_after_init csr_masks;
#define HENVCFG_VALID_MASK 0xe0000003000000ffUL
#define HSTATEEN0_VALID_MASK 0xde00000000000007UL
+bool __ro_after_init svpbmt_enabled;
+
void __init init_csr_masks(void)
{
/*
@@ -79,6 +81,10 @@ void __init init_csr_masks(void)
INIT_RO_ONE_MASK(HSTATEEN0, hstateen0);
}
+ svpbmt_enabled = (riscv_isa_extension_available(NULL,
+ RISCV_ISA_EXT_svpbmt)) && (ENVCFG_PBMTE &
+ csr_masks.henvcfg);
+
#undef INIT_CSR_MASK
#undef INIT_RO_ONE_MASK
}
@@ -97,8 +103,8 @@ static void vcpu_csr_init(struct vcpu *v)
*/
v->arch.hcounteren = HCOUNTEREN_TM;
- if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) )
- v->arch.henvcfg = ENVCFG_PBMTE & csr_masks.henvcfg;
+ if ( svpbmt_enabled )
+ v->arch.henvcfg = ENVCFG_PBMTE;
if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
{
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index 1977634efc..a7d087ff52 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)
@@ -42,7 +43,21 @@
* 01 - NC Non-cacheable, idempotent, weakly-ordered Main Memory
* 10 - IO Non-cacheable, non-idempotent, strongly-ordered I/O memory
* 11 - Rsvd Reserved for future standard use
+ *
+ * These bits are only meaningful when Svpbmt is enabled. Otherwise they must
+ * stay 0 (PMA).
*/
+extern bool svpbmt_enabled;
+static inline unsigned long pte_pbmt_nocache(void)
+{
+ return svpbmt_enabled ? BIT(61, UL) : 0;
+}
+
+static inline unsigned long pte_pbmt_io(void)
+{
+ return svpbmt_enabled ? BIT(62, UL) : 0;
+}
+
#define PTE_PBMT_NOCACHE BIT(61, UL)
#define PTE_PBMT_IO BIT(62, UL)
@@ -53,6 +68,7 @@
#define PAGE_HYPERVISOR_RX (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE | PTE_ACCESSED)
#define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
+
/*
* PAGE_HYPERVISOR_NOCACHE is used for ioremap().
*
@@ -60,8 +76,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_io())
+#define PAGE_HYPERVISOR_WC (PAGE_HYPERVISOR_RW | pte_pbmt_nocache())
/*
* The PTE format does not contain the following bits within itself;
@@ -82,7 +98,7 @@ enum pbmt_type {
#define PTE_ACCESS_MASK (PTE_READABLE | PTE_WRITABLE | PTE_EXECUTABLE)
-#define PTE_PBMT_MASK (PTE_PBMT_NOCACHE | PTE_PBMT_IO)
+#define PTE_PBMT_MASK (BIT(61, UL) | BIT(62, UL))
/* Calculate the offsets into the pagetables for a given VA */
#define pt_linear_offset(lvl, va) ((va) >> XEN_PT_LEVEL_SHIFT(lvl))
diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index 22ad4a2aee..15cbc92b76 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -658,7 +658,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_io();
break;
default:
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension
2026-09-10 9:34 ` [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension Baptiste Le Duc
@ 2026-09-21 15:57 ` Jan Beulich
2026-09-22 14:38 ` Oleksii Kurochko
2026-09-28 13:21 ` Baptiste Le Duc
2026-09-22 14:48 ` Oleksii Kurochko
1 sibling, 2 replies; 35+ messages in thread
From: Jan Beulich @ 2026-09-21 15:57 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Oleksii Kurochko,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini
On 10.09.2026 11:34, Baptiste Le Duc wrote:
> 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)
> - 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 and use caches for RAM
> accesses, based on the target physical address.
Provided firmware got absolutely everything right.
> Furthermore, on platforms that either feature fully hardware-coherent DMA
> or don't 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.
Yet a fully coherent platform should also be possible to somehow identify?
> Drop Svpbmt from required_extensions. Introduce svpbmt_enabled, a
> __ro_after_init flag computed once in init_csr_masks() from ISA
> availability and the henvcfg.PBMTE bit. Xen cannot read menvcfg.PBMTE
> directly, since menvcfg is M-mode-only and unreadable from HS-mode, but the
> spec guarantees henvcfg.PBMTE reads as zero whenever menvcfg.PBMTE is zero,
> so checking henvcfg.PBMTE alone is sufficient.
I don't understand this logic. If menvcfg.PBMTE is non-zero, we know
nothing about (or from) henvcfg.PBMTE's setting.
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -47,6 +47,8 @@ static struct csr_masks __ro_after_init csr_masks;
> #define HENVCFG_VALID_MASK 0xe0000003000000ffUL
> #define HSTATEEN0_VALID_MASK 0xde00000000000007UL
>
> +bool __ro_after_init svpbmt_enabled;
> +
> void __init init_csr_masks(void)
> {
> /*
> @@ -79,6 +81,10 @@ void __init init_csr_masks(void)
> INIT_RO_ONE_MASK(HSTATEEN0, hstateen0);
> }
>
> + svpbmt_enabled = (riscv_isa_extension_available(NULL,
> + RISCV_ISA_EXT_svpbmt)) && (ENVCFG_PBMTE &
> + csr_masks.henvcfg);
Line wrapping wants doing entirely differently here. One of the style-
conforming options is
svpbmt_enabled =
riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) &&
(csr_masks.henvcfg & ENVCFG_PBMTE);
> --- 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)
> @@ -42,7 +43,21 @@
> * 01 - NC Non-cacheable, idempotent, weakly-ordered Main Memory
> * 10 - IO Non-cacheable, non-idempotent, strongly-ordered I/O memory
> * 11 - Rsvd Reserved for future standard use
> + *
> + * These bits are only meaningful when Svpbmt is enabled. Otherwise they must
> + * stay 0 (PMA).
> */
> +extern bool svpbmt_enabled;
> +static inline unsigned long pte_pbmt_nocache(void)
> +{
> + return svpbmt_enabled ? BIT(61, UL) : 0;
> +}
> +
> +static inline unsigned long pte_pbmt_io(void)
> +{
> + return svpbmt_enabled ? BIT(62, UL) : 0;
> +}
Why open-code ...
> #define PTE_PBMT_NOCACHE BIT(61, UL)
> #define PTE_PBMT_IO BIT(62, UL)
... what is still available here?
> @@ -53,6 +68,7 @@
> #define PAGE_HYPERVISOR_RX (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE | PTE_ACCESSED)
>
> #define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
> +
> /*
> * PAGE_HYPERVISOR_NOCACHE is used for ioremap().
> *
Stray change?
> @@ -82,7 +98,7 @@ enum pbmt_type {
>
> #define PTE_ACCESS_MASK (PTE_READABLE | PTE_WRITABLE | PTE_EXECUTABLE)
>
> -#define PTE_PBMT_MASK (PTE_PBMT_NOCACHE | PTE_PBMT_IO)
> +#define PTE_PBMT_MASK (BIT(61, UL) | BIT(62, UL))
I don't understand the need for this change.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension
2026-09-21 15:57 ` Jan Beulich
@ 2026-09-22 14:38 ` Oleksii Kurochko
2026-09-28 13:21 ` Baptiste Le Duc
1 sibling, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 14:38 UTC (permalink / raw)
To: Jan Beulich, Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On 9/21/26 5:57 PM, Jan Beulich wrote:
> On 10.09.2026 11:34, Baptiste Le Duc wrote:
>> 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)
>> - 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 and use caches for RAM
>> accesses, based on the target physical address.
> Provided firmware got absolutely everything right.
Yes. S-mode has no standard way to discover PMAs, so it has to trust the
platform here. Note though that PMAs are in most implementations fixed
in hardware rather than programmed by M-mode firmware, so this is mostly
a matter of the platform's memory map being correct (and of the DT/ACPI
describing it correctly), which we rely on anyway.
>
>> Furthermore, on platforms that either feature fully hardware-coherent DMA
>> or don't 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.
> Yet a fully coherent platform should also be possible to somehow identify?
To some degree, yes, via firmware tables: on RISC-V DT devices are
treated as DMA-coherent unless marked with the "dma-noncoherent"
property, and with ACPI coherency is expressed via _CCA.
What matters for Svpbmt specifically is whether a non-coherent device
could be used at all: without Svpbmt no NC mapping can be established
through page tables, and without Zicbom there is no standard way to do
cache maintenance. If neither is available and the DT describes a
"dma-noncoherent" device, Xen will want to warn and refuse to assign
such a device to a domain or something like that.
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension
2026-09-21 15:57 ` Jan Beulich
2026-09-22 14:38 ` Oleksii Kurochko
@ 2026-09-28 13:21 ` Baptiste Le Duc
1 sibling, 0 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-28 13:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Baptiste Le Duc, xen-devel, Alistair Francis, Connor Davis,
Oleksii Kurochko, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini
On 2026-09-21 17:57:05+02:00, Jan Beulich wrote:
> On 10.09.2026 11:34, Baptiste Le Duc wrote:
>
> > 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)
> > - 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 and use caches for RAM
> > accesses, based on the target physical address.
>
> Provided firmware got absolutely everything right.
Agreed, but that dependency isn't new. Even with Svpbmt, all RAM mappings
(and most of Xen's own) use PBMT=PMA, so we already rely on the PMAs being
correct there; Svpbmt only lets us override them for ioremap() and
p2m_mmio_direct_io. I'll say this explicitly in v3 rather than claiming
it's "safe" unconditionally.
> > Furthermore, on platforms that either feature fully hardware-coherent DMA
> > or don't 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.
>
> Yet a fully coherent platform should also be possible to somehow identify?
Yes, through the device tree: a device (or one of its parents) can be
marked "dma-noncoherent", with DMA being coherent by default, which is also
how Linux handles it on RISC-V (ARCH_DMA_DEFAULT_COHERENT).
Xen itself doesn't drive any DMA-capable device on RISC-V at this point, so
the only affected devices are those passed through to guests. In v3 I'm
adding a check in arch_handle_passthrough_prop() which warns when a device
marked "dma-noncoherent" is assigned while Svpbmt is unavailable. I went
for a warning rather than a refusal, like Linux does (WARN_TAINT()) for a
non-coherent device it has no means to handle.
> > Drop Svpbmt from required_extensions. Introduce svpbmt_enabled, a
> > __ro_after_init flag computed once in init_csr_masks() from ISA
> > availability and the henvcfg.PBMTE bit. Xen cannot read menvcfg.PBMTE
> > directly, since menvcfg is M-mode-only and unreadable from HS-mode, but the
> > spec guarantees henvcfg.PBMTE reads as zero whenever menvcfg.PBMTE is zero,
> > so checking henvcfg.PBMTE alone is sufficient.
>
> I don't understand this logic. If menvcfg.PBMTE is non-zero, we know
> nothing about (or from) henvcfg.PBMTE's setting.
I explained this badly. What's checked isn't the current value of
henvcfg.PBMTE but whether the bit is writable: init_csr_masks() identifies
which bits of henvcfg (and of other CSRs) are writable, by setting them,
reading the register back into csr_masks and restoring the original value.
As per the menvcfg description in the privileged spec ("for
implementations with the hypervisor extension, henvcfg.PBMTE is
read-only zero if menvcfg.PBMTE is zero"), the contrapositive is that
if henvcfg.PBMTE is writable (ENVCFG_PBMTE being present in
csr_masks.henvcfg), then menvcfg.PBMTE is set, i.e. firmware has enabled
Svpbmt for S-mode and G-stage address translation. I'll reword the
description accordingly.
> > --- a/xen/arch/riscv/domain.c
> > +++ b/xen/arch/riscv/domain.c
> > @@ -47,6 +47,8 @@ static struct csr_masks __ro_after_init csr_masks;
> > #define HENVCFG_VALID_MASK 0xe0000003000000ffUL
> > #define HSTATEEN0_VALID_MASK 0xde00000000000007UL
> >
> > +bool __ro_after_init svpbmt_enabled;
> > +
> > void __init init_csr_masks(void)
> > {
> > /*
> > @@ -79,6 +81,10 @@ void __init init_csr_masks(void)
> > INIT_RO_ONE_MASK(HSTATEEN0, hstateen0);
> > }
> >
> > + svpbmt_enabled = (riscv_isa_extension_available(NULL,
> > + RISCV_ISA_EXT_svpbmt)) && (ENVCFG_PBMTE &
> > + csr_masks.henvcfg);
>
> Line wrapping wants doing entirely differently here. One of the style-
> conforming options is
>
> svpbmt_enabled =
> riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) &&
> (csr_masks.henvcfg & ENVCFG_PBMTE);
Will do, thanks.
> > --- 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)
> > @@ -42,7 +43,21 @@
> > * 01 - NC Non-cacheable, idempotent, weakly-ordered Main Memory
> > * 10 - IO Non-cacheable, non-idempotent, strongly-ordered I/O memory
> > * 11 - Rsvd Reserved for future standard use
> > + *
> > + * These bits are only meaningful when Svpbmt is enabled. Otherwise they must
> > + * stay 0 (PMA).
> > */
> > +extern bool svpbmt_enabled;
> > +static inline unsigned long pte_pbmt_nocache(void)
> > +{
> > + return svpbmt_enabled ? BIT(61, UL) : 0;
> > +}
> > +
> > +static inline unsigned long pte_pbmt_io(void)
> > +{
> > + return svpbmt_enabled ? BIT(62, UL) : 0;
> > +}
>
> Why open-code ...
>
> > #define PTE_PBMT_NOCACHE BIT(61, UL)
> > #define PTE_PBMT_IO BIT(62, UL)
>
> ... what is still available here?
No good reason, v3 uses PTE_PBMT_NOCACHE / PTE_PBMT_IO.
> > @@ -53,6 +68,7 @@
> > #define PAGE_HYPERVISOR_RX (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE | PTE_ACCESSED)
> >
> > #define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
> > +
> > /*
> > * PAGE_HYPERVISOR_NOCACHE is used for ioremap().
> > *
>
> Stray change?
Yes, will drop.
> > @@ -82,7 +98,7 @@ enum pbmt_type {
> >
> > #define PTE_ACCESS_MASK (PTE_READABLE | PTE_WRITABLE | PTE_EXECUTABLE)
> >
> > -#define PTE_PBMT_MASK (PTE_PBMT_NOCACHE | PTE_PBMT_IO)
> > +#define PTE_PBMT_MASK (BIT(61, UL) | BIT(62, UL))
>
> I don't understand the need for this change.
There's none, it's a leftover from an earlier iteration. Will drop.
Thanks,
Baptiste
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension
2026-09-10 9:34 ` [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension Baptiste Le Duc
2026-09-21 15:57 ` Jan Beulich
@ 2026-09-22 14:48 ` Oleksii Kurochko
1 sibling, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 14:48 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Jan Beulich, Stefano Stabellini,
Roger Pau Monné, Julien Grall, Anthony PERARD, Andrew Cooper,
Michal Orzel, Connor Davis
On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> 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)
> - 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 and use caches for RAM
> accesses, based on the target physical address.
>
> Furthermore, on platforms that either feature fully hardware-coherent DMA
> or don't 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.
>
> Drop Svpbmt from required_extensions. Introduce svpbmt_enabled, a
> __ro_after_init flag computed once in init_csr_masks() from ISA
> availability and the henvcfg.PBMTE bit. Xen cannot read menvcfg.PBMTE
> directly, since menvcfg is M-mode-only and unreadable from HS-mode, but the
> spec guarantees henvcfg.PBMTE reads as zero whenever menvcfg.PBMTE is zero,
> so checking henvcfg.PBMTE alone is sufficient. Use svpbmt_enabled in
> pte_pbmt_nocache()/pte_pbmt_io(), two new inline helpers that mask the PBMT
> encoding down to 0 when Svpbmt is unavailable.
>
> Also switch vcpu_csr_init() branch to determine if Svpbmt was enabled to
> svpbmt_enabled as it does the same logic.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
>
> ---
> Changes since v1:
> - Replace the pte_pbmt() macro, which re-checked
> riscv_isa_extension_available() on every call, with a svpbmt_enabled
> flag cached once in init_csr_masks().
> - Add pte_pbmt_nocache()/pte_pbmt_io() inline helpers instead, used by
> PAGE_HYPERVISOR_NOCACHE/WC and p2m_pte_from_mfn().
> - Switch vcpu_csr_init() to the same cached svpbmt_enabled flag instead
> of re-deriving Svpbmt availability itself.
> ---
> xen/arch/riscv/cpufeature.c | 1 -
> xen/arch/riscv/domain.c | 10 ++++++++--
> xen/arch/riscv/include/asm/page.h | 22 +++++++++++++++++++---
> xen/arch/riscv/p2m.c | 2 +-
> 4 files changed, 28 insertions(+), 7 deletions(-)
docs/misc/riscv/booting.txt isn't updated.
>
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index 19454544a7..986a6dec78 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -158,7 +158,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/domain.c b/xen/arch/riscv/domain.c
> index 2819ff4e7c..f6f20824e3 100644
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -47,6 +47,8 @@ static struct csr_masks __ro_after_init csr_masks;
> #define HENVCFG_VALID_MASK 0xe0000003000000ffUL
> #define HSTATEEN0_VALID_MASK 0xde00000000000007UL
>
> +bool __ro_after_init svpbmt_enabled;
svpbmt_enabled controls Xen's own stage-1 page tables, but it's defined
in domain.c (guest CSR setup) and declared in page.h. That's defensible,
because the value is computed in init_csr_masks() from
csr_masks.henvcfg. It would still read better in cpufeature.c, with the
declaration in cpufeature.h, or at least with a comment next to the
definition saying why it's in domain.c.
> +
> void __init init_csr_masks(void)
> {
> /*
> @@ -79,6 +81,10 @@ void __init init_csr_masks(void)
> INIT_RO_ONE_MASK(HSTATEEN0, hstateen0);
> }
>
> + svpbmt_enabled = (riscv_isa_extension_available(NULL,
> + RISCV_ISA_EXT_svpbmt)) && (ENVCFG_PBMTE &
> + csr_masks.henvcfg);
> +
svpbmt_enabled stays false until init_csr_masks() runs in start_xen().
Any ioremap(), ioremap_wc() or p2m MMIO mapping created before that
silently gets PMA instead of IO. Nothing does this today; setup before
that point only uses PAGE_HYPERVISOR_RW. Consider an ASSERT or a comment
in pte_pbmt_*(), or setting the flag earlier, e.g. right after
riscv_fill_hwcap() (if it is possible, considering that you are using
csr_masks.henvcfg probably it can't) .
> #undef INIT_CSR_MASK
> #undef INIT_RO_ONE_MASK
> }
> @@ -97,8 +103,8 @@ static void vcpu_csr_init(struct vcpu *v)
> */
> v->arch.hcounteren = HCOUNTEREN_TM;
>
> - if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) )
> - v->arch.henvcfg = ENVCFG_PBMTE & csr_masks.henvcfg;
> + if ( svpbmt_enabled )
> + v->arch.henvcfg = ENVCFG_PBMTE;
>
> if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> {
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> index 1977634efc..a7d087ff52 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>
Do we really need this?
The helpers only read extern bool svpbmt_enabled and don't call
riscv_isa_extension_available(). Adding the include only widens the
header dependencies.
> #include <asm/page-bits.h>
>
> #define VPN_MASK (PAGETABLE_ENTRIES - 1UL)
> @@ -42,7 +43,21 @@
> * 01 - NC Non-cacheable, idempotent, weakly-ordered Main Memory
> * 10 - IO Non-cacheable, non-idempotent, strongly-ordered I/O memory
> * 11 - Rsvd Reserved for future standard use
> + *
> + * These bits are only meaningful when Svpbmt is enabled. Otherwise they must
> + * stay 0 (PMA).
> */
> +extern bool svpbmt_enabled;
Shouldn't be here an empty line?
Thanks.
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 4/6] xen/riscv: make Zihintpause no longer a required extension
2026-09-10 9:30 [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Baptiste Le Duc
` (2 preceding siblings ...)
2026-09-10 9:34 ` [PATCH v2 3/6] xen/riscv: make Svpbmt no longer a required extension Baptiste Le Duc
@ 2026-09-10 9:34 ` Baptiste Le Duc
2026-09-22 12:27 ` Jan Beulich
2026-09-22 14:50 ` Oleksii Kurochko
2026-09-10 9:34 ` [PATCH v2 5/6] xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu() Baptiste Le Duc
` (2 subsequent siblings)
6 siblings, 2 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-10 9:34 UTC (permalink / raw)
To: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: Baptiste Le Duc, xen-devel
required_extensions[] panics at boot if Zihintpause is missing, but Xen
never actually depends on it: cpu_relax() only emits the "pause" hint when
the extension is implemented, otherwise it emits `0x0100000F`, a legally
valid FENCE instruction (`FENCE W, 0`) rather than a native NOP. FENCE is
guaranteed by the RISC-V base ISA, so it never raises an illegal
instruction fault. With an empty successor set, it enforces no
memory-ordering constraints and thus architecturally behaves 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>
---
Changes since v1:
- rewrite commit message.
---
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 986a6dec78..41bb1d2e80 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -156,7 +156,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),
};
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 4/6] xen/riscv: make Zihintpause no longer a required extension
2026-09-10 9:34 ` [PATCH v2 4/6] xen/riscv: make Zihintpause " Baptiste Le Duc
@ 2026-09-22 12:27 ` Jan Beulich
2026-09-22 14:26 ` Oleksii Kurochko
2026-09-22 14:50 ` Oleksii Kurochko
1 sibling, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2026-09-22 12:27 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Oleksii Kurochko,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini
On 10.09.2026 11:34, 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" hint when
> the extension is implemented, otherwise it emits `0x0100000F`, a legally
> valid FENCE instruction (`FENCE W, 0`) rather than a native NOP.
Just that PAUSE's encoding is 0x0100000F. I.e. what is emitted is always
the same, and hence discussing the encoding aspect here doesn't help
justify the change. NOP or not also doesn't really matter here. The
specific hint encoding looks to fall into what prior to Zihintpause would
have been covered by "Designated for future standard use", and hence ...
> FENCE is
> guaranteed by the RISC-V base ISA, so it never raises an illegal
> instruction fault. With an empty successor set, it enforces no
> memory-ordering constraints and thus architecturally behaves as a NOP.
... there indeed should be no concern for any platform playing by the
rules.
> 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>
> ---
> Changes since v1:
> - rewrite commit message.
I fear another round of re-writing is going to be necessary, sorry.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 4/6] xen/riscv: make Zihintpause no longer a required extension
2026-09-22 12:27 ` Jan Beulich
@ 2026-09-22 14:26 ` Oleksii Kurochko
2026-09-22 15:10 ` Jan Beulich
0 siblings, 1 reply; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 14:26 UTC (permalink / raw)
To: Jan Beulich, Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On 9/22/26 2:27 PM, Jan Beulich wrote:
> On 10.09.2026 11:34, 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" hint when
>> the extension is implemented, otherwise it emits `0x0100000F`, a legally
>> valid FENCE instruction (`FENCE W, 0`) rather than a native NOP.
>
> Just that PAUSE's encoding is 0x0100000F. I.e. what is emitted is always
> the same, and hence discussing the encoding aspect here doesn't help
> justify the change. NOP or not also doesn't really matter here. The
> specific hint encoding looks to fall into what prior to Zihintpause would
> have been covered by "Designated for future standard use", and hence ...
>
>> FENCE is
>> guaranteed by the RISC-V base ISA, so it never raises an illegal
>> instruction fault. With an empty successor set, it enforces no
>> memory-ordering constraints and thus architecturally behaves as a NOP.
>
> ... there indeed should be no concern for any platform playing by the
> rules.
>
>> 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>
>> ---
>> Changes since v1:
>> - rewrite commit message.
>
> I fear another round of re-writing is going to be necessary, sorry.
Would it be better:
required_extensions[] panics at boot if Zihintpause is missing, but Xen
does not strictly require hardware support for it.
The PAUSE hint (Zihintpause extension) is encoded as FENCE W, 0
(0x0100000F). In accordance with the RISC-V Unprivileged ISA, HINTs are
encoded in the space of valid standard instructions. On platforms
without Zihintpause support, executing 0x0100000F is treated as a
standard FENCE with an empty successor set, which acts as a NOP and
never generates an illegal instruction trap.
Therefore, Zihintpause is purely an optimization hint. Drop it from
required_extensions[] so systems without explicit Zihintpause support
can boot Xen successfully.
?
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 4/6] xen/riscv: make Zihintpause no longer a required extension
2026-09-22 14:26 ` Oleksii Kurochko
@ 2026-09-22 15:10 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2026-09-22 15:10 UTC (permalink / raw)
To: Oleksii Kurochko
Cc: xen-devel, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Baptiste Le Duc
On 22.09.2026 16:26, Oleksii Kurochko wrote:
> On 9/22/26 2:27 PM, Jan Beulich wrote:
>> On 10.09.2026 11:34, 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" hint when
>>> the extension is implemented, otherwise it emits `0x0100000F`, a legally
>>> valid FENCE instruction (`FENCE W, 0`) rather than a native NOP.
>>
>> Just that PAUSE's encoding is 0x0100000F. I.e. what is emitted is always
>> the same, and hence discussing the encoding aspect here doesn't help
>> justify the change. NOP or not also doesn't really matter here. The
>> specific hint encoding looks to fall into what prior to Zihintpause would
>> have been covered by "Designated for future standard use", and hence ...
>>
>>> FENCE is
>>> guaranteed by the RISC-V base ISA, so it never raises an illegal
>>> instruction fault. With an empty successor set, it enforces no
>>> memory-ordering constraints and thus architecturally behaves as a NOP.
>>
>> ... there indeed should be no concern for any platform playing by the
>> rules.
>>
>>> 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>
>>> ---
>>> Changes since v1:
>>> - rewrite commit message.
>>
>> I fear another round of re-writing is going to be necessary, sorry.
>
> Would it be better:
Quite a bit, yes; just one thing ...
> required_extensions[] panics at boot if Zihintpause is missing, but Xen
> does not strictly require hardware support for it.
... here: required_extensions[] isn't a function and hence cannot "panic".
Jan
> The PAUSE hint (Zihintpause extension) is encoded as FENCE W, 0
> (0x0100000F). In accordance with the RISC-V Unprivileged ISA, HINTs are
> encoded in the space of valid standard instructions. On platforms
> without Zihintpause support, executing 0x0100000F is treated as a
> standard FENCE with an empty successor set, which acts as a NOP and
> never generates an illegal instruction trap.
>
> Therefore, Zihintpause is purely an optimization hint. Drop it from
> required_extensions[] so systems without explicit Zihintpause support
> can boot Xen successfully.
>
> ?
>
> ~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 4/6] xen/riscv: make Zihintpause no longer a required extension
2026-09-10 9:34 ` [PATCH v2 4/6] xen/riscv: make Zihintpause " Baptiste Le Duc
2026-09-22 12:27 ` Jan Beulich
@ 2026-09-22 14:50 ` Oleksii Kurochko
1 sibling, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 14:50 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Jan Beulich, Stefano Stabellini, Roger Pau Monné,
Julien Grall, Anthony PERARD, Andrew Cooper, Connor Davis,
Alistair Francis, Michal Orzel
On 9/10/26 11:34 AM, 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" hint when
> the extension is implemented, otherwise it emits `0x0100000F`, a legally
> valid FENCE instruction (`FENCE W, 0`) rather than a native NOP. FENCE is
> guaranteed by the RISC-V base ISA, so it never raises an illegal
> instruction fault. With an empty successor set, it enforces no
> memory-ordering constraints and thus architecturally behaves 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>
> ---
> Changes since v1:
> - rewrite commit message.
> ---
> xen/arch/riscv/cpufeature.c | 1 -
> 1 file changed, 1 deletion(-)
>
Please update also booting.txt?
Thanks.
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 5/6] xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu()
2026-09-10 9:30 [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Baptiste Le Duc
` (3 preceding siblings ...)
2026-09-10 9:34 ` [PATCH v2 4/6] xen/riscv: make Zihintpause " Baptiste Le Duc
@ 2026-09-10 9:34 ` Baptiste Le Duc
2026-09-22 12:31 ` Jan Beulich
2026-09-22 14:21 ` Oleksii Kurochko
2026-09-10 9:34 ` [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start Baptiste Le Duc
2026-09-10 9:47 ` [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Jan Beulich
6 siblings, 2 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-10 9:34 UTC (permalink / raw)
To: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: Baptiste Le Duc, xen-devel
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.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
Changes since v1:
- rewrite commit message
---
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)
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 5/6] xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu()
2026-09-10 9:34 ` [PATCH v2 5/6] xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu() Baptiste Le Duc
@ 2026-09-22 12:31 ` Jan Beulich
2026-09-22 14:21 ` Oleksii Kurochko
1 sibling, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2026-09-22 12:31 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Alistair Francis, Connor Davis, Oleksii Kurochko,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini
On 10.09.2026 11:34, Baptiste Le Duc wrote:
> 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.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 5/6] xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu()
2026-09-10 9:34 ` [PATCH v2 5/6] xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu() Baptiste Le Duc
2026-09-22 12:31 ` Jan Beulich
@ 2026-09-22 14:21 ` Oleksii Kurochko
1 sibling, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 14:21 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Julien Grall, Alistair Francis, Anthony PERARD,
Andrew Cooper, Jan Beulich, Michal Orzel, Stefano Stabellini,
Roger Pau Monné, Connor Davis
On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> 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.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
> Changes since v1:
> - rewrite commit message
> ---
> 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)
>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start
2026-09-10 9:30 [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Baptiste Le Duc
` (4 preceding siblings ...)
2026-09-10 9:34 ` [PATCH v2 5/6] xen/riscv: flush speculatively cached Bare-mode TLB entries in turn_on_mmu() Baptiste Le Duc
@ 2026-09-10 9:34 ` Baptiste Le Duc
2026-09-16 8:54 ` Zhang Zheng
` (2 more replies)
2026-09-10 9:47 ` [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Jan Beulich
6 siblings, 3 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-10 9:34 UTC (permalink / raw)
To: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: Baptiste Le Duc, xen-devel, Zheng Zhang
check_pgtbl_mode_support() declares level_map_mask as bare `unsigned`, i.e.
a 32-bit type while it derives from a paddr_t which is in both RV32/RV64 a
64-bit type. Storing that value into a 32-bit local silently drops any set
bits above bit 31.
The mask is then used as:
aligned_load_start = load_start & level_map_mask;
load_start is `unsigned long` (64-bit on riscv64) and if it requires more
than 32 bits to represent, because load_start zero-extend to 64 bits, we
would drop some load_start's bits during the AND.
Widen level_map_mask to `unsigned long`, matching the width of the physical
address.
Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
Reported-by: Zheng Zhang <zhangzheng@iscas.ac.cn>
Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
---
Changes since v1:
- new patch
---
Question:
I would think replacing unsigned long by paddr_t would be better in this
case but for consistency with other variables in the function I just kept
unsigned long.
However, there are many variables in mm.c which are unsigned long while
they are, in reality, physical addresses and could technically be paddr_t.
Using paddr_t would also let us bypass the compiler's decision on what
unsigned long extends to (u32 or u64, depending on the target), and
therefore be more generic. I've seen similar code in Arm using this
convention, and found nothing on the mailing list explaining the original
choice of unsigned long over paddr_t.
Replacing every such field would be a fairly large change, so I'm asking
for your opinion on whether it's worth doing.
---
xen/arch/riscv/mm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 53bebbcabf..e7f2491257 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -180,7 +180,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
bool is_mode_supported = false;
unsigned int index;
unsigned int page_table_level = (mmu_desc->num_levels - 1);
- unsigned level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
+ unsigned long level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
unsigned long aligned_load_start = load_start & level_map_mask;
unsigned long aligned_page_size = XEN_PT_LEVEL_SIZE(page_table_level);
--
2.55.0
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start
2026-09-10 9:34 ` [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start Baptiste Le Duc
@ 2026-09-16 8:54 ` Zhang Zheng
2026-09-22 12:43 ` Jan Beulich
2026-09-22 14:21 ` Oleksii Kurochko
2 siblings, 0 replies; 35+ messages in thread
From: Zhang Zheng @ 2026-09-16 8:54 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: Alistair Francis, Connor Davis, Oleksii Kurochko, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini, xen-devel, Zheng Zhang
On Thu, 10 Sep 2026 11:34:54 +0200, Baptiste Le Duc <baptiste.le-duc@vates.tech> wrote:
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index 53bebbcabf..e7f2491257 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -180,7 +180,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
> bool is_mode_supported = false;
> unsigned int index;
> unsigned int page_table_level = (mmu_desc->num_levels - 1);
> - unsigned level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
> + unsigned long level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
>
> unsigned long aligned_load_start = load_start & level_map_mask;
> unsigned long aligned_page_size = XEN_PT_LEVEL_SIZE(page_table_level);
This resolves the issue where Xen fails to correctly mask the upper 32 bits of
the physical address. Which make xen failed to boot on the SpacemiT K3, where
U-Boot boots the kernel image located at physical address 0x1_4000_0000 by default.
In sv39, page_table_level=2 :
uboot bootm addr : 0x0000_0001_4000_0000
^
unsigned level_map_mask = 0x0000_0000_c000_0000
unsigned long level_map_mask = 0xffff_ffff_c000_0000
Tested-by: Zheng Zhang <zhangzheng@iscas.ac.cn>
--
Zhang Zheng <zhangzheng@iscas.ac.cn>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start
2026-09-10 9:34 ` [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start Baptiste Le Duc
2026-09-16 8:54 ` Zhang Zheng
@ 2026-09-22 12:43 ` Jan Beulich
2026-09-22 14:21 ` Oleksii Kurochko
2 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2026-09-22 12:43 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Zheng Zhang, Alistair Francis, Connor Davis,
Oleksii Kurochko, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini
On 10.09.2026 11:34, Baptiste Le Duc wrote:
> check_pgtbl_mode_support() declares level_map_mask as bare `unsigned`, i.e.
> a 32-bit type while it derives from a paddr_t which is in both RV32/RV64 a
> 64-bit type. Storing that value into a 32-bit local silently drops any set
> bits above bit 31.
>
> The mask is then used as:
>
> aligned_load_start = load_start & level_map_mask;
>
> load_start is `unsigned long` (64-bit on riscv64) and if it requires more
> than 32 bits to represent, because load_start zero-extend to 64 bits, we
> would drop some load_start's bits during the AND.
>
> Widen level_map_mask to `unsigned long`, matching the width of the physical
> address.
>
> Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
> Reported-by: Zheng Zhang <zhangzheng@iscas.ac.cn>
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
The change is okay as is, so
Reviewed-by: Jan Beulich <jbeulich@suse.com>
But see below.
> ---
> Question:
> I would think replacing unsigned long by paddr_t would be better in this
> case but for consistency with other variables in the function I just kept
> unsigned long.
>
> However, there are many variables in mm.c which are unsigned long while
> they are, in reality, physical addresses and could technically be paddr_t.
> Using paddr_t would also let us bypass the compiler's decision on what
> unsigned long extends to (u32 or u64, depending on the target), and
> therefore be more generic. I've seen similar code in Arm using this
> convention, and found nothing on the mailing list explaining the original
> choice of unsigned long over paddr_t.
The mask here is applied to an incoming linear address, so imo unsigned
long is the correct type.
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -180,7 +180,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
> bool is_mode_supported = false;
> unsigned int index;
> unsigned int page_table_level = (mmu_desc->num_levels - 1);
> - unsigned level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
> + unsigned long level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
>
> unsigned long aligned_load_start = load_start & level_map_mask;
level_map_mask is used exclusively here. Without that intermediate variable
no problem would have existed in the first place. Hence perhaps worth
considering
unsigned long aligned_load_start =
load_start & XEN_PT_LEVEL_MAP_MASK(page_table_level);
as an alternative?
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start
2026-09-10 9:34 ` [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start Baptiste Le Duc
2026-09-16 8:54 ` Zhang Zheng
2026-09-22 12:43 ` Jan Beulich
@ 2026-09-22 14:21 ` Oleksii Kurochko
2 siblings, 0 replies; 35+ messages in thread
From: Oleksii Kurochko @ 2026-09-22 14:21 UTC (permalink / raw)
To: Baptiste Le Duc, Alistair Francis, Connor Davis, Andrew Cooper,
Anthony PERARD, Michal Orzel, Jan Beulich, Julien Grall,
Roger Pau Monné, Stefano Stabellini
Cc: xen-devel, Zheng Zhang
On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> check_pgtbl_mode_support() declares level_map_mask as bare `unsigned`, i.e.
> a 32-bit type while it derives from a paddr_t which is in both RV32/RV64 a
> 64-bit type. Storing that value into a 32-bit local silently drops any set
> bits above bit 31.
>
> The mask is then used as:
>
> aligned_load_start = load_start & level_map_mask;
>
> load_start is `unsigned long` (64-bit on riscv64) and if it requires more
> than 32 bits to represent, because load_start zero-extend to 64 bits, we
> would drop some load_start's bits during the AND.
>
> Widen level_map_mask to `unsigned long`, matching the width of the physical
> address.
>
> Fixes: e66003e7be19 ("xen/riscv: introduce setup_initial_pages")
> Reported-by: Zheng Zhang <zhangzheng@iscas.ac.cn>
> Signed-off-by: Baptiste Le Duc <baptiste.le-duc@vates.tech>
> ---
> Changes since v1:
> - new patch
> ---
> Question:
> I would think replacing unsigned long by paddr_t would be better in this
> case but for consistency with other variables in the function I just kept
> unsigned long.
>
> However, there are many variables in mm.c which are unsigned long while
> they are, in reality, physical addresses and could technically be paddr_t.
> Using paddr_t would also let us bypass the compiler's decision on what
> unsigned long extends to (u32 or u64, depending on the target), and
> therefore be more generic. I've seen similar code in Arm using this
> convention, and found nothing on the mailing list explaining the original
> choice of unsigned long over paddr_t.
>
> Replacing every such field would be a fairly large change, so I'm asking
> for your opinion on whether it's worth doing.
> ---
I think if to do that it will better to do step by step where real use
cases which lead to some problem will happen.
Specifically here it looks like `unsigned long` should be enough ...
> xen/arch/riscv/mm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index 53bebbcabf..e7f2491257 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -180,7 +180,7 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
> bool is_mode_supported = false;
> unsigned int index;
> unsigned int page_table_level = (mmu_desc->num_levels - 1);
> - unsigned level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
> + unsigned long level_map_mask = XEN_PT_LEVEL_MAP_MASK(page_table_level);
>
> unsigned long aligned_load_start = load_start & level_map_mask;
> unsigned long aligned_page_size = XEN_PT_LEVEL_SIZE(page_table_level);
>
... what you actually did.
LGTM: Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Thanks for the fix!
~ Oleksii
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs
2026-09-10 9:30 [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Baptiste Le Duc
` (5 preceding siblings ...)
2026-09-10 9:34 ` [PATCH v2 6/6] xen/riscv: fix level_map_mask truncation on load_start Baptiste Le Duc
@ 2026-09-10 9:47 ` Jan Beulich
2026-09-10 9:56 ` Baptiste Le Duc
6 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2026-09-10 9:47 UTC (permalink / raw)
To: Baptiste Le Duc
Cc: xen-devel, Zheng Zhang, Alistair Francis, Connor Davis,
Oleksii Kurochko, Andrew Cooper, Anthony PERARD, Michal Orzel,
Julien Grall, Roger Pau Monné, Stefano Stabellini
On 10.09.2026 11:30, Baptiste Le Duc wrote:
> This series introduces bugs fixes that were found while bringing up CI
> support for the HiFive Premier P550 board with a basic smoke test. The
> board-support series itself will follow separately as it depends on
> PLIC/vPLIC and dom0less support that have not been upstreamed yet. This
> series carries only the independent fixes found along the way, none of them
> need the board-support series to apply.
>
> This series:
> 1: Fix Svade/Svadu A/D bit handling
> 2: Set A/D bits in Xen's own page-table mappings under Svade
> 3: Make Svpbmt no longer a required extension
> 4: Make Zihintpause no longer a required extension
> 5: Flush speculatively cached Bare-mode TLB entries in turn_on_mmu()
> 6: Fix level_map_mask truncation on load_start
>
> CI pipeline:
> https://gitlab.com/xen-project/people/baptleduc/xen/-/pipelines/2836284276
>
> ---
> Changes since v1:
> - address ML comments
> - rename some patchs
> - add new patch-fix: 242dd1f890e4 ("xen/riscv: fix level_map_mask
> truncation on load_start") discovered when working on Spacemit K3 support
>
> To: Zheng Zhang <Zheng Zhang <zhangzheng@iscas.ac.cn>
> To: Alistair Francis <alistair.francis@wdc.com>
> To: Connor Davis <connojdavis@gmail.com>
> To: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> To: Andrew Cooper <andrew.cooper3@citrix.com>
> To: Anthony PERARD <anthony.perard@vates.tech>
> To: Michal Orzel <michal.orzel@amd.com>
> To: Jan Beulich <jbeulich@suse.com>
> To: Julien Grall <julien@xen.org>
> To: Roger Pau Monné <roger@xenproject.org>
> To: Stefano Stabellini <sstabellini@kernel.org>
> Cc: xen-devel@lists.xenproject.org
Please can you adhere to patch submission rules? Patches are to be sent To:
the list, with relevant people Cc:-ed.
Jan
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs
2026-09-10 9:47 ` [PATCH v2 0/6] xen/riscv: fix boot on missing extensions and MMU setup bugs Jan Beulich
@ 2026-09-10 9:56 ` Baptiste Le Duc
0 siblings, 0 replies; 35+ messages in thread
From: Baptiste Le Duc @ 2026-09-10 9:56 UTC (permalink / raw)
To: Jan Beulich
Cc: Baptiste Le Duc, xen-devel, Zheng Zhang, Alistair Francis,
Connor Davis, Oleksii Kurochko, Andrew Cooper, Anthony PERARD,
Michal Orzel, Julien Grall, Roger Pau Monné,
Stefano Stabellini
On 2026-09-10 11:47 +0200, Jan Beulich wrote:
> On 10.09.2026 11:30, Baptiste Le Duc wrote:
> > This series introduces bugs fixes that were found while bringing up CI
> > support for the HiFive Premier P550 board with a basic smoke test. The
> > board-support series itself will follow separately as it depends on
> > PLIC/vPLIC and dom0less support that have not been upstreamed yet. This
> > series carries only the independent fixes found along the way, none of them
> > need the board-support series to apply.
> >
> > This series:
> > 1: Fix Svade/Svadu A/D bit handling
> > 2: Set A/D bits in Xen's own page-table mappings under Svade
> > 3: Make Svpbmt no longer a required extension
> > 4: Make Zihintpause no longer a required extension
> > 5: Flush speculatively cached Bare-mode TLB entries in turn_on_mmu()
> > 6: Fix level_map_mask truncation on load_start
> >
> > CI pipeline:
> > https://gitlab.com/xen-project/people/baptleduc/xen/-/pipelines/2836284276
> >
> > ---
> > Changes since v1:
> > - address ML comments
> > - rename some patchs
> > - add new patch-fix: 242dd1f890e4 ("xen/riscv: fix level_map_mask
> > truncation on load_start") discovered when working on Spacemit K3 support
> >
> > To: Zheng Zhang <Zheng Zhang <zhangzheng@iscas.ac.cn>
> > To: Alistair Francis <alistair.francis@wdc.com>
> > To: Connor Davis <connojdavis@gmail.com>
> > To: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > To: Andrew Cooper <andrew.cooper3@citrix.com>
> > To: Anthony PERARD <anthony.perard@vates.tech>
> > To: Michal Orzel <michal.orzel@amd.com>
> > To: Jan Beulich <jbeulich@suse.com>
> > To: Julien Grall <julien@xen.org>
> > To: Roger Pau Monné <roger@xenproject.org>
> > To: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: xen-devel@lists.xenproject.org
>
> Please can you adhere to patch submission rules? Patches are to be sent To:
> the list, with relevant people Cc:-ed.
>
Sorry, first time I used b4 tool which automatically added the To: and I
missed that during the dry-run... I'd like to do a .b4-config with the
submission rules that could be added in the repo, do you think it could
be a good idea?
> Jan
>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread