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
