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
