Hi jan
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Wednesday, June 22, 2022 5:24 PM
> To: Penny Zheng <penny.zh...@arm.com>
> Cc: Wei Chen <wei.c...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> Julien Grall <jul...@xen.org>; Stefano Stabellini <sstabell...@kernel.org>;
> Wei Liu <w...@xen.org>; xen-devel@lists.xenproject.org
> 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
