Re: [PATCH v2 12/17] xen/riscv: Implement p2m_free_entry() and related helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2025 15:05, Oleksii Kurochko wrote:
> This patch introduces a working implementation of p2m_free_entry() for RISC-V
> based on ARM's implementation of p2m_free_entry(), enabling proper cleanup
> of page table entries in the P2M (physical-to-machine) mapping.
> 
> Only few things are changed:
> - Use p2m_force_flush_sync() instead of p2m_tlb_flush_sync() as latter
>   isn't implemented on RISC-V.
> - Introduce and use p2m_type_radix_get() to get a type of p2m entry as
>   RISC-V's PTE doesn't have enough space to store all necessary types so
>   a type is stored in a radix tree.
> 
> Key additions include:
> - p2m_free_entry(): Recursively frees page table entries at all levels. It
>   handles both regular and superpage mappings and ensures that TLB entries
>   are flushed before freeing intermediate tables.
> - p2m_put_page() and helpers:
>   - p2m_put_4k_page(): Clears GFN from xenheap pages if applicable.
>   - p2m_put_2m_superpage(): Releases foreign page references in a 2MB
>     superpage.
>   - p2m_type_radix_get(): Extracts the stored p2m_type from the radix tree
>     using the PTE.
> - p2m_free_page(): Returns a page either to the domain's freelist or to
>   the domheap, depending on whether the domain is hardware-backed.

What is "hardware-backed"?

> Defines XEN_PT_ENTRIES in asm/page.h to simplify loops over page table
> entries.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V2:
>  - New patch. It was a part of a big patch "xen/riscv: implement p2m mapping
>    functionality" which was splitted to smaller.
>  - s/p2m_is_superpage/p2me_is_superpage.

See my earlier comments regarding naming.

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -345,11 +345,33 @@ static pte_t *p2m_get_root_pointer(struct p2m_domain 
> *p2m, gfn_t gfn)
>      return __map_domain_page(p2m->root + root_table_indx);
>  }
>  
> +static p2m_type_t p2m_type_radix_get(struct p2m_domain *p2m, pte_t pte)

Does it matter to callers that ...

> +{
> +    void *ptr;
> +    gfn_t gfn = mfn_to_gfn(p2m->domain, mfn_from_pte(pte));
> +
> +    ptr = radix_tree_lookup(&p2m->p2m_type, gfn_x(gfn));
> +
> +    if ( !ptr )
> +        return p2m_invalid;
> +
> +    return radix_tree_ptr_to_int(ptr);
> +}

... this is a radix tree lookup? IOW does "radix" need to be part of the
function name? Also "get" may want to move forward in the name, to better
match the naming of other functions.

> +/*
> + * In the case of the P2M, the valid bit is used for other purpose. Use
> + * the type to check whether an entry is valid.
> + */
>  static inline bool p2me_is_valid(struct p2m_domain *p2m, pte_t pte)
>  {
> -    panic("%s: isn't implemented for now\n", __func__);
> +    return p2m_type_radix_get(p2m, pte) != p2m_invalid;
> +}

No checking of the valid bit?

> -    return false;
> +static inline bool p2me_is_superpage(struct p2m_domain *p2m, pte_t pte,
> +                                    unsigned int level)
> +{
> +    return p2me_is_valid(p2m, pte) && (pte.pte & PTE_ACCESS_MASK) &&
> +           (level > 0);

In such combinations of conditions it's usually helpful to put the
cheapest check(s) first. IOW what point is there in doing a radix
tree lookup when the other two conditions aren't satisfied? (FTAOD
applies elsewhere as well, even within this same patch.)

> @@ -404,11 +426,127 @@ static int p2m_next_level(struct p2m_domain *p2m, bool 
> alloc_tbl,
>      return GUEST_TABLE_MAP_NONE;
>  }
>  
> +static void p2m_put_foreign_page(struct page_info *pg)
> +{
> +    /*
> +     * It's safe to do the put_page here because page_alloc will
> +     * flush the TLBs if the page is reallocated before the end of
> +     * this loop.
> +     */
> +    put_page(pg);

Is the comment really true? The page allocator will flush the normal
TLBs, but not the stage-2 ones. Yet those are what you care about here,
aiui.

> +/* Put any references on the single 4K page referenced by mfn. */
> +static void p2m_put_4k_page(mfn_t mfn, p2m_type_t type)
> +{
> +    /* TODO: Handle other p2m types */
> +
> +    /* Detect the xenheap page and mark the stored GFN as invalid. */
> +    if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
> +        page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);

Is this a valid thing to do? How do you make sure the respective uses
(in gnttab's shared and status page arrays) are / were also removed?

> +}
> +
> +/* Put any references on the superpage referenced by mfn. */
> +static void p2m_put_2m_superpage(mfn_t mfn, p2m_type_t type)
> +{
> +    struct page_info *pg;
> +    unsigned int i;
> +
> +    ASSERT(mfn_valid(mfn));
> +
> +    pg = mfn_to_page(mfn);
> +
> +    for ( i = 0; i < XEN_PT_ENTRIES; i++, pg++ )
> +        p2m_put_foreign_page(pg);
> +}
> +
> +/* Put any references on the page referenced by pte. */
> +static void p2m_put_page(struct p2m_domain *p2m, const pte_t pte,
> +                         unsigned int level)
> +{
> +    mfn_t mfn = pte_get_mfn(pte);
> +    p2m_type_t p2m_type = p2m_type_radix_get(p2m, pte);

This gives you the type of the 1st page. What guarantees that all other pages
in a superpage are of the exact same type?

> +    ASSERT(p2me_is_valid(p2m, 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.
> +     */

This is pretty unsatisfactory. Imo, if you don't deal with that right away,
you're setting yourself up for a significant re-write.

> +    if ( level == 1 )
> +        return p2m_put_2m_superpage(mfn, p2m_type);
> +    else if ( level == 0 )
> +        return p2m_put_4k_page(mfn, p2m_type);

Use switch() right away?

> +}
> +
> +static void p2m_free_page(struct domain *d, struct page_info *pg)
> +{
> +    if ( is_hardware_domain(d) )
> +        free_domheap_page(pg);

Why's the hardware domain different here? It should have a pool just like
all other domains have.

> +    else
> +    {
> +        spin_lock(&d->arch.paging.lock);
> +        page_list_add_tail(pg, &d->arch.paging.p2m_freelist);
> +        spin_unlock(&d->arch.paging.lock);
> +    }
> +}
> +
>  /* Free pte sub-tree behind an entry */
>  static void p2m_free_entry(struct p2m_domain *p2m,
>                             pte_t entry, unsigned int level)
>  {
> -    panic("%s: hasn't been implemented yet\n", __func__);
> +    unsigned int i;
> +    pte_t *table;
> +    mfn_t mfn;
> +    struct page_info *pg;
> +
> +    /* Nothing to do if the entry is invalid. */
> +    if ( !p2me_is_valid(p2m, entry) )
> +        return;

Does this actually apply to intermediate page tables (which you handle
later in the function), when that's (only) a P2M type check?

> +    if ( p2me_is_superpage(p2m, entry, level) || (level == 0) )
> +    {
> +#ifdef CONFIG_IOREQ_SERVER
> +        /*
> +         * If this gets called then either the entry was replaced by an entry
> +         * with a different base (valid case) or the shattering of a 
> superpage
> +         * has failed (error case).
> +         * So, at worst, the spurious mapcache invalidation might be sent.
> +         */
> +        if ( p2m_is_ram( p2m_type_radix_get(p2m, entry)) &&

Nit: Style.

> +             domain_has_ioreq_server(p2m->domain) )
> +            ioreq_request_mapcache_invalidate(p2m->domain);
> +#endif
> +
> +        p2m_put_page(p2m, entry, level);
> +
> +        return;
> +    }
> +
> +    table = map_domain_page(pte_get_mfn(entry));
> +    for ( i = 0; i < XEN_PT_ENTRIES; i++ )
> +        p2m_free_entry(p2m, *(table + i), level - 1);

Better table[i]?

Jan