Re: [PATCH v2 03/16] xen/riscv: introduce support of Svpbmt extension

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/13/25 6:00 PM, Jan Beulich wrote:

> On 06.05.2025 18:51, Oleksii Kurochko wrote:
> > Svpbmt extension is necessary for chaning the memory type for a page contains
> > a combination of attributes that indicate the cacheability, idempotency,
> > and ordering properties for access to that page.
> >       
> The title suggest use of the extension is optional.
>
> > --- a/xen/arch/riscv/Kconfig
> > +++ b/xen/arch/riscv/Kconfig
> > @@ -10,11 +10,25 @@ config RISCV
> >  config RISCV_64
> >  	def_bool y
> >  	select 64BIT
> > +	select HAS_SVPBMT
> >       
> Such redundant ...
>
> >  config ARCH_DEFCONFIG
> >  	string
> >  	default "arch/riscv/configs/tiny64_defconfig"
> >  
> > +config HAS_SVPBMT
> > +	bool
> > +	depends on RISCV_64
> >       
> ... dependencies are frowned upon, afaik. And it's pretty certainly not
> needed here.
>
> > +	help
> > +	  This config enables usage of Svpbmt ISA-extension ( Supervisor-mode:
> > +	  page-based memory types).
> > +
> > +	  The memory type for a page contains a combination of attributes
> > +	  that indicate the cacheability, idempotency, and ordering
> > +	  properties for access to that page.
> > +
> > +	  The Svpbmt extension is only available on 64-bit cpus.
> >       
> I don't mind the help text, but for a prompt-less option it's of little
> use (beyond what a comment could also achieve).
>     
I'll drop "depends on RISCV_64" for HAS_SVOBMT and leave only 'select HAS_SVPBMT'
and move the help text to commit messaage.

> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -46,6 +46,8 @@
> >  #define PAGE_HYPERVISOR_RX          (PTE_VALID | PTE_READABLE | PTE_EXECUTABLE)
> >  
> >  #define PAGE_HYPERVISOR             PAGE_HYPERVISOR_RW
> > +#define PAGE_HYPERVISOR_NOCACHE     (PAGE_HYPERVISOR_RW | PTE_PMBT_IO)
> > +#define PAGE_HYPERVISOR_WC          (PAGE_HYPERVISOR_RW | PTE_PMBT_NOCACHE)
> >       
> Hmm, odd - NOCACHE doesn't really mean "no cache" then? I think this
> would require a comment then.
>     
According to the table (Svpbmt Memory Type definitions) in the comment below these
definitions both of them (PTE_PMBT_IO and PTE_PMBT_NOCACHE) are non-cachable.

I wasn't sure what and for what should be used so I did in sync with Arm which
defines "#define PAGE_HYPERVISOR_NOCACHE (_PAGE_DEVICE|MT_DEVICE_nGnRE)" where
MT_DEVICE_nGnRE is equivalent of PTE_PMBT_IO.

I can add the comment above definition of PAGE_HYPERVISOR_NOCACHE:
/* Non-cacheable, non-idempotent, strongly-ordered I/O memory */.

Something similar then I can add for PAGE_HYPERVISOR_WC:
/* Non-cacheable, idempotent, weakly-ordered Main Memory */

> > @@ -56,8 +58,21 @@
> >  #define PTE_SMALL       BIT(10, UL)
> >  #define PTE_POPULATE    BIT(11, UL)
> >  
> > +/*
> > + * [62:61] Svpbmt Memory Type definitions:
> > + *
> > + *  00 - PMA    Normal Cacheable, No change to implied PMA memory type
> > + *  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
> > + */
> > +#define PTE_PMBT_NOCACHE    BIT(61, UL)
> > +#define PTE_PMBT_IO         BIT(62, UL)
> >       
> Unlike PTE_SMALL and PTE_POPULATE these are arch-defined; I think they
> want to move up to where the other arch-defined bits are, thus also
> maping them appear before their first use.
>     
Sure, I'll move them up.

Thanks.

~ Oleksii