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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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);

> > @@ -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.

~ Oleksii