| Message ID | 20210518052113.725808-11-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
Hi Penny, On 18/05/2021 06:21, Penny Zheng wrote: > This commit introduces allocate_static_memory to allocate static memory as > guest RAM for domain on Static Allocation. > > It uses alloc_domstatic_pages to allocate pre-defined static memory banks > for this domain, and uses guest_physmap_add_page to set up P2M table, > guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/arch/arm/domain_build.c | 157 +++++++++++++++++++++++++++++++++++- > 1 file changed, 155 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 30b55588b7..9f662313ad 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct domain *d, > return true; > } > > +/* > + * #ram_index and #ram_index refer to the index and starting address of guest > + * memory kank stored in kinfo->mem. > + * Static memory at #smfn of #tot_size shall be mapped #sgfn, and > + * #sgfn will be next guest address to map when returning. > + */ > +static bool __init allocate_static_bank_memory(struct domain *d, > + struct kernel_info *kinfo, > + int ram_index, Please use unsigned. > + paddr_t ram_addr, > + gfn_t* sgfn, I am confused, what is the difference between ram_addr and sgfn? > + mfn_t smfn, > + paddr_t tot_size) > +{ > + int res; > + struct membank *bank; > + paddr_t _size = tot_size; > + > + bank = &kinfo->mem.bank[ram_index]; > + bank->start = ram_addr; > + bank->size = bank->size + tot_size; > + > + while ( tot_size > 0 ) > + { > + unsigned int order = get_allocation_size(tot_size); > + > + res = guest_physmap_add_page(d, *sgfn, smfn, order); > + if ( res ) > + { > + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); > + return false; > + } > + > + *sgfn = gfn_add(*sgfn, 1UL << order); > + smfn = mfn_add(smfn, 1UL << order); > + tot_size -= (1ULL << (PAGE_SHIFT + order)); > + } > + > + kinfo->mem.nr_banks = ram_index + 1; > + kinfo->unassigned_mem -= _size; > + > + return true; > +} > + > static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo) > { > unsigned int i; > @@ -480,6 +524,116 @@ fail: > (unsigned long)kinfo->unassigned_mem >> 10); > } > > +/* Allocate memory from static memory as RAM for one specific domain d. */ > +static void __init allocate_static_memory(struct domain *d, > + struct kernel_info *kinfo) > +{ > + int nr_banks, _banks = 0; AFAICT, _banks is the index in the array. I think it would be clearer if it is caller 'bank' or 'idx'. > + size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE; > + paddr_t bank_start, bank_size; > + gfn_t sgfn; > + mfn_t smfn; > + > + kinfo->mem.nr_banks = 0; > + sgfn = gaddr_to_gfn(GUEST_RAM0_BASE); > + nr_banks = d->arch.static_mem.nr_banks; > + ASSERT(nr_banks >= 0); > + > + if ( kinfo->unassigned_mem <= 0 ) > + goto fail; > + > + while ( _banks < nr_banks ) > + { > + bank_start = d->arch.static_mem.bank[_banks].start; > + smfn = maddr_to_mfn(bank_start); > + bank_size = d->arch.static_mem.bank[_banks].size; The variable name are slightly confusing because it doesn't tell whether this is physical are guest RAM. You might want to consider to prefix them with p (resp. g) for physical (resp. guest) RAM. > + > + if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start, 0) ) > + { > + printk(XENLOG_ERR > + "%pd: cannot allocate static memory" > + "(0x%"PRIx64" - 0x%"PRIx64")", bank_start and bank_size are both paddr_t. So this should be PRIpaddr. > + d, bank_start, bank_start + bank_size); > + goto fail; > + } > + > + /* > + * By default, it shall be mapped to the fixed guest RAM address > + * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. > + * Starting from RAM0(GUEST_RAM0_BASE). > + */ Ok. So you are first trying to exhaust the guest bank 0 and then moved to bank 1. This wasn't entirely clear from the design document. I am fine with that, but in this case, the developper should not need to know that (in fact this is not part of the ABI). Regarding this code, I am a bit concerned about the scalability if we introduce a second bank. Can we have an array of the possible guest banks and increment the index when exhausting the current bank? Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Tuesday, May 18, 2021 8:06 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 10/10] xen/arm: introduce allocate_static_memory > > Hi Penny, > > On 18/05/2021 06:21, Penny Zheng wrote: > > This commit introduces allocate_static_memory to allocate static > > memory as guest RAM for domain on Static Allocation. > > > > It uses alloc_domstatic_pages to allocate pre-defined static memory > > banks for this domain, and uses guest_physmap_add_page to set up P2M > > table, guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > xen/arch/arm/domain_build.c | 157 > +++++++++++++++++++++++++++++++++++- > > 1 file changed, 155 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 30b55588b7..9f662313ad 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct > domain *d, > > return true; > > } > > > > +/* > > + * #ram_index and #ram_index refer to the index and starting address > > +of guest > > + * memory kank stored in kinfo->mem. > > + * Static memory at #smfn of #tot_size shall be mapped #sgfn, and > > + * #sgfn will be next guest address to map when returning. > > + */ > > +static bool __init allocate_static_bank_memory(struct domain *d, > > + struct kernel_info *kinfo, > > + int ram_index, > > Please use unsigned. > > > + paddr_t ram_addr, > > + gfn_t* sgfn, > > I am confused, what is the difference between ram_addr and sgfn? > We need to constructing kinfo->mem(guest RAM banks) here, and we are indexing in static_mem(physical ram banks). Multiple physical ram banks consist of one guest ram bank(like, GUEST_RAM0). ram_addr here will either be GUEST_RAM0_BASE, or GUEST_RAM1_BASE, for now. I kinds struggled in how to name it. And maybe it shall not be a parameter here. Maybe I should switch.. case.. on the ram_index, if its 0, its GUEST_RAM0_BASE, And if its 1, its GUEST_RAM1_BASE. > > + mfn_t smfn, > > + paddr_t tot_size) { > > + int res; > > + struct membank *bank; > > + paddr_t _size = tot_size; > > + > > + bank = &kinfo->mem.bank[ram_index]; > > + bank->start = ram_addr; > > + bank->size = bank->size + tot_size; > > + > > + while ( tot_size > 0 ) > > + { > > + unsigned int order = get_allocation_size(tot_size); > > + > > + res = guest_physmap_add_page(d, *sgfn, smfn, order); > > + if ( res ) > > + { > > + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); > > + return false; > > + } > > + > > + *sgfn = gfn_add(*sgfn, 1UL << order); > > + smfn = mfn_add(smfn, 1UL << order); > > + tot_size -= (1ULL << (PAGE_SHIFT + order)); > > + } > > + > > + kinfo->mem.nr_banks = ram_index + 1; > > + kinfo->unassigned_mem -= _size; > > + > > + return true; > > +} > > + > > static void __init allocate_memory(struct domain *d, struct kernel_info > *kinfo) > > { > > unsigned int i; > > @@ -480,6 +524,116 @@ fail: > > (unsigned long)kinfo->unassigned_mem >> 10); > > } > > > > +/* Allocate memory from static memory as RAM for one specific domain > > +d. */ static void __init allocate_static_memory(struct domain *d, > > + struct kernel_info > > +*kinfo) { > > + int nr_banks, _banks = 0; > > AFAICT, _banks is the index in the array. I think it would be clearer if it is > caller 'bank' or 'idx'. > Sure, I’ll use the 'bank' here. > > + size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE; > > + paddr_t bank_start, bank_size; > > + gfn_t sgfn; > > + mfn_t smfn; > > + > > + kinfo->mem.nr_banks = 0; > > + sgfn = gaddr_to_gfn(GUEST_RAM0_BASE); > > + nr_banks = d->arch.static_mem.nr_banks; > > + ASSERT(nr_banks >= 0); > > + > > + if ( kinfo->unassigned_mem <= 0 ) > > + goto fail; > > + > > + while ( _banks < nr_banks ) > > + { > > + bank_start = d->arch.static_mem.bank[_banks].start; > > + smfn = maddr_to_mfn(bank_start); > > + bank_size = d->arch.static_mem.bank[_banks].size; > > The variable name are slightly confusing because it doesn't tell whether this > is physical are guest RAM. You might want to consider to prefix them with p > (resp. g) for physical (resp. guest) RAM. Sure, I'll rename to make it more clearly. > > > + > > + if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start, > 0) ) > > + { > > + printk(XENLOG_ERR > > + "%pd: cannot allocate static memory" > > + "(0x%"PRIx64" - 0x%"PRIx64")", > > bank_start and bank_size are both paddr_t. So this should be PRIpaddr. Sure, I'll change > > > + d, bank_start, bank_start + bank_size); > > + goto fail; > > + } > > + > > + /* > > + * By default, it shall be mapped to the fixed guest RAM address > > + * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. > > + * Starting from RAM0(GUEST_RAM0_BASE). > > + */ > > Ok. So you are first trying to exhaust the guest bank 0 and then moved to > bank 1. This wasn't entirely clear from the design document. > > I am fine with that, but in this case, the developper should not need to know > that (in fact this is not part of the ABI). > > Regarding this code, I am a bit concerned about the scalability if we introduce > a second bank. > > Can we have an array of the possible guest banks and increment the index > when exhausting the current bank? > Correct me if I understand wrongly, What you suggest here is that we make an array of guest banks, right now, including GUEST_RAM0 and GUEST_RAM1. And if later, adding more guest banks, it will not bring scalability problem here, right? > Cheers, > > -- > Julien Grall Cheers Penny Zheng
On 19/05/2021 08:27, Penny Zheng wrote: > Hi Julien > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Tuesday, May 18, 2021 8:06 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 10/10] xen/arm: introduce allocate_static_memory >> >> Hi Penny, >> >> On 18/05/2021 06:21, Penny Zheng wrote: >>> This commit introduces allocate_static_memory to allocate static >>> memory as guest RAM for domain on Static Allocation. >>> >>> It uses alloc_domstatic_pages to allocate pre-defined static memory >>> banks for this domain, and uses guest_physmap_add_page to set up P2M >>> table, guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE. >>> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> --- >>> xen/arch/arm/domain_build.c | 157 >> +++++++++++++++++++++++++++++++++++- >>> 1 file changed, 155 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 30b55588b7..9f662313ad 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct >> domain *d, >>> return true; >>> } >>> >>> +/* >>> + * #ram_index and #ram_index refer to the index and starting address >>> +of guest >>> + * memory kank stored in kinfo->mem. >>> + * Static memory at #smfn of #tot_size shall be mapped #sgfn, and >>> + * #sgfn will be next guest address to map when returning. >>> + */ >>> +static bool __init allocate_static_bank_memory(struct domain *d, >>> + struct kernel_info *kinfo, >>> + int ram_index, >> >> Please use unsigned. >> >>> + paddr_t ram_addr, >>> + gfn_t* sgfn, >> >> I am confused, what is the difference between ram_addr and sgfn? >> > > We need to constructing kinfo->mem(guest RAM banks) here, and > we are indexing in static_mem(physical ram banks). Multiple physical ram > banks consist of one guest ram bank(like, GUEST_RAM0). > > ram_addr here will either be GUEST_RAM0_BASE, or GUEST_RAM1_BASE, > for now. I kinds struggled in how to name it. And maybe it shall not be a > parameter here. > > Maybe I should switch.. case.. on the ram_index, if its 0, its GUEST_RAM0_BASE, > And if its 1, its GUEST_RAM1_BASE. You only need to set kinfo->mem.bank[ram_index].start once. This is when you know the bank is first used. AFAICT, this function will map the memory for a range start at ``sgfn``. It doesn't feel this belongs to the function. The same remark is valid for kinfo->mem.nr_banks. >>> + mfn_t smfn, >>> + paddr_t tot_size) { >>> + int res; >>> + struct membank *bank; >>> + paddr_t _size = tot_size; >>> + >>> + bank = &kinfo->mem.bank[ram_index]; >>> + bank->start = ram_addr; >>> + bank->size = bank->size + tot_size; >>> + >>> + while ( tot_size > 0 ) >>> + { >>> + unsigned int order = get_allocation_size(tot_size); >>> + >>> + res = guest_physmap_add_page(d, *sgfn, smfn, order); >>> + if ( res ) >>> + { >>> + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); >>> + return false; >>> + } >>> + >>> + *sgfn = gfn_add(*sgfn, 1UL << order); >>> + smfn = mfn_add(smfn, 1UL << order); >>> + tot_size -= (1ULL << (PAGE_SHIFT + order)); >>> + } >>> + >>> + kinfo->mem.nr_banks = ram_index + 1; >>> + kinfo->unassigned_mem -= _size; >>> + >>> + return true; >>> +} >>> + >>> static void __init allocate_memory(struct domain *d, struct kernel_info >> *kinfo) >>> { >>> unsigned int i; >>> @@ -480,6 +524,116 @@ fail: >>> (unsigned long)kinfo->unassigned_mem >> 10); >>> } >>> >>> +/* Allocate memory from static memory as RAM for one specific domain >>> +d. */ static void __init allocate_static_memory(struct domain *d, >>> + struct kernel_info >>> +*kinfo) { >>> + int nr_banks, _banks = 0; >> >> AFAICT, _banks is the index in the array. I think it would be clearer if it is >> caller 'bank' or 'idx'. >> > > Sure, I’ll use the 'bank' here. > >>> + size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE; >>> + paddr_t bank_start, bank_size; >>> + gfn_t sgfn; >>> + mfn_t smfn; >>> + >>> + kinfo->mem.nr_banks = 0; >>> + sgfn = gaddr_to_gfn(GUEST_RAM0_BASE); >>> + nr_banks = d->arch.static_mem.nr_banks; >>> + ASSERT(nr_banks >= 0); >>> + >>> + if ( kinfo->unassigned_mem <= 0 ) >>> + goto fail; >>> + >>> + while ( _banks < nr_banks ) >>> + { >>> + bank_start = d->arch.static_mem.bank[_banks].start; >>> + smfn = maddr_to_mfn(bank_start); >>> + bank_size = d->arch.static_mem.bank[_banks].size; >> >> The variable name are slightly confusing because it doesn't tell whether this >> is physical are guest RAM. You might want to consider to prefix them with p >> (resp. g) for physical (resp. guest) RAM. > > Sure, I'll rename to make it more clearly. > >> >>> + >>> + if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start, >> 0) ) >>> + { >>> + printk(XENLOG_ERR >>> + "%pd: cannot allocate static memory" >>> + "(0x%"PRIx64" - 0x%"PRIx64")", >> >> bank_start and bank_size are both paddr_t. So this should be PRIpaddr. > > Sure, I'll change > >> >>> + d, bank_start, bank_start + bank_size); >>> + goto fail; >>> + } >>> + >>> + /* >>> + * By default, it shall be mapped to the fixed guest RAM address >>> + * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. >>> + * Starting from RAM0(GUEST_RAM0_BASE). >>> + */ >> >> Ok. So you are first trying to exhaust the guest bank 0 and then moved to >> bank 1. This wasn't entirely clear from the design document. >> >> I am fine with that, but in this case, the developper should not need to know >> that (in fact this is not part of the ABI). >> >> Regarding this code, I am a bit concerned about the scalability if we introduce >> a second bank. >> >> Can we have an array of the possible guest banks and increment the index >> when exhausting the current bank? >> > > Correct me if I understand wrongly, > > What you suggest here is that we make an array of guest banks, right now, including > GUEST_RAM0 and GUEST_RAM1. And if later, adding more guest banks, it will not > bring scalability problem here, right? Yes. This should also reduce the current complexity of the code. Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Thursday, May 20, 2021 4:10 AM > 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 10/10] xen/arm: introduce allocate_static_memory > > > > On 19/05/2021 08:27, Penny Zheng wrote: > > Hi Julien > > > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: Tuesday, May 18, 2021 8:06 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 10/10] xen/arm: introduce allocate_static_memory > >> > >> Hi Penny, > >> > >> On 18/05/2021 06:21, Penny Zheng wrote: > >>> This commit introduces allocate_static_memory to allocate static > >>> memory as guest RAM for domain on Static Allocation. > >>> > >>> It uses alloc_domstatic_pages to allocate pre-defined static memory > >>> banks for this domain, and uses guest_physmap_add_page to set up P2M > >>> table, guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE. > >>> > >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> > >>> --- > >>> xen/arch/arm/domain_build.c | 157 > >> +++++++++++++++++++++++++++++++++++- > >>> 1 file changed, 155 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/xen/arch/arm/domain_build.c > >>> b/xen/arch/arm/domain_build.c index 30b55588b7..9f662313ad 100644 > >>> --- a/xen/arch/arm/domain_build.c > >>> +++ b/xen/arch/arm/domain_build.c > >>> @@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct > >> domain *d, > >>> return true; > >>> } > >>> > >>> +/* > >>> + * #ram_index and #ram_index refer to the index and starting > >>> +address of guest > >>> + * memory kank stored in kinfo->mem. > >>> + * Static memory at #smfn of #tot_size shall be mapped #sgfn, and > >>> + * #sgfn will be next guest address to map when returning. > >>> + */ > >>> +static bool __init allocate_static_bank_memory(struct domain *d, > >>> + struct kernel_info *kinfo, > >>> + int ram_index, > >> > >> Please use unsigned. > >> > >>> + paddr_t ram_addr, > >>> + gfn_t* sgfn, > >> > >> I am confused, what is the difference between ram_addr and sgfn? > >> > > > > We need to constructing kinfo->mem(guest RAM banks) here, and we are > > indexing in static_mem(physical ram banks). Multiple physical ram > > banks consist of one guest ram bank(like, GUEST_RAM0). > > > > ram_addr here will either be GUEST_RAM0_BASE, or GUEST_RAM1_BASE, > for > > now. I kinds struggled in how to name it. And maybe it shall not be a > > parameter here. > > > > Maybe I should switch.. case.. on the ram_index, if its 0, its > > GUEST_RAM0_BASE, And if its 1, its GUEST_RAM1_BASE. > > You only need to set kinfo->mem.bank[ram_index].start once. This is when > you know the bank is first used. > > AFAICT, this function will map the memory for a range start at ``sgfn``. > It doesn't feel this belongs to the function. > > The same remark is valid for kinfo->mem.nr_banks. > Ok. I finally totally understand what you suggest here. I'll try to let the action related to setting kinfo->mem.bank[ram_index].start/ kinfo->mem.bank[ram_index].size/ kinfo->mem. nr_banks out of this function, and only keep the simple functionality of mapping the memory for a range start at ``sgfn``. > >>> + mfn_t smfn, > >>> + paddr_t tot_size) { > >>> + int res; > >>> + struct membank *bank; > >>> + paddr_t _size = tot_size; > >>> + > >>> + bank = &kinfo->mem.bank[ram_index]; > >>> + bank->start = ram_addr; > >>> + bank->size = bank->size + tot_size; > >>> + > >>> + while ( tot_size > 0 ) > >>> + { > >>> + unsigned int order = get_allocation_size(tot_size); > >>> + > >>> + res = guest_physmap_add_page(d, *sgfn, smfn, order); > >>> + if ( res ) > >>> + { > >>> + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); > >>> + return false; > >>> + } > >>> + > >>> + *sgfn = gfn_add(*sgfn, 1UL << order); > >>> + smfn = mfn_add(smfn, 1UL << order); > >>> + tot_size -= (1ULL << (PAGE_SHIFT + order)); > >>> + } > >>> + > >>> + kinfo->mem.nr_banks = ram_index + 1; > >>> + kinfo->unassigned_mem -= _size; > >>> + > >>> + return true; > >>> +} > >>> + > >>> static void __init allocate_memory(struct domain *d, struct > >>> kernel_info > >> *kinfo) > >>> { > >>> unsigned int i; > >>> @@ -480,6 +524,116 @@ fail: > >>> (unsigned long)kinfo->unassigned_mem >> 10); > >>> } > >>> > >>> +/* Allocate memory from static memory as RAM for one specific > >>> +domain d. */ static void __init allocate_static_memory(struct domain *d, > >>> + struct kernel_info > >>> +*kinfo) { > >>> + int nr_banks, _banks = 0; > >> > >> AFAICT, _banks is the index in the array. I think it would be clearer > >> if it is caller 'bank' or 'idx'. > >> > > > > Sure, I’ll use the 'bank' here. > > > >>> + size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE; > >>> + paddr_t bank_start, bank_size; > >>> + gfn_t sgfn; > >>> + mfn_t smfn; > >>> + > >>> + kinfo->mem.nr_banks = 0; > >>> + sgfn = gaddr_to_gfn(GUEST_RAM0_BASE); > >>> + nr_banks = d->arch.static_mem.nr_banks; > >>> + ASSERT(nr_banks >= 0); > >>> + > >>> + if ( kinfo->unassigned_mem <= 0 ) > >>> + goto fail; > >>> + > >>> + while ( _banks < nr_banks ) > >>> + { > >>> + bank_start = d->arch.static_mem.bank[_banks].start; > >>> + smfn = maddr_to_mfn(bank_start); > >>> + bank_size = d->arch.static_mem.bank[_banks].size; > >> > >> The variable name are slightly confusing because it doesn't tell > >> whether this is physical are guest RAM. You might want to consider to > >> prefix them with p (resp. g) for physical (resp. guest) RAM. > > > > Sure, I'll rename to make it more clearly. > > > >> > >>> + > >>> + if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, > >>> + bank_start, > >> 0) ) > >>> + { > >>> + printk(XENLOG_ERR > >>> + "%pd: cannot allocate static memory" > >>> + "(0x%"PRIx64" - 0x%"PRIx64")", > >> > >> bank_start and bank_size are both paddr_t. So this should be PRIpaddr. > > > > Sure, I'll change > > > >> > >>> + d, bank_start, bank_start + bank_size); > >>> + goto fail; > >>> + } > >>> + > >>> + /* > >>> + * By default, it shall be mapped to the fixed guest RAM address > >>> + * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. > >>> + * Starting from RAM0(GUEST_RAM0_BASE). > >>> + */ > >> > >> Ok. So you are first trying to exhaust the guest bank 0 and then > >> moved to bank 1. This wasn't entirely clear from the design document. > >> > >> I am fine with that, but in this case, the developper should not need > >> to know that (in fact this is not part of the ABI). > >> > >> Regarding this code, I am a bit concerned about the scalability if we > >> introduce a second bank. > >> > >> Can we have an array of the possible guest banks and increment the > >> index when exhausting the current bank? > >> > > > > Correct me if I understand wrongly, > > > > What you suggest here is that we make an array of guest banks, right > > now, including > > GUEST_RAM0 and GUEST_RAM1. And if later, adding more guest banks, it > > will not bring scalability problem here, right? > > Yes. This should also reduce the current complexity of the code. > > Cheers, > > -- > Julien Grall Cheers Penny
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 30b55588b7..9f662313ad 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct domain *d, return true; } +/* + * #ram_index and #ram_index refer to the index and starting address of guest + * memory kank stored in kinfo->mem. + * Static memory at #smfn of #tot_size shall be mapped #sgfn, and + * #sgfn will be next guest address to map when returning. + */ +static bool __init allocate_static_bank_memory(struct domain *d, + struct kernel_info *kinfo, + int ram_index, + paddr_t ram_addr, + gfn_t* sgfn, + mfn_t smfn, + paddr_t tot_size) +{ + int res; + struct membank *bank; + paddr_t _size = tot_size; + + bank = &kinfo->mem.bank[ram_index]; + bank->start = ram_addr; + bank->size = bank->size + tot_size; + + while ( tot_size > 0 ) + { + unsigned int order = get_allocation_size(tot_size); + + res = guest_physmap_add_page(d, *sgfn, smfn, order); + if ( res ) + { + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); + return false; + } + + *sgfn = gfn_add(*sgfn, 1UL << order); + smfn = mfn_add(smfn, 1UL << order); + tot_size -= (1ULL << (PAGE_SHIFT + order)); + } + + kinfo->mem.nr_banks = ram_index + 1; + kinfo->unassigned_mem -= _size; + + return true; +} + static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo) { unsigned int i; @@ -480,6 +524,116 @@ fail: (unsigned long)kinfo->unassigned_mem >> 10); } +/* Allocate memory from static memory as RAM for one specific domain d. */ +static void __init allocate_static_memory(struct domain *d, + struct kernel_info *kinfo) +{ + int nr_banks, _banks = 0; + size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE; + paddr_t bank_start, bank_size; + gfn_t sgfn; + mfn_t smfn; + + kinfo->mem.nr_banks = 0; + sgfn = gaddr_to_gfn(GUEST_RAM0_BASE); + nr_banks = d->arch.static_mem.nr_banks; + ASSERT(nr_banks >= 0); + + if ( kinfo->unassigned_mem <= 0 ) + goto fail; + + while ( _banks < nr_banks ) + { + bank_start = d->arch.static_mem.bank[_banks].start; + smfn = maddr_to_mfn(bank_start); + bank_size = d->arch.static_mem.bank[_banks].size; + + if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start, 0) ) + { + printk(XENLOG_ERR + "%pd: cannot allocate static memory" + "(0x%"PRIx64" - 0x%"PRIx64")", + d, bank_start, bank_start + bank_size); + goto fail; + } + + /* + * By default, it shall be mapped to the fixed guest RAM address + * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`. + * Starting from RAM0(GUEST_RAM0_BASE). + */ + if ( ram0_size ) + { + /* RAM0 at most holds GUEST_RAM0_SIZE. */ + if ( ram0_size >= bank_size ) + { + if ( !allocate_static_bank_memory(d, kinfo, + 0, GUEST_RAM0_BASE, + &sgfn, smfn, bank_size) ) + goto fail; + + ram0_size = ram0_size - bank_size; + _banks++; + continue; + } + else /* bank_size > ram0_size */ + { + if ( !allocate_static_bank_memory(d, kinfo, + 0, GUEST_RAM0_BASE, + &sgfn, smfn, ram0_size) ) + goto fail; + + /* The whole RAM0 is consumed. */ + ram0_size -= ram0_size; + /* This bank hasn't been totally mapped, seeking to RAM1. */ + bank_size = bank_size - ram0_size; + smfn = mfn_add(smfn, ram0_size >> PAGE_SHIFT); + sgfn = gaddr_to_gfn(GUEST_RAM1_BASE); + } + } + + if ( ram1_size >= bank_size ) + { + if ( !allocate_static_bank_memory(d, kinfo, + 1, GUEST_RAM1_BASE, + &sgfn, smfn, bank_size) ) + goto fail; + + ram1_size = ram1_size - bank_size; + _banks++; + continue; + } + else + /* + * If RAM1 still couldn't meet the requirement, + * no way to seek for now. + */ + goto fail; + } + + if ( kinfo->unassigned_mem ) + goto fail; + + for( int i = 0; i < kinfo->mem.nr_banks; i++ ) + { + printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n", + d, + i, + kinfo->mem.bank[i].start, + kinfo->mem.bank[i].start + kinfo->mem.bank[i].size, + /* Don't want format this as PRIpaddr (16 digit hex) */ + (unsigned long)(kinfo->mem.bank[i].size >> 20)); + } + + return; + +fail: + panic("Failed to allocate requested domain memory." + /* Don't want format this as PRIpaddr (16 digit hex) */ + " %ldKB unallocated. Fix the VMs configurations.\n", + (unsigned long)kinfo->unassigned_mem >> 10); +} + static int __init write_properties(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -2497,8 +2651,7 @@ static int __init construct_domU(struct domain *d, d->arch.type = kinfo.type; #endif if ( static_mem ) - /* allocate_static_memory(d, &kinfo); */ - BUG(); + allocate_static_memory(d, &kinfo); else allocate_memory(d, &kinfo);
This commit introduces allocate_static_memory to allocate static memory as guest RAM for domain on Static Allocation. It uses alloc_domstatic_pages to allocate pre-defined static memory banks for this domain, and uses guest_physmap_add_page to set up P2M table, guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- xen/arch/arm/domain_build.c | 157 +++++++++++++++++++++++++++++++++++- 1 file changed, 155 insertions(+), 2 deletions(-)