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

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

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

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

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

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

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

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

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

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

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

Checking the ISA string alone isn't sufficient.

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

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

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

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

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

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

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

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

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

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

~ Oleksii

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