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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/6/25 2:43 PM, Jan Beulich wrote:

> 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().
>     
Does it only the question of function naming now?

IMO, ideally it would be nice to have it in p2m_init(), but there is no a lot of
sense to detect supported modes each time a domain is constructed. And it is the
reason why I put directly to start_xen().

> > --- 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.
>     
I agree that it is enough to have only *_MASK.

I can do a separate cleanup patch (as what mentioned below were already introduced
before) which will drop:
 HGATP32_MODE_SHIFT, HGATP32_VMID_SHIFT, HGATP64_MODE_SHIFT, HGATP64_VMID_SHIFT
and rename:
 HGATP*_PPN to HGATP*_PPN_MASK

Does it make sense?

> > --- /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?
>     
Good point. It should be 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 considered traversing the modes[] array in the opposite order so that the largest
mode would be checked first. However, I decided that 39x4 is sufficiently large and
provides a good balance between the number of page tables and supported address
space, at least for now.

> I wonder whether you wouldn't be better off recording all supported modes right
> away.
>     
What would be the use case for recording and storing all supported modes?
For example, would it be used to indicate which mode is preferable for a guest
domain via the device tree?
Also, I’d like to note that it probably doesn’t make much sense to record all
supported modes. If we traverse the modes[] array in the opposite order—checking
Sv57 first—then, according to the RISC-V specification:
- Implementations that support Sv57 must also support Sv48.
- Implementations that support Sv48 must also support Sv39.
So if Sv57 is supported then lower modes are supported too. (except Sv32 for RV32)
Based on this, it seems reasonable to start checking from Sv57, right?

Thanks.

~ Oleksii