| Message ID | 20210518052113.725808-8-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
On 18.05.2021 07:21, Penny Zheng wrote: > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2447,6 +2447,9 @@ int assign_pages( > { > ASSERT(page_get_owner(&pg[i]) == NULL); > page_set_owner(&pg[i], d); > + /* use page_set_reserved_owner to set its reserved domain owner. */ > + if ( (pg[i].count_info & PGC_reserved) ) > + page_set_reserved_owner(&pg[i], d); Now this is puzzling: What's the point of setting two owner fields to the same value? I also don't recall you having introduced page_set_reserved_owner() for x86, so how is this going to build there? > @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( > return pg; > } > > +/* > + * Allocate nr_pfns contiguous pages, starting at #start, of static memory, > + * then assign them to one specific domain #d. > + * It is the equivalent of alloc_domheap_pages for static memory. > + */ > +struct page_info *alloc_domstatic_pages( > + struct domain *d, unsigned long nr_pfns, paddr_t start, > + unsigned int memflags) > +{ > + struct page_info *pg = NULL; > + unsigned long dma_size; > + > + ASSERT(!in_irq()); > + > + if ( memflags & MEMF_no_owner ) > + memflags |= MEMF_no_refcount; > + > + if ( !dma_bitsize ) > + memflags &= ~MEMF_no_dma; > + else > + { > + dma_size = 1ul << bits_to_zone(dma_bitsize); > + /* Starting address shall meet the DMA limitation. */ > + if ( dma_size && start < dma_size ) > + return NULL; It is the entire range (i.e. in particular the last byte) which needs to meet such a restriction. I'm not convinced though that DMA width restrictions and static allocation are sensible to coexist. > + } > + > + pg = alloc_staticmem_pages(nr_pfns, start, memflags); > + if ( !pg ) > + return NULL; > + > + if ( d && !(memflags & MEMF_no_owner) ) > + { > + if ( memflags & MEMF_no_refcount ) > + { > + unsigned long i; > + > + for ( i = 0; i < nr_pfns; i++ ) > + pg[i].count_info = PGC_extra; > + } Is this as well as the MEMF_no_owner case actually meaningful for statically allocated pages? Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, May 18, 2021 3:35 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; julien@xen.org > Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages > > On 18.05.2021 07:21, Penny Zheng wrote: > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2447,6 +2447,9 @@ int assign_pages( > > { > > ASSERT(page_get_owner(&pg[i]) == NULL); > > page_set_owner(&pg[i], d); > > + /* use page_set_reserved_owner to set its reserved domain owner. > */ > > + if ( (pg[i].count_info & PGC_reserved) ) > > + page_set_reserved_owner(&pg[i], d); > > Now this is puzzling: What's the point of setting two owner fields to the same > value? I also don't recall you having introduced > page_set_reserved_owner() for x86, so how is this going to build there? > Thanks for pointing out that it will fail on x86. As for the same value, sure, I shall change it to domid_t domid to record its reserved owner. Only domid is enough for differentiate. And even when domain get rebooted, struct domain may be destroyed, but domid will stays The same. Major user cases for domain on static allocation are referring to the whole system are static, No runtime creation. > > @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( > > return pg; > > } > > > > +/* > > + * Allocate nr_pfns contiguous pages, starting at #start, of static > > +memory, > > + * then assign them to one specific domain #d. > > + * It is the equivalent of alloc_domheap_pages for static memory. > > + */ > > +struct page_info *alloc_domstatic_pages( > > + struct domain *d, unsigned long nr_pfns, paddr_t start, > > + unsigned int memflags) > > +{ > > + struct page_info *pg = NULL; > > + unsigned long dma_size; > > + > > + ASSERT(!in_irq()); > > + > > + if ( memflags & MEMF_no_owner ) > > + memflags |= MEMF_no_refcount; > > + > > + if ( !dma_bitsize ) > > + memflags &= ~MEMF_no_dma; > > + else > > + { > > + dma_size = 1ul << bits_to_zone(dma_bitsize); > > + /* Starting address shall meet the DMA limitation. */ > > + if ( dma_size && start < dma_size ) > > + return NULL; > > It is the entire range (i.e. in particular the last byte) which needs to meet such > a restriction. I'm not convinced though that DMA width restrictions and static > allocation are sensible to coexist. > FWIT, if starting address meets the limitation, the last byte, greater than starting address shall meet it too. > > + } > > + > > + pg = alloc_staticmem_pages(nr_pfns, start, memflags); > > + if ( !pg ) > > + return NULL; > > + > > + if ( d && !(memflags & MEMF_no_owner) ) > > + { > > + if ( memflags & MEMF_no_refcount ) > > + { > > + unsigned long i; > > + > > + for ( i = 0; i < nr_pfns; i++ ) > > + pg[i].count_info = PGC_extra; > > + } > > Is this as well as the MEMF_no_owner case actually meaningful for statically > allocated pages? > Thanks for pointing out. Truly, we do not need to take it considered. > Jan
Hi Penny, Title: s/intruduce/introduce/ On 18/05/2021 06:21, Penny Zheng wrote: > alloc_domstatic_pages is the equivalent of alloc_domheap_pages for > static mmeory, and it is to allocate nr_pfns pages of static memory > and assign them to one specific domain. > > It uses alloc_staticmen_pages to get nr_pages pages of static memory, > then on success, it will use assign_pages to assign those pages to > one specific domain, including using page_set_reserved_owner to set its > reserved domain owner. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/common/page_alloc.c | 53 +++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/mm.h | 4 ++++ > 2 files changed, 57 insertions(+) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 0eb9f22a00..f1f1296a61 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2447,6 +2447,9 @@ int assign_pages( > { > ASSERT(page_get_owner(&pg[i]) == NULL); > page_set_owner(&pg[i], d); > + /* use page_set_reserved_owner to set its reserved domain owner. */ > + if ( (pg[i].count_info & PGC_reserved) ) > + page_set_reserved_owner(&pg[i], d); I have skimmed through the rest of the series and couldn't find anyone calling page_get_reserved_owner(). The value is also going to be the exact same as page_set_owner(). So why do we need it? > smp_wmb(); /* Domain pointer must be visible before updating refcnt. */ > pg[i].count_info = > (pg[i].count_info & PGC_extra) | PGC_allocated | 1; This will clobber PGC_reserved. > @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( > return pg; > } > > +/* > + * Allocate nr_pfns contiguous pages, starting at #start, of static memory, s/nr_pfns/nr_mfns/ > + * then assign them to one specific domain #d. > + * It is the equivalent of alloc_domheap_pages for static memory. > + */ > +struct page_info *alloc_domstatic_pages( > + struct domain *d, unsigned long nr_pfns, paddr_t start, s/nr_pfns/nf_mfns/. Also, I would the third parameter to be an mfn_t. > + unsigned int memflags) > +{ > + struct page_info *pg = NULL; > + unsigned long dma_size; > + > + ASSERT(!in_irq()); > + > + if ( memflags & MEMF_no_owner ) > + memflags |= MEMF_no_refcount; > + > + if ( !dma_bitsize ) > + memflags &= ~MEMF_no_dma; > + else > + { > + dma_size = 1ul << bits_to_zone(dma_bitsize); > + /* Starting address shall meet the DMA limitation. */ > + if ( dma_size && start < dma_size ) > + return NULL; > + } > + > + pg = alloc_staticmem_pages(nr_pfns, start, memflags); > + if ( !pg ) > + return NULL; > + > + if ( d && !(memflags & MEMF_no_owner) ) > + { > + if ( memflags & MEMF_no_refcount ) > + { > + unsigned long i; > + > + for ( i = 0; i < nr_pfns; i++ ) > + pg[i].count_info = PGC_extra; > + } > + if ( assign_pages(d, pg, nr_pfns, memflags) ) > + { > + free_staticmem_pages(pg, nr_pfns, memflags & MEMF_no_scrub); > + return NULL; > + } > + } > + > + return pg; > +} > + > void free_domheap_pages(struct page_info *pg, unsigned int order) > { > struct domain *d = page_get_owner(pg); > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index dcf9daaa46..e45987f0ed 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -111,6 +111,10 @@ unsigned long __must_check domain_adjust_tot_pages(struct domain *d, > int domain_set_outstanding_pages(struct domain *d, unsigned long pages); > void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages); > > +/* Static Memory */ > +struct page_info *alloc_domstatic_pages(struct domain *d, > + unsigned long nr_pfns, paddr_t start, unsigned int memflags); > + > /* Domain suballocator. These functions are *not* interrupt-safe.*/ > void init_domheap_pages(paddr_t ps, paddr_t pe); > struct page_info *alloc_domheap_pages( > Cheers,
On 18.05.2021 10:57, Penny Zheng wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, May 18, 2021 3:35 PM >> >> On 18.05.2021 07:21, Penny Zheng wrote: >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -2447,6 +2447,9 @@ int assign_pages( >>> { >>> ASSERT(page_get_owner(&pg[i]) == NULL); >>> page_set_owner(&pg[i], d); >>> + /* use page_set_reserved_owner to set its reserved domain owner. >> */ >>> + if ( (pg[i].count_info & PGC_reserved) ) >>> + page_set_reserved_owner(&pg[i], d); >> >> Now this is puzzling: What's the point of setting two owner fields to the same >> value? I also don't recall you having introduced >> page_set_reserved_owner() for x86, so how is this going to build there? >> > > Thanks for pointing out that it will fail on x86. > As for the same value, sure, I shall change it to domid_t domid to record its reserved owner. > Only domid is enough for differentiate. > And even when domain get rebooted, struct domain may be destroyed, but domid will stays > The same. Will it? Are you intending to put in place restrictions that make it impossible for the ID to get re-used by another domain? > Major user cases for domain on static allocation are referring to the whole system are static, > No runtime creation. Right, but that's not currently enforced afaics. If you would enforce it, it may simplify a number of things. >>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( >>> return pg; >>> } >>> >>> +/* >>> + * Allocate nr_pfns contiguous pages, starting at #start, of static >>> +memory, >>> + * then assign them to one specific domain #d. >>> + * It is the equivalent of alloc_domheap_pages for static memory. >>> + */ >>> +struct page_info *alloc_domstatic_pages( >>> + struct domain *d, unsigned long nr_pfns, paddr_t start, >>> + unsigned int memflags) >>> +{ >>> + struct page_info *pg = NULL; >>> + unsigned long dma_size; >>> + >>> + ASSERT(!in_irq()); >>> + >>> + if ( memflags & MEMF_no_owner ) >>> + memflags |= MEMF_no_refcount; >>> + >>> + if ( !dma_bitsize ) >>> + memflags &= ~MEMF_no_dma; >>> + else >>> + { >>> + dma_size = 1ul << bits_to_zone(dma_bitsize); >>> + /* Starting address shall meet the DMA limitation. */ >>> + if ( dma_size && start < dma_size ) >>> + return NULL; >> >> It is the entire range (i.e. in particular the last byte) which needs to meet such >> a restriction. I'm not convinced though that DMA width restrictions and static >> allocation are sensible to coexist. >> > > FWIT, if starting address meets the limitation, the last byte, greater than starting > address shall meet it too. I'm afraid I don't know what you're meaning to tell me here. Jan
Hi Penny, On 18/05/2021 09:57, Penny Zheng wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, May 18, 2021 3:35 PM >> To: Penny Zheng <Penny.Zheng@arm.com> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >> <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; julien@xen.org >> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages >> >> On 18.05.2021 07:21, Penny Zheng wrote: >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -2447,6 +2447,9 @@ int assign_pages( >>> { >>> ASSERT(page_get_owner(&pg[i]) == NULL); >>> page_set_owner(&pg[i], d); >>> + /* use page_set_reserved_owner to set its reserved domain owner. >> */ >>> + if ( (pg[i].count_info & PGC_reserved) ) >>> + page_set_reserved_owner(&pg[i], d); >> >> Now this is puzzling: What's the point of setting two owner fields to the same >> value? I also don't recall you having introduced >> page_set_reserved_owner() for x86, so how is this going to build there? >> > > Thanks for pointing out that it will fail on x86. > As for the same value, sure, I shall change it to domid_t domid to record its reserved owner. > Only domid is enough for differentiate. > And even when domain get rebooted, struct domain may be destroyed, but domid will stays > The same. > Major user cases for domain on static allocation are referring to the whole system are static, > No runtime creation. One may want to have static memory yet doesn't care about the domid. So I am not in favor to restrict about the domid unless there is no other way. Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Tuesday, May 18, 2021 6:30 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com> > Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages > > Hi Penny, > > Title: s/intruduce/introduce/ > Thx~ > On 18/05/2021 06:21, Penny Zheng wrote: > > alloc_domstatic_pages is the equivalent of alloc_domheap_pages for > > static mmeory, and it is to allocate nr_pfns pages of static memory > > and assign them to one specific domain. > > > > It uses alloc_staticmen_pages to get nr_pages pages of static memory, > > then on success, it will use assign_pages to assign those pages to one > > specific domain, including using page_set_reserved_owner to set its > > reserved domain owner. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > xen/common/page_alloc.c | 53 > +++++++++++++++++++++++++++++++++++++++++ > > xen/include/xen/mm.h | 4 ++++ > > 2 files changed, 57 insertions(+) > > > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index > > 0eb9f22a00..f1f1296a61 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2447,6 +2447,9 @@ int assign_pages( > > { > > ASSERT(page_get_owner(&pg[i]) == NULL); > > page_set_owner(&pg[i], d); > > + /* use page_set_reserved_owner to set its reserved domain owner. > */ > > + if ( (pg[i].count_info & PGC_reserved) ) > > + page_set_reserved_owner(&pg[i], d); > > I have skimmed through the rest of the series and couldn't find anyone > calling page_get_reserved_owner(). The value is also going to be the exact > same as page_set_owner(). > > So why do we need it? > In my first intent, This two helper page_get_reserved_owner/ page_set_reserved_owner and the new field `reserved` in page_info are all for rebooting domain on static allocation. I was considering that, when implementing rebooting domain on static allocation, memory will be relinquished and right now, all freed back to heap, which is not suitable for static memory here. ` relinquish_memory(d, &d->page_list) --> put_page --> free_domheap_page` For pages in PGC_reserved, now, I am considering that, other than giving it back to heap, maybe creating a new global `struct page_info*[DOMID]` value to hold. So it is better to have a new field in struct page_info, as follows, to hold such info. /* Page is reserved. */ struct { /* * Reserved Owner of this page, * if this page is reserved to a specific domain. */ domid_t reserved_owner; } reserved; But this patch Serie is not going to include this feature, and I will delete related helpers and values. > > smp_wmb(); /* Domain pointer must be visible before updating > refcnt. */ > > pg[i].count_info = > > (pg[i].count_info & PGC_extra) | PGC_allocated | 1; > > This will clobber PGC_reserved. > related changes have been included into the commit of "0008-xen-arm-introduce-reserved_page_list.patch". > > @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( > > return pg; > > } > > > > +/* > > + * Allocate nr_pfns contiguous pages, starting at #start, of static > > +memory, > > s/nr_pfns/nr_mfns/ > Sure. > > + * then assign them to one specific domain #d. > > + * It is the equivalent of alloc_domheap_pages for static memory. > > + */ > > +struct page_info *alloc_domstatic_pages( > > + struct domain *d, unsigned long nr_pfns, paddr_t start, > > s/nr_pfns/nf_mfns/. Also, I would the third parameter to be an mfn_t. > Sure. > > + unsigned int memflags) > > +{ > > + struct page_info *pg = NULL; > > + unsigned long dma_size; > > + > > + ASSERT(!in_irq()); > > + > > + if ( memflags & MEMF_no_owner ) > > + memflags |= MEMF_no_refcount; > > + > > + if ( !dma_bitsize ) > > + memflags &= ~MEMF_no_dma; > > + else > > + { > > + dma_size = 1ul << bits_to_zone(dma_bitsize); > > + /* Starting address shall meet the DMA limitation. */ > > + if ( dma_size && start < dma_size ) > > + return NULL; > > + } > > + > > + pg = alloc_staticmem_pages(nr_pfns, start, memflags); > > + if ( !pg ) > > + return NULL; > > + > > + if ( d && !(memflags & MEMF_no_owner) ) > > + { > > + if ( memflags & MEMF_no_refcount ) > > + { > > + unsigned long i; > > + > > + for ( i = 0; i < nr_pfns; i++ ) > > + pg[i].count_info = PGC_extra; > > + } > > + if ( assign_pages(d, pg, nr_pfns, memflags) ) > > + { > > + free_staticmem_pages(pg, nr_pfns, memflags & MEMF_no_scrub); > > + return NULL; > > + } > > + } > > + > > + return pg; > > +} > > + > > void free_domheap_pages(struct page_info *pg, unsigned int order) > > { > > struct domain *d = page_get_owner(pg); diff --git > > a/xen/include/xen/mm.h b/xen/include/xen/mm.h index > > dcf9daaa46..e45987f0ed 100644 > > --- a/xen/include/xen/mm.h > > +++ b/xen/include/xen/mm.h > > @@ -111,6 +111,10 @@ unsigned long __must_check > domain_adjust_tot_pages(struct domain *d, > > int domain_set_outstanding_pages(struct domain *d, unsigned long > pages); > > void get_outstanding_claims(uint64_t *free_pages, uint64_t > > *outstanding_pages); > > > > +/* Static Memory */ > > +struct page_info *alloc_domstatic_pages(struct domain *d, > > + unsigned long nr_pfns, paddr_t start, unsigned int memflags); > > + > > /* Domain suballocator. These functions are *not* interrupt-safe.*/ > > void init_domheap_pages(paddr_t ps, paddr_t pe); > > struct page_info *alloc_domheap_pages( > > > > Cheers, > > -- > Julien Grall Cheers, Penny
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Tuesday, May 18, 2021 8:13 PM > To: Penny Zheng <Penny.Zheng@arm.com>; Jan Beulich <jbeulich@suse.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages > > Hi Penny, > > On 18/05/2021 09:57, Penny Zheng wrote: > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Tuesday, May 18, 2021 3:35 PM > >> To: Penny Zheng <Penny.Zheng@arm.com> > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > >> <Wei.Chen@arm.com>; nd <nd@arm.com>; xen- > devel@lists.xenproject.org; > >> sstabellini@kernel.org; julien@xen.org > >> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages > >> > >> On 18.05.2021 07:21, Penny Zheng wrote: > >>> --- a/xen/common/page_alloc.c > >>> +++ b/xen/common/page_alloc.c > >>> @@ -2447,6 +2447,9 @@ int assign_pages( > >>> { > >>> ASSERT(page_get_owner(&pg[i]) == NULL); > >>> page_set_owner(&pg[i], d); > >>> + /* use page_set_reserved_owner to set its reserved domain owner. > >> */ > >>> + if ( (pg[i].count_info & PGC_reserved) ) > >>> + page_set_reserved_owner(&pg[i], d); > >> > >> Now this is puzzling: What's the point of setting two owner fields to > >> the same value? I also don't recall you having introduced > >> page_set_reserved_owner() for x86, so how is this going to build there? > >> > > > > Thanks for pointing out that it will fail on x86. > > As for the same value, sure, I shall change it to domid_t domid to record its > reserved owner. > > Only domid is enough for differentiate. > > And even when domain get rebooted, struct domain may be destroyed, but > > domid will stays The same. > > Major user cases for domain on static allocation are referring to the > > whole system are static, No runtime creation. > > One may want to have static memory yet doesn't care about the domid. So I > am not in favor to restrict about the domid unless there is no other way. > The user case you bring up here is that static memory pool? Right now, the user cases are mostly restricted to static system. If we bring runtime allocation, `xl` here, it will add a lot more complexity. But if the system has static behavior, the domid is also static. On rebooting domain from static memory pool, it brings up more discussion, like do we intend to give the memory back to static memory pool when rebooting, if so, ram could be allocated from different place compared with the previous one. And it kinds destroys system static behavior. or not giving back, just store it in global variable `struct page_info *[DOMID]` like the others. > Cheers, > > -- > Julien Grall Cheers Penny Zheng
On 19/05/2021 08:52, Penny Zheng wrote: > Hi Julien Hi Penny, > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Tuesday, May 18, 2021 8:13 PM >> To: Penny Zheng <Penny.Zheng@arm.com>; Jan Beulich <jbeulich@suse.com> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >> <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org >> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages >> >> Hi Penny, >> >> On 18/05/2021 09:57, Penny Zheng wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: Tuesday, May 18, 2021 3:35 PM >>>> To: Penny Zheng <Penny.Zheng@arm.com> >>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >>>> <Wei.Chen@arm.com>; nd <nd@arm.com>; xen- >> devel@lists.xenproject.org; >>>> sstabellini@kernel.org; julien@xen.org >>>> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages >>>> >>>> On 18.05.2021 07:21, Penny Zheng wrote: >>>>> --- a/xen/common/page_alloc.c >>>>> +++ b/xen/common/page_alloc.c >>>>> @@ -2447,6 +2447,9 @@ int assign_pages( >>>>> { >>>>> ASSERT(page_get_owner(&pg[i]) == NULL); >>>>> page_set_owner(&pg[i], d); >>>>> + /* use page_set_reserved_owner to set its reserved domain owner. >>>> */ >>>>> + if ( (pg[i].count_info & PGC_reserved) ) >>>>> + page_set_reserved_owner(&pg[i], d); >>>> >>>> Now this is puzzling: What's the point of setting two owner fields to >>>> the same value? I also don't recall you having introduced >>>> page_set_reserved_owner() for x86, so how is this going to build there? >>>> >>> >>> Thanks for pointing out that it will fail on x86. >>> As for the same value, sure, I shall change it to domid_t domid to record its >> reserved owner. >>> Only domid is enough for differentiate. >>> And even when domain get rebooted, struct domain may be destroyed, but >>> domid will stays The same. >>> Major user cases for domain on static allocation are referring to the >>> whole system are static, No runtime creation. >> >> One may want to have static memory yet doesn't care about the domid. So I >> am not in favor to restrict about the domid unless there is no other way. >> > > The user case you bring up here is that static memory pool? No. The use case I am talking about is an user who wants to give a specific memory region to the guest but doesn't care about which domid was allocated to the guest. > > Right now, the user cases are mostly restricted to static system. > If we bring runtime allocation, `xl` here, it will add a lot more complexity. > But if the system has static behavior, the domid is also static. I read this as the admin would have to specify the domain ID in the Device-Tree. Is that what you meant? If so, then I don't see why we should mandate that. I would mind less if by static you mean the domid will be allocated by Xen and then not changed accross reboot. > > On rebooting domain from static memory pool, it brings up more discussion, like > do we intend to give the memory back to static memory pool when rebooting, > if so, ram could be allocated from different place compared with the previous one. You should have all the information in the Device-Tree to re-assign the correct regions. So why would it be allocated from a different place? Cheers,
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, May 18, 2021 7:23 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; julien@xen.org > Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages > > On 18.05.2021 10:57, Penny Zheng wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Tuesday, May 18, 2021 3:35 PM > >> > >> On 18.05.2021 07:21, Penny Zheng wrote: > >>> --- a/xen/common/page_alloc.c > >>> +++ b/xen/common/page_alloc.c > >>> @@ -2447,6 +2447,9 @@ int assign_pages( > >>> { > >>> ASSERT(page_get_owner(&pg[i]) == NULL); > >>> page_set_owner(&pg[i], d); > >>> + /* use page_set_reserved_owner to set its reserved domain owner. > >> */ > >>> + if ( (pg[i].count_info & PGC_reserved) ) > >>> + page_set_reserved_owner(&pg[i], d); > >> > >> Now this is puzzling: What's the point of setting two owner fields to > >> the same value? I also don't recall you having introduced > >> page_set_reserved_owner() for x86, so how is this going to build there? > >> > > > > Thanks for pointing out that it will fail on x86. > > As for the same value, sure, I shall change it to domid_t domid to record its > reserved owner. > > Only domid is enough for differentiate. > > And even when domain get rebooted, struct domain may be destroyed, but > > domid will stays The same. > > Will it? Are you intending to put in place restrictions that make it impossible > for the ID to get re-used by another domain? > > > Major user cases for domain on static allocation are referring to the > > whole system are static, No runtime creation. > > Right, but that's not currently enforced afaics. If you would enforce it, it may > simplify a number of things. > > >>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( > >>> return pg; > >>> } > >>> > >>> +/* > >>> + * Allocate nr_pfns contiguous pages, starting at #start, of static > >>> +memory, > >>> + * then assign them to one specific domain #d. > >>> + * It is the equivalent of alloc_domheap_pages for static memory. > >>> + */ > >>> +struct page_info *alloc_domstatic_pages( > >>> + struct domain *d, unsigned long nr_pfns, paddr_t start, > >>> + unsigned int memflags) > >>> +{ > >>> + struct page_info *pg = NULL; > >>> + unsigned long dma_size; > >>> + > >>> + ASSERT(!in_irq()); > >>> + > >>> + if ( memflags & MEMF_no_owner ) > >>> + memflags |= MEMF_no_refcount; > >>> + > >>> + if ( !dma_bitsize ) > >>> + memflags &= ~MEMF_no_dma; > >>> + else > >>> + { > >>> + dma_size = 1ul << bits_to_zone(dma_bitsize); > >>> + /* Starting address shall meet the DMA limitation. */ > >>> + if ( dma_size && start < dma_size ) > >>> + return NULL; > >> > >> It is the entire range (i.e. in particular the last byte) which needs > >> to meet such a restriction. I'm not convinced though that DMA width > >> restrictions and static allocation are sensible to coexist. > >> > > > > FWIT, if starting address meets the limitation, the last byte, greater > > than starting address shall meet it too. > > I'm afraid I don't know what you're meaning to tell me here. > Referring to alloc_domheap_pages, if `dma_bitsize` is none-zero value, it will use alloc_heap_pages to allocate pages from [dma_zone + 1, zone_hi], `dma_zone + 1` pointing to address larger than 2^(dma_zone + 1). So I was setting address limitation for the starting address. > Jan Cheers Penny
On 21.05.2021 08:41, Penny Zheng wrote: > Hi Jan > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, May 18, 2021 7:23 PM >> To: Penny Zheng <Penny.Zheng@arm.com> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >> <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; julien@xen.org >> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages >> >> On 18.05.2021 10:57, Penny Zheng wrote: >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: Tuesday, May 18, 2021 3:35 PM >>>> >>>> On 18.05.2021 07:21, Penny Zheng wrote: >>>>> --- a/xen/common/page_alloc.c >>>>> +++ b/xen/common/page_alloc.c >>>>> @@ -2447,6 +2447,9 @@ int assign_pages( >>>>> { >>>>> ASSERT(page_get_owner(&pg[i]) == NULL); >>>>> page_set_owner(&pg[i], d); >>>>> + /* use page_set_reserved_owner to set its reserved domain owner. >>>> */ >>>>> + if ( (pg[i].count_info & PGC_reserved) ) >>>>> + page_set_reserved_owner(&pg[i], d); >>>> >>>> Now this is puzzling: What's the point of setting two owner fields to >>>> the same value? I also don't recall you having introduced >>>> page_set_reserved_owner() for x86, so how is this going to build there? >>>> >>> >>> Thanks for pointing out that it will fail on x86. >>> As for the same value, sure, I shall change it to domid_t domid to record its >> reserved owner. >>> Only domid is enough for differentiate. >>> And even when domain get rebooted, struct domain may be destroyed, but >>> domid will stays The same. >> >> Will it? Are you intending to put in place restrictions that make it impossible >> for the ID to get re-used by another domain? >> >>> Major user cases for domain on static allocation are referring to the >>> whole system are static, No runtime creation. >> >> Right, but that's not currently enforced afaics. If you would enforce it, it may >> simplify a number of things. >> >>>>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( >>>>> return pg; >>>>> } >>>>> >>>>> +/* >>>>> + * Allocate nr_pfns contiguous pages, starting at #start, of static >>>>> +memory, >>>>> + * then assign them to one specific domain #d. >>>>> + * It is the equivalent of alloc_domheap_pages for static memory. >>>>> + */ >>>>> +struct page_info *alloc_domstatic_pages( >>>>> + struct domain *d, unsigned long nr_pfns, paddr_t start, >>>>> + unsigned int memflags) >>>>> +{ >>>>> + struct page_info *pg = NULL; >>>>> + unsigned long dma_size; >>>>> + >>>>> + ASSERT(!in_irq()); >>>>> + >>>>> + if ( memflags & MEMF_no_owner ) >>>>> + memflags |= MEMF_no_refcount; >>>>> + >>>>> + if ( !dma_bitsize ) >>>>> + memflags &= ~MEMF_no_dma; >>>>> + else >>>>> + { >>>>> + dma_size = 1ul << bits_to_zone(dma_bitsize); >>>>> + /* Starting address shall meet the DMA limitation. */ >>>>> + if ( dma_size && start < dma_size ) >>>>> + return NULL; >>>> >>>> It is the entire range (i.e. in particular the last byte) which needs >>>> to meet such a restriction. I'm not convinced though that DMA width >>>> restrictions and static allocation are sensible to coexist. >>>> >>> >>> FWIT, if starting address meets the limitation, the last byte, greater >>> than starting address shall meet it too. >> >> I'm afraid I don't know what you're meaning to tell me here. >> > > Referring to alloc_domheap_pages, if `dma_bitsize` is none-zero value, > it will use alloc_heap_pages to allocate pages from [dma_zone + 1, > zone_hi], `dma_zone + 1` pointing to address larger than 2^(dma_zone + 1). > So I was setting address limitation for the starting address. But does this zone concept apply to static pages at all? Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Friday, May 21, 2021 3:09 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; julien@xen.org > Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages > > On 21.05.2021 08:41, Penny Zheng wrote: > > Hi Jan > > > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Tuesday, May 18, 2021 7:23 PM > >> To: Penny Zheng <Penny.Zheng@arm.com> > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > >> <Wei.Chen@arm.com>; nd <nd@arm.com>; xen-devel@lists.xenproject.org; > >> sstabellini@kernel.org; julien@xen.org > >> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages > >> > >> On 18.05.2021 10:57, Penny Zheng wrote: > >>>> From: Jan Beulich <jbeulich@suse.com> > >>>> Sent: Tuesday, May 18, 2021 3:35 PM > >>>> > >>>> On 18.05.2021 07:21, Penny Zheng wrote: > >>>>> --- a/xen/common/page_alloc.c > >>>>> +++ b/xen/common/page_alloc.c > >>>>> @@ -2447,6 +2447,9 @@ int assign_pages( > >>>>> { > >>>>> ASSERT(page_get_owner(&pg[i]) == NULL); > >>>>> page_set_owner(&pg[i], d); > >>>>> + /* use page_set_reserved_owner to set its reserved domain owner. > >>>> */ > >>>>> + if ( (pg[i].count_info & PGC_reserved) ) > >>>>> + page_set_reserved_owner(&pg[i], d); > >>>> > >>>> Now this is puzzling: What's the point of setting two owner fields > >>>> to the same value? I also don't recall you having introduced > >>>> page_set_reserved_owner() for x86, so how is this going to build there? > >>>> > >>> > >>> Thanks for pointing out that it will fail on x86. > >>> As for the same value, sure, I shall change it to domid_t domid to > >>> record its > >> reserved owner. > >>> Only domid is enough for differentiate. > >>> And even when domain get rebooted, struct domain may be destroyed, > >>> but domid will stays The same. > >> > >> Will it? Are you intending to put in place restrictions that make it > >> impossible for the ID to get re-used by another domain? > >> > >>> Major user cases for domain on static allocation are referring to > >>> the whole system are static, No runtime creation. > >> > >> Right, but that's not currently enforced afaics. If you would enforce > >> it, it may simplify a number of things. > >> > >>>>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( > >>>>> return pg; > >>>>> } > >>>>> > >>>>> +/* > >>>>> + * Allocate nr_pfns contiguous pages, starting at #start, of > >>>>> +static memory, > >>>>> + * then assign them to one specific domain #d. > >>>>> + * It is the equivalent of alloc_domheap_pages for static memory. > >>>>> + */ > >>>>> +struct page_info *alloc_domstatic_pages( > >>>>> + struct domain *d, unsigned long nr_pfns, paddr_t start, > >>>>> + unsigned int memflags) > >>>>> +{ > >>>>> + struct page_info *pg = NULL; > >>>>> + unsigned long dma_size; > >>>>> + > >>>>> + ASSERT(!in_irq()); > >>>>> + > >>>>> + if ( memflags & MEMF_no_owner ) > >>>>> + memflags |= MEMF_no_refcount; > >>>>> + > >>>>> + if ( !dma_bitsize ) > >>>>> + memflags &= ~MEMF_no_dma; > >>>>> + else > >>>>> + { > >>>>> + dma_size = 1ul << bits_to_zone(dma_bitsize); > >>>>> + /* Starting address shall meet the DMA limitation. */ > >>>>> + if ( dma_size && start < dma_size ) > >>>>> + return NULL; > >>>> > >>>> It is the entire range (i.e. in particular the last byte) which > >>>> needs to meet such a restriction. I'm not convinced though that DMA > >>>> width restrictions and static allocation are sensible to coexist. > >>>> > >>> > >>> FWIT, if starting address meets the limitation, the last byte, > >>> greater than starting address shall meet it too. > >> > >> I'm afraid I don't know what you're meaning to tell me here. > >> > > > > Referring to alloc_domheap_pages, if `dma_bitsize` is none-zero value, > > it will use alloc_heap_pages to allocate pages from [dma_zone + 1, > > zone_hi], `dma_zone + 1` pointing to address larger than 2^(dma_zone + 1). > > So I was setting address limitation for the starting address. > > But does this zone concept apply to static pages at all? > Oh, so sorry. I finally got what you were asking here. Hmm, I was using the logic in bits_to_zone to do the address bits translation. But I got, it will bring confusion. I'll fix it. Thx. > Jan
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 0eb9f22a00..f1f1296a61 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2447,6 +2447,9 @@ int assign_pages( { ASSERT(page_get_owner(&pg[i]) == NULL); page_set_owner(&pg[i], d); + /* use page_set_reserved_owner to set its reserved domain owner. */ + if ( (pg[i].count_info & PGC_reserved) ) + page_set_reserved_owner(&pg[i], d); smp_wmb(); /* Domain pointer must be visible before updating refcnt. */ pg[i].count_info = (pg[i].count_info & PGC_extra) | PGC_allocated | 1; @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages( return pg; } +/* + * Allocate nr_pfns contiguous pages, starting at #start, of static memory, + * then assign them to one specific domain #d. + * It is the equivalent of alloc_domheap_pages for static memory. + */ +struct page_info *alloc_domstatic_pages( + struct domain *d, unsigned long nr_pfns, paddr_t start, + unsigned int memflags) +{ + struct page_info *pg = NULL; + unsigned long dma_size; + + ASSERT(!in_irq()); + + if ( memflags & MEMF_no_owner ) + memflags |= MEMF_no_refcount; + + if ( !dma_bitsize ) + memflags &= ~MEMF_no_dma; + else + { + dma_size = 1ul << bits_to_zone(dma_bitsize); + /* Starting address shall meet the DMA limitation. */ + if ( dma_size && start < dma_size ) + return NULL; + } + + pg = alloc_staticmem_pages(nr_pfns, start, memflags); + if ( !pg ) + return NULL; + + if ( d && !(memflags & MEMF_no_owner) ) + { + if ( memflags & MEMF_no_refcount ) + { + unsigned long i; + + for ( i = 0; i < nr_pfns; i++ ) + pg[i].count_info = PGC_extra; + } + if ( assign_pages(d, pg, nr_pfns, memflags) ) + { + free_staticmem_pages(pg, nr_pfns, memflags & MEMF_no_scrub); + return NULL; + } + } + + return pg; +} + void free_domheap_pages(struct page_info *pg, unsigned int order) { struct domain *d = page_get_owner(pg); diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index dcf9daaa46..e45987f0ed 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -111,6 +111,10 @@ unsigned long __must_check domain_adjust_tot_pages(struct domain *d, int domain_set_outstanding_pages(struct domain *d, unsigned long pages); void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages); +/* Static Memory */ +struct page_info *alloc_domstatic_pages(struct domain *d, + unsigned long nr_pfns, paddr_t start, unsigned int memflags); + /* Domain suballocator. These functions are *not* interrupt-safe.*/ void init_domheap_pages(paddr_t ps, paddr_t pe); struct page_info *alloc_domheap_pages(
alloc_domstatic_pages is the equivalent of alloc_domheap_pages for static mmeory, and it is to allocate nr_pfns pages of static memory and assign them to one specific domain. It uses alloc_staticmen_pages to get nr_pages pages of static memory, then on success, it will use assign_pages to assign those pages to one specific domain, including using page_set_reserved_owner to set its reserved domain owner. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- xen/common/page_alloc.c | 53 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/mm.h | 4 ++++ 2 files changed, 57 insertions(+)