Re: [for 4.22 v5 05/18] xen/riscv: add root page table allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2025 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -3,6 +3,7 @@
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/macros.h>
> +#include <xen/domain_page.h>
>  #include <xen/mm.h>
>  #include <xen/paging.h>
>  #include <xen/rwlock.h>
> @@ -103,6 +104,70 @@ void __init pre_gstage_init(void)
>      vmid_init();
>  }
>  
> +static void clear_and_clean_page(struct page_info *page, bool clean_dcache)
> +{
> +    clear_domain_page(page_to_mfn(page));
> +
> +    /*
> +     * If the IOMMU doesn't support coherent walks and the p2m tables are
> +     * shared between the CPU and IOMMU, it is necessary to clean the
> +     * d-cache.
> +     */
> +    if ( clean_dcache )
> +        clean_dcache_va_range(page, PAGE_SIZE);

This cleans part of frame_table[], but not the memory page in question.

> --- a/xen/arch/riscv/paging.c
> +++ b/xen/arch/riscv/paging.c
> @@ -4,46 +4,67 @@
>  #include <xen/sched.h>
>  #include <xen/spinlock.h>
>  
> +static int paging_ret_page_to_domheap(struct domain *d)
> +{
> +    struct page_info *page;
> +
> +    ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> +    /* Return memory to domheap. */
> +    page = page_list_remove_head(&d->arch.paging.freelist);
> +    if( page )
> +    {
> +        d->arch.paging.total_pages--;
> +        free_domheap_page(page);
> +    }
> +    else
> +    {
> +        printk(XENLOG_ERR
> +                "Failed to free P2M pages, P2M freelist is empty.\n");

Nit: See earlier remark regarding full stops in log messages. The double
"P2M" also looks unnecessary to me.

> +static int paging_add_page_to_freelist(struct domain *d)
> +{
> +    struct page_info *page;
> +
> +    ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> +    /* Need to allocate more memory from domheap */
> +    page = alloc_domheap_page(d, MEMF_no_owner);
> +    if ( page == NULL )
> +    {
> +        printk(XENLOG_ERR "Failed to allocate pages.\n");

Again. (Also log messages typically wouldn't start with a capital letter,
unless of course it's e.g. an acronym.)

> @@ -55,6 +76,39 @@ int paging_freelist_adjust(struct domain *d, unsigned long 
> pages,
>      return 0;
>  }
>  
> +int paging_refill_from_domheap(struct domain *d, unsigned int nr_pages)
> +{
> +    ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> +    for ( unsigned int i = 0; i < nr_pages; i++ )
> +    {
> +        int rc = paging_add_page_to_freelist(d);

The anomaly is more pronounced here, with the other function name in context:
paging_refill_from_domheap() doesn't suggest there's a page (or several) being
handed to it. paging_add_page_to_freelist() suggests one of its parameter
would want to be struct page_info *. Within the naming model you chose, maybe
paging_refill_from_domheap_one() or paging_refill_one_from_domheap()? Or
simply _paging_refill_from_domheap()?

> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return 0;
> +}
> +
> +int paging_ret_to_domheap(struct domain *d, unsigned int nr_pages)
> +{
> +    ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> +    if ( d->arch.paging.total_pages < nr_pages )
> +        return false;
> +
> +    for ( unsigned int i = 0; i < nr_pages; i++ )
> +    {
> +        int rc = paging_ret_page_to_domheap(d);

Somewhat similarly here. Maybe simply insert "one" in the name?

Jan