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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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