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

[Jan Beulich <jbeulich@suse.com>]

On 17.09.2025 23:55, 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>
> @@ -95,6 +96,70 @@ void __init gstage_mode_detect(void)
>      local_hfence_gvma_all();
>  }
>  
> +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);
> +}
> +
> +unsigned long construct_hgatp(struct p2m_domain *p2m, uint16_t vmid)

pointer-to-const?

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

> @@ -55,6 +76,37 @@ int paging_freelist_adjust(struct domain *d, unsigned long pages,
>      return 0;
>  }
>  
> +int paging_ret_pages_to_freelist(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_ret_page_to_freelist(d);
> +        if ( rc )

Nit (style): Blank line between declaration(s) and statement(s) please.

> +            return rc;
> +    }
> +
> +    return 0;
> +}
> +
> +int paging_ret_pages_to_domheap(struct domain *d, unsigned int nr_pages)
> +{
> +    ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> +    if ( ACCESS_ONCE(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);
> +        if ( rc )

Same here.

Jan