Re: [PATCH v6 02/19] xen/riscv: detect and initialize G-stage mode

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/8/25 5:22 PM, Jan Beulich wrote:
> On 24.11.2025 13:33, Oleksii Kurochko wrote:
> > +static void __init gstage_mode_detect(void)
> > +{
> > +    static const struct gstage_mode_desc 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;
> Can't this move ...
>
> > +    for ( mode_idx = ARRAY_SIZE(modes); mode_idx-- > 0; )
> ... into here? You don't use the variable outside of the loop.
Agree, mode_idx isn't used outside of the loop anymore, so I will move
declaration of it into for header.

> > +    {
> > +        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 )
> > +        {
> > +            max_gstage_mode.mode = modes[mode_idx].mode;
> > +            max_gstage_mode.paging_levels = modes[mode_idx].paging_levels;
> > +            safe_strcpy(max_gstage_mode.name, modes[mode_idx].name);
> This looks as if you were overwriting .rodata here (the string literal
> "Bare"). You aren't, but why can't the whole copying be a single struct
> assignment?
Agree, it could be just:
 max_gstage_mode = modes[mode_idx];

> Preferably with the adjustments:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks.

~ Oleksii