Re: [PATCH v4 13/18] xen/riscv: implement p2m_next_level()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/22/25 7:35 PM, Jan Beulich wrote:

> On 17.09.2025 23:55, Oleksii Kurochko wrote:
> > Implement the p2m_next_level() function, which enables traversal and dynamic
> > allocation of intermediate levels (if necessary) in the RISC-V
> > p2m (physical-to-machine) page table hierarchy.
> >
> > To support this, the following helpers are introduced:
> > - page_to_p2m_table(): Constructs non-leaf PTEs pointing to next-level page
> >   tables with correct attributes.
> > - p2m_alloc_page(): Allocates page table pages, supporting both hardware and
> >   guest domains.
> > - p2m_create_table(): Allocates and initializes a new page table page and
> >   installs it into the hierarchy.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V4:
> >  - make `page` argument of page_to_p2m_table pointer-to-const.
> >  - Move p2m_next_level()'s local variable `ret` to the more narrow space where
> >    it is really used.
> >  - Drop stale ASSERT() in p2m_next_level().
> >  - Stray blank after * in declaration of paging_alloc_page().
> >       
> When you deal with comments like this, can you please make sure you
> apply them to at least a patch as a whole, if not the entire series?
> I notice ...
>
> > --- a/xen/arch/riscv/include/asm/paging.h
> > +++ b/xen/arch/riscv/include/asm/paging.h
> > @@ -15,4 +15,6 @@ int paging_ret_pages_to_freelist(struct domain *d, unsigned int nr_pages);
> >  
> >  void paging_free_page(struct domain *d, struct page_info *pg);
> >  
> > +struct page_info * paging_alloc_page(struct domain *d);
> >       
> ... there's still a stray blank here. With this dropped:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>     
Thanks.
> I have one other question, though:
>
> > +/*
> > + * Allocate a new page table page with an extra metadata page and hook it
> > + * in via the given entry.
> > + */
> > +static int p2m_create_table(struct p2m_domain *p2m, pte_t *entry)
> > +{
> > +    struct page_info *page;
> > +
> > +    ASSERT(!pte_is_valid(*entry));
> >       
> Isn't this going to get in the way of splitting superpages? The caller
> will need to initialize *entry just for this assertion to not trigger.
>     
The superpage splitting function doesn’t use p2m_create_table(). It calls
p2m_alloc_table(), then fills the table, and finally updates the entry
using p2m_write_pte(). So this shouldn’t be an issue.
Ohh, I just noticed, the comment should be updated, since an extra metadata
page is no longer allocated here.
~ Oleksii