Re: [PATCH v4 13/18] xen/riscv: implement p2m_next_level()

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]


On 9/22/25 7:35 PM, Jan Beulich wrote:
> On 17.09.2025 23:55, Oleksii Kurochko wrote:
>> Implement the p2m_next_level() function, which enables traversal and dynamic
>> allocation of intermediate levels (if necessary) in the RISC-V
>> p2m (physical-to-machine) page table hierarchy.
>>
>> To support this, the following helpers are introduced:
>> - page_to_p2m_table(): Constructs non-leaf PTEs pointing to next-level page
>>    tables with correct attributes.
>> - p2m_alloc_page(): Allocates page table pages, supporting both hardware and
>>    guest domains.
>> - p2m_create_table(): Allocates and initializes a new page table page and
>>    installs it into the hierarchy.
>>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
>> ---
>> Changes in V4:
>>   - make `page` argument of page_to_p2m_table pointer-to-const.
>>   - Move p2m_next_level()'s local variable `ret` to the more narrow space where
>>     it is really used.
>>   - Drop stale ASSERT() in p2m_next_level().
>>   - Stray blank after * in declaration of paging_alloc_page().
> When you deal with comments like this, can you please make sure you
> apply them to at least a patch as a whole, if not the entire series?
> I notice ...
>
>> --- a/xen/arch/riscv/include/asm/paging.h
>> +++ b/xen/arch/riscv/include/asm/paging.h
>> @@ -15,4 +15,6 @@ int paging_ret_pages_to_freelist(struct domain *d, unsigned int nr_pages);
>>   
>>   void paging_free_page(struct domain *d, struct page_info *pg);
>>   
>> +struct page_info * paging_alloc_page(struct domain *d);
> ... there's still a stray blank here. With this dropped:
> Acked-by: Jan Beulich<jbeulich@suse.com>

Thanks.

> I have one other question, though:
>
>> +/*
>> + * Allocate a new page table page with an extra metadata page and hook it
>> + * in via the given entry.
>> + */
>> +static int p2m_create_table(struct p2m_domain *p2m, pte_t *entry)
>> +{
>> +    struct page_info *page;
>> +
>> +    ASSERT(!pte_is_valid(*entry));
> Isn't this going to get in the way of splitting superpages? The caller
> will need to initialize *entry just for this assertion to not trigger.

The superpage splitting function doesn’t use|p2m_create_table()|. It calls
|p2m_alloc_table()|, then fills the table, and finally updates the entry
using|p2m_write_pte()|. So this shouldn’t be an issue.

Ohh, I just noticed, the comment should be updated, since an extra metadata
page is no longer allocated here.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 3698 bytes --]