Re: [for 4.22 v5 11/18] xen/riscv: Implement p2m_free_subtree() and related helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.11.2025 12:36, Oleksii Kurochko wrote:
> On 11/10/25 4:29 PM, Jan Beulich wrote:
>> On 20.10.2025 17:57, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/p2m.c
>>> +++ b/xen/arch/riscv/p2m.c
>>> @@ -17,6 +17,8 @@
>>>   #include <asm/riscv_encoding.h>
>>>   #include <asm/vmid.h>
>>>   
>>> +#define P2M_SUPPORTED_LEVEL_MAPPING 2
>> I fear without a comment it's left unclear what this is / represents.
> 
> Probably just renaming it to|P2M_MAX_SUPPORTED_LEVEL_MAPPING| would make it 
> clearer,
> wouldn’t it?
> Otherwise, I can add the following comment:
> /*
>   * At the moment, only 4K, 2M, and 1G mappings are supported for G-stage
>   * translation. Therefore, the maximum supported page-table level is 2,
>   * which corresponds to 1G mappings.
>   */

Both the name change and the comment, if you ask me.

>>> @@ -403,11 +415,147 @@ static int p2m_next_level(struct p2m_domain *p2m, 
>>> bool alloc_tbl,
>>>       return P2M_TABLE_MAP_NONE;
>>>   }
>>>   
>>> +static void p2m_put_foreign_page(struct page_info *pg)
>>> +{
>>> +    /*
>>> +     * It’s safe to call put_page() here because arch_flush_tlb_mask()
>>> +     * will be invoked if the page is reallocated, which will trigger a
>>> +     * flush of the guest TLBs.
>>> +     */
>>> +    put_page(pg);
>>> +}
>>> +
>>> +/* Put any references on the single 4K page referenced by mfn. */
>> To me this and ...
>>
>>> +static void p2m_put_4k_page(mfn_t mfn, p2m_type_t type)
>>> +{
>>> +    /* TODO: Handle other p2m types */
>>> +
>>> +    if ( p2m_is_foreign(type) )
>>> +    {
>>> +        ASSERT(mfn_valid(mfn));
>>> +        p2m_put_foreign_page(mfn_to_page(mfn));
>>> +    }
>>> +}
>>> +
>>> +/* Put any references on the superpage referenced by mfn. */
>> ... to a lesser degree this comment are potentially misleading. Down here at
>> least there is something plural-ish (the 4k pages that the 2M one consists
>> of), but especially for the single page case above "any" could easily mean
>> "anything that's still outstanding, anywhere". I'm also not quite sure "on"
>> is really what you mean (I'm not a native speaker, so my gut feeling may be
>> wrong here).
> 
> Then I could suggest the following instead:
>    /* Put the reference associated with the 4K page identified by mfn. */
> and
>   /* Put the references associated with the superpage identified by mfn. */
> 
> I think the comments could be omitted, since the function names already make
> this clear.

Okay with me.

Jan