Re: [PATCH v7 18/19] xen/riscv: introduce metadata table to store P2M type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2025 17:55, Oleksii Kurochko wrote:
> @@ -370,24 +396,101 @@ static struct page_info *p2m_alloc_page(struct 
> p2m_domain *p2m)
>      return pg;
>  }
>  
> -static int p2m_set_type(pte_t *pte, p2m_type_t t)
> +/*
> + * `pte` – PTE entry for which the type `t` will be stored.
> + *
> + * If `t` is `p2m_ext_storage`, both `ctx` and `p2m` must be provided.

Stale comment? There's no ...

> + */
> +static void p2m_set_type(pte_t *pte, p2m_type_t t,
> +                         const struct p2m_pte_ctx *ctx)

... "p2m" among the parameters anymore. Furthermore, would any caller pass in
p2m_ext_storage? Judging from the code you may mean "If `t` is greater or
equal to `p2m_first_external` ..."

>  {
> -    int rc = 0;
> +    struct page_info **md_pg;
> +    struct md_t *metadata = NULL;
>  
> -    if ( t > p2m_first_external )
> -        panic("unimplemeted\n");
> -    else
> -        pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
> +    /*
> +     * It is sufficient to compare ctx->index with PAGETABLE_ENTRIES because,
> +     * even for the p2m root page table (which is a 16 KB page allocated as
> +     * four 4 KB pages), calc_offset() guarantees that the page-table index
> +     * will always fall within the range [0, 511].
> +     */
> +    ASSERT(ctx && ctx->index < PAGETABLE_ENTRIES);
>  
> -    return rc;
> +    /*
> +     * At the moment, p2m_get_root_pointer() returns one of four possible p2m
> +     * root pages, so there is no need to search for the correct ->pt_page
> +     * here.
> +     * Non-root page tables are 4 KB pages, so simply using ->pt_page is
> +     * sufficient.
> +     */
> +    md_pg = &ctx->pt_page->v.md.pg;
> +
> +    if ( !*md_pg && (t >= p2m_first_external) )
> +    {
> +        BUG_ON(ctx->level > P2M_MAX_SUPPORTED_LEVEL_MAPPING);

With this, ...

> +        if ( ctx->level <= P2M_MAX_SUPPORTED_LEVEL_MAPPING )

... this isn't needed (dead code). Things would be different with ASSERT().

Also, isn't this a requirement independent of P2M type? In which case it should
be moved out of the if()? Yet then, further code in the function (including in
the body of this if()) doesn't look to be using ->level. Then why the check?

> @@ -477,7 +580,14 @@ static void p2m_set_permission(pte_t *e, p2m_type_t t)
>      }
>  }
>  
> -static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t, bool is_table)
> +/*
> + * If p2m_pte_from_mfn() is called with p2m_pte_ctx = NULL,
> + * it means the function is working with a page table for which the `t`
> + * should not be applicable. Otherwise, the function is handling a leaf PTE
> + * for which `t` is applicable.
> + */
> +static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t,
> +                              struct p2m_pte_ctx *p2m_pte_ctx)

Name the parameter just "ctx", as you have it elsewhere?

> @@ -679,12 +804,14 @@ static void p2m_free_page(struct p2m_domain *p2m, 
> struct page_info *pg)
>  
>  /* Free pte sub-tree behind an entry */
>  static void p2m_free_subtree(struct p2m_domain *p2m,
> -                             pte_t entry, unsigned int level)
> +                             pte_t entry,
> +                             const struct p2m_pte_ctx *p2m_pte_ctx)

Same question here then.

> @@ -756,6 +891,10 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, 
> pte_t *entry,
>      unsigned int next_level = level - 1;
>      unsigned int level_order = P2M_LEVEL_ORDER(next_level);
>  
> +    struct p2m_pte_ctx p2m_pte_ctx;
> +    /* Init with p2m_invalid just to make compiler happy. */
> +    p2m_type_t old_type = p2m_invalid;
> +
>      /*
>       * This should only be called with target != level and the entry is
>       * a superpage.
> @@ -777,6 +916,24 @@ static bool p2m_split_superpage(struct p2m_domain *p2m, 
> pte_t *entry,
>  
>      table = __map_domain_page(page);
>  
> +    p2m_pte_ctx.p2m = p2m;

To play safe and have all struct fields initialized (all others implicitly),
better make this the initializer of the variable? Then you could shorten ...

> +    if ( MASK_EXTR(entry->pte, P2M_TYPE_PTE_BITS_MASK) == p2m_ext_storage )
> +    {
> +        p2m_pte_ctx.pt_page = tbl_pg;
> +        p2m_pte_ctx.index = offsets[level];
> +        /*
> +         * It doesn't really matter what is a value for a level as
> +         * p2m_get_type() doesn't need it, so it is initialized just in case.
> +         */
> +        p2m_pte_ctx.level = level;

... the comment here and really omit the assignment of .level.

> @@ -840,6 +1004,7 @@ static int p2m_set_entry(struct p2m_domain *p2m,
>       * are still allowed.
>       */
>      bool removing_mapping = mfn_eq(mfn, INVALID_MFN);
> +    struct p2m_pte_ctx tmp_ctx;
>      P2M_BUILD_LEVEL_OFFSETS(p2m, offsets, gfn_to_gaddr(gfn));
>  
>      ASSERT(p2m_is_write_locked(p2m));
> @@ -882,6 +1047,8 @@ static int p2m_set_entry(struct p2m_domain *p2m,
>  
>      entry = table + offsets[level];
>  
> +    tmp_ctx.p2m = p2m;

Again better make this the variable's initializer?

> @@ -970,7 +1147,9 @@ static int p2m_set_entry(struct p2m_domain *p2m,
>      if ( pte_is_valid(orig_pte) &&
>           (!pte_is_valid(*entry) ||
>            !mfn_eq(pte_get_mfn(*entry), pte_get_mfn(orig_pte))) )
> -        p2m_free_subtree(p2m, orig_pte, level);
> +    {
> +        p2m_free_subtree(p2m, orig_pte, &tmp_ctx);
> +    }

Why braces all of the sudden?

Jan