RE: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, June 22, 2022 5:24 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@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 v7 7/9] xen/arm: unpopulate memory when domain is
> static
> 
> On 20.06.2022 04:44, Penny Zheng wrote:
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2498,6 +2498,10 @@ void free_domheap_pages(struct page_info *pg,
> unsigned int order)
> >          }
> >
> >          free_heap_pages(pg, order, scrub);
> > +
> > +        /* Add page on the resv_page_list *after* it has been freed. */
> > +        if ( unlikely(pg->count_info & PGC_static) )
> > +            put_static_pages(d, pg, order);
> 
> Unless I'm overlooking something the list addition done there / ...
> 
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -90,6 +90,15 @@ void free_staticmem_pages(struct page_info *pg,
> unsigned long nr_mfns,
> >                            bool need_scrub);  int
> > acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> >                              unsigned int memflags);
> > +#ifdef CONFIG_STATIC_MEMORY
> > +#define put_static_pages(d, page, order) ({                 \
> > +    unsigned int i;                                         \
> > +    for ( i = 0; i < (1 << (order)); i++ )                  \
> > +        page_list_add_tail((pg) + i, &(d)->resv_page_list); \
> > +})
> 
> ... here isn't guarded by any lock. Feels like we've been there before.
> It's not really clear to me why the freeing of staticmem pages needs to be
> split like this - if it wasn't split, the list addition would "naturally" 
> occur with
> the lock held, I think.

Reminded by you and Julien, I need to add a lock for 
operations(free/allocation) on
resv_page_list, I'll guard the put_static_pages with d->page_alloc_lock. And 
bring
back the lock in acquire_reserved_page.

put_static_pages, that is, adding pages to the reserved list, is only for 
freeing static
pages on runtime. In static page initialization stage, I also use 
free_statimem_pages,
and in which stage, I think the domain has not been constructed at all. So I 
prefer
the freeing of staticmem pages is split into two parts: free_staticmem_pages and
put_static_pages 

> 
> Furthermore careful with the local variable name used here. Consider what
> would happen with an invocation of
> 
>     put_static_pages(d, page, i);
> 
> To common approach is to suffix an underscore to the variable name.
> Such names are not supposed to be used outside of macros definitions, and
> hence there's then no potential for such a conflict.
> 

Understood!! I will change "unsigned int i" to "unsigned int _i";

> Finally I think you mean (1u << (order)) to be on the safe side against UB if
> order could ever reach 31. Then again - is "order" as a parameter needed
> here in the first place? Wasn't it that staticmem operations are limited to
> order-0 regions?

Yes, right now, the actual usage is limited to order-0, how about I add 
assertion here
and remove order parameter:

        /* Add page on the resv_page_list *after* it has been freed. */
        if ( unlikely(pg->count_info & PGC_static) )
        {
            ASSERT(!order);
            put_static_pages(d, pg);
        }

> Jan