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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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?

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.

Jan