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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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