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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2022 11:22, Penny Zheng wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1622,6 +1622,8 @@ void put_page(struct page_info *page)
>  
>      if ( unlikely((nx & PGC_count_mask) == 0) )
>      {
> +        if ( unlikely(nx & PGC_static) )
> +            free_domstatic_page(page);
>          free_domheap_page(page);

Didn't you have "else" there in the proposal you made while discussing
v7? You also don't alter free_domheap_page() to skip static pages.

> @@ -2652,9 +2650,48 @@ void __init free_staticmem_pages(struct page_info *pg, 
> unsigned long nr_mfns,
>              scrub_one_page(pg);
>          }
>  
> -        /* In case initializing page of static memory, mark it PGC_static. */
>          pg[i].count_info |= PGC_static;
>      }
> +
> +    spin_unlock(&heap_lock);
> +}
> +
> +void free_domstatic_page(struct page_info *page)
> +{
> +    struct domain *d = page_get_owner(page);
> +    bool drop_dom_ref, need_scrub;
> +
> +    ASSERT_ALLOC_CONTEXT();
> +
> +    if ( likely(d) )
> +    {
> +        /* NB. May recursively lock from relinquish_memory(). */
> +        spin_lock_recursive(&d->page_alloc_lock);
> +
> +        arch_free_heap_page(d, page);
> +
> +        /*
> +         * Normally we expect a domain to clear pages before freeing them,
> +         * if it cares about the secrecy of their contents. However, after
> +         * a domain has died we assume responsibility for erasure. We do
> +         * scrub regardless if option scrub_domheap is set.
> +         */
> +        need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;

May I suggest that instead of copying the comment you simply add
one here referring to the other one? Otoh I'm not sure about the
"dying" case: What happens to a domain's static pages after its
death? Isn't it that they cannot be re-used? If so, scrubbing is
pointless. And whether the other reasons to scrub actually apply
to static pages also isn't quite clear to me.

> +        drop_dom_ref = !domain_adjust_tot_pages(d, -1);
> +
> +        spin_unlock_recursive(&d->page_alloc_lock);
> +    }
> +    else
> +    {
> +        drop_dom_ref = false;
> +        need_scrub = true;
> +    }

Why this "else"? I can't see any way unowned paged would make it here.
Instead you could e.g. have another ASSERT() at the top of the function.

Jan