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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>>>   /* 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.

Jan