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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.12.2025 17:42, Oleksii Kurochko wrote:
> 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?

This is all a little (or maybe even pretty) subtle. On x86 PGC_state_inuse is 0.
Together with logic guaranteeing that PGC_need_scrub would be clear by the time
a page is handed to the caller, what alloc_heap_pages() does is actually to
clear the field.

> Interestingly, in the shadow p2m page free code, there is logic that resets
> |count_info| to zero:

Yes, because iirc there used to be a strict requirement that pages need to be
handed back to the allocator the way they came out of it: With .count_info
clear. mark_page_free() documents / handles a couple of special cases now.

Jan