Re: [PATCH v4 11/18] xen/riscv: Implement p2m_free_subtree() and related helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2025 23:55, Oleksii Kurochko wrote:
> @@ -342,11 +354,147 @@ static int p2m_next_level(struct p2m_domain *p2m, bool 
> alloc_tbl,
>      return P2M_TABLE_MAP_NONE;
>  }
>  
> +static void p2m_put_foreign_page(struct page_info *pg)
> +{
> +    /*
> +     * It’s safe to call put_page() here because arch_flush_tlb_mask()
> +     * will be invoked if the page is reallocated before the end of
> +     * this loop, which will trigger a flush of the guest TLBs.
> +     */
> +    put_page(pg);
> +}

What is "this loop" referring to in the comment? There's no loop here.

> +/* Put any references on the page referenced by pte. */
> +static void p2m_put_page(const pte_t pte, unsigned int level, p2m_type_t 
> p2mt)
> +{
> +    mfn_t mfn = pte_get_mfn(pte);
> +
> +    ASSERT(pte_is_valid(pte));
> +
> +    /*
> +     * TODO: Currently we don't handle level 2 super-page, Xen is not
> +     * preemptible and therefore some work is needed to handle such
> +     * superpages, for which at some point Xen might end up freeing memory
> +     * and therefore for such a big mapping it could end up in a very long
> +     * operation.
> +     */
> +    switch ( level )
> +    {
> +    case 1:
> +        return p2m_put_2m_superpage(mfn, p2mt);
> +
> +    case 0:
> +        return p2m_put_4k_page(mfn, p2mt);
> +
> +    default:
> +        assert_failed("Unsupported level");

I don't think assert_failed() is supposed to be used directly. What's
wrong with using ASSERT_UNREACHABLE() here?

> --- a/xen/arch/riscv/paging.c
> +++ b/xen/arch/riscv/paging.c
> @@ -107,6 +107,14 @@ int paging_ret_pages_to_domheap(struct domain *d, 
> unsigned int nr_pages)
>      return 0;
>  }
>  
> +void paging_free_page(struct domain *d, struct page_info *pg)
> +{
> +    spin_lock(&d->arch.paging.lock);
> +    page_list_add_tail(pg, &d->arch.paging.freelist);
> +    ACCESS_ONCE(d->arch.paging.total_pages)++;

More a question to other REST maintainers than to you: Is this kind of
use of ACCESS_ONCE() okay? By the wording, one might assume a single
memory access, yet only x86 can actually carry out an increment (or
alike) of an item in memory in a single insn.

Jan