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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/11/25 10:39 AM, Jan Beulich wrote:
> On 10.12.2025 13:44, Oleksii Kurochko wrote:
> > On 12/10/25 8:06 AM, Jan Beulich wrote:
> > > On 09.12.2025 18:09, Oleksii Kurochko wrote:
> > > > On 12/9/25 2:47 PM, Jan Beulich wrote:
> > > > > On 24.11.2025 13:33, Oleksii Kurochko wrote:
> > > > > > +            *md_pg = p2m_alloc_page(p2m);
> > > > > > +            if ( !*md_pg )
> > > > > > +            {
> > > > > > +                printk("%pd: can't allocate metadata page\n", p2m->domain);
> > > > > > +                domain_crash(p2m->domain);
> > > > > > +
> > > > > > +                return;
> > > > > > +            }
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    if ( *md_pg )
> > > > > > +        metadata = __map_domain_page(*md_pg);
> > > > > > +
> > > > > > +    if ( t >= p2m_first_external )
> > > > > > +    {
> > > > > > +        metadata[ctx->index].type = t;
> > > > > > +
> > > > > > +        t = p2m_ext_storage;
> > > > > > +    }
> > > > > > +    else if ( metadata )
> > > > > > +        metadata[ctx->index].type = p2m_invalid;
> > > > > > +
> > > > > > +    pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
> > > > > > +
> > > > > > +    unmap_domain_page(metadata);
> > > > > >     }
> > > > > Just to mention (towards future work): Once a metadata page goes back to be
> > > > > entirely zero-filled, it could as well be hooked off and returned to the pool.
> > > > > Not doing so may mean detaining an unused page indefinitely.
> > > > Won’t that already happen when p2m_free_table() is called?
> > > Well, that's when both page table and metadata table are freed. But what if a
> > > leaf page table is moving back to holding all p2m_ram_rw mappings? Then the
> > > metadata page is unused, but will remain allocated.
> > Good point...
> >
> > This could be a rather expensive operation, since in the code:
> >     +    else if ( metadata )
> >     +        metadata[ctx->index].type = p2m_invalid;
> > we would have to check all other metadata entries to determine whether they are
> > (p2m_invalid) or not, and return the page to the pool.
> >
> > It would be nice to have something like metadata.used_entries_num, but the 
> > entire
> > page is used for type entries.
> > As an option, we could reserve 8 bits to store a counter of the number of used
> > entries in the metadata page, and then use metadata[0].used_entries_num to check
> > whether it is zero. If it is zero, we could simply return the metadata page to 
> > the
> > pool in the “else if (metadata)” case mentioned above.
> >
> > How bad is this idea? Any better suggestions?
> First, as said in my initial reply: This may not need taking care of right away.
> It will need keeping in mind, of course.
I am thinking if it won't be too intrusive, I think that I am okay to introduce 
that
now.

> As to suggestions - hardly any of the fields in struct page_info for the page
> can be used when the page is a metadata one. Simply record the count there?
I suppose that|union u| could be used.
The only thing that confuses me is the shadow paging implementation on x86.
In|struct page_info|, it has the following:
    /* Context-dependent fields follow... */
    union {

        /* Page is in use: ((count_info & PGC_count_mask) != 0). */
        struct {
            /* Type reference count and various PGT_xxx flags and fields. */
            unsigned long type_info;
        } inuse;

        /* Page is in use as a shadow: count_info == 0. */
        struct {
           ....
        } sh;

        /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
        union {

So it seems that something in the shadow code must set|count_info| to zero for
shadow pages. But I cannot find where this happens. I would expect it to be done
in|shadow_alloc()|, when the page is taken from the free list. However, pages
from the free list donot have|count_info == 0| since|alloc_heap_pages() 
|initializes|count_info|.
What guarantees that|count_info| will be zero for shadow tables?

Interestingly, in the shadow p2m page free code, there is logic that resets
|count_info| to zero:
{
    struct domain *owner = page_get_owner(pg);

    /* Should still have no owner and count zero. */
    if ( owner || (pg->count_info & PGC_count_mask) )
    {
        printk(XENLOG_ERR
               "d%d: Odd p2m page %"PRI_mfn" d=%d c=%lx t=%"PRtype_info"\n",
               d->domain_id, mfn_x(page_to_mfn(pg)),
               owner ? owner->domain_id : DOMID_INVALID,
               pg->count_info, pg->u.inuse.type_info);
        pg->count_info &= ~PGC_count_mask;
        page_set_owner(pg, NULL);
    }
...

And another question: since|u.sh.*| is updated, it effectively overwrites
|u.inuse.type_info|. But|u.inuse.type_info| is used by|free_domheap_pages()|,
which is called from|shadow_free()|:
void free_domheap_pages(struct page_info *pg, unsigned int order)
{
...
                if ( pg[i].u.inuse.type_info & PGT_count_mask )
                {
                    printk(XENLOG_ERR
                           "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
                           i, mfn_x(page_to_mfn(pg + i)),
                           pg[i].count_info, pg[i].v.free.order,
                           pg[i].u.free.val, pg[i].tlbflush_timestamp);
                    BUG();
                }
...

Why is it acceptable that|u.inuse.type_info| will likely not be zero, since
|u.sh.*| modifies the same union field, at least during shadow page allocation?



> Finally, as to "rather expensive": Scanning a 4k page to hold all zeroes can't
> be all that expensive? In any event that expensiveness needs weighing carefully
> against the risk of getting the counter maintenance wrong.
It depends on how often|p2m_set_type()| will be called.
In the worst case, it will require a loop that performs up to 512 iterations
to scan a 4 KB page (512 * sizeof(struct mt_d) = 4096), which I think could be
expensive if|p2m_set_type()| is invoked frequently.

~ Oleksii