Re: [PATCH v9 8/8] xen: retrieve reserved pages on populate_physmap

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Penny,

> On 9 Aug 2022, at 09:58, Penny Zheng <Penny.Zheng@xxxxxxx> wrote:
> 
> Hi jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, August 9, 2022 4:27 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 v9 8/8] xen: retrieve reserved pages on
>> populate_physmap
>> 
>> On 09.08.2022 10:07, Penny Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Tuesday, August 9, 2022 3:59 PM
>>>> 
>>>> On 09.08.2022 09:53, Penny Zheng wrote:
>>>>>> -----Original Message-----
>>>>>> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
>>>>>> Of Jan Beulich
>>>>>> Sent: Monday, July 25, 2022 11:44 PM
>>>>>> 
>>>>>> On 20.07.2022 07:46, 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>
>>>>>>> ---
>>>>>>> v9 changes:
>>>>>>> - Use ASSERT_ALLOC_CONTEXT() in acquire_reserved_page
>>>>>>> - Add free_staticmem_pages to undo prepare_staticmem_pages when
>>>>>>> assign_domstatic_pages fails
>>>>>> 
>>>>>> May I suggest to re-consider naming of the various functions?
>>>>>> Undoing what "prepare" did by "free" is, well, counterintuitive.
>>>>>> 
>>>>> 
>>>>> How about change the name "prepare_staticmem_pages" to
>>>> "allocate_staticmem_pages"?
>>>> 
>>>> Perhaps - if what the function does really resembles allocation in some
>> way.
>>>> So far I wasn't really certain in that regard, and hence I was
>>>> wondering whether "prepare" doesn't better describe what it does, but
>>>> then its inverse also doesn't really "free" anything.
>>>> 
>>> 
>>> Hmmmm, “prepare” with “destroy” in its inverse? Do you have any
>> suggestion in mind?
>> 
>> To be honest I was hoping you would make an attempt at finding a suitable
>> pair of verbs. To me "destroy" is more the opposite of "create", and I'm
>> unable to think of a good opposite of "prepare" (short of resorting to
>> "unprepare"); if I really needed to come up with something then it would
>> likely be "cleanup", albeit I'd not be overly happy with that either.
>> 
> 
> Maybe unprepare is better here, I was searching linux code for the help, and
> they are using prepare/unprepare as a pair of verbs a lot in drivers codes.
> 
> For the renaming here, I suggest to fix it with a new commit, since 
> free_staticmem_pages
> has already been merged.

I think that unprepare is ok and it make sense to do this in an independent 
patch.

@jan: can you confirm that you agree with this way to go ?

Cheers
Bertrand