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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.04.2026 12:47, Oleksii Kurochko wrote:
> On 4/1/26 3:19 PM, Jan Beulich wrote:
>> On 10.03.2026 18:08, Oleksii Kurochko wrote:
>>> --- 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.

Sure, and I didn't say "where it was before". I said "as it was before", i.e.
without dedicated initializers.

Jan

> 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