Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2022 11:36, Penny Zheng wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -35,6 +35,10 @@
>  #include <asm/guest.h>
>  #endif
>  
> +#ifndef is_domain_on_static_allocation
> +#define is_domain_on_static_allocation(d) 0

Nit: "false", not "0".

> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d, unsigned long 
> gmfn)
>       * device must retrieve the same pfn when the hypercall populate_physmap
>       * is called.
>       *
> +     * When domain on static allocation, they should always get pages from 
> the
> +     * reserved static region when the hypercall populate_physmap is called.
> +     *
>       * For this purpose (and to match populate_physmap() behavior), the page
>       * is kept allocated.
>       */
> -    if ( !rc && !is_domain_direct_mapped(d) )
> +    if ( !rc && !(is_domain_direct_mapped(d) ||
> +                  is_domain_on_static_allocation(d)) )
>          put_page_alloc_ref(page);
>  
>      put_page(page);
> +#ifdef CONFIG_STATIC_MEMORY
> +    /*
> +     * When domain on static allocation, we shall store pages to 
> resv_page_list,
> +     * so the hypercall populate_physmap could retrieve pages from it,
> +     * rather than allocating from heap.
> +     */
> +    if ( is_domain_on_static_allocation(d) )
> +    {
> +        page_list_add_tail(page, &d->resv_page_list);
> +        d->resv_pages++;
> +    }
> +#endif

I think this is wrong, as a result of not integrating with put_page().
The page should only go on that list once its last ref was dropped. I
don't recall for sure, but iirc staticmem pages are put on the
domain's page list just like other pages would be. But then you also
corrupt the list when this isn't the last ref which is put.

As a result I also think that you shouldn't need to touch the earlier
if().

Jan