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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/30/26 5:34 PM, Jan Beulich wrote:
> 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.
Then I will drop that sentence now and avoid similar in the future.

> > 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?
It is what spec uses, for example:
 Figure 108. Sv39x4 virtual address (guest physical address).

I can just use GPA.

> And what is "less VA wide bits"?
They could be configured to different modes: IOMMU lets say Sv39 and MMU 
- Sv48, so IOMMU could work with 39-bit GPA, but MMU - with 48-bit GPAs.

> > --- 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.
Yes, before p2m_init(). Probably, as you made a note in the commit 
message, this part could be dropped too.
> > + * 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.
Agree, +1 should be dropped. I think I mistakenly interpret PADDR_BITS 
as highest possible bit set, so 55 intead of 56.
 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.
Agree, PADDR_BITS should be enough here to be used as initializer.


> > @@ -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.
I didn't get you here.

If we want Xen uses Sv39 for G-stage, we want to limit guest's 56-bit 
GPA to 39-bit GPA, but not the maximum supported by h/w mode for G-stage 
mode.
~ Oleksii