Re: [PATCH v3 06/20] xen/riscv: add root page table allocation

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/7/25 5:57 PM, Jan Beulich wrote:

> On 07.08.2025 15:35, Oleksii Kurochko wrote:
> > On 8/5/25 12:43 PM, Jan Beulich wrote:
> > > On 31.07.2025 17:58, Oleksii Kurochko wrote:
> > > > +static int p2m_alloc_root_table(struct p2m_domain *p2m)
> > > > +{
> > > > +    struct domain *d = p2m->domain;
> > > > +    struct page_info *page;
> > > > +    const unsigned int nr_root_pages = P2M_ROOT_PAGES;
> > > > +
> > > > +    /*
> > > > +     * Return back nr_root_pages to assure the root table memory is also
> > > > +     * accounted against the P2M pool of the domain.
> > > > +     */
> > > > +    if ( !paging_ret_pages_to_domheap(d, nr_root_pages) )
> > > > +        return -ENOMEM;
> > > > +
> > > > +    page = p2m_allocate_root(d);
> > > > +    if ( !page )
> > > > +        return -ENOMEM;
> > > > +
> > > > +    p2m->root = page;
> > > > +
> > > > +    return 0;
> > > > +}
> > > >           
> > > In the success case, shouldn't you bump the paging pool's total_pages by
> > > P2M_ROOT_PAGES? (As the freeing side is missing so far, it's not easy to
> > > tell whether there's [going to be] a balancing problem in the long run.
> > > In the short run there certainly is.)
> > >         
> > I think that total_pages should be updated only in case when page is added
> > to freelist.
> > In the case of p2m root table, we just returning some pages to domheap and
> > durint that decreasing an amount of total_pages as freelist has lesser pages,
> > and then just allocate pages from domheap without adding them to freelist.
> >       
> But how's freeing of a root table going to look like?
>     
We have saved pointer to first page of P2M_ROOT_PAGES allocated for root page
table which is stored in p2m->root. Then when a domain is going to be destroyed,
then do something like:
    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
        clear_and_clean_page(p2m->root + i);
...

> Logically that group
> of 4 pages would be put back into the pool. And from that the pool's
> total_pages should reflect that right after successful allocation.
>     
... I think instead of having the loop mentioned above we could add root table
pages to p2m->pages (as you suggested) in p2m_allocate_root() and then a domain
is being destroyed just do the following:
  while ( (pg = page_list_remove_head(&p2m->pages)) )
  {
      p2m_free_page(p2m->domain, pg);
And it will be a job of internals of p2m_free_page() -> paging_free_page() to
adjust freelist's total_pages and return back page(s) allocated for root table
to the freelist. (Note: the current implementation of paging_free_page() just
add a page to freelist without updating of freelist's total_pages what looks
incorrect. And it will be enough as total_pages is present only for freelist
and there is not separate total_pages (or something similar) for p2m->pages).

~ Oleksii