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.
> --- a/xen/arch/riscv/paging.c
> +++ b/xen/arch/riscv/paging.c
> @@ -4,46 +4,67 @@
> #include <xen/sched.h>
> #include <xen/spinlock.h>
>
> +static int paging_ret_page_to_domheap(struct domain *d)
> +{
> + struct page_info *page;
> +
> + ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> + /* Return memory to domheap. */
> + page = page_list_remove_head(&d->arch.paging.freelist);
> + if( page )
> + {
> + d->arch.paging.total_pages--;
> + free_domheap_page(page);
> + }
> + else
> + {
> + printk(XENLOG_ERR
> + "Failed to free P2M pages, P2M freelist is empty.\n");
Nit: See earlier remark regarding full stops in log messages. The double
"P2M" also looks unnecessary to me.
> +static int paging_add_page_to_freelist(struct domain *d)
> +{
> + struct page_info *page;
> +
> + ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> + /* Need to allocate more memory from domheap */
> + page = alloc_domheap_page(d, MEMF_no_owner);
> + if ( page == NULL )
> + {
> + printk(XENLOG_ERR "Failed to allocate pages.\n");
Again. (Also log messages typically wouldn't start with a capital letter,
unless of course it's e.g. an acronym.)
> @@ -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()?
> + if ( rc )
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +int paging_ret_to_domheap(struct domain *d, unsigned int nr_pages)
> +{
> + ASSERT(spin_is_locked(&d->arch.paging.lock));
> +
> + if ( d->arch.paging.total_pages < nr_pages )
> + return false;
> +
> + for ( unsigned int i = 0; i < nr_pages; i++ )
> + {
> + int rc = paging_ret_page_to_domheap(d);
Somewhat similarly here. Maybe simply insert "one" in the name?
Jan
