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

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

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