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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/1/26 3:19 PM, Jan Beulich wrote:
> 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.
There is an optional mmu-type property for each cpu:
https://github.com/riscv-non-isa/riscv-device-tree-doc/blob/master/bindings/riscv/cpus.txt#L73



> > 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.)
Yes. According to the link above the following options could be passed:
                            "riscv,sv32"
                            "riscv,sv39"
                            "riscv,sv48"

> > 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?
I wasn't able to find better source for arch-specific definitions. For 
example, the source mentioned above has outdated mmu-type properties 
which should also contain riscv,sv57 and riscv,none.
> > --- 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?
I need modes[] to be outside of gstage_mode_detect() as it then could be 
re-used. For example, if expected G-stage mode should be passed by DTS 
property then in DTS property we'll have something like:
        chosen {
            ...
            DOMU1 {
                mmu-type="riscv,sv48";
                ...
            }
            ...
        }

And I will need to have another functions something like:
  static unsigned int find_gstage_mode(const char *mmu_type) {...}
which will re-use modes[] to find a correspondent mode and return an 
index (or return just correspondent mode) for that mode to then re-use 
it to initialize p2m->mode:
    p2m->mode = &modes[find_gstage_mode(mmu_type)];

~ Oleksii