On 18.04.2022 14:22, Penny Zheng wrote:
> Pages as guest RAM for static domain, shall be reserved to this domain only.
Is there "used" missing as the 2nd word of the sentence?
> So in case reserved pages being used for other purpose, users
> shall not free them back to heap, even when last ref gets dropped.
>
> free_staticmem_pages will be called by free_domheap_pages in runtime
> for static domain freeing memory resource, so let's drop the __init
> flag.
>
> Signed-off-by: Penny Zheng <penny.zh...@arm.com>
> ---
> v2 changes:
> - new commit
> ---
> xen/common/page_alloc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
With this diffstat the patch subject prefix is somewhat misleading;
I first thought I could skip this patch.
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2488,7 +2488,13 @@ void free_domheap_pages(struct page_info *pg, unsigned
> int order)
> scrub = 1;
> }
>
> - free_heap_pages(pg, order, scrub);
> +#ifdef CONFIG_STATIC_MEMORY
> + if ( pg->count_info & PGC_reserved )
> + /* Reserved page shall not go back to the heap. */
> + free_staticmem_pages(pg, 1 << order, scrub);
1UL with, in particular, the function parameter by "unsigned long".
By calling free_staticmem_pages() at runtime, you make the previous race
free (because of init-time only) update of .count_info there racy. Making
a clone of that function just for this difference would likely be
excessive, so I'd suggest to change the code there to
/* In case initializing page of static memory, mark it PGC_reserved. */
if ( !(pg[i].count_info & PGC_reserved) )
pg[i].count_info |= PGC_reserved;
> + else
> +#endif
> + free_heap_pages(pg, order, scrub);
Of course it would be nice to avoid the #ifdef-ary here. May I ask
that you introduce a stub free_staticmem_pages() for the
!CONFIG_STATIC_MEMORY case, such that the construct can become
if ( !(pg->count_info & PGC_reserved) )
free_heap_pages(pg, order, scrub);
else
/* Reserved page shall not go back to the heap. */
free_staticmem_pages(pg, 1 << order, scrub);
Another question is whether the distinction should be made here in
the first place. Would it perhaps better belong in free_heap_pages()
itself, thus also covering other potential call sites? Of course
this depends on where, long term, reserved pages can / will be used.
For domains to be truly static, Xen's own allocations to manage the
domain may also want to come from the reserved set ...
> @@ -2636,7 +2642,7 @@ struct domain *get_pg_owner(domid_t domid)
>
> #ifdef CONFIG_STATIC_MEMORY
> /* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
> -void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> bool need_scrub)
This line now wants its indentation adjusted.
Jan
