Re: [PATCH v2 1/6] xen/arm: do not free reserved memory into heap

[Jan Beulich <jbeulich@xxxxxxxx>]

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