| Message ID | 1474350751-5650-1-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Peng, On 20/09/2016 07:52, van.freenix@gmail.com wrote: > From: Peng Fan <peng.fan@nxp.com> > > On AArch64 SoCs, some IPs may only have the capability to access > 32bits address space. The physical memory assigned for Dom0 maybe s/32bits/32 bits/ > not in 4GB address space, then the IPs will not work properly. > So need to allocate memory under 4GB for Dom0. > > There is no restriction that how much lowmem needs to be allocated for > Dom0. Dom0 now use 1:1 mapping, but DomU does not use 1:1 mapping, > there is no need to reserve lowmem for DomU, so allocate lowmem as much > as possible for Dom0. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > > This patch is to resolve the issue mentioned in > https://lists.xen.org/archives/html/xen-devel/2016-09/msg00235.html > > V2: > Remove the bootargs dom0_lowmem introduced in V1. > Following "https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01459.html" > to allocate as much as possible lowmem. > > Tested results: > (XEN) Allocating 1:1 mappings totalling 2048MB for dom0: > (XEN) BANK[0] 0x00000088000000-0x000000f8000000 (1792MB) > (XEN) BANK[1] 0x000009e0000000-0x000009f0000000 (256MB) > 1792M allocated in 4GB address space. > > xen/arch/arm/domain_build.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 35ab08d..0b9b85c 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -240,11 +240,11 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) As mentioned on V1, please update the comment above this function. > const unsigned int min_low_order = > get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); > const unsigned int min_order = get_order_from_bytes(MB(4)); > - struct page_info *pg; > + struct page_info *pg = NULL; > unsigned int order = get_11_allocation_size(kinfo->unassigned_mem); > int i; > > - bool_t lowmem = is_32bit_domain(d); > + bool_t lowmem = true; > unsigned int bits; > > /* > @@ -265,22 +265,28 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) > */ > while ( order >= min_low_order ) > { > - for ( bits = order ; bits <= (lowmem ? 32 : PADDR_BITS); bits++ ) > + for ( bits = order ; bits <= 32 ; bits++ ) > { > pg = alloc_domheap_pages(d, order, MEMF_bits(bits)); > if ( pg != NULL ) > + { > + if ( !insert_11_bank(d, kinfo, pg, order) ) > + BUG(); /* Cannot fail for first bank */ > + > goto got_bank0; > + } > } > order--; > } > > - panic("Unable to allocate first memory bank"); > + if ( pg == NULL ) > + { > + printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); > + lowmem = false; > + } This new behavior should be documented in the commit message, with some rationale why it is fine to do it for ARM32. > > got_bank0: > > - if ( !insert_11_bank(d, kinfo, pg, order) ) > - BUG(); /* Cannot fail for first bank */ > - > /* Now allocate more memory and fill in additional banks */ This comment should be updated. > > order = get_11_allocation_size(kinfo->unassigned_mem); > Regards,
Hello Julien, On Tue, Sep 20, 2016 at 10:36:27AM +0200, Julien Grall wrote: >Hello Peng, > >On 20/09/2016 07:52, van.freenix@gmail.com wrote: >>From: Peng Fan <peng.fan@nxp.com> >> >>On AArch64 SoCs, some IPs may only have the capability to access >>32bits address space. The physical memory assigned for Dom0 maybe > >s/32bits/32 bits/ Fix in V3. > >>not in 4GB address space, then the IPs will not work properly. >>So need to allocate memory under 4GB for Dom0. >> >>There is no restriction that how much lowmem needs to be allocated for >>Dom0. Dom0 now use 1:1 mapping, but DomU does not use 1:1 mapping, >>there is no need to reserve lowmem for DomU, so allocate lowmem as much >>as possible for Dom0. >> >>Signed-off-by: Peng Fan <peng.fan@nxp.com> >>Cc: Stefano Stabellini <sstabellini@kernel.org> >>Cc: Julien Grall <julien.grall@arm.com> >>--- >> >>This patch is to resolve the issue mentioned in >>https://lists.xen.org/archives/html/xen-devel/2016-09/msg00235.html >> >>V2: >> Remove the bootargs dom0_lowmem introduced in V1. >> Following "https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01459.html" >> to allocate as much as possible lowmem. >> >> Tested results: >> (XEN) Allocating 1:1 mappings totalling 2048MB for dom0: >> (XEN) BANK[0] 0x00000088000000-0x000000f8000000 (1792MB) >> (XEN) BANK[1] 0x000009e0000000-0x000009f0000000 (256MB) >> 1792M allocated in 4GB address space. >> >> xen/arch/arm/domain_build.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >>diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>index 35ab08d..0b9b85c 100644 >>--- a/xen/arch/arm/domain_build.c >>+++ b/xen/arch/arm/domain_build.c >>@@ -240,11 +240,11 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) > >As mentioned on V1, please update the comment above this function. About this part: * 3. For 32-bit dom0 we want to place as much of the RAM as we * reasonably can below 4GB, so that it can be used by non-LPAE * enabled kernels. Changed to this V3: For dom0 we want to place as much of the RAM as we reasonably can below 4GB, so the devices have the limitation to access 64 bits address space can work properly and it can also be used by non-LPAE enabled kernels for 32-bit dom0. About " * For 32-bit domain we require that the initial allocation for the * first bank is under 4G. " Changed to this in V3: " For 32-bit domain we require that the initial allocation for the first bank is under 4G. For 64-bit domain, the first bank is preferred to be allocated under 4GB. " Is this ok? > >> const unsigned int min_low_order = >> get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); >> const unsigned int min_order = get_order_from_bytes(MB(4)); >>- struct page_info *pg; >>+ struct page_info *pg = NULL; >> unsigned int order = get_11_allocation_size(kinfo->unassigned_mem); >> int i; >> >>- bool_t lowmem = is_32bit_domain(d); >>+ bool_t lowmem = true; >> unsigned int bits; >> >> /* >>@@ -265,22 +265,28 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) >> */ >> while ( order >= min_low_order ) >> { >>- for ( bits = order ; bits <= (lowmem ? 32 : PADDR_BITS); bits++ ) >>+ for ( bits = order ; bits <= 32 ; bits++ ) >> { >> pg = alloc_domheap_pages(d, order, MEMF_bits(bits)); >> if ( pg != NULL ) >>+ { >>+ if ( !insert_11_bank(d, kinfo, pg, order) ) >>+ BUG(); /* Cannot fail for first bank */ >>+ >> goto got_bank0; >>+ } >> } >> order--; >> } >> >>- panic("Unable to allocate first memory bank"); >>+ if ( pg == NULL ) >>+ { >>+ printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); >>+ lowmem = false; >>+ } > >This new behavior should be documented in the commit message, with some >rationale why it is fine to do it for ARM32. Consider the comments of the function: 1. The dom0 kernel should be loaded within the first 128MB of RAM. This is necessary at least for Linux zImage kernels, which are all we support today. 4. For 32-bit dom0 the kernel must be located below 4GB. I think I may need to add back the panic when first bank is failed to be allocated under 4GB. Will use the following code in V3 and document this in commit log. if ( pg == NULL ) { if ( is_32bit_domain(d) ) panic("Unable to allocate first memory bank"); printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); lowmem = false; } > >> >> got_bank0: >> >>- if ( !insert_11_bank(d, kinfo, pg, order) ) >>- BUG(); /* Cannot fail for first bank */ >>- >> /* Now allocate more memory and fill in additional banks */ > >This comment should be updated. Add more comments? I did not modify code below the comment and the comment is correct I think. Thanks, Peng. > >> >> order = get_11_allocation_size(kinfo->unassigned_mem); >> > >Regards, > >-- >Julien Grall
On 20/09/16 10:10, Peng Fan wrote: > Hello Julien, Hello Peng, > On Tue, Sep 20, 2016 at 10:36:27AM +0200, Julien Grall wrote: >> Hello Peng, >> >> On 20/09/2016 07:52, van.freenix@gmail.com wrote: >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> On AArch64 SoCs, some IPs may only have the capability to access >>> 32bits address space. The physical memory assigned for Dom0 maybe >> >> s/32bits/32 bits/ > > Fix in V3. > >> >>> not in 4GB address space, then the IPs will not work properly. >>> So need to allocate memory under 4GB for Dom0. >>> >>> There is no restriction that how much lowmem needs to be allocated for >>> Dom0. Dom0 now use 1:1 mapping, but DomU does not use 1:1 mapping, >>> there is no need to reserve lowmem for DomU, so allocate lowmem as much >>> as possible for Dom0. >>> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com> >>> Cc: Stefano Stabellini <sstabellini@kernel.org> >>> Cc: Julien Grall <julien.grall@arm.com> >>> --- >>> >>> This patch is to resolve the issue mentioned in >>> https://lists.xen.org/archives/html/xen-devel/2016-09/msg00235.html >>> >>> V2: >>> Remove the bootargs dom0_lowmem introduced in V1. >>> Following "https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01459.html" >>> to allocate as much as possible lowmem. >>> >>> Tested results: >>> (XEN) Allocating 1:1 mappings totalling 2048MB for dom0: >>> (XEN) BANK[0] 0x00000088000000-0x000000f8000000 (1792MB) >>> (XEN) BANK[1] 0x000009e0000000-0x000009f0000000 (256MB) >>> 1792M allocated in 4GB address space. >>> >>> xen/arch/arm/domain_build.c | 20 +++++++++++++------- >>> 1 file changed, 13 insertions(+), 7 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 35ab08d..0b9b85c 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -240,11 +240,11 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) >> >> As mentioned on V1, please update the comment above this function. > > About this part: > * 3. For 32-bit dom0 we want to place as much of the RAM as we > * reasonably can below 4GB, so that it can be used by non-LPAE > * enabled kernels. > > Changed to this V3: > For dom0 we want to place as much of the RAM as we reasonably can > below 4GB, so the devices have the limitation to access 64 bits > address space can work properly and it can also be used by > non-LPAE enabled kernels for 32-bit dom0. > > About > " > * For 32-bit domain we require that the initial allocation for the > * first bank is under 4G. > " > > Changed to this in V3: > > " > For 32-bit domain we require that the initial allocation for the > first bank is under 4G. For 64-bit domain, the first bank is preferred > to be allocated under 4GB. > " > > Is this ok? I think so. > >> >>> const unsigned int min_low_order = >>> get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); >>> const unsigned int min_order = get_order_from_bytes(MB(4)); >>> - struct page_info *pg; >>> + struct page_info *pg = NULL; >>> unsigned int order = get_11_allocation_size(kinfo->unassigned_mem); >>> int i; >>> >>> - bool_t lowmem = is_32bit_domain(d); >>> + bool_t lowmem = true; >>> unsigned int bits; >>> >>> /* >>> @@ -265,22 +265,28 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) >>> */ >>> while ( order >= min_low_order ) >>> { >>> - for ( bits = order ; bits <= (lowmem ? 32 : PADDR_BITS); bits++ ) >>> + for ( bits = order ; bits <= 32 ; bits++ ) >>> { >>> pg = alloc_domheap_pages(d, order, MEMF_bits(bits)); >>> if ( pg != NULL ) >>> + { >>> + if ( !insert_11_bank(d, kinfo, pg, order) ) >>> + BUG(); /* Cannot fail for first bank */ >>> + >>> goto got_bank0; >>> + } >>> } >>> order--; >>> } >>> >>> - panic("Unable to allocate first memory bank"); >>> + if ( pg == NULL ) >>> + { >>> + printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); >>> + lowmem = false; >>> + } >> >> This new behavior should be documented in the commit message, with some >> rationale why it is fine to do it for ARM32. > > Consider the comments of the function: > 1. The dom0 kernel should be loaded within the first 128MB of RAM. This > is necessary at least for Linux zImage kernels, which are all we > support today. > 4. For 32-bit dom0 the kernel must be located below 4GB. > > I think I may need to add back the panic when first bank is failed to be allocated > under 4GB. It occurred to me that the first bank below 4GB is a mandatory for 32-bit domain, because the kernel is booted with paging disabled. > > Will use the following code in V3 and document this in commit log. > if ( pg == NULL ) > { > if ( is_32bit_domain(d) ) > panic("Unable to allocate first memory bank"); > > printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); > lowmem = false; > } > > >> >>> >>> got_bank0: >>> >>> - if ( !insert_11_bank(d, kinfo, pg, order) ) >>> - BUG(); /* Cannot fail for first bank */ >>> - >>> /* Now allocate more memory and fill in additional banks */ >> >> This comment should be updated. > > Add more comments? I did not modify code below the comment and the comment > is correct I think. Well, you can reach here with the bank 0 not allocated, so "now allocate more memory..." is inaccurate for me. When you do change, you also need to see whether it fits with the rest of the function. Regards,
On Wed, Sep 21, 2016 at 11:39:11AM +0100, Julien Grall wrote: > > >On 20/09/16 10:10, Peng Fan wrote: >>Hello Julien, > >Hello Peng, > >>On Tue, Sep 20, 2016 at 10:36:27AM +0200, Julien Grall wrote: >>>Hello Peng, >>> >>>On 20/09/2016 07:52, van.freenix@gmail.com wrote: >>>>From: Peng Fan <peng.fan@nxp.com> >>>> >>>>On AArch64 SoCs, some IPs may only have the capability to access >>>>32bits address space. The physical memory assigned for Dom0 maybe >>> >>>s/32bits/32 bits/ >> >>Fix in V3. >> >>> >>>>not in 4GB address space, then the IPs will not work properly. >>>>So need to allocate memory under 4GB for Dom0. >>>> >>>>There is no restriction that how much lowmem needs to be allocated for >>>>Dom0. Dom0 now use 1:1 mapping, but DomU does not use 1:1 mapping, >>>>there is no need to reserve lowmem for DomU, so allocate lowmem as much >>>>as possible for Dom0. >>>> >>>>Signed-off-by: Peng Fan <peng.fan@nxp.com> >>>>Cc: Stefano Stabellini <sstabellini@kernel.org> >>>>Cc: Julien Grall <julien.grall@arm.com> >>>>--- >>>> >>>>This patch is to resolve the issue mentioned in >>>>https://lists.xen.org/archives/html/xen-devel/2016-09/msg00235.html >>>> >>>>V2: >>>>Remove the bootargs dom0_lowmem introduced in V1. >>>>Following "https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01459.html" >>>>to allocate as much as possible lowmem. >>>> >>>>Tested results: >>>>(XEN) Allocating 1:1 mappings totalling 2048MB for dom0: >>>>(XEN) BANK[0] 0x00000088000000-0x000000f8000000 (1792MB) >>>>(XEN) BANK[1] 0x000009e0000000-0x000009f0000000 (256MB) >>>>1792M allocated in 4GB address space. >>>> >>>>xen/arch/arm/domain_build.c | 20 +++++++++++++------- >>>>1 file changed, 13 insertions(+), 7 deletions(-) >>>> >>>>diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>>index 35ab08d..0b9b85c 100644 >>>>--- a/xen/arch/arm/domain_build.c >>>>+++ b/xen/arch/arm/domain_build.c >>>>@@ -240,11 +240,11 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) >>> >>>As mentioned on V1, please update the comment above this function. >> >>About this part: >>* 3. For 32-bit dom0 we want to place as much of the RAM as we >>* reasonably can below 4GB, so that it can be used by non-LPAE >>* enabled kernels. >> >>Changed to this V3: >>For dom0 we want to place as much of the RAM as we reasonably can >>below 4GB, so the devices have the limitation to access 64 bits >>address space can work properly and it can also be used by >>non-LPAE enabled kernels for 32-bit dom0. >> >>About >>" >> * For 32-bit domain we require that the initial allocation for the >> * first bank is under 4G. >>" >> >>Changed to this in V3: >> >>" >>For 32-bit domain we require that the initial allocation for the >>first bank is under 4G. For 64-bit domain, the first bank is preferred >>to be allocated under 4GB. >>" >> >>Is this ok? > >I think so. Thanks. Update in v3. > >> >>> >>>> const unsigned int min_low_order = >>>> get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); >>>> const unsigned int min_order = get_order_from_bytes(MB(4)); >>>>- struct page_info *pg; >>>>+ struct page_info *pg = NULL; >>>> unsigned int order = get_11_allocation_size(kinfo->unassigned_mem); >>>> int i; >>>> >>>>- bool_t lowmem = is_32bit_domain(d); >>>>+ bool_t lowmem = true; >>>> unsigned int bits; >>>> >>>> /* >>>>@@ -265,22 +265,28 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) >>>> */ >>>> while ( order >= min_low_order ) >>>> { >>>>- for ( bits = order ; bits <= (lowmem ? 32 : PADDR_BITS); bits++ ) >>>>+ for ( bits = order ; bits <= 32 ; bits++ ) >>>> { >>>> pg = alloc_domheap_pages(d, order, MEMF_bits(bits)); >>>> if ( pg != NULL ) >>>>+ { >>>>+ if ( !insert_11_bank(d, kinfo, pg, order) ) >>>>+ BUG(); /* Cannot fail for first bank */ >>>>+ >>>> goto got_bank0; >>>>+ } >>>> } >>>> order--; >>>> } >>>> >>>>- panic("Unable to allocate first memory bank"); >>>>+ if ( pg == NULL ) >>>>+ { >>>>+ printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); >>>>+ lowmem = false; >>>>+ } >>> >>>This new behavior should be documented in the commit message, with some >>>rationale why it is fine to do it for ARM32. >> >>Consider the comments of the function: >> 1. The dom0 kernel should be loaded within the first 128MB of RAM. This >> is necessary at least for Linux zImage kernels, which are all we >> support today. >> 4. For 32-bit dom0 the kernel must be located below 4GB. >> >>I think I may need to add back the panic when first bank is failed to be allocated >>under 4GB. > >It occurred to me that the first bank below 4GB is a mandatory for 32-bit >domain, because the kernel is booted with paging disabled. Yeah. I'll add back the panic for 32bit domain. Add document this in commit message. > >> >>Will use the following code in V3 and document this in commit log. >> if ( pg == NULL ) >> { >> if ( is_32bit_domain(d) ) >> panic("Unable to allocate first memory bank"); >> >> printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); >> lowmem = false; >> } >> >> >>> >>>> >>>> got_bank0: >>>> >>>>- if ( !insert_11_bank(d, kinfo, pg, order) ) >>>>- BUG(); /* Cannot fail for first bank */ >>>>- >>>> /* Now allocate more memory and fill in additional banks */ >>> >>>This comment should be updated. >> >>Add more comments? I did not modify code below the comment and the comment >>is correct I think. > >Well, you can reach here with the bank 0 not allocated, so "now allocate more >memory..." is inaccurate for me. When you do change, you also need to see >whether it fits with the rest of the function. ok. Will update this in v3. Thanks, Peng. > >Regards, > >-- >Julien Grall
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 35ab08d..0b9b85c 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -240,11 +240,11 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) const unsigned int min_low_order = get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); const unsigned int min_order = get_order_from_bytes(MB(4)); - struct page_info *pg; + struct page_info *pg = NULL; unsigned int order = get_11_allocation_size(kinfo->unassigned_mem); int i; - bool_t lowmem = is_32bit_domain(d); + bool_t lowmem = true; unsigned int bits; /* @@ -265,22 +265,28 @@ static void allocate_memory(struct domain *d, struct kernel_info *kinfo) */ while ( order >= min_low_order ) { - for ( bits = order ; bits <= (lowmem ? 32 : PADDR_BITS); bits++ ) + for ( bits = order ; bits <= 32 ; bits++ ) { pg = alloc_domheap_pages(d, order, MEMF_bits(bits)); if ( pg != NULL ) + { + if ( !insert_11_bank(d, kinfo, pg, order) ) + BUG(); /* Cannot fail for first bank */ + goto got_bank0; + } } order--; } - panic("Unable to allocate first memory bank"); + if ( pg == NULL ) + { + printk(XENLOG_INFO "No bank has been allocated below 4GB.\n"); + lowmem = false; + } got_bank0: - if ( !insert_11_bank(d, kinfo, pg, order) ) - BUG(); /* Cannot fail for first bank */ - /* Now allocate more memory and fill in additional banks */ order = get_11_allocation_size(kinfo->unassigned_mem);