Re: [PATCH v1 04/27] xen/riscv: rework G-stage mode handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 18:08, Oleksii Kurochko wrote:
> Rework G-stage mode handling to make the selected mode descriptor reusable
> outside of p2m initialization.
> 
> As max_gstage_mode is going to be reused by code that creates CPU nodes for
> guest domains, not only max_gstage_mode->mode but also max_gstage_mode->name
> is required.

I guess I'm not DT-savvy enough to understand why that would be.

> To support this, make max_gstage_mode a global pointer to one of
> the entries in a global modes[] array, and remove get_max_supported_mode().
> 
> Update struct p2m_domain to store a pointer to a mode descriptor instead of
> embedding the structure directly.
> 
> Refactor the modes[] array so that mode->name contains only the MMU scheme
> name (without the "x4" suffix), as this value is reused when filling the
> maximum MMU type passed to the guest. According to DT bindings [1], the MMU
> type must not include the "x4" suffix. Use "none" for the Bare mode to match
> the DT binding requirements.

I expect this DT aspect is also why Sv changes to sv in the table? (Which
is a little unhelpful for the printk() where it's used.)

> Adjust modes[]->paging_levels to represent the maximum paging level rather
> than the total number of levels. This ensures that P2M_ROOT_LEVEL() and its
> users behave correctly without relying on hardcoded p2m mode values.
> 
> Finally, drop __initconst from the modes[] declaration, as the array is
> referenced via p2m->mode and max_gstage_mode beyond the init stage.
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/riscv/cpus.yaml?h=v6.19-rc3#n82

Is a reference into Linux doc really providing something "canonical"? Surely
there's an independent spec somewhere?

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -45,18 +45,32 @@ 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",
> -};
> -
>  /*
>   * Set to the maximum configured support for IPA bits, so the number of IPA 
> bits can be
>   * restricted by external entity (e.g. IOMMU).
>   */
>  unsigned int __read_mostly p2m_ipa_bits = PADDR_BITS;
>  
> +static const struct gstage_mode_desc modes[] = {

As a function scope static this was a fine identifier. Please consider whether
with the wider scope gstage_modes[] might not be better.

> +    /*
> +     * 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.
> +     */
> +    [0] = { HGATP_MODE_OFF,    0, "none" },
> +#ifdef CONFIG_RISCV_32
> +    [1] = { HGATP_MODE_SV32X4, 1, "sv32" }
> +#else
> +    [2] = { HGATP_MODE_SV39X4, 2, "sv39" },
> +    [3] = { HGATP_MODE_SV48X4, 3, "sv48" },
> +    [4] = { HGATP_MODE_SV57X4, 4, "sv57" },
> +#endif
> +};

The dedicated initializer form isn't adding any value here (whereas it slightly
hampers readability). You really don't want the array to be sparsely populated,
so perhaps better to leave as it was before?

Jan