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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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.

> 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.

Jan