Re: [PATCH v3 08/12] xen/riscv: rework G-stage mode handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.04.2026 17:54, Oleksii Kurochko wrote:
> --- 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 modes[] = {

As before, I'm of the clear opinion that this is too generic an identifier
for use at file scope.

> @@ -331,8 +324,40 @@ 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(const char *mmu_type)

Nit (style): Stray blank after *.

>  {
> +    for ( unsigned int mode_idx = 0; mode_idx < ARRAY_SIZE(modes); 
> mode_idx++ )

Can't the variable be just idx or even i? Would likely help readability
some.

> +    {
> +        if ( !strcasecmp(mmu_type, modes[mode_idx].name) )
> +        {
> +            if ( modes[mode_idx].mode == HGATP_MODE_OFF ||

Instead of this, start the loop at index 1, with ...

> +                 modes[mode_idx].mode > max_gstage_mode->mode )
> +                break;
> +
> +            return &modes[mode_idx];
> +        }
> +    }
> +
> +    ASSERT(modes[0].mode == HGATP_MODE_OFF);

... this moved up?

> +    dprintk(XENLOG_ERR, "Requested G-stage mode (%s) isn't supported\n",
> +            mmu_type);
> +
> +    /*
> +     * Return the Bare-mode sentinel. p2m_init() will reject it with
> +     * -EINVAL, producing the appropriate domain-creation failure.
> +     */
> +    return &modes[0];
> +}

Yet better return NULL on error?

> +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 *m = &modes[0];

It being temporary it may not matter much, but couldn't this be 
__ro_after_init?`
At least one domain needs creating during boot, so ...

> @@ -341,6 +366,27 @@ 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(config->arch.gstage_mode);
> +
> +    if ( p2m->mode->mode == HGATP_MODE_OFF )
> +        return -EINVAL;
> +
> +    if ( m->mode == HGATP_MODE_OFF )
> +        m = p2m->mode;

... this path won't be taken post-init.

> --- 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 MMU mode for the guest (e.g. "sv39", "sv48", "sv57").
> +     * Must be set; an empty string is invalid.
> +     */
> +    char gstage_mode[8];
>  };

I have to say that I find it odd to use a string literal for this purpose.
Specifying the number of wanted address bits would feel more natural. Plus
the strings named aren't valid G-stage modes afaict - they lack the x4.

Jan