Re: [PATCH v2 09/11] xen/riscv: introduce p2m_gpa_bits

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2026 17:29, Oleksii Kurochko wrote:
> p2m_gpa_bits is used by common/device-tree/domain-build.c thereby when
> CONFIG_DOMAIN_BUILD_HELPERS=y it is necessary to have p2m_gpa_bits properly
> defined as it is going to be used to find unused regions.
> 
> Introduce default_gstage_mode to have ability to limit p2m_gpa_bits before
> p2m_init() is being called as it will be too late.

This is a somewhat strange way of describing things. Of course you want to
establish globals before doing any per-domain setup.

> Limit p2m_gpa_bits in guest_mm_init() as it could be that default G-stage
> MMU mode uses less VA wide bits than IOMMU,

How does a VA come into play here? And what is "less VA wide bits"?

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -51,6 +51,24 @@ static struct gstage_mode_desc __ro_after_init 
> max_gstage_mode = {
>      .name = "Bare",
>  };
>  
> +static struct gstage_mode_desc __ro_after_init default_gstage_mode = {
> +    .mode = HGATP_MODE_SV39X4,
> +    .paging_levels = 2,
> +    .name = "Sv39x4",
> +};
> +
> +/*
> + * Set to the maximum configured support for GPA bits, so the number of GPA
> + * bits can be restricted by an external entity (e.g. IOMMU) and the
> + * restriction must happen before the call of guest_mm_init().

DYM before p2m_init()? Because you do the calculation in the named
function.

> + * The widest G-stage mode defined by the RISC-V specification is Sv57x4,
> + * which yields 59-bit GPAs: Sv57 maps 57-bit VAs onto 56-bit PAs 
> (PADDR_BITS),
> + * and the G-stage "x4" extension widens the address space by a further 2 
> bits,
> + * hence PADDR_BITS + 1 + P2M_ROOT_EXTRA_BITS.
> + */

I fear I don't follow. I can't explain the +1 at all. And adding in
P2M_ROOT_EXTRA_BITS seems wrong too: Whatever the width of output of
guest paging _is_ the width of input to stage-2 paging. There's no way
for a guest to encode 2 extra bits.

> @@ -191,8 +209,13 @@ static void __init gstage_mode_detect(void)
>  
>  void __init guest_mm_init(void)
>  {
> +    unsigned int gpa_bits;
> +    unsigned int paging_levels = default_gstage_mode.paging_levels;

Deriving a global from a default, when ...

>      gstage_mode_detect();
>  
> +    ASSERT(default_gstage_mode.paging_levels <= 
> max_gstage_mode.paging_levels);

... the default isn't the maximum possible, isn't going to fly.

Jan