Re: [PATCH v7 9/9] xen: retrieve reserved pages on populate_physmap

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 27/06/2022 11:11, Penny Zheng wrote:
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: Saturday, June 25, 2022 3:51 AM
> > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Wei Chen <Wei.Chen@xxxxxxx>; Andrew Cooper
> > <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> > Jan Beulich <jbeulich@xxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> > Wei Liu <wl@xxxxxxx>
> > Subject: Re: [PATCH v7 9/9] xen: retrieve reserved pages on
> > populate_physmap
> >
> > Hi Penny,
> >
> > On 20/06/2022 03:44, Penny Zheng wrote:
> > > When a static domain populates memory through populate_physmap at
> > > runtime, it shall retrieve reserved pages from resv_page_list to make
> > > sure that guest RAM is still restricted in statically configured memory
> > regions.
> > > This commit also introduces a new helper acquire_reserved_page to make
> > it work.
> > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > > ---
> > > v7 changes:
> > > - remove the lock, since we add the page to rsv_page_list after it has
> > > been totally freed.
> > Hmmm... Adding the page after it has been totally freed doesn't mean you
> > can get away with the lock. AFAICT you can still have concurrent free/allocate
> > that could modify the list.
> >
> > Therefore if you add/remove pages without the list, you would end up to
> > corrupt the list.
> >
> > If you disagree, then please point out which lock (or mechanism) will prevent
> > concurrent access.
> Ok. Combined with the last serie comments, actually, you suggest that we need 
> to add
> two locks, right?
We at least need the second lock (i.e. d->page_alloc_lock). The first 
lock (i.e.) may not be necessary if all the static memory are allocated 
for a domain. So you can guarantee ordering by adding to the resv_page_list.
Unless there are an ordering issue between the locks, I would for now 
suggest to keep both. We can refine this afterwards.
Cheers,

--
Julien Grall