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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]


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()|?

~ Oleksii


[-- Attachment #2: Type: text/html, Size: 3043 bytes --]