Re: [PATCH v2 13/17] xen/riscv: Implement p2m_entry_from_mfn() and support PBMT configuration

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2025 10:52, Oleksii Kurochko wrote:
> On 7/23/25 11:46 AM, Jan Beulich wrote:
>>> I assume that I have in this case to take some pages for an intermediate 
>>> page
>>> table from freelist P2M pool, set an owner domain to NULL (pg->inuse.domain 
>>> = NULL).
>>>
>>> Then in this case it isn't clear why pg->list can't be re-used to link 
>>> several pages
>>> for intermediate page table purposes + metadata? Is it because pg->list can 
>>> be not
>>> empty? In this case it isn't clear if I could use a page, which has 
>>> threaded pages.
>> Actually looks like I was mis-remembering. Pages removed from freelist indeed
>> aren't put on any other list, so the linking fields are available for use. I
>> guess I had x86 shadow code in mind, where the linking fields are further 
>> used.
> 
> Perhaps, I misunderstood you about "linking fields", but it seems like I 
> can't reuse
> struct page_info->list as it is used by page_list_add() which is called by 
> p2m_alloc_page()
> to allocate page(s) for an intermediate page table:
>     static inline void
>     page_list_add(struct page_info *page, struct page_list_head *head)
>     {
>          list_add(&page->list, head);
>     }
> 
>      struct page_info * paging_alloc_page(struct domain *d)
>      {
>          struct page_info *pg;
> 
>          spin_lock(&d->arch.paging.lock);
>          pg = page_list_remove_head(&d->arch.paging.freelist);
>          spin_unlock(&d->arch.paging.lock);
> 
>          INIT_LIST_HEAD(&pg->list);
> 
>          return pg;
>      }
> 
>      static struct page_info *p2m_alloc_page(struct domain *d)
>      {
>          struct page_info *pg = paging_alloc_page(d);
> 
>          if ( pg )
>              page_list_add(pg, &p2m_get_hostp2m(d)->pages);
> 
>          return pg;
>      }
> 
> So I have to reuse another field from struct page_info. It seems like it 
> won't be an
> issue if to add a new struct page_list_entry metadata_list to 'union v':
>      union {
>          /* Page is in use */
>          struct {
>              /* Owner of this page (NULL if page is anonymous). */
>              struct domain *domain;
>          } inuse;
> 
>          /* Page is on a free list. */
>          struct {
>              /* Order-size of the free chunk this page is the head of. */
>              unsigned int order;
>          } free;
> +
> +       struct page_list_entry metadata_list;
>      } v;
> 
> Am I missing something?

Well, you're doubling the size of that union then, aren't you? As was mentioned
quite some time ago, struct page_info needs quite a bit of care when you mean
to add new fields there. Question is whether for the purpose here you actually
need a doubly-linked list. A single pointer would be fine to put there.

Jan