Re: [PATCH v4 08/11] xen/riscv: rework G-stage mode handling

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/4/26 4:23 PM, Jan Beulich wrote:
> On 28.04.2026 16:33, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/dom0less-build.c
> > @@ -0,0 +1,70 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/bootfdt.h>
> > +#include <xen/device_tree.h>
> > +#include <xen/init.h>
> > +
> > +#include <asm/p2m.h>
> > +
> > +int __init arch_parse_dom0less_node(struct dt_device_node *node,
> > +                                    struct boot_domain *bd)
> > +{
> > +    const char *mmu_type;
> > +    unsigned long bits;
> > +    const char *end;
> > +
> > +    if ( dt_property_read_string(node, "mmu-type", &mmu_type) )
> > +    {
> > +        dprintk(XENLOG_WARNING, "mmu-type property is missing in guest domain "
> > +                "node. %s will be used as fallback\n", max_gstage_mode->name);
> > +
> > +        bits = P2M_GFN_LEVEL_SHIFT(max_gstage_mode->paging_levels + 1);
> > +
> > +        goto out;
> > +    }
> > +
> > +    if ( !strcasecmp(mmu_type, "riscv,none") )
> > +    {
> > +        dprintk(XENLOG_ERR, "Bare mode isn't supported by Xen\n");
> > +
> > +        return -EOPNOTSUPP;
> > +    }
> > +
> > +    if ( strncasecmp(mmu_type, "riscv,sv", 8) )
> > +    {
> > +        dprintk(XENLOG_ERR, "mmu-type value \"%s\" is incorrect\n", mmu_type);
> > +
> > +        return -EINVAL;
> > +    }
> > +
> > +    bits = simple_strtoul(mmu_type + 8, &end, 10);
> > +    if ( (*end != '\0') || (end == mmu_type + 8) )
> > +    {
> > +        dprintk(XENLOG_ERR, "mmu-type value \"%s\" is incorrect\n", mmu_type);
> > +
> > +        return -EINVAL;
> > +    }
> > +
> > + out:
> > +    if ( bits > (UINT8_MAX - P2M_ROOT_EXTRA_BITS) )
> > +    {
> > +        dprintk(XENLOG_ERR, "gstage addr bits value overflows uint8\n");
> > +
> > +        return -EINVAL;
> > +    }
> > +    /*
> > +     * The correct value of bits will be checked in p2m_init() by call of
> > +     * find_gstage_mode_by_bits().
> > +     *
> > +     * As mmu-type property contains one of string:
> > +     *  - riscv,sv32
> > +     *  - riscv,sv39
> > +     *  - riscv,sv48
> > +     *  - riscv,sv57
> Or about any other riscv,sv<N> with N up to somewhere around 250. I see
> that ...
>
> > +     * it is needed to add '+P2M_ROOT_EXTRA_BITS' as for G-stage mode GPAs
> > +     * are extended by P2M_ROOT_EXTRA_BITS.
> > +     */
> > +    bd->create_cfg.arch.gstage_addr_bits = bits + P2M_ROOT_EXTRA_BITS;
> ... the value calculated here is later checked for validity, so it's
> really only the comment which may want clarifying a little.
I will update the comment to:

/*
 * The mmu-type property may specify any riscv,sv<N> string, but only the
 * following are currently supported:
 *  - riscv,sv32
 *  - riscv,sv39
 *  - riscv,sv48
 *  - riscv,sv57
 * Any other value will be rejected by find_gstage_mode_by_bits().
 *
 * P2M_ROOT_EXTRA_BITS is added because for G-stage mode, GPAs are
 * extended by that many bits.
 */

> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -45,12 +45,27 @@ struct p2m_pte_ctx {
> >       unsigned int level;          /* Paging level at which the PTE resides. */
> >   };
> >   
> > -static struct gstage_mode_desc __ro_after_init max_gstage_mode = {
> > -    .mode = HGATP_MODE_OFF,
> > -    .paging_levels = 0,
> > -    .name = "Bare",
> > +/* Values should be sorted by ->mode in this array */
> > +static const struct gstage_mode_desc gstage_modes[] = {
> > +    /*
> > +     * 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.
> > +     */
> > +    { HGATP_MODE_OFF,    0, "none" },
> > +#ifdef CONFIG_RISCV_32
> > +    { HGATP_MODE_SV32X4, 1, "sv32" },
> > +#else
> > +    { HGATP_MODE_SV39X4, 2, "sv39" },
> > +    { HGATP_MODE_SV48X4, 3, "sv48" },
> > +    { HGATP_MODE_SV57X4, 4, "sv57" },
> > +#endif
> >   };
> >   
> > +const struct gstage_mode_desc * __ro_after_init max_gstage_mode = &gstage_modes[0];
> Nit: Overlong line (and, strictly speaking, a stray blank after *).
I will put "&gstage_modes[0];" on next line.

> > @@ -331,8 +324,35 @@ static int p2m_alloc_root_table(struct p2m_domain *p2m)
> >       return 0;
> >   }
> >   
> > -int p2m_init(struct domain *d)
> > +static const struct gstage_mode_desc *find_gstage_mode_by_bits(
> Is "_by_bits" adding much value to the function name? Especially ...
>
> > +    unsigned char gpa_bits)
> ... seeing that the parameter name is making things pretty clear?
Maybe not too much, I will drop that.

> > +int p2m_init(struct domain *d, const struct xen_domctl_createdomain *config)
> >   {
> > +    /*
> > +     * TODO: This static is a temporary constraint: all guests must use the
> > +     * same MMU mode because p2m_gpa_bits is not yet per-domain.
> > +     * Drop this once per-domain p2m_gpa_bits is introduced.
> > +     */
> > +    static const struct gstage_mode_desc __ro_after_init *m = &gstage_modes[0];
> >       struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >   
> >       /*
> > @@ -341,6 +361,33 @@ int p2m_init(struct domain *d)
> >        */
> >       p2m->domain = d;
> >   
> > +    if ( !config )
> > +    {
> > +        dprintk(XENLOG_ERR, "NULL config is passed\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    p2m->mode = find_gstage_mode_by_bits(config->arch.gstage_addr_bits);
> > +
> > +    if ( !p2m->mode )
> > +    {
> > +        dprintk(XENLOG_ERR,
> > +                "Unsupported or unavailable gstage addr bits: %u\n",
> > +                config->arch.gstage_addr_bits);
> > +
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( m->mode == HGATP_MODE_OFF )
> > +        m = p2m->mode;
> > +
> > +    if ( m->mode != p2m->mode->mode )
> Since m always points into gstage_modes[], do you really need the extra
> indirection to compare the two ->mode fields? You could simply compare
> the pointers, couldn't you?
Agree, just "if ( m != p2m->mode )" will work.

> > --- a/xen/include/public/arch-riscv.h
> > +++ b/xen/include/public/arch-riscv.h
> > @@ -56,6 +56,11 @@ typedef struct vcpu_guest_context vcpu_guest_context_t;
> >   DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> >   
> >   struct xen_arch_domainconfig {
> > +    /*
> > +     * G-stage GPA address width in bits.
> > +     * Valid values: 34 (sv32x4), 41 (sv39x4), 50 (sv48x4), 59 (sv57x4).
> > +     */
> > +    unsigned char gstage_addr_bits;
> Fixed-width types only in the public interface please.
I will use uint8_t then.

> Also, isn't the field effectively describing the maximum width of a
> guest (physical) address? In which case - simply gaddr_bits?
gaddr_bits would be okay in this case.

Thanks.

~ Oleksii