| Message ID | 20210607024318.3988467-7-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
On 07.06.2021 04:43, Penny Zheng wrote: > alloc_staticmem_pages aims to allocate nr_mfns contiguous pages of > static memory. And it is the equivalent of alloc_heap_pages for static > memory. Here only covers allocating at specified starting address. > > For each page, it shall check if the page is reserved(PGC_reserved) > and free. It shall also do a set of necessary initialization, which are > mostly the same ones in alloc_heap_pages, like, following the same > cache-coherency policy and turning page status into PGC_state_inuse, etc. > > alloc_domstatic_pages is the equivalent of alloc_domheap_pages for > static mmeory, and it is to allocate nr_mfns pages of static memory > and assign them to one specific domain. > > It uses alloc_staticmen_pages to get nr_mfns pages of static memory, > then on success, it will use assign_pages_nr to assign those pages to > one specific domain. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > changes v2: > - use mfn_valid() to do validation > - change pfn-named to mfn-named > - put CONFIG_STATIC_ALLOCATION around to remove dead codes > - correct off-by-one indentation > - remove meaningless MEMF_no_owner case > - leave zone concept out of DMA limitation check > --- > xen/common/page_alloc.c | 129 ++++++++++++++++++++++++++++++++++++++++ > xen/include/xen/mm.h | 2 + > 2 files changed, 131 insertions(+) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index e244d2e52e..a0eea5f1a4 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( > return pg; > } > > +#ifdef CONFIG_STATIC_ALLOCATION > +/* > + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. > + * It is the equivalent of alloc_heap_pages for static memory > + */ > +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, > + mfn_t smfn, > + unsigned int memflags) > +{ > + bool need_tlbflush = false; > + uint32_t tlbflush_timestamp = 0; > + unsigned long i; > + struct page_info *pg; > + > + /* For now, it only supports allocating at specified address. */ > + if ( !mfn_valid(smfn) || !nr_mfns ) > + { > + printk(XENLOG_ERR > + "Invalid %lu static memory starting at %"PRI_mfn"\n", Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't think I would recognize that the "0" is the count of pages. > + nr_mfns, mfn_x(smfn)); > + return NULL; > + } > + pg = mfn_to_page(smfn); > + > + for ( i = 0; i < nr_mfns; i++ ) > + { > + /* > + * Reference count must continuously be zero for free pages > + * of static memory(PGC_reserved). > + */ > + ASSERT(pg[i].count_info & PGC_reserved); What logic elsewhere guarantees that this will hold? ASSERT()s are to verify that assumptions are met. But I don't think you can sensibly assume the caller knows the range is reserved (and free), or else you could get away without any allocation function. > + if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free ) > + { > + printk(XENLOG_ERR > + "Reference count must continuously be zero for free pages" > + "pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x\n", > + i, mfn_x(page_to_mfn(pg + i)), > + pg[i].count_info, pg[i].tlbflush_timestamp); > + BUG(); > + } The same applies here at least until proper locking gets added, which I guess is happening in the next patch when really it would need to happen right here. Furthermore I don't see why you don't fold ASSERT() and if into if ( pg[i].count_info != (PGC_state_free | PGC_reserved) ) After all PGC_reserved is not similar to PGC_need_scrub, which alloc_heap_pages() masks out the way you also have it here. As to the printk() - the extra verbosity compared to the original isn't helpful or necessary imo. The message needs to be distinguishable from the other one, yes, so it would better mention "static" in some way. But the prefix you have is too long for my taste (and lacks a separating blank anyway). As a separate matter - have you given up on the concept of reserving particular memory ranges for _particular_ guests? The cover letter, saying "Static Allocation refers to system or sub-system(domains) for which memory areas are pre-defined by configuration using physical address ranges" as the very first thing, doesn't seem to suggest so. > + if ( !(memflags & MEMF_no_tlbflush) ) > + accumulate_tlbflush(&need_tlbflush, &pg[i], > + &tlbflush_timestamp); > + > + /* > + * Preserve flag PGC_reserved and change page state > + * to PGC_state_inuse. > + */ > + pg[i].count_info = (pg[i].count_info & PGC_reserved) | PGC_state_inuse; Why not pg[i].count_info = PGC_state_inuse | PGC_reserved; ? Again, PGC_reserved is sufficiently different from PGC_need_scrub. > + /* Initialise fields which have other uses for free pages. */ > + pg[i].u.inuse.type_info = 0; > + page_set_owner(&pg[i], NULL); > + > + /* > + * Ensure cache and RAM are consistent for platforms where the > + * guest can control its own visibility of/through the cache. > + */ > + flush_page_to_ram(mfn_x(page_to_mfn(&pg[i])), > + !(memflags & MEMF_no_icache_flush)); > + } > + > + if ( need_tlbflush ) > + filtered_flush_tlb_mask(tlbflush_timestamp); Depending on whether static pages have a designated owner, this may (as suggested before) not be necessary. > @@ -2326,7 +2395,11 @@ int assign_pages_nr( > > for ( i = 0; i < nr_pfns; i++ ) > { > +#ifdef CONFIG_STATIC_ALLOCATION > + ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved))); > +#else > ASSERT(!(pg[i].count_info & ~PGC_extra)); > +#endif > if ( pg[i].count_info & PGC_extra ) > extra_pages++; > } > @@ -2365,7 +2438,12 @@ int assign_pages_nr( > page_set_owner(&pg[i], d); > smp_wmb(); /* Domain pointer must be visible before updating refcnt. */ > pg[i].count_info = > +#ifdef CONFIG_STATIC_ALLOCATION > + (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1; > +#else > (pg[i].count_info & PGC_extra) | PGC_allocated | 1; > +#endif Both hunks' #ifdef-ary needs to be avoided, e.g. by #ifndef CONFIG_STATIC_ALLOCATION # define PGC_reserved 0 #endif near the top of the file. > @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages( > return pg; > } > > +#ifdef CONFIG_STATIC_ALLOCATION > +/* > + * Allocate nr_mfns contiguous pages, starting at #smfn, 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_mfns, mfn_t smfn, > + unsigned int memflags) > +{ > + struct page_info *pg = NULL; > + unsigned long dma_size; > + > + ASSERT(!in_irq()); > + > + if ( !dma_bitsize ) > + memflags &= ~MEMF_no_dma; > + else > + { > + if ( (dma_bitsize - PAGE_SHIFT) > 0 ) > + { > + dma_size = 1ul << (dma_bitsize - PAGE_SHIFT); > + /* Starting address shall meet the DMA limitation. */ > + if ( mfn_x(smfn) < dma_size ) > + return NULL; I think I did ask this on v1 already: Why the first page? Static memory regions, unlike buddy allocator zones, can cross power-of-2 address boundaries. Hence it ought to be the last page that gets checked for fitting address width restriction requirements. And then - is this necessary at all? Shouldn't "pre-defined by configuration using physical address ranges" imply the memory designated for a guest fits its DMA needs? Jan
Hi > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Thursday, June 10, 2021 6:23 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; julien@xen.org > Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and > alloc_domstatic_pages > > On 07.06.2021 04:43, Penny Zheng wrote: > > alloc_staticmem_pages aims to allocate nr_mfns contiguous pages of > > static memory. And it is the equivalent of alloc_heap_pages for static > > memory. Here only covers allocating at specified starting address. > > > > For each page, it shall check if the page is reserved(PGC_reserved) > > and free. It shall also do a set of necessary initialization, which > > are mostly the same ones in alloc_heap_pages, like, following the same > > cache-coherency policy and turning page status into PGC_state_inuse, etc. > > > > alloc_domstatic_pages is the equivalent of alloc_domheap_pages for > > static mmeory, and it is to allocate nr_mfns pages of static memory > > and assign them to one specific domain. > > > > It uses alloc_staticmen_pages to get nr_mfns pages of static memory, > > then on success, it will use assign_pages_nr to assign those pages to > > one specific domain. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > changes v2: > > - use mfn_valid() to do validation > > - change pfn-named to mfn-named > > - put CONFIG_STATIC_ALLOCATION around to remove dead codes > > - correct off-by-one indentation > > - remove meaningless MEMF_no_owner case > > - leave zone concept out of DMA limitation check > > --- > > xen/common/page_alloc.c | 129 > ++++++++++++++++++++++++++++++++++++++++ > > xen/include/xen/mm.h | 2 + > > 2 files changed, 131 insertions(+) > > > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index > > e244d2e52e..a0eea5f1a4 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( > > return pg; > > } > > > > +#ifdef CONFIG_STATIC_ALLOCATION > > +/* > > + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. > > + * It is the equivalent of alloc_heap_pages for static memory */ > > +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, > > + mfn_t smfn, > > + unsigned int memflags) > > +{ > > + bool need_tlbflush = false; > > + uint32_t tlbflush_timestamp = 0; > > + unsigned long i; > > + struct page_info *pg; > > + > > + /* For now, it only supports allocating at specified address. */ > > + if ( !mfn_valid(smfn) || !nr_mfns ) > > + { > > + printk(XENLOG_ERR > > + "Invalid %lu static memory starting at %"PRI_mfn"\n", > > Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't > think I would recognize that the "0" is the count of pages. > Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? > > + nr_mfns, mfn_x(smfn)); > > + return NULL; > > + } > > + pg = mfn_to_page(smfn); > > + > > + for ( i = 0; i < nr_mfns; i++ ) > > + { > > + /* > > + * Reference count must continuously be zero for free pages > > + * of static memory(PGC_reserved). > > + */ > > + ASSERT(pg[i].count_info & PGC_reserved); > > What logic elsewhere guarantees that this will hold? ASSERT()s are to verify > that assumptions are met. But I don't think you can sensibly assume the caller > knows the range is reserved (and free), or else you could get away without any > allocation function. > The caller shall only call alloc_staticmem_pages when it knows range is reserved, like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages for now. Normal domain uses alloc_heap_pages/alloc_domheap_pages to do the allocation. Proper initialization must happen before allocation. Init_staticmem_pages shall be called before alloc_staticmem_pages. And we set PGC_reserved in init_staticmem_pages. So here I use ASSERT()s to check whether above proper initialization is met. > > + if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free ) > > + { > > + printk(XENLOG_ERR > > + "Reference count must continuously be zero for free pages" > > + "pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x\n", > > + i, mfn_x(page_to_mfn(pg + i)), > > + pg[i].count_info, pg[i].tlbflush_timestamp); > > + BUG(); > > + } > > The same applies here at least until proper locking gets added, which I guess is > happening in the next patch when really it would need to happen right here. > Ok, I will combine two commits together, and add locking here. I thought the content of this commit is a little bit too much, so maybe adding the proper lock shall be created as a new patch.
On 06.07.2021 07:58, Penny Zheng wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Thursday, June 10, 2021 6:23 PM >> >> On 07.06.2021 04:43, Penny Zheng wrote: >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( >>> return pg; >>> } >>> >>> +#ifdef CONFIG_STATIC_ALLOCATION >>> +/* >>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. >>> + * It is the equivalent of alloc_heap_pages for static memory */ >>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, >>> + mfn_t smfn, >>> + unsigned int memflags) >>> +{ >>> + bool need_tlbflush = false; >>> + uint32_t tlbflush_timestamp = 0; >>> + unsigned long i; >>> + struct page_info *pg; >>> + >>> + /* For now, it only supports allocating at specified address. */ >>> + if ( !mfn_valid(smfn) || !nr_mfns ) >>> + { >>> + printk(XENLOG_ERR >>> + "Invalid %lu static memory starting at %"PRI_mfn"\n", >> >> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't >> think I would recognize that the "0" is the count of pages. > > Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? This still doesn't convey _both_ parts of the if() that would cause the log message to be issued. >>> + nr_mfns, mfn_x(smfn)); >>> + return NULL; >>> + } >>> + pg = mfn_to_page(smfn); >>> + >>> + for ( i = 0; i < nr_mfns; i++ ) >>> + { >>> + /* >>> + * Reference count must continuously be zero for free pages >>> + * of static memory(PGC_reserved). >>> + */ >>> + ASSERT(pg[i].count_info & PGC_reserved); >> >> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify >> that assumptions are met. But I don't think you can sensibly assume the caller >> knows the range is reserved (and free), or else you could get away without any >> allocation function. > > The caller shall only call alloc_staticmem_pages when it knows range is reserved, > like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages > for now. If the caller knows the static ranges, this isn't really "allocation". I.e. I then question the need for "allocating" in the first place. >>> + if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free ) >>> + { >>> + printk(XENLOG_ERR >>> + "Reference count must continuously be zero for free pages" >>> + "pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x\n", >>> + i, mfn_x(page_to_mfn(pg + i)), >>> + pg[i].count_info, pg[i].tlbflush_timestamp); >>> + BUG(); >>> + } >> >> The same applies here at least until proper locking gets added, which I guess is >> happening in the next patch when really it would need to happen right here. >> > > Ok, I will combine two commits together, and add locking here. > I thought the content of this commit is a little bit too much, so maybe > adding the proper lock shall be created as a new patch.
Hi Jan & Penny, On 06/07/2021 07:53, Jan Beulich wrote: > On 06.07.2021 07:58, Penny Zheng wrote: >>> From: Jan Beulich <jbeulich@suse.com> >>> Sent: Thursday, June 10, 2021 6:23 PM >>> >>> On 07.06.2021 04:43, Penny Zheng wrote: >>>> --- a/xen/common/page_alloc.c >>>> +++ b/xen/common/page_alloc.c >>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( >>>> return pg; >>>> } >>>> >>>> +#ifdef CONFIG_STATIC_ALLOCATION >>>> +/* >>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. >>>> + * It is the equivalent of alloc_heap_pages for static memory */ >>>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, >>>> + mfn_t smfn, >>>> + unsigned int memflags) >>>> +{ >>>> + bool need_tlbflush = false; >>>> + uint32_t tlbflush_timestamp = 0; >>>> + unsigned long i; >>>> + struct page_info *pg; >>>> + >>>> + /* For now, it only supports allocating at specified address. */ >>>> + if ( !mfn_valid(smfn) || !nr_mfns ) >>>> + { >>>> + printk(XENLOG_ERR >>>> + "Invalid %lu static memory starting at %"PRI_mfn"\n", >>> >>> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't >>> think I would recognize that the "0" is the count of pages. >> >> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? > > This still doesn't convey _both_ parts of the if() that would cause > the log message to be issued. > >>>> + nr_mfns, mfn_x(smfn)); >>>> + return NULL; >>>> + } >>>> + pg = mfn_to_page(smfn); >>>> + >>>> + for ( i = 0; i < nr_mfns; i++ ) >>>> + { >>>> + /* >>>> + * Reference count must continuously be zero for free pages >>>> + * of static memory(PGC_reserved). >>>> + */ >>>> + ASSERT(pg[i].count_info & PGC_reserved); >>> >>> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify >>> that assumptions are met. But I don't think you can sensibly assume the caller >>> knows the range is reserved (and free), or else you could get away without any >>> allocation function. >> >> The caller shall only call alloc_staticmem_pages when it knows range is reserved, >> like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages >> for now. > > If the caller knows the static ranges, this isn't really "allocation". > I.e. I then question the need for "allocating" in the first place. We still need to setup the page so the reference counting works properly. So can you clarify whether you are objecting on the name? If yes, do you have a better suggestion? Cheers,
On 06.07.2021 11:39, Julien Grall wrote: > Hi Jan & Penny, > > On 06/07/2021 07:53, Jan Beulich wrote: >> On 06.07.2021 07:58, Penny Zheng wrote: >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: Thursday, June 10, 2021 6:23 PM >>>> >>>> On 07.06.2021 04:43, Penny Zheng wrote: >>>>> --- a/xen/common/page_alloc.c >>>>> +++ b/xen/common/page_alloc.c >>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( >>>>> return pg; >>>>> } >>>>> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION >>>>> +/* >>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. >>>>> + * It is the equivalent of alloc_heap_pages for static memory */ >>>>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, >>>>> + mfn_t smfn, >>>>> + unsigned int memflags) >>>>> +{ >>>>> + bool need_tlbflush = false; >>>>> + uint32_t tlbflush_timestamp = 0; >>>>> + unsigned long i; >>>>> + struct page_info *pg; >>>>> + >>>>> + /* For now, it only supports allocating at specified address. */ >>>>> + if ( !mfn_valid(smfn) || !nr_mfns ) >>>>> + { >>>>> + printk(XENLOG_ERR >>>>> + "Invalid %lu static memory starting at %"PRI_mfn"\n", >>>> >>>> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't >>>> think I would recognize that the "0" is the count of pages. >>> >>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? >> >> This still doesn't convey _both_ parts of the if() that would cause >> the log message to be issued. >> >>>>> + nr_mfns, mfn_x(smfn)); >>>>> + return NULL; >>>>> + } >>>>> + pg = mfn_to_page(smfn); >>>>> + >>>>> + for ( i = 0; i < nr_mfns; i++ ) >>>>> + { >>>>> + /* >>>>> + * Reference count must continuously be zero for free pages >>>>> + * of static memory(PGC_reserved). >>>>> + */ >>>>> + ASSERT(pg[i].count_info & PGC_reserved); >>>> >>>> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify >>>> that assumptions are met. But I don't think you can sensibly assume the caller >>>> knows the range is reserved (and free), or else you could get away without any >>>> allocation function. >>> >>> The caller shall only call alloc_staticmem_pages when it knows range is reserved, >>> like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages >>> for now. >> >> If the caller knows the static ranges, this isn't really "allocation". >> I.e. I then question the need for "allocating" in the first place. > > We still need to setup the page so the reference counting works > properly. So can you clarify whether you are objecting on the name? If > yes, do you have a better suggestion? It's not the name alone but the disconnect between name and actual behavior. Note that here we've started from an ASSERT(), which is reasonable to have if the caller knows where static pages are, but which should be converted to an if() when talking about allocation (i.e. the caller may not have enough knowledge). If it's not really allocation, how about naming it "acquire" or "obtain" (and the config option perhaps STATIC_MEMORY)? Jan
Hi, On 06/07/2021 10:59, Jan Beulich wrote: > On 06.07.2021 11:39, Julien Grall wrote: >> Hi Jan & Penny, >> >> On 06/07/2021 07:53, Jan Beulich wrote: >>> On 06.07.2021 07:58, Penny Zheng wrote: >>>>> From: Jan Beulich <jbeulich@suse.com> >>>>> Sent: Thursday, June 10, 2021 6:23 PM >>>>> >>>>> On 07.06.2021 04:43, Penny Zheng wrote: >>>>>> --- a/xen/common/page_alloc.c >>>>>> +++ b/xen/common/page_alloc.c >>>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( >>>>>> return pg; >>>>>> } >>>>>> >>>>>> +#ifdef CONFIG_STATIC_ALLOCATION >>>>>> +/* >>>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. >>>>>> + * It is the equivalent of alloc_heap_pages for static memory */ >>>>>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, >>>>>> + mfn_t smfn, >>>>>> + unsigned int memflags) >>>>>> +{ >>>>>> + bool need_tlbflush = false; >>>>>> + uint32_t tlbflush_timestamp = 0; >>>>>> + unsigned long i; >>>>>> + struct page_info *pg; >>>>>> + >>>>>> + /* For now, it only supports allocating at specified address. */ >>>>>> + if ( !mfn_valid(smfn) || !nr_mfns ) >>>>>> + { >>>>>> + printk(XENLOG_ERR >>>>>> + "Invalid %lu static memory starting at %"PRI_mfn"\n", >>>>> >>>>> Reading a log containing e.g. "Invalid 0 static memory starting at ..." I don't >>>>> think I would recognize that the "0" is the count of pages. >>>> >>>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? >>> >>> This still doesn't convey _both_ parts of the if() that would cause >>> the log message to be issued. >>> >>>>>> + nr_mfns, mfn_x(smfn)); >>>>>> + return NULL; >>>>>> + } >>>>>> + pg = mfn_to_page(smfn); >>>>>> + >>>>>> + for ( i = 0; i < nr_mfns; i++ ) >>>>>> + { >>>>>> + /* >>>>>> + * Reference count must continuously be zero for free pages >>>>>> + * of static memory(PGC_reserved). >>>>>> + */ >>>>>> + ASSERT(pg[i].count_info & PGC_reserved); >>>>> >>>>> What logic elsewhere guarantees that this will hold? ASSERT()s are to verify >>>>> that assumptions are met. But I don't think you can sensibly assume the caller >>>>> knows the range is reserved (and free), or else you could get away without any >>>>> allocation function. >>>> >>>> The caller shall only call alloc_staticmem_pages when it knows range is reserved, >>>> like, alloc_staticmem_pages is only called in the context of alloc_domstatic_pages >>>> for now. >>> >>> If the caller knows the static ranges, this isn't really "allocation". >>> I.e. I then question the need for "allocating" in the first place. >> >> We still need to setup the page so the reference counting works >> properly. So can you clarify whether you are objecting on the name? If >> yes, do you have a better suggestion? > > It's not the name alone but the disconnect between name and actual > behavior. Note that here we've started from an ASSERT(), which is > reasonable to have if the caller knows where static pages are, but > which should be converted to an if() when talking about allocation > (i.e. the caller may not have enough knowledge). If it's not really > allocation, how about naming it "acquire" or "obtain" (and the > config option perhaps STATIC_MEMORY)? The caller will fetch the information from the Device Tree but it doesn't sanity check it. I think it can be easy to make mistake as the information will be scattered in various node of the DT. So I think it would be better if we have a check in both prod and debug build to ease the integration. Regarding the name itself, anyone of the one you suggested are fine with me because to me the difference between them is too subtle to be understood by most of the users. Cheers,
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, July 6, 2021 2:54 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; Julien Grall <julien@xen.org> > Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and > alloc_domstatic_pages > > On 06.07.2021 07:58, Penny Zheng wrote: > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Thursday, June 10, 2021 6:23 PM > >> > >> On 07.06.2021 04:43, Penny Zheng wrote: > >>> --- a/xen/common/page_alloc.c > >>> +++ b/xen/common/page_alloc.c > >>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( > >>> return pg; > >>> } > >>> > >>> +#ifdef CONFIG_STATIC_ALLOCATION > >>> +/* > >>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. > >>> + * It is the equivalent of alloc_heap_pages for static memory */ > >>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, > >>> + mfn_t smfn, > >>> + unsigned int > >>> +memflags) { > >>> + bool need_tlbflush = false; > >>> + uint32_t tlbflush_timestamp = 0; > >>> + unsigned long i; > >>> + struct page_info *pg; > >>> + > >>> + /* For now, it only supports allocating at specified address. */ > >>> + if ( !mfn_valid(smfn) || !nr_mfns ) > >>> + { > >>> + printk(XENLOG_ERR > >>> + "Invalid %lu static memory starting at > >>> + %"PRI_mfn"\n", > >> > >> Reading a log containing e.g. "Invalid 0 static memory starting at > >> ..." I don't think I would recognize that the "0" is the count of pages. > > > > Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? > > This still doesn't convey _both_ parts of the if() that would cause the log > message to be issued. > Sorry. How about " printk(XENLOG_ERR "Either out-of-range static memory starting at %"PRI_mfn"" "or invalid number of pages: %ul.\n", mfn_x(smfn), nr_mfns); " > >>> + nr_mfns, mfn_x(smfn)); > >>> + return NULL; > >>> + } > >>> + pg = mfn_to_page(smfn); > >>> + > >>> + for ( i = 0; i < nr_mfns; i++ ) > >>> + { > >>> + /* > >>> + * Reference count must continuously be zero for free pages > >>> + * of static memory(PGC_reserved). > >>> + */ > >>> + ASSERT(pg[i].count_info & PGC_reserved); > >> > >> What logic elsewhere guarantees that this will hold? ASSERT()s are to > >> verify that assumptions are met. But I don't think you can sensibly > >> assume the caller knows the range is reserved (and free), or else you > >> could get away without any allocation function. > > > > The caller shall only call alloc_staticmem_pages when it knows range > > is reserved, like, alloc_staticmem_pages is only called in the context > > of alloc_domstatic_pages for now. > > If the caller knows the static ranges, this isn't really "allocation". > I.e. I then question the need for "allocating" in the first place. > > >>> + if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free ) > >>> + { > >>> + printk(XENLOG_ERR > >>> + "Reference count must continuously be zero for free pages" > >>> + "pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x\n", > >>> + i, mfn_x(page_to_mfn(pg + i)), > >>> + pg[i].count_info, pg[i].tlbflush_timestamp); > >>> + BUG(); > >>> + } > >> > >> The same applies here at least until proper locking gets added, which > >> I guess is happening in the next patch when really it would need to happen > right here. > >> > > > > Ok, I will combine two commits together, and add locking here. > > I thought the content of this commit is a little bit too much, so > > maybe adding the proper lock shall be created as a new patch.
On 08.07.2021 11:09, Penny Zheng wrote: > Hi Jan > >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, July 6, 2021 2:54 PM >> To: Penny Zheng <Penny.Zheng@arm.com> >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; Julien Grall <julien@xen.org> >> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and >> alloc_domstatic_pages >> >> On 06.07.2021 07:58, Penny Zheng wrote: >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: Thursday, June 10, 2021 6:23 PM >>>> >>>> On 07.06.2021 04:43, Penny Zheng wrote: >>>>> --- a/xen/common/page_alloc.c >>>>> +++ b/xen/common/page_alloc.c >>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( >>>>> return pg; >>>>> } >>>>> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION >>>>> +/* >>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. >>>>> + * It is the equivalent of alloc_heap_pages for static memory */ >>>>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, >>>>> + mfn_t smfn, >>>>> + unsigned int >>>>> +memflags) { >>>>> + bool need_tlbflush = false; >>>>> + uint32_t tlbflush_timestamp = 0; >>>>> + unsigned long i; >>>>> + struct page_info *pg; >>>>> + >>>>> + /* For now, it only supports allocating at specified address. */ >>>>> + if ( !mfn_valid(smfn) || !nr_mfns ) >>>>> + { >>>>> + printk(XENLOG_ERR >>>>> + "Invalid %lu static memory starting at >>>>> + %"PRI_mfn"\n", >>>> >>>> Reading a log containing e.g. "Invalid 0 static memory starting at >>>> ..." I don't think I would recognize that the "0" is the count of pages. >>> >>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? >> >> This still doesn't convey _both_ parts of the if() that would cause the log >> message to be issued. >> > > Sorry. How about > " > printk(XENLOG_ERR > "Either out-of-range static memory starting at %"PRI_mfn"" > "or invalid number of pages: %ul.\n", > mfn_x(smfn), nr_mfns); > " I'm sorry - while now you convey both aspects, the message has become too verbose. What's wrong with "Invalid static memory request: ... pages at ...\"? But I wonder anyway if a log message is appropriate here in the first place. >>>>> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages( >>>>> return pg; >>>>> } >>>>> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION >>>>> +/* >>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, 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_mfns, mfn_t smfn, >>>>> + unsigned int memflags) >>>>> +{ >>>>> + struct page_info *pg = NULL; >>>>> + unsigned long dma_size; >>>>> + >>>>> + ASSERT(!in_irq()); >>>>> + >>>>> + if ( !dma_bitsize ) >>>>> + memflags &= ~MEMF_no_dma; >>>>> + else >>>>> + { >>>>> + if ( (dma_bitsize - PAGE_SHIFT) > 0 ) >>>>> + { >>>>> + dma_size = 1ul << (dma_bitsize - PAGE_SHIFT); >>>>> + /* Starting address shall meet the DMA limitation. */ >>>>> + if ( mfn_x(smfn) < dma_size ) >>>>> + return NULL; >>>> >>>> I think I did ask this on v1 already: Why the first page? Static >>>> memory regions, unlike buddy allocator zones, can cross power-of-2 >>>> address boundaries. Hence it ought to be the last page that gets >>>> checked for fitting address width restriction requirements. >>>> >>>> And then - is this necessary at all? Shouldn't "pre-defined by >>>> configuration using physical address ranges" imply the memory >>>> designated for a guest fits its DMA needs? >>>> >>> >>> Hmmm, In my understanding, here is the DMA restriction when using buddy >> allocator: >>> else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi ) >>> pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, >>> d); dma_zone is restricting the starting buddy allocator zone, so I am >>> thinking that here, it shall restrict the first page. >>> >>> imo, if let user define, it also could be missing DMA restriction? >> >> Did you read my earlier reply? Again: The difference is that ordinary >> allocations (buddies) can't cross zone boundaries. Hence it is irrelevant if you >> check DMA properties on the first or last page - both will have the same >> number of significant bits. The same is - afaict - not true for static allocation >> ranges. >> > > True. > > Ordinary allocations (buddies) can't cross zone boundaries, So I understand that > following the logic in "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" > pages of the smallest address shall be allocated from "dma_zone + 1", like you > said, it is irrelevant if you check DMA properties on the first or last pages, but imo, no matter > first or last page, both shall be larger than (2^(dma_zone + 1)). > > Taking 32 as dma_bitsize, then the memory with this DMA restriction allocated by > "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" shall be at least > more than 4G. DMA restrictions are always "needs to be no more than N bits". > That’s why I keep comparing the first page of static allocation, that I am following the > "more than" logic here. > > But you're right, I got a little investigation on ARM DMA limitation, still taking dma_bitsize=32 > as an example, we want that the actually allocated memory is smaller than 4G, not more than. > So I think the logic behind this code line > " alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, d);" is not right for ARM, and it shall > be changed to "alloc_heap_pages(zone_lo, dma_zone + 1, order, memflags, d);" as correction. But this step is to _avoid_ the DMA-reserved part of the heap. The caller requests address restricted memory by passing suitable memflags. If the request doesn't require access to the DMA- reserved part of the heap (dma_zone < zone_hi) we first try to get memory from there. Only if that fails will we fall back and try taking memory from the lower region. IOW the problem with your code is more fundamental: You use dma_bitsize when really you ought to extract the caller's requested restriction (if any) from memflags. I would assume that dma_bitsize is orthogonal to static memory, i.e. you don't need to try to preserve low memory. Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Thursday, July 8, 2021 6:06 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; xen- > devel@lists.xenproject.org; sstabellini@kernel.org; Julien Grall > <julien@xen.org>; Wei Chen <Wei.Chen@arm.com> > Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and > alloc_domstatic_pages > > On 08.07.2021 11:09, Penny Zheng wrote: > > Hi Jan > > > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Tuesday, July 6, 2021 2:54 PM > >> To: Penny Zheng <Penny.Zheng@arm.com> > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > >> <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > >> sstabellini@kernel.org; Julien Grall <julien@xen.org> > >> Subject: Re: [PATCH 6/9] xen/arm: introduce alloc_staticmem_pages and > >> alloc_domstatic_pages > >> > >> On 06.07.2021 07:58, Penny Zheng wrote: > >>>> From: Jan Beulich <jbeulich@suse.com> > >>>> Sent: Thursday, June 10, 2021 6:23 PM > >>>> > >>>> On 07.06.2021 04:43, Penny Zheng wrote: > >>>>> --- a/xen/common/page_alloc.c > >>>>> +++ b/xen/common/page_alloc.c > >>>>> @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( > >>>>> return pg; > >>>>> } > >>>>> > >>>>> +#ifdef CONFIG_STATIC_ALLOCATION > >>>>> +/* > >>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, of static > memory. > >>>>> + * It is the equivalent of alloc_heap_pages for static memory */ > >>>>> +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, > >>>>> + mfn_t smfn, > >>>>> + unsigned int > >>>>> +memflags) { > >>>>> + bool need_tlbflush = false; > >>>>> + uint32_t tlbflush_timestamp = 0; > >>>>> + unsigned long i; > >>>>> + struct page_info *pg; > >>>>> + > >>>>> + /* For now, it only supports allocating at specified address. */ > >>>>> + if ( !mfn_valid(smfn) || !nr_mfns ) > >>>>> + { > >>>>> + printk(XENLOG_ERR > >>>>> + "Invalid %lu static memory starting at > >>>>> + %"PRI_mfn"\n", > >>>> > >>>> Reading a log containing e.g. "Invalid 0 static memory starting at > >>>> ..." I don't think I would recognize that the "0" is the count of pages. > >>> > >>> Sure. How about "try to allocate out of range page %"PRI_mfn"\n"? > >> > >> This still doesn't convey _both_ parts of the if() that would cause > >> the log message to be issued. > >> > > > > Sorry. How about > > " > > printk(XENLOG_ERR > > "Either out-of-range static memory starting at %"PRI_mfn"" > > "or invalid number of pages: %ul.\n", > > mfn_x(smfn), nr_mfns); > > " > > I'm sorry - while now you convey both aspects, the message has become too > verbose. What's wrong with "Invalid static memory request: ... pages at ...\"? > But I wonder anyway if a log message is appropriate here in the first place. > Sorry for my poor English writing. :/ Just having a habit(maybe not a good habit) of printing error messages, if you think the log itself is verbose here, I'll remove it. ;) > >>>>> @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages( > >>>>> return pg; > >>>>> } > >>>>> > >>>>> +#ifdef CONFIG_STATIC_ALLOCATION > >>>>> +/* > >>>>> + * Allocate nr_mfns contiguous pages, starting at #smfn, 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_mfns, mfn_t smfn, > >>>>> + unsigned int memflags) > >>>>> +{ > >>>>> + struct page_info *pg = NULL; > >>>>> + unsigned long dma_size; > >>>>> + > >>>>> + ASSERT(!in_irq()); > >>>>> + > >>>>> + if ( !dma_bitsize ) > >>>>> + memflags &= ~MEMF_no_dma; > >>>>> + else > >>>>> + { > >>>>> + if ( (dma_bitsize - PAGE_SHIFT) > 0 ) > >>>>> + { > >>>>> + dma_size = 1ul << (dma_bitsize - PAGE_SHIFT); > >>>>> + /* Starting address shall meet the DMA limitation. */ > >>>>> + if ( mfn_x(smfn) < dma_size ) > >>>>> + return NULL; > >>>> > >>>> I think I did ask this on v1 already: Why the first page? Static > >>>> memory regions, unlike buddy allocator zones, can cross power-of-2 > >>>> address boundaries. Hence it ought to be the last page that gets > >>>> checked for fitting address width restriction requirements. > >>>> > >>>> And then - is this necessary at all? Shouldn't "pre-defined by > >>>> configuration using physical address ranges" imply the memory > >>>> designated for a guest fits its DMA needs? > >>>> > >>> > >>> Hmmm, In my understanding, here is the DMA restriction when using > >>> buddy > >> allocator: > >>> else if ( (dma_zone = bits_to_zone(dma_bitsize)) < zone_hi ) > >>> pg = alloc_heap_pages(dma_zone + 1, zone_hi, order, > >>> memflags, d); dma_zone is restricting the starting buddy allocator > >>> zone, so I am thinking that here, it shall restrict the first page. > >>> > >>> imo, if let user define, it also could be missing DMA restriction? > >> > >> Did you read my earlier reply? Again: The difference is that ordinary > >> allocations (buddies) can't cross zone boundaries. Hence it is > >> irrelevant if you check DMA properties on the first or last page - > >> both will have the same number of significant bits. The same is - > >> afaict - not true for static allocation ranges. > >> > > > > True. > > > > Ordinary allocations (buddies) can't cross zone boundaries, So I > > understand that following the logic in "alloc_heap_pages(dma_zone + 1, > zone_hi, order, memflags, d);" > > pages of the smallest address shall be allocated from "dma_zone + 1", > > like you said, it is irrelevant if you check DMA properties on the > > first or last pages, but imo, no matter first or last page, both shall be larger > than (2^(dma_zone + 1)). > > > > Taking 32 as dma_bitsize, then the memory with this DMA restriction > > allocated by "alloc_heap_pages(dma_zone + 1, zone_hi, order, memflags, > > d);" shall be at least more than 4G. > > DMA restrictions are always "needs to be no more than N bits". > > > That’s why I keep comparing the first page of static allocation, that > > I am following the "more than" logic here. > > > > But you're right, I got a little investigation on ARM DMA limitation, > > still taking dma_bitsize=32 as an example, we want that the actually > allocated memory is smaller than 4G, not more than. > > So I think the logic behind this code line " alloc_heap_pages(dma_zone > > + 1, zone_hi, order, memflags, d);" is not right for ARM, and it shall > > be changed to "alloc_heap_pages(zone_lo, dma_zone + 1, order, memflags, > d);" as correction. > > But this step is to _avoid_ the DMA-reserved part of the heap. Thanks for the explanation!!! I totally mis-understood the usage of dma_bitsize. It turns out to be the DMA-reserved... Putting DMA limitation on zone_lo is absolutely right on ARM also. > The caller requests address restricted memory by passing suitable memflags. If > the request doesn't require access to the DMA- reserved part of the heap > (dma_zone < zone_hi) we first try to get memory from there. Only if that fails > will we fall back and try taking memory from the lower region. IOW the > problem with your code is more fundamental: You use dma_bitsize when > really you ought to extract the caller's requested restriction (if any) from > memflags. I would assume that dma_bitsize is orthogonal to static memory, i.e. > you don't need to try to preserve low memory. > True, No need to preserve low memory for static memory. > Jan Penny Zheng
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index e244d2e52e..a0eea5f1a4 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1065,6 +1065,75 @@ static struct page_info *alloc_heap_pages( return pg; } +#ifdef CONFIG_STATIC_ALLOCATION +/* + * Allocate nr_mfns contiguous pages, starting at #smfn, of static memory. + * It is the equivalent of alloc_heap_pages for static memory + */ +static struct page_info *alloc_staticmem_pages(unsigned long nr_mfns, + mfn_t smfn, + unsigned int memflags) +{ + bool need_tlbflush = false; + uint32_t tlbflush_timestamp = 0; + unsigned long i; + struct page_info *pg; + + /* For now, it only supports allocating at specified address. */ + if ( !mfn_valid(smfn) || !nr_mfns ) + { + printk(XENLOG_ERR + "Invalid %lu static memory starting at %"PRI_mfn"\n", + nr_mfns, mfn_x(smfn)); + return NULL; + } + pg = mfn_to_page(smfn); + + for ( i = 0; i < nr_mfns; i++ ) + { + /* + * Reference count must continuously be zero for free pages + * of static memory(PGC_reserved). + */ + ASSERT(pg[i].count_info & PGC_reserved); + if ( (pg[i].count_info & ~PGC_reserved) != PGC_state_free ) + { + printk(XENLOG_ERR + "Reference count must continuously be zero for free pages" + "pg[%lu] MFN %"PRI_mfn" c=%#lx t=%#x\n", + i, mfn_x(page_to_mfn(pg + i)), + pg[i].count_info, pg[i].tlbflush_timestamp); + BUG(); + } + + if ( !(memflags & MEMF_no_tlbflush) ) + accumulate_tlbflush(&need_tlbflush, &pg[i], + &tlbflush_timestamp); + + /* + * Preserve flag PGC_reserved and change page state + * to PGC_state_inuse. + */ + pg[i].count_info = (pg[i].count_info & PGC_reserved) | PGC_state_inuse; + /* Initialise fields which have other uses for free pages. */ + pg[i].u.inuse.type_info = 0; + page_set_owner(&pg[i], NULL); + + /* + * Ensure cache and RAM are consistent for platforms where the + * guest can control its own visibility of/through the cache. + */ + flush_page_to_ram(mfn_x(page_to_mfn(&pg[i])), + !(memflags & MEMF_no_icache_flush)); + } + + if ( need_tlbflush ) + filtered_flush_tlb_mask(tlbflush_timestamp); + + return pg; +} +#endif + /* Remove any offlined page in the buddy pointed to by head. */ static int reserve_offlined_page(struct page_info *head) { @@ -2326,7 +2395,11 @@ int assign_pages_nr( for ( i = 0; i < nr_pfns; i++ ) { +#ifdef CONFIG_STATIC_ALLOCATION + ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved))); +#else ASSERT(!(pg[i].count_info & ~PGC_extra)); +#endif if ( pg[i].count_info & PGC_extra ) extra_pages++; } @@ -2365,7 +2438,12 @@ int assign_pages_nr( page_set_owner(&pg[i], d); smp_wmb(); /* Domain pointer must be visible before updating refcnt. */ pg[i].count_info = +#ifdef CONFIG_STATIC_ALLOCATION + (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1; +#else (pg[i].count_info & PGC_extra) | PGC_allocated | 1; +#endif + page_list_add_tail(&pg[i], page_to_list(d, &pg[i])); } @@ -2434,6 +2512,57 @@ struct page_info *alloc_domheap_pages( return pg; } +#ifdef CONFIG_STATIC_ALLOCATION +/* + * Allocate nr_mfns contiguous pages, starting at #smfn, 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_mfns, mfn_t smfn, + unsigned int memflags) +{ + struct page_info *pg = NULL; + unsigned long dma_size; + + ASSERT(!in_irq()); + + if ( !dma_bitsize ) + memflags &= ~MEMF_no_dma; + else + { + if ( (dma_bitsize - PAGE_SHIFT) > 0 ) + { + dma_size = 1ul << (dma_bitsize - PAGE_SHIFT); + /* Starting address shall meet the DMA limitation. */ + if ( mfn_x(smfn) < dma_size ) + return NULL; + } + } + + pg = alloc_staticmem_pages(nr_mfns, smfn, memflags); + if ( !pg ) + return NULL; + + /* Right now, MEMF_no_owner case is meaningless here. */ + ASSERT(d); + if ( memflags & MEMF_no_refcount ) + { + unsigned long i; + + for ( i = 0; i < nr_mfns; i++ ) + pg[i].count_info |= PGC_extra; + } + if ( assign_pages_nr(d, pg, nr_mfns, memflags) ) + { + free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub); + return NULL; + } + + return pg; +} +#endif + 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 25d970e857..a07bd02923 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -89,6 +89,8 @@ bool scrub_free_pages(void); /* Static Allocation */ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, bool need_scrub); +struct page_info *alloc_domstatic_pages(struct domain *d, + unsigned long nr_mfns, mfn_t smfn, unsigned int memflags); #endif /* Map machine page range in Xen virtual address space. */
alloc_staticmem_pages aims to allocate nr_mfns contiguous pages of static memory. And it is the equivalent of alloc_heap_pages for static memory. Here only covers allocating at specified starting address. For each page, it shall check if the page is reserved(PGC_reserved) and free. It shall also do a set of necessary initialization, which are mostly the same ones in alloc_heap_pages, like, following the same cache-coherency policy and turning page status into PGC_state_inuse, etc. alloc_domstatic_pages is the equivalent of alloc_domheap_pages for static mmeory, and it is to allocate nr_mfns pages of static memory and assign them to one specific domain. It uses alloc_staticmen_pages to get nr_mfns pages of static memory, then on success, it will use assign_pages_nr to assign those pages to one specific domain. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- changes v2: - use mfn_valid() to do validation - change pfn-named to mfn-named - put CONFIG_STATIC_ALLOCATION around to remove dead codes - correct off-by-one indentation - remove meaningless MEMF_no_owner case - leave zone concept out of DMA limitation check --- xen/common/page_alloc.c | 129 ++++++++++++++++++++++++++++++++++++++++ xen/include/xen/mm.h | 2 + 2 files changed, 131 insertions(+)