RE: [PATCH 04/10] xen/arm: static memory initialization

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, May 18, 2021 6:43 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [PATCH 04/10] xen/arm: static memory initialization
> 
> On 18.05.2021 11:51, Penny Zheng wrote:
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Tuesday, May 18, 2021 3:16 PM
> >>
> >> On 18.05.2021 07:21, Penny Zheng wrote:
> >>> This patch introduces static memory initialization, during system
> >>> RAM boot
> >> up.
> >>>
> >>> New func init_staticmem_pages is the equivalent of init_heap_pages,
> >>> responsible for static memory initialization.
> >>>
> >>> Helper func free_staticmem_pages is the equivalent of
> >>> free_heap_pages, to free nr_pfns pages of static memory.
> >>> For each page, it includes the following steps:
> >>> 1. change page state from in-use(also initialization state) to free
> >>> state and grant PGC_reserved.
> >>> 2. set its owner NULL and make sure this page is not a guest frame
> >>> any more
> >>
> >> But isn't the goal (as per the previous patch) to associate such
> >> pages with a _specific_ domain?
> >>
> >
> > Free_staticmem_pages are alike free_heap_pages, are not used only for
> initialization.
> > Freeing used pages to unused are also included.
> > Here, setting its owner NULL is to set owner in used field NULL.
> 
> I'm afraid I still don't understand.
> 

When initializing heap, xen is using free_heap_pages to do the initialization.
And also when normal domain get destroyed/rebooted, xen is using 
free_domheap_pages,
which calls free_heap_pages to free the pages.

So here, since free_staticmem_pages is the equivalent of the free_heap_pages 
for static
memory, I'm also considering both two scenarios here. And if it is domain get 
destroyed/rebooted,
Page state is in inuse state(PGC_inuse), and the page_info.v.inuse.domain is 
holding the
domain owner.
When freeing then, we need to switch the page state to free state(PGC_free) and 
set its inuse owner to NULL.

I'll clarify it more clearly in commit message.

> > Still, I need to add more explanation here.
> 
> Yes please.
> 
> >>> @@ -1512,6 +1515,49 @@ static void free_heap_pages(
> >>>      spin_unlock(&heap_lock);
> >>>  }
> >>>
> >>> +/* Equivalent of free_heap_pages to free nr_pfns pages of static
> >>> +memory. */ static void free_staticmem_pages(struct page_info *pg,
> >> unsigned long nr_pfns,
> >>> +                                 bool need_scrub)
> >>
> >> Right now this function gets called only from an __init one. Unless
> >> it is intended to gain further callers, it should be marked __init itself 
> >> then.
> >> Otherwise it should be made sure that other architectures don't
> >> include this (dead there) code.
> >>
> >
> > Sure, I'll add __init. Thx.
> 
> Didn't I see a 2nd call to the function later in the series? That one doesn't
> permit the function to be __init, iirc.
> 
> Jan

Cheers

Penny