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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> @@ -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?

> +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?

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

Also, isn't the field effectively describing the maximum width of a
guest (physical) address? In which case - simply gaddr_bits?

Jan