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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]


On 9/18/25 5:54 PM, Jan Beulich wrote:
> 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?

No specific reason now. In the first version of this function they were used
directly to write a value to CSR register which is 'unsigned long'.
Considering that MASK_INSR() and MASK_EXTR() are used, 'char' should be enough
to describe mode.

>
>> +        unsigned int paging_levels;
>> +        const char *name;
> More efficiently char[8]?

I wanted to be sure that the name will always have correct length. But I agree
that char[8] is more efficient and a length could be checked "manually". I will
use char[8] instead of 'char *'.

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

I will add the following:
   Bare mode is always supported, regardless of SXLEN.

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

I will take that into account in the future. For now, I think that this part
of the comment could be just dropped as here it doesn't matter what is a scheme
of Sv32.

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

It makes sense. I will move the call to gstage_mode_detect() into p2m_init()
when the latter is introduced.
Probably, I will move the current patch after p2m_init() is introduced to make
gstage_mode_detect() static function.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 4683 bytes --]