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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/18/25 2:16 PM, Jan Beulich wrote:
> 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` ..."
By|p2m |I meant|ctx->p2m|, but I agree it would be better to refer to|ctx->p2m 
|explicitly or just update the comment to the following:
  * If `t` >= p2m_first_external, a valid `ctx` must be provided.
This looks clear enough to me now.

> >   {
> > -    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().
Agreed, the|if| condition isn’t needed.

If my understanding is correct, this would be different with|ASSERT()|, because
|ASSERT()| does nothing when|NDEBUG=y|, so an incorrect value of|ctx->level 
|could be missed, right?

> Also, isn't this a requirement independent of P2M type? In which case it should
> be moved out of the if()?
Right, it is independent and should be move 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?
I agree that, at least, the check should be dropped, and|BUG_ON()| can likely be
dropped as well.|ctx->level| isn’t really something|p2m_set_type()| should care
about, since the PTE is passed as an argument and is therefore expected to be
valid; consequently,|ctx->level| is expected to be valid too.

> > @@ -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.
Agree, the initializer looks better here, and the assignment could be omitted 
for now.

I am curious whether|p2m_get_type()| might one day need|p2m_pte_ctx.level|. If 
so, and
someone were to change|level| between the call to|p2m_get_type()| and the
initialization of|p2m_pte_ctx|, we could run into trouble. Perhaps it is safer 
to keep
the assignment or would it be enough during code review to check that if|level 
|changes
between the call to|p2m_get_type()| and the initialization 
of|p2m_pte_ctx|,|p2m_get_type() |still does not depend on|p2m_pte_ctx->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?
Sure, it would be better.

> > @@ -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?
Likely I've experimented with something and missed to clean up the code
after the experiments. I'll drop them.

Thanks.

~ Oleksii