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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, July 8, 2022 8:48 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v8 2/9] xen: do not free reserved memory into heap
> 
> 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.
> 

Yes, "else" is needed

> > @@ -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.
> 

Yes, Julien also raised the same question once before while we have been 
discussing
about how to make the scrubbing static pages asynchronously.

Right now, static memory is either reserved as guest memory or as shared memory,
which both cannot be re-used, so as you said, scrubbing is pointless at the 
moment.

So here I'll only keep the scrub_debug option, as synchronously scrubbing is 
already in
free_staticmem_pages.

> > +        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.
> 

True, true. ASSERT(d) will be added

> Jan