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
