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

[Julien Grall <julien@xen.org>]

Hi Penny,

On 27/06/2022 11:11, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Saturday, June 25, 2022 3:51 AM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
>> Jan Beulich <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
>> Wei Liu <wl@xen.org>
>> 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@arm.com>
>>> ---
>>> 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