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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, March 30, 2022 5:53 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang <Henry.Wang@xxxxxxx>;
> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on
> static allocation
> 
> 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.

Yes, staticmem pages are put on the domain's page list.
Here, I tried to only destroy the P2M mapping, and keep the page still allocated
to this domain.
resv_page_list is just providing an easy way to track down the unpopulated 
memory. 
'''
But then you also corrupt the list when this isn't the last
ref which is put.
'''
I'm sorry, would you like to be more specific on this comment?
I want these pages to linked in the domain's page list, then it could be
freed properly when domain get destroyed through relinquish_memory.

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