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

[Jan Beulich <jbeulich@xxxxxxxx>]

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