Re: [PATCH v3 13/20] xen/riscv: Implement p2m_free_subtree() and related helpers

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/14/25 5:17 PM, Jan Beulich wrote:

> On 14.08.2025 17:09, Oleksii Kurochko wrote:
> > On 8/6/25 5:55 PM, Jan Beulich wrote:
> > > On 31.07.2025 17:58, Oleksii Kurochko wrote:
> > > > +/* Put any references on the page referenced by pte. */
> > > > +static void p2m_put_page(const pte_t pte, unsigned int level)
> > > > +{
> > > > +    mfn_t mfn = pte_get_mfn(pte);
> > > > +    p2m_type_t p2m_type = p2m_get_type(pte);
> > > > +
> > > > +    ASSERT(pte_is_valid(pte));
> > > > +
> > > > +    /*
> > > > +     * TODO: Currently we don't handle level 2 super-page, Xen is not
> > > > +     * preemptible and therefore some work is needed to handle such
> > > > +     * superpages, for which at some point Xen might end up freeing memory
> > > > +     * and therefore for such a big mapping it could end up in a very long
> > > > +     * operation.
> > > > +     */
> > > > +    switch ( level )
> > > > +    {
> > > > +    case 1:
> > > > +        return p2m_put_2m_superpage(mfn, p2m_type);
> > > > +
> > > > +    case 0:
> > > > +        return p2m_put_4k_page(mfn, p2m_type);
> > > > +    }
> > > >           
> > > Yet despite the comment not even an assertion for level 2 and up?
> > >         
> > Not sure that an ASSERT() is needed here as a reference(s) for such page(s)
> > will be put during domain_relinquish_resources() as there we could do preemption.
> > Something like Arm does here:
> >    https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/arm/mmu/p2m.c?ref_type=heads#L1587
> >
> > I'm thinking that probably it makes sense to put only 4k page(s) and
> > all other cases postpone until domain_relinquish_resources() is called.
> >       
> How can you defer to domain cleanup? How would handling of foreign mappings
> (or e.g. ballooning? not sure) work when you don't drop references as
> necessary?
>     
I was confused by the code in relinquish_p2m_mapping(), since it removes
foreign mappings from the P2M. My current understanding is that it is called
for foreign mappings that weren’t explicitly unmapped, in order to drop the
page reference taken when the mapping was created. Initially, I thought it
would be enough to just perform the (un)map in the P2M page tables to have
foreign mapping working, but that could result in a page never being fully
released, which would in turn break or confuse other logic.

So, yes, I agree that your initial suggestion to add ASSERT() is useful to
be sure that no one is using level 2 super-pages for foreign mapping.

> > > >   /* 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;
> > > > +
> > > > +    /* Nothing to do if the entry is invalid. */
> > > > +    if ( !pte_is_valid(entry) )
> > > > +        return;
> > > > +
> > > > +    if ( pte_is_superpage(entry, level) || (level == 0) )
> > > >           
> > > Perhaps swap the two conditions around?
> > >
> > > > +    {
> > > > +#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(p2m_get_type(p2m, entry)) &&
> > > > +             domain_has_ioreq_server(p2m->domain) )
> > > > +            ioreq_request_mapcache_invalidate(p2m->domain);
> > > > +#endif
> > > > +
> > > > +        p2m_put_page(entry, level);
> > > > +
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    table = map_domain_page(pte_get_mfn(entry));
> > > > +    for ( i = 0; i < XEN_PT_ENTRIES; i++ )
> > > > +        p2m_free_subtree(p2m, table[i], level - 1);
> > > >           
> > > In p2m_put_page() you comment towards concerns for level >= 2; no similar
> > > concerns for the resulting recursion here?
> > >         
> > This function is generic enough to handle any level.
> >
> > Except that it is possible that it will be needed, for example, to split 1G mapping
> > into something smaller then p2m_free_subtree() could be called for freeing a subtree
> > of 1gb mapping.
> >       
> The question wasn't about it being generic enough, but it possibly taking
> too much time for level >= 2.
>     
In this terms it makes sense to add such an assertion which will check that we are
working with levels <= 2.

Thanks.
~ Oleksii