Re: [PATCH v8 2/3] xen/riscv: introduce metadata table to store P2M type

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/29/25 4:13 PM, Jan Beulich wrote:
> On 22.12.2025 17:37, Oleksii Kurochko wrote:
> > @@ -370,24 +396,96 @@ 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` >= p2m_first_external, a valid `ctx` must be provided.
> > + */
> > +static void p2m_set_type(pte_t *pte, p2m_type_t t,
> > +                         const struct p2m_pte_ctx *ctx)
> >   {
> > -    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) )
> > +    {
> > +        /*
> > +         * Since p2m_alloc_page() initializes an allocated page with
> > +         * zeros, p2m_invalid is expected to have the value 0 as well.
> > +         */
> > +        BUILD_BUG_ON(p2m_invalid);
> > +
> > +        ASSERT(ctx->p2m);
> I think I previously asked for this to be moved out of the if(). Else you
> may not notice a caller side issue until a point where a metadata page
> actually needs allocating. (This could simply be folded into the earlier
> ASSERT().)
I think that I understand your intention and okay to fold ASSERT(ctx->p2m)
into the earlier ASSERT().
Just want to note that generally if the metadata page has been already
allocated and then p2m_set_type() will be called with ctx->p2m == NULL then
nothing serious will happen as basically ctx->p2m is needed in this function
only for allocation of metadata page.

~ Oleksii