On 31.03.2022 08:13, Penny Zheng wrote:
> Hi Jan
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Wednesday, March 30, 2022 5:53 PM
>> To: Penny Zheng <penny.zh...@arm.com>
>> Cc: Wei Chen <wei.c...@arm.com>; Henry Wang <henry.w...@arm.com>;
>> Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap
>> <george.dun...@citrix.com>; Julien Grall <jul...@xen.org>; Stefano Stabellini
>> <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; xen-
>> de...@lists.xenproject.org
>> 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
