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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>
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.

Jan