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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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