Re: [PATCH v6 2/9] xen: do not free reserved memory into heap

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 07/06/2022 08:30, Penny Zheng wrote:
> Pages used as guest RAM for static domain, shall be reserved to this
> domain only.
> 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_heap_pages in runtime
> for static domain freeing memory resource, so let's drop the __init
> flag.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> v6 changes:
> - adapt to PGC_static
> - remove #ifdef aroud function declaration
> ---
> v5 changes:
> - In order to avoid stub functions, we #define PGC_staticmem to non-zero only
> when CONFIG_STATIC_MEMORY
> - use "unlikely()" around pg->count_info & PGC_staticmem
> - remove pointless "if", since mark_page_free() is going to set count_info
> to PGC_state_free and by consequence clear PGC_staticmem
> - move #define PGC_staticmem 0 to mm.h
> ---
> v4 changes:
> - no changes
> ---
> v3 changes:
> - fix possible racy issue in free_staticmem_pages()
> - introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case
> - move the change to free_heap_pages() to cover other potential call sites
> - fix the indentation
> ---
> v2 changes:
> - new commit
> ---
>   xen/arch/arm/include/asm/mm.h |  4 +++-
>   xen/common/page_alloc.c       | 12 +++++++++---
>   xen/include/xen/mm.h          |  2 --
>   3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index fbff11c468..7442893e77 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -108,9 +108,11 @@ struct page_info
>     /* Page is Xen heap? */
>   #define _PGC_xen_heap     PG_shift(2)
>   #define PGC_xen_heap      PG_mask(1, 2)
> -  /* Page is static memory */
NITpicking: You added this comment in patch #1 and now removing the 
space. Any reason to drop the space?
> +#ifdef CONFIG_STATIC_MEMORY
I think this change ought to be explained in the commit message. AFAIU, 
this is necessary to allow the compiler to remove code and avoid linking 
issues. Is that correct?
> +/* Page is static memory */
>   #define _PGC_static    PG_shift(3)
>   #define PGC_static     PG_mask(1, 3)
> +#endif
>   /* ... */
>   /* Page is broken? */
>   #define _PGC_broken       PG_shift(7)
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9e5c757847..6876869fa6 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1443,6 +1443,13 @@ static void free_heap_pages(
>   
>       ASSERT(order <= MAX_ORDER);
>   
> +    if ( unlikely(pg->count_info & PGC_static) )
> +    {
> +        /* Pages of static memory shall not go back to the heap. */
> +        free_staticmem_pages(pg, 1UL << order, need_scrub);
I can't remember whether I asked this before (I couldn't find a thread).

free_staticmem_pages() doesn't seem to be protected by any lock. So how 
do you prevent the concurrent access to the page info with the acquire part?
Cheers,

--
Julien Grall