Re: [PATCH v4 01/18] xen/riscv: detect and initialize G-stage mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2025 23:55, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/p2m.c
> @@ -0,0 +1,91 @@
> +/* 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 long __ro_after_init gstage_mode;
> +
> +void __init gstage_mode_detect(void)
> +{
> +    unsigned int mode_idx;
> +
> +    const struct {

static and __initconst.

> +        unsigned long mode;

Here and also for the global var: Why "long", when it's at most 4 bits?

> +        unsigned int paging_levels;
> +        const char *name;

More efficiently char[8]?

> +    } modes[] = {
> +        /*
> +         * Based on the RISC-V spec:
> +         *   When SXLEN=32, the only other valid setting for MODE is Sv32,

The use of "other" is lacking some context here.

> +         *   a paged virtual-memory scheme described in Section 10.3.

Section numbers tend to change. Either to disambiguate by also spcifying
the doc version, or (preferably) you give the section title instead.

> +         *   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
> +    };
> +
> +    gstage_mode = HGATP_MODE_OFF;
> +
> +    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;
> +        }
> +    }
> +
> +    if ( gstage_mode == HGATP_MODE_OFF )
> +        panic("Xen expects that G-stage won't be Bare mode\n");
> +
> +    printk("%s: G-stage mode is %s\n", __func__, modes[mode_idx].name);

I don't think the function name matters here at all.

> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -22,6 +22,7 @@
>  #include <asm/early_printk.h>
>  #include <asm/fixmap.h>
>  #include <asm/intc.h>
> +#include <asm/p2m.h>
>  #include <asm/sbi.h>
>  #include <asm/setup.h>
>  #include <asm/traps.h>
> @@ -148,6 +149,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>  
>      console_init_postirq();
>  
> +    gstage_mode_detect();

I find it odd for something as fine grained as this to be called from top-
level start_xen(). Imo this wants to be a sub-function of whatever does
global paging and/or p2m preparations (or even more generally guest ones).

Jan