Re: [for 4.22 v5 05/18] xen/riscv: add root page table allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2025 11:53, Oleksii Kurochko wrote:
> On 11/6/25 3:25 PM, Jan Beulich wrote:
>> On 20.10.2025 17:57, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/p2m.c
>>> +++ b/xen/arch/riscv/p2m.c
>>> @@ -3,6 +3,7 @@
>>>   #include <xen/init.h>
>>>   #include <xen/lib.h>
>>>   #include <xen/macros.h>
>>> +#include <xen/domain_page.h>
>>>   #include <xen/mm.h>
>>>   #include <xen/paging.h>
>>>   #include <xen/rwlock.h>
>>> @@ -103,6 +104,70 @@ void __init pre_gstage_init(void)
>>>       vmid_init();
>>>   }
>>>   
>>> +static void clear_and_clean_page(struct page_info *page, bool clean_dcache)
>>> +{
>>> +    clear_domain_page(page_to_mfn(page));
>>> +
>>> +    /*
>>> +     * If the IOMMU doesn't support coherent walks and the p2m tables are
>>> +     * shared between the CPU and IOMMU, it is necessary to clean the
>>> +     * d-cache.
>>> +     */
>>> +    if ( clean_dcache )
>>> +        clean_dcache_va_range(page, PAGE_SIZE);
>> This cleans part of frame_table[], but not the memory page in question.
> 
> Oh, right, we need to map the domain page first.
> 
> Would it make sense to avoid using|clear_domain_page()| in order to prevent
> calling|map_domain_page()| twice (once inside|clear_domain_page()| and once
> before|clean_dcache_va_range()|), and instead do it like this:
>      void *p = __map_domain_page(page);
> 
>      clear_page(p);
> 
>      /*
>       * If the IOMMU doesn't support coherent walks and the p2m tables are
>       * shared between the CPU and IOMMU, it is necessary to clean the
>       * d-cache.
>       */
>      if ( clean_dcache )
>          clean_dcache_va_range(p, PAGE_SIZE);
> 
>      unmap_domain_page(p);

Certainly.

>>> @@ -55,6 +76,39 @@ int paging_freelist_adjust(struct domain *d, unsigned 
>>> long pages,
>>>       return 0;
>>>   }
>>>   
>>> +int paging_refill_from_domheap(struct domain *d, unsigned int nr_pages)
>>> +{
>>> +    ASSERT(spin_is_locked(&d->arch.paging.lock));
>>> +
>>> +    for ( unsigned int i = 0; i < nr_pages; i++ )
>>> +    {
>>> +        int rc = paging_add_page_to_freelist(d);
>> The anomaly is more pronounced here, with the other function name in context:
>> paging_refill_from_domheap() doesn't suggest there's a page (or several) 
>> being
>> handed to it. paging_add_page_to_freelist() suggests one of its parameter
>> would want to be struct page_info *. Within the naming model you chose, maybe
>> paging_refill_from_domheap_one() or paging_refill_one_from_domheap()? Or
>> simply _paging_refill_from_domheap()?
> 
> Thanks for suggestions. I like the option with "_*" as it is more clearly 
> marks it
> as an internal helper without introducing "_one" suffix. I will use the same 
> approach
> for paging_ret_page_to_domheap(): 
> s/paging_ret_page_to_domheap/_paging_ret_to_domheap().
> 
> Shouldn't we use "__*" instead of "_*" or "__*" is reserved for something 
> else? "__*" is
> used quite frequent in Xen code base.

And wrongly so. "__*" are reserved to the implementation (i.e. compiler / 
library).
Whereas "_*" (with the letter following the _ not being an upper-case one) is
dedicated to file scope identifiers. (That's mandated by the library part of the
spec, but imo we're well-advised to follow that, because even if we don't link 
to
any libraries, the compiler using certain symbols [e.g. __builtin_*()] is still
[potentially] getting in our way.)

Jan