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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/13/25 5:32 PM, Jan Beulich wrote:

> On 13.11.2025 17:18, Oleksii Kurochko wrote:
> > 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?
> >       
> Not just, no. My point is that you shouldn't pollute start_xen() with calls to
> dozens of special-purpose functions. There wants to be one call dealing with
> everything guest-mm related, I think.
>     
I think I understand your point now. I’ll introduce guest_mm_init() and move
everything related to it inside that function (at the moment, this includes
the VMID initialization and G-stage mode detection).

> > 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().
> >       
> No per-domain function wants to be used for this, I agree. Hence why I pointed
> you at x86'es paging_init().
>
> > > > --- /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?
> >       
> Why device tree? That's what's exposed to guests, isn't it? Here we talk about
> what Xen uses to run guests. And that can vary from guest to guest.
>     
At the same time, the bootloader also passes a device tree to Xen, and Xen
uses it, at least—to determine the RAM addresses and sizes. I also referred to
device tree because it can indicate the largest MMU mode supported on a hart:
  mmu-type:
    description:
      Identifies the largest MMU address translation mode supported by
      this hart.  These values originate from the RISC-V Privileged
      Specification document, available from
      https://riscv.org/specifications/
    $ref: /schemas/types.yaml#/definitions/string
    enum:
      - riscv,sv32
      - riscv,sv39
      - riscv,sv48
      - riscv,sv57
      - riscv,none
And so, I thought, that Xen could also re-use this information and use it as starting
value for Mode detection. But considering how much modes are supported by RISC-V spec,
it seems that it won't be too long just to detect which are supported in the way it
is done now.

> > 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?
> >       
> No. Bigger guests want running in 48x4, huge ones in 57x4 (each: if available),
> and most ones in 39x4. It doesn't matter what direction you do the checks, you
> want to know what you have available.
>     
My point was that if we change the direction, then once we find the first (largest)
supported MMU mode, there is no need to check the others (lower modes) as according
to the RISC-V specification, the lower modes must be supported automatically.

~ Oleksii