Re: [PATCH v4 05/18] xen/riscv: add root page table allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2025 17:40, Oleksii Kurochko wrote:
> 
> On 9/20/25 12:14 AM, Jan Beulich wrote:
>> On 17.09.2025 23:55, Oleksii Kurochko wrote:
>>
>>> +{
>>> +    return MASK_INSR(mfn_x(page_to_mfn(p2m->root)), HGATP_PPN) |
>>> +           MASK_INSR(gstage_mode, HGATP_MODE_MASK) |
>>> +           MASK_INSR(vmid, HGATP_VMID_MASK);
>>> +}
>>> +
>>> +static int p2m_alloc_root_table(struct p2m_domain *p2m)
>>> +{
>>> +    struct domain *d = p2m->domain;
>>> +    struct page_info *page;
>>> +    int rc;
>>> +
>>> +    /*
>>> +     * Return back P2M_ROOT_PAGES to assure the root table memory is also
>>> +     * accounted against the P2M pool of the domain.
>>> +     */
>>> +    if ( (rc = paging_ret_pages_to_domheap(d, P2M_ROOT_PAGES)) )
>>> +        return rc;
>> I read the "ret" in the name as "return" here. However, ...
>>
>>> +    /*
>>> +     * As mentioned in the Priviliged Architecture Spec (version 20240411)
>>> +     * in Section 18.5.1, for the paged virtual-memory schemes  (Sv32x4,
>>> +     * Sv39x4, Sv48x4, and Sv57x4), the root page table is 16 KiB and must
>>> +     * be aligned to a 16-KiB boundary.
>>> +     */
>>> +    page = alloc_domheap_pages(d, P2M_ROOT_ORDER, MEMF_no_owner);
>>> +    if ( !page )
>>> +    {
>>> +        /*
>>> +         * If allocation of root table pages fails, the pages acquired 
>>> above
>>> +         * must be returned to the freelist to maintain proper freelist
>>> +         * balance.
>>> +         */
>>> +        paging_ret_pages_to_freelist(d, P2M_ROOT_PAGES);
>> ... "return" doesn't make sense here, so I wonder what the "ret" here means.
> 
> In both cases,|ret| was supposed to mean/"return"/, since in both cases we
> “return” memory.
> I agree that in the case of|paging_ret_pages_to_freelist()|, the flow is
> slightly different: a page is allocated from the domheap and then added back
> to the freelist. That looks more like/adding/ than/returning/. Still, I felt 
> that
> “return” could also apply here, as the page is being given back.
> 
> For more clarity, do you think it would make sense to rename
> |paging_ret_pages_to_freelist()| to|paging_add_page_to_freelist()|?

In place of "add" I'd perhaps use "refill" and then really refill_from_domheap
to properly indicate the opposite of ret_to_domheap. (For both of these: If
already you want to use such long-ish names.)

Jan