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

[Oleksii Kurochko]

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