Re: [for 4.22 v5 01/18] xen/riscv: detect and initialize G-stage mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2025 17:57, Oleksii Kurochko wrote:
> Changes in V5:
>  - Add static and __initconst for local variable modes[] in
>    gstage_mode_detect().
>  - Change type for gstage_mode from 'unsigned long' to 'unsigned char'.
>  - Update the comment inisde defintion if modes[] variable in
>    gstage_mode_detect():
>    - Add information about Bare mode.
>    - Drop "a paged virtual-memory scheme described in Section 10.3" as it 
> isn't
>      relevant here.
>  - Drop printing of function name when chosen G-stage mode message is printed.
>  - Drop the call of gstage_mode_detect() from start_xen(). It will be added 
> into
>    p2m_init() when the latter will be introduced.

Well, thanks, but ...

>  - Introduce pre_gstage_init().

... the same comment that I gave before now applies here: This doesn't look to
belong directly in start_xen(). In x86'es terms I'd say this is a tiny part of
paging_init().

> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -7,6 +7,7 @@ obj-y += intc.o
>  obj-y += irq.o
>  obj-y += mm.o
>  obj-y += pt.o
> +obj-y += p2m.o

Nit: Please keep things sorted (numbers sort before letters).

> --- a/xen/arch/riscv/include/asm/p2m.h
> +++ b/xen/arch/riscv/include/asm/p2m.h
> @@ -6,6 +6,8 @@
>  
>  #include <asm/page-bits.h>
>  
> +extern unsigned char gstage_mode;

Better move down some, at the very least ...

>  #define paddr_bits PADDR_BITS

... past more fundamental #define-s?

> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> @@ -131,13 +131,16 @@
>  #define HGATP_MODE_SV32X4            _UL(1)
>  #define HGATP_MODE_SV39X4            _UL(8)
>  #define HGATP_MODE_SV48X4            _UL(9)
> +#define HGATP_MODE_SV57X4            _UL(10)
>  
>  #define HGATP32_MODE_SHIFT           31
> +#define HGATP32_MODE_MASK            _UL(0x80000000)

Please can we avoid redundant (and then not even connected) #define-s? I
don't see why you would need HGATP32_MODE_SHIFT when you have
HGATP32_MODE_MASK. Similarly ...

>  #define HGATP32_VMID_SHIFT           22
>  #define HGATP32_VMID_MASK            _UL(0x1FC00000)

... here, while ...

>  #define HGATP32_PPN                  _UL(0x003FFFFF)

... here the constant isn't even suffixed with _MASK.

> --- /dev/null
> +++ b/xen/arch/riscv/p2m.c
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/macros.h>
> +#include <xen/sections.h>
> +
> +#include <asm/csr.h>
> +#include <asm/flushtlb.h>
> +#include <asm/riscv_encoding.h>
> +
> +unsigned char __ro_after_init gstage_mode;
> +
> +static void __init gstage_mode_detect(void)
> +{
> +    static const struct {
> +        unsigned char mode;
> +        unsigned int paging_levels;
> +        const char name[8];
> +    } modes[] __initconst = {
> +        /*
> +         * Based on the RISC-V spec:
> +         *   Bare mode is always supported, regardless of SXLEN.
> +         *   When SXLEN=32, the only other valid setting for MODE is Sv32.
> +         *   When SXLEN=64, three paged virtual-memory schemes are defined:
> +         *   Sv39, Sv48, and Sv57.
> +         */
> +#ifdef CONFIG_RISCV_32
> +        { HGATP_MODE_SV32X4, 2, "Sv32x4" }
> +#else
> +        { HGATP_MODE_SV39X4, 3, "Sv39x4" },
> +        { HGATP_MODE_SV48X4, 4, "Sv48x4" },
> +        { HGATP_MODE_SV57X4, 5, "Sv57x4" },
> +#endif
> +    };
> +
> +    unsigned int mode_idx;
> +
> +    gstage_mode = HGATP_MODE_OFF;

Why is this not the variable's initializer?

> +    for ( mode_idx = 0; mode_idx < ARRAY_SIZE(modes); mode_idx++ )
> +    {
> +        unsigned long mode = modes[mode_idx].mode;
> +
> +        csr_write(CSR_HGATP, MASK_INSR(mode, HGATP_MODE_MASK));
> +
> +        if ( MASK_EXTR(csr_read(CSR_HGATP), HGATP_MODE_MASK) == mode )
> +        {
> +            gstage_mode = mode;
> +            break;
> +        }
> +    }

I take it that using the first available mode is only transient. To support 
bigger
guests, you may need to pick 48x4 or even 57x4 no matter that 39x4 is available.
I wonder whether you wouldn't be better off recording all supported modes right
away.

Jan