Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2022 08:13, Penny Zheng wrote:
> Hi Jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, March 30, 2022 5:53 PM
>> To: Penny Zheng <Penny.Zheng@xxxxxxx>
>> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang <Henry.Wang@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 v1 3/5] xen/arm: unpopulate memory when domain on
>> static allocation
>>
>> On 30.03.2022 11:36, Penny Zheng wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -35,6 +35,10 @@
>>>  #include <asm/guest.h>
>>>  #endif
>>>
>>> +#ifndef is_domain_on_static_allocation #define
>>> +is_domain_on_static_allocation(d) 0
>>
>> Nit: "false", not "0".
>>
>>> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d,
>> unsigned long gmfn)
>>>       * device must retrieve the same pfn when the hypercall
>> populate_physmap
>>>       * is called.
>>>       *
>>> +     * When domain on static allocation, they should always get pages from
>> the
>>> +     * reserved static region when the hypercall populate_physmap is 
>>> called.
>>> +     *
>>>       * For this purpose (and to match populate_physmap() behavior), the 
>>> page
>>>       * is kept allocated.
>>>       */
>>> -    if ( !rc && !is_domain_direct_mapped(d) )
>>> +    if ( !rc && !(is_domain_direct_mapped(d) ||
>>> +                  is_domain_on_static_allocation(d)) )
>>>          put_page_alloc_ref(page);
>>>
>>>      put_page(page);
>>> +#ifdef CONFIG_STATIC_MEMORY
>>> +    /*
>>> +     * When domain on static allocation, we shall store pages to
>> resv_page_list,
>>> +     * so the hypercall populate_physmap could retrieve pages from it,
>>> +     * rather than allocating from heap.
>>> +     */
>>> +    if ( is_domain_on_static_allocation(d) )
>>> +    {
>>> +        page_list_add_tail(page, &d->resv_page_list);
>>> +        d->resv_pages++;
>>> +    }
>>> +#endif
>>
>> I think this is wrong, as a result of not integrating with put_page().
>> The page should only go on that list once its last ref was dropped. I don't 
>> recall
>> for sure, but iirc staticmem pages are put on the domain's page list just 
>> like
>> other pages would be. But then you also corrupt the list when this isn't the 
>> last
>> ref which is put.
> 
> Yes, staticmem pages are put on the domain's page list.
> Here, I tried to only destroy the P2M mapping, and keep the page still 
> allocated
> to this domain.

Well, much depends on what you call "allocated". For populate_physmap
you then take pages from resv_page_list. So I'd like to distinguish
"allocated" from "reserved": The pages are allocated to the domain
when they're on the normal page list; they're reserved when they're
on the new list you introduce. And what you want to initiate here is
an "allocated" -> "reserved" transition.

> resv_page_list is just providing an easy way to track down the unpopulated 
> memory. 
> '''
> But then you also corrupt the list when this isn't the last
> ref which is put.
> '''
> I'm sorry, would you like to be more specific on this comment?
> I want these pages to linked in the domain's page list, then it could be
> freed properly when domain get destroyed through relinquish_memory.

Clearly they can't be on both lists. Hence you can put them on the
new list only _after_ having taken them off the "normal" list. That
"taking off the normal list" should happen when the last ref is
dropped, not here - see free_domheap_pages()'s uses of
arch_free_heap_page(), recalling that free_domheap_page() is what
put_page() calls upon dropping the last ref.

Jan