| Message ID | 20220330093617.3870589-4-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | populate/unpopulate memory when domain on static | expand |
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. As a result I also think that you shouldn't need to touch the earlier if(). Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Wednesday, March 30, 2022 5:53 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; > Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap > <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini > <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen- > devel@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. 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. > > As a result I also think that you shouldn't need to touch the earlier if(). > > Jan
On 31.03.2022 08:13, Penny Zheng wrote: > Hi Jan > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Wednesday, March 30, 2022 5:53 PM >> To: Penny Zheng <Penny.Zheng@arm.com> >> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; >> Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap >> <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini >> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen- >> devel@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
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Thursday, March 31, 2022 3:06 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; > Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap > <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini > <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on > static allocation > > On 31.03.2022 08:13, Penny Zheng wrote: > > Hi Jan > > > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Wednesday, March 30, 2022 5:53 PM > >> To: Penny Zheng <Penny.Zheng@arm.com> > >> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang > <Henry.Wang@arm.com>; > >> Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap > >> <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano > >> Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen- > >> devel@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. > Right, right, I've missed the critical point "they can't be on both lists". Here is a thing, put_page is a very common helper that it is also beening used when freeing memory on domain deconstruction. At that time, I don't want to put these pages in resv_page_list, I'd like them to be freed to the heap. This putting pages in resv_page_list thing is only for unpopulating memory on the runtime. So I'd suggest introducing a new helper put_pages_resv(...) to do the work. About before you mentioned that "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. " Is there a possibility that I still keep the pages allocated but just moving them from normal page list to the new resv_page_list? Of course, a few extra things needed to be covered, like domain_adjust_tot_pages, scrubing the pages. Another reason I want to keep page allocated is that if putting pages in resv_page_list upon dropping the last ref, we need to do a lot things on pages to totally let it free, like set its owner to NULL, changing page state from in_use to free, etc. Later when populating them, we shall do the exact in backwards, setting the owner back to the domain, changing the state from free back to in_use, which seems a bit useless. And actually for domain on static allocation, these staticmem pages are reserved from the very beginning, and when it is allocated to the domain, it forever belongs to the domain, and it could never be used in any other way. Later when populating the memory, we could just move the pages from resv_page_list back to the normal list, and also domain_adjust_tot_pages. Another thing I'd consider to be affected is that when domain is dying, and doing relinquish_memory, I need extra relinquishing for pages in resv_page_list. > Jan
Hi Jan, On 30/03/2022 10:52, Jan Beulich wrote: > 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". I think we also want to evaluate d so the behavior on x86 and arm is the same. Something like ((void)(d), false). Cheers,
Hi Penny, On 31/03/2022 11:30, Penny Zheng wrote: > Another reason I want to keep page allocated is that if putting pages in > resv_page_list upon dropping the last ref, we need to do a lot things on > pages to totally let it free, like set its owner to NULL, changing page state > from in_use to free, etc. This is not only about optimization here. Bad things can happen if you let a page in state free that is not meant to be used by the buddy allocator (e.g. it was reserved for static memory). I discovered it earlier this year when trying to optimize init_heap_pages() for Live-Update. It was quite hard to debug because the corruption very rarely happened. So let me explain it before you face the same issue :). free_heap_pages() will try to merge the about-to-be-freed chunk with the predecessor and/or successor. To know if the page can be merged, the algorithm is looking at whether the chunk is suitably aligned (e.g. same order) and if the page is in state free. AFAICT, the pages belonging to the buddy allocator could be right next to region reserved memory. So there is a very slim chance that free_heap_pages() may decide to merge a chunk from the static region with the about-to-be-free chunk. Nothing very good will ensue. Technically, this is already a bug in the already merged implementation of the static memory allocator. I can see two ways to fix it: 1) Update free_heap_pages() to check whether the page has PGC_reserved set. 2) Use a different state for pages used by the static allocator. So far my preference is leaning towards 1. But I would like to hear other opinions. Cheers,
Hi Julien and Jan > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Saturday, April 2, 2022 2:54 AM > To: Penny Zheng <Penny.Zheng@arm.com>; Jan Beulich <jbeulich@suse.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; > Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap > <george.dunlap@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei > Liu <wl@xen.org>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on > static allocation > > Hi Penny, > > On 31/03/2022 11:30, Penny Zheng wrote: > > Another reason I want to keep page allocated is that if putting pages > > in resv_page_list upon dropping the last ref, we need to do a lot > > things on pages to totally let it free, like set its owner to NULL, > > changing page state from in_use to free, etc. > This is not only about optimization here. Bad things can happen if you let a > page in state free that is not meant to be used by the buddy allocator (e.g. it > was reserved for static memory). > > I discovered it earlier this year when trying to optimize > init_heap_pages() for Live-Update. It was quite hard to debug because the > corruption very rarely happened. So let me explain it before you face the same > issue :). > > free_heap_pages() will try to merge the about-to-be-freed chunk with the > predecessor and/or successor. To know if the page can be merged, the > algorithm is looking at whether the chunk is suitably aligned (e.g. same > order) and if the page is in state free. > > AFAICT, the pages belonging to the buddy allocator could be right next to > region reserved memory. So there is a very slim chance that > free_heap_pages() may decide to merge a chunk from the static region with > the about-to-be-free chunk. Nothing very good will ensue. > Oh,,, that's a thousand true. If the free static region is the buddy of the about-to-be-free chunk, current code will merge the static region to the heap, which is unacceptable. I'll fix it in this patch serie too and I'm also preferring option 1~ And for unpopulating memory on runtime, I'll do what Jan suggests, adding a new logic of moving the page from d->page_list to d->resv_page_list in arch_free_heap_page() for reserved pages. > Technically, this is already a bug in the already merged implementation of the > static memory allocator. > > I can see two ways to fix it: > 1) Update free_heap_pages() to check whether the page has PGC_reserved > set. > 2) Use a different state for pages used by the static allocator. > > So far my preference is leaning towards 1. But I would like to hear other > opinions. > > Cheers, > > -- > Julien Grall
On 31.03.2022 12:30, Penny Zheng wrote: > Hi Jan > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Thursday, March 31, 2022 3:06 PM >> To: Penny Zheng <Penny.Zheng@arm.com> >> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; >> Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap >> <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini >> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen- >> devel@lists.xenproject.org >> Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on >> static allocation >> >> On 31.03.2022 08:13, Penny Zheng wrote: >>> Hi Jan >>> >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: Wednesday, March 30, 2022 5:53 PM >>>> To: Penny Zheng <Penny.Zheng@arm.com> >>>> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang >> <Henry.Wang@arm.com>; >>>> Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap >>>> <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano >>>> Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen- >>>> devel@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. >> > > Right, right, I've missed the critical point "they can't be on both lists". > Here is a thing, put_page is a very common helper that it is also beening > used when freeing memory on domain deconstruction. At that time, I > don't want to put these pages in resv_page_list, I'd like them to be > freed to the heap. This putting pages in resv_page_list thing is only for > unpopulating memory on the runtime. So I'd suggest introducing a > new helper put_pages_resv(...) to do the work. I'd like to suggest to avoid introducing special case helpers as much as possible. put_page() would better remain the single, common function to use when dropping a ref. Any special treatment for certain kinds of pages should happen without the callers needing to care. > About before you mentioned that "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. " Is there a possibility that I > still keep the pages allocated but just moving them from normal page list > to the new resv_page_list? Of course, a few extra things needed to be > covered, like domain_adjust_tot_pages, scrubing the pages. It's all a matter of definition. What I said (and what you quoted) was merely based on my understanding of your intentions. > Another reason I want to keep page allocated is that if putting pages in > resv_page_list upon dropping the last ref, we need to do a lot things on > pages to totally let it free, like set its owner to NULL, changing page state > from in_use to free, etc. Later when populating them, we shall do the > exact in backwards, setting the owner back to the domain, changing the > state from free back to in_use, which seems a bit useless. And actually > for domain on static allocation, these staticmem pages are reserved > from the very beginning, and when it is allocated to the domain, it > forever belongs to the domain, and it could never be used in any other way. > > Later when populating the memory, we could just move the pages from > resv_page_list back to the normal list, and also domain_adjust_tot_pages. I don't mind the particular model you want to implement. What I do care about is that the end result is consistent within itself, with as little special casing (potentially) all over the code base as possible. Jan
diff --git a/xen/common/domain.c b/xen/common/domain.c index 351029f8b2..e572f27fce 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -602,6 +602,10 @@ struct domain *domain_create(domid_t domid, INIT_PAGE_LIST_HEAD(&d->page_list); INIT_PAGE_LIST_HEAD(&d->extra_page_list); INIT_PAGE_LIST_HEAD(&d->xenpage_list); +#ifdef CONFIG_STATIC_MEMORY + INIT_PAGE_LIST_HEAD(&d->resv_page_list); +#endif + spin_lock_init(&d->node_affinity_lock); d->node_affinity = NODE_MASK_ALL; diff --git a/xen/common/memory.c b/xen/common/memory.c index 69b0cd1e50..2afc3c6f10 100644 --- 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 +#endif + struct memop_args { /* INPUT */ struct domain *domain; /* Domain to be affected. */ @@ -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 #ifdef CONFIG_X86 out_put_gfn: diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 406d9bc610..d7e047bf36 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -376,6 +376,9 @@ struct domain struct page_list_head page_list; /* linked list */ struct page_list_head extra_page_list; /* linked list (size extra_pages) */ struct page_list_head xenpage_list; /* linked list (size xenheap_pages) */ +#ifdef CONFIG_STATIC_MEMORY + struct page_list_head resv_page_list; /* linked list (size resv_pages) */ +#endif /* * This field should only be directly accessed by domain_adjust_tot_pages() @@ -389,6 +392,9 @@ struct domain unsigned int extra_pages; /* pages not included in domain_tot_pages() */ atomic_t shr_pages; /* shared pages */ atomic_t paged_pages; /* paged-out pages */ +#ifdef CONFIG_STATIC_MEMORY + unsigned int resv_pages; /* reserved pages from static region. */ +#endif /* Scheduling. */ void *sched_priv; /* scheduler-specific data */
Today when a domain unpopulates the memory on runtime, they will always hand the memory over to the heap allocator. And it will be a problem if domain on static allocation. Guest RAM for domain on static allocation is static memory, which is reserved to only this domain, so it shall never go back to heap. For above purpose, this commit tries to keep page allocated and store it in page list d->resv_page_list on guest_remove_page, when domain on static allocation. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- xen/common/domain.c | 4 ++++ xen/common/memory.c | 22 +++++++++++++++++++++- xen/include/xen/sched.h | 6 ++++++ 3 files changed, 31 insertions(+), 1 deletion(-)