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

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

On 2026-08-28 17:58:14+02:00, Oleksii Kurochko wrote:
> 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 <baptiste.le-duc@xxxxxxxxxx>
> > ---
> >   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().
What do you think of something like that:

```
diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 2819ff4e7c..1b07d75105 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -47,6 +47,10 @@ static struct csr_masks __ro_after_init csr_masks;
 #define HENVCFG_VALID_MASK 0xe0000003000000ffUL
 #define HSTATEEN0_VALID_MASK 0xde00000000000007UL

+static unsigned int __ro_after_init is_pbmte;
+unsigned long __ro_after_init pte_pbmt_io;
+unsigned long __ro_after_init pte_pbmt_nocache;
+
 void __init init_csr_masks(void)
 {
     /*
@@ -79,6 +83,18 @@ void __init init_csr_masks(void)
         INIT_RO_ONE_MASK(HSTATEEN0, hstateen0);
     }

+    is_pbmte = (riscv_isa_extension_available(NULL,
+                RISCV_ISA_EXT_svpbmt)) && (ENVCFG_PBMTE &
+                csr_masks.henvcfg);
+
+    if (!is_pbmte)
+        pte_pbmt_io = pte_pbmt_nocache = 0;
+
+    else {
+        pte_pbmt_nocache = PTE_PBMT_NOCACHE;
+        pte_pbmt_io = PTE_PBMT_IO;
+    }
+
 #undef INIT_CSR_MASK
 #undef INIT_RO_ONE_MASK
 }
@@ -97,8 +113,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 ( is_pbmte )
+        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 1cc01289a0..2ac2731eda 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -55,8 +55,6 @@

 #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW

-#define pte_pbmt(pbmt) \
-    (riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svpbmt) ? (pbmt) : 0UL)
 /*
  * PAGE_HYPERVISOR_NOCACHE is used for ioremap().
  *
@@ -64,8 +62,10 @@
  * 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(PTE_PBMT_IO))
-#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | 
pte_pbmt(PTE_PBMT_NOCACHE))
+extern unsigned long pte_pbmt_nocache;
+extern unsigned long pte_pbmt_io;
+#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | pte_pbmt_io)
+#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | pte_pbmt_nocache)
```
This avoids having to recompute
PAGE_HYPERVISOR_NOCACHE/PAGE_HYPERVISOR_WC at each call site. However, I
don't like that PTE_PBMT_IO/pte_pbmt_io (resp.
PTE_PBMT_NOCACHE/pte_pbmt_nocache) end up coexisting, that's confusing
as we should not use PTE_PBMT_IO/PTE_PBMT_NOCACHE.

Another solution could be:
```
/* page.h */
extern bool svpbmt_enabled;

static inline unsigned long pte_pbmt_nocache(void)
{
    return svpbmt_enabled ? PTE_PBMT_NOCACHE : 0;
}

static inline unsigned long pte_pbmt_io(void)
{
    return svpbmt_enabled ? PTE_PBMT_IO : 0;
}

/* domain.c */

static unsigned int __ro_after_init svpbmt_enabled;
void __init init_csr_masks(void)
{
    ...

    svpbmt_enabled = (riscv_isa_extension_available(NULL,
                RISCV_ISA_EXT_svpbmt)) && (ENVCFG_PBMTE &
                csr_masks.henvcfg);
}
```
What do you think?

> 
>  > +
>  > +#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