Re: [for 4.22 v5 11/18] xen/riscv: Implement p2m_free_subtree() and related helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2025 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/p2m.h
> +++ b/xen/arch/riscv/include/asm/p2m.h
> @@ -110,6 +110,8 @@ typedef enum {
>      p2m_mmio_direct_io, /* Read/write mapping of genuine Device MMIO area,
>                             PTE_PBMT_IO will be used for such mappings */
>      p2m_ext_storage,    /* Following types'll be stored outsude PTE bits: */
> +    p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */
> +    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */
>  
>      /* Sentinel — not a real type, just a marker for comparison */
>      p2m_first_external = p2m_ext_storage,
> @@ -120,15 +122,28 @@ static inline p2m_type_t 
> arch_dt_passthrough_p2m_type(void)
>      return p2m_mmio_direct_io;
>  }
>  
> +/*
> + * Bits 8 and 9 are reserved for use by supervisor software;
> + * the implementation shall ignore this field.
> + * We are going to use to save in these bits frequently used types to avoid
> + * get/set of a type from radix tree.
> + */
> +#define P2M_TYPE_PTE_BITS_MASK  0x300

Better use PTE_RSW in place of the raw number?

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -17,6 +17,8 @@
>  #include <asm/riscv_encoding.h>
>  #include <asm/vmid.h>
>  
> +#define P2M_SUPPORTED_LEVEL_MAPPING 2

I fear without a comment it's left unclear what this is / represents.

> @@ -403,11 +415,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, which will trigger a
> +     * flush of the guest TLBs.
> +     */
> +    put_page(pg);
> +}
> +
> +/* Put any references on the single 4K page referenced by mfn. */

To me this and ...

> +static void p2m_put_4k_page(mfn_t mfn, p2m_type_t type)
> +{
> +    /* TODO: Handle other p2m types */
> +
> +    if ( p2m_is_foreign(type) )
> +    {
> +        ASSERT(mfn_valid(mfn));
> +        p2m_put_foreign_page(mfn_to_page(mfn));
> +    }
> +}
> +
> +/* Put any references on the superpage referenced by mfn. */

... to a lesser degree this comment are potentially misleading. Down here at
least there is something plural-ish (the 4k pages that the 2M one consists
of), but especially for the single page case above "any" could easily mean
"anything that's still outstanding, anywhere". I'm also not quite sure "on"
is really what you mean (I'm not a native speaker, so my gut feeling may be
wrong here).

> +static void p2m_put_2m_superpage(mfn_t mfn, p2m_type_t type)
> +{
> +    struct page_info *pg;
> +    unsigned int i;
> +
> +    /*
> +     * TODO: Handle other p2m types, but be aware that any changes to handle
> +     * different types should require an update on the relinquish code to
> +     * handle preemption.
> +     */

I guess if I was to address this TODO, I wouldn't know what the latter part
of the sentence is warning me of.

> +    if ( !p2m_is_foreign(type) )
> +        return;

Are super-page foreign mappings actually intended to be permitted, conceptually?

>  /* Free pte sub-tree behind an entry */
>  static void p2m_free_subtree(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;
> +
> +    /*
> +     * Check if the level is valid: only 4K - 2M - 1G mappings are supported.
> +     * To support levels > 2, the implementation of p2m_free_subtree() would
> +     * need to be updated, as the current recursive approach could consume
> +     * excessive time and memory.
> +     */
> +    ASSERT(level <= P2M_SUPPORTED_LEVEL_MAPPING);
> +
> +    /* Nothing to do if the entry is invalid. */
> +    if ( !pte_is_valid(entry) )
> +        return;
> +
> +    if ( (level == 0) || pte_is_superpage(entry, level) )

Considering what pte_is_superpage() expands to, simply pte_is_mapping()?

> +    {
> +        p2m_type_t p2mt = p2m_get_type(entry);
> +
> +#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(p2mt) &&
> +             domain_has_ioreq_server(p2m->domain) )
> +            ioreq_request_mapcache_invalidate(p2m->domain);
> +#endif
> +
> +        p2m_put_page(entry, level, p2mt);
> +
> +        return;
> +    }
> +
> +    table = map_domain_page(pte_get_mfn(entry));
> +    for ( i = 0; i < P2M_PAGETABLE_ENTRIES(level); i++ )
> +        p2m_free_subtree(p2m, table[i], level - 1);
> +
> +    unmap_domain_page(table);

Please can the use of blank lines in such cases be symmetric: Either have them
ahead of and after the loop, or have them nowhere?

> @@ -435,7 +583,7 @@ static int p2m_set_entry(struct p2m_domain *p2m,
>       * Check if the level target is valid: we only support
>       * 4K - 2M - 1G mapping.
>       */
> -    ASSERT(target <= 2);
> +    ASSERT(target <= P2M_SUPPORTED_LEVEL_MAPPING);

Ah, this is where that constant comes into play. It wants moving to the earlier
patch, and with this being the purpose I guess it also wants to include MAX in
its name.

Jan