Re: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is static

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 27/04/2022 10:27, Penny Zheng wrote:
> Today when a domain unpopulates the memory on runtime, they will always
> hand the memory back to the heap allocator. And it will be a problem if domain
> is static.
>
> Pages as guest RAM for static domain shall be reserved to only this domain
> and not be used for any other purposes, so they shall never go back to heap
> allocator.
>
> This commit puts reserved pages on the new list resv_page_list only after
> having taken them off the "normal" list, when the last ref dropped.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> v3 changes:
> - have page_list_del() just once out of the if()
> - remove resv_pages counter
> - make arch_free_heap_page be an expression, not a compound statement.
> ---
> v2 changes:
> - put reserved pages on resv_page_list after having taken them off
> the "normal" list
> ---
>   xen/arch/arm/include/asm/mm.h | 12 ++++++++++++
>   xen/common/domain.c           |  4 ++++
>   xen/include/xen/sched.h       |  3 +++
>   3 files changed, 19 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 424aaf2823..c6426c1705 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info *page);
>   
>   unsigned int arch_get_dma_bitsize(void);
>   
> +/*
> + * Put free pages on the resv page list after having taken them
> + * off the "normal" page list, when pages from static memory
> + */
> +#ifdef CONFIG_STATIC_MEMORY
> +#define arch_free_heap_page(d, pg) ({                   \
> +    page_list_del(pg, page_to_list(d, pg));             \
> +    if ( (pg)->count_info & PGC_reserved )              \
> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> +})
> +#endif
I am a bit puzzled how this is meant to work.

Looking at the code, arch_free_heap_page() will be called from 
free_domheap_pages(). If I am not mistaken, reserved pages are not 
considered as xen heap pages, so we would go in the else which will end 
up to call free_heap_pages().
free_heap_pages() will end up to add the page in the heap allocator and 
corrupt the d->resv_page_list because there are only one link list.
What did I miss?

Cheers,

--
Julien Grall