| Message ID | 1397044276-30185-3-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | Accepted |
| Commit | bef15de706d796f0d9f16fd3b69d6b5cada2ce9b |
| Headers | show |
Hi Ian, On 04/09/2014 12:51 PM, Ian Campbell wrote: > This now uses the same decision tree as libxc (which is much easier to test). > > The main change is to explictly handle the placement at 128MB or end of RAM as s/explictly/explicitly/ > two cases, rather than combining with MIN. The effect is the same but the code > is clearer. > > Secondly the attempt to place the mopules right after the kernel is removed, s/mopules/modules/ > since it is redundant with the case where placing them at the end of RAM ends > up abutting the kernel. > > Also round the kernel size up to a 2MB boundary. A bit surprised that it was not done before :). > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > I'm sure to regret playing with this yet again... > --- > xen/arch/arm/kernel.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > index bc625a4..1102392 100644 > --- a/xen/arch/arm/kernel.c > +++ b/xen/arch/arm/kernel.c > @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info, > const paddr_t rambase = info->mem.bank[0].start; > const paddr_t ramsize = info->mem.bank[0].size; > const paddr_t ramend = rambase + ramsize; > - const paddr_t kernsize = kernend - kernbase; > + const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase; You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you roundup directly kernend?
On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote: > Hi Ian, > > On 04/09/2014 12:51 PM, Ian Campbell wrote: > > This now uses the same decision tree as libxc (which is much easier to test). > > > > The main change is to explictly handle the placement at 128MB or end of RAM as > > s/explictly/explicitly/ > s/mopules/modules/ Gah, fingers not working right today it seems. > > since it is redundant with the case where placing them at the end of RAM ends > > up abutting the kernel. > > > > Also round the kernel size up to a 2MB boundary. > > A bit surprised that it was not done before :). Me too. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > --- > > I'm sure to regret playing with this yet again... > > --- > > xen/arch/arm/kernel.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > > index bc625a4..1102392 100644 > > --- a/xen/arch/arm/kernel.c > > +++ b/xen/arch/arm/kernel.c > > @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info, > > const paddr_t rambase = info->mem.bank[0].start; > > const paddr_t ramsize = info->mem.bank[0].size; > > const paddr_t ramend = rambase + ramsize; > > - const paddr_t kernsize = kernend - kernbase; > > + const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase; > > You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you > roundup directly kernend? It's passed as a paramter, and it's not possible to round it before using it here (code before declarations), so I'd have to make kernsize non-const and initialise it after the rounding. I didn't think it was worth it. Ian.
On 04/09/2014 01:57 PM, Ian Campbell wrote: > On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote: >> Hi Ian, >> >> On 04/09/2014 12:51 PM, Ian Campbell wrote: >>> This now uses the same decision tree as libxc (which is much easier to test). >>> >>> The main change is to explictly handle the placement at 128MB or end of RAM as >> >> s/explictly/explicitly/ >> s/mopules/modules/ > > Gah, fingers not working right today it seems. > >>> since it is redundant with the case where placing them at the end of RAM ends >>> up abutting the kernel. >>> >>> Also round the kernel size up to a 2MB boundary. >> >> A bit surprised that it was not done before :). > > Me too. > >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>> --- >>> I'm sure to regret playing with this yet again... >>> --- >>> xen/arch/arm/kernel.c | 22 ++++++++++++---------- >>> 1 file changed, 12 insertions(+), 10 deletions(-) >>> >>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >>> index bc625a4..1102392 100644 >>> --- a/xen/arch/arm/kernel.c >>> +++ b/xen/arch/arm/kernel.c >>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info, >>> const paddr_t rambase = info->mem.bank[0].start; >>> const paddr_t ramsize = info->mem.bank[0].size; >>> const paddr_t ramend = rambase + ramsize; >>> - const paddr_t kernsize = kernend - kernbase; >>> + const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase; >> >> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you >> roundup directly kernend? > > It's passed as a paramter, and it's not possible to round it before > using it here (code before declarations), so I'd have to make kernsize > non-const and initialise it after the rounding. I didn't think it was > worth it. In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)). As I understand, we don't need to round up on the second if expression (see code below). + if ( ramend >= ram128mb + modsize && kernend < ram128mb ) + modbase = ram128mb; + else if ( ramend - modsize > ROUNDUP(kernend, MB(2)) ) + modbase = ramend - modsize; + else if ( kernbase - rambase > modsize ) Regards,
On Wed, 2014-04-09 at 14:09 +0100, Julien Grall wrote: > On 04/09/2014 01:57 PM, Ian Campbell wrote: > > On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote: > >> Hi Ian, > >> > >> On 04/09/2014 12:51 PM, Ian Campbell wrote: > >>> This now uses the same decision tree as libxc (which is much easier to test). > >>> > >>> The main change is to explictly handle the placement at 128MB or end of RAM as > >> > >> s/explictly/explicitly/ > >> s/mopules/modules/ > > > > Gah, fingers not working right today it seems. > > > >>> since it is redundant with the case where placing them at the end of RAM ends > >>> up abutting the kernel. > >>> > >>> Also round the kernel size up to a 2MB boundary. > >> > >> A bit surprised that it was not done before :). > > > > Me too. > > > >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > >>> --- > >>> I'm sure to regret playing with this yet again... > >>> --- > >>> xen/arch/arm/kernel.c | 22 ++++++++++++---------- > >>> 1 file changed, 12 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > >>> index bc625a4..1102392 100644 > >>> --- a/xen/arch/arm/kernel.c > >>> +++ b/xen/arch/arm/kernel.c > >>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info, > >>> const paddr_t rambase = info->mem.bank[0].start; > >>> const paddr_t ramsize = info->mem.bank[0].size; > >>> const paddr_t ramend = rambase + ramsize; > >>> - const paddr_t kernsize = kernend - kernbase; > >>> + const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase; > >> > >> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you > >> roundup directly kernend? > > > > It's passed as a paramter, and it's not possible to round it before > > using it here (code before declarations), so I'd have to make kernsize > > non-const and initialise it after the rounding. I didn't think it was > > worth it. > > In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)). > > As I understand, we don't need to round up on the second if expression > (see code below). It ensures that the modules don't start until at least the 2M boundary after the kernel's end. The userspace side does the same. Whether that's really worthwhile I don't know. > > + if ( ramend >= ram128mb + modsize && kernend < ram128mb ) > + modbase = ram128mb; > + else if ( ramend - modsize > ROUNDUP(kernend, MB(2)) ) > + modbase = ramend - modsize; > + else if ( kernbase - rambase > modsize ) > > Regards, >
On 04/09/2014 02:15 PM, Ian Campbell wrote: > On Wed, 2014-04-09 at 14:09 +0100, Julien Grall wrote: >> On 04/09/2014 01:57 PM, Ian Campbell wrote: >>> On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote: >>>> Hi Ian, >>>> >>>> On 04/09/2014 12:51 PM, Ian Campbell wrote: >>>>> This now uses the same decision tree as libxc (which is much easier to test). >>>>> >>>>> The main change is to explictly handle the placement at 128MB or end of RAM as >>>> >>>> s/explictly/explicitly/ >>>> s/mopules/modules/ >>> >>> Gah, fingers not working right today it seems. >>> >>>>> since it is redundant with the case where placing them at the end of RAM ends >>>>> up abutting the kernel. >>>>> >>>>> Also round the kernel size up to a 2MB boundary. >>>> >>>> A bit surprised that it was not done before :). >>> >>> Me too. >>> >>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>>>> --- >>>>> I'm sure to regret playing with this yet again... >>>>> --- >>>>> xen/arch/arm/kernel.c | 22 ++++++++++++---------- >>>>> 1 file changed, 12 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c >>>>> index bc625a4..1102392 100644 >>>>> --- a/xen/arch/arm/kernel.c >>>>> +++ b/xen/arch/arm/kernel.c >>>>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info, >>>>> const paddr_t rambase = info->mem.bank[0].start; >>>>> const paddr_t ramsize = info->mem.bank[0].size; >>>>> const paddr_t ramend = rambase + ramsize; >>>>> - const paddr_t kernsize = kernend - kernbase; >>>>> + const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase; >>>> >>>> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you >>>> roundup directly kernend? >>> >>> It's passed as a paramter, and it's not possible to round it before >>> using it here (code before declarations), so I'd have to make kernsize >>> non-const and initialise it after the rounding. I didn't think it was >>> worth it. >> >> In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)). >> >> As I understand, we don't need to round up on the second if expression >> (see code below). > > It ensures that the modules don't start until at least the 2M boundary > after the kernel's end. The userspace side does the same. The userspace is consistent with the usage of kernend. It's not the case here. There is no reason to mix the usage of kernend with and without ROUNDUP. In another point, I don't think 2MB-aligned is enough to ensure the kernel won't decompress on the device tree.
On Wed, 2014-04-09 at 14:24 +0100, Julien Grall wrote: > On 04/09/2014 02:15 PM, Ian Campbell wrote: > > On Wed, 2014-04-09 at 14:09 +0100, Julien Grall wrote: > >> On 04/09/2014 01:57 PM, Ian Campbell wrote: > >>> On Wed, 2014-04-09 at 13:54 +0100, Julien Grall wrote: > >>>> Hi Ian, > >>>> > >>>> On 04/09/2014 12:51 PM, Ian Campbell wrote: > >>>>> This now uses the same decision tree as libxc (which is much easier to test). > >>>>> > >>>>> The main change is to explictly handle the placement at 128MB or end of RAM as > >>>> > >>>> s/explictly/explicitly/ > >>>> s/mopules/modules/ > >>> > >>> Gah, fingers not working right today it seems. > >>> > >>>>> since it is redundant with the case where placing them at the end of RAM ends > >>>>> up abutting the kernel. > >>>>> > >>>>> Also round the kernel size up to a 2MB boundary. > >>>> > >>>> A bit surprised that it was not done before :). > >>> > >>> Me too. > >>> > >>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > >>>>> --- > >>>>> I'm sure to regret playing with this yet again... > >>>>> --- > >>>>> xen/arch/arm/kernel.c | 22 ++++++++++++---------- > >>>>> 1 file changed, 12 insertions(+), 10 deletions(-) > >>>>> > >>>>> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c > >>>>> index bc625a4..1102392 100644 > >>>>> --- a/xen/arch/arm/kernel.c > >>>>> +++ b/xen/arch/arm/kernel.c > >>>>> @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info, > >>>>> const paddr_t rambase = info->mem.bank[0].start; > >>>>> const paddr_t ramsize = info->mem.bank[0].size; > >>>>> const paddr_t ramend = rambase + ramsize; > >>>>> - const paddr_t kernsize = kernend - kernbase; > >>>>> + const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase; > >>>> > >>>> You are using ROUNDUP(kernend, MB(2)) in few places, why didn't you > >>>> roundup directly kernend? > >>> > >>> It's passed as a paramter, and it's not possible to round it before > >>> using it here (code before declarations), so I'd have to make kernsize > >>> non-const and initialise it after the rounding. I didn't think it was > >>> worth it. > >> > >> In this case I'm lost... You are mixing kernend and ROUNDUP(kernend, MB(2)). > >> > >> As I understand, we don't need to round up on the second if expression > >> (see code below). > > > > It ensures that the modules don't start until at least the 2M boundary > > after the kernel's end. The userspace side does the same. > > The userspace is consistent with the usage of kernend. It's not the case > here. There is no reason to mix the usage of kernend with and without > ROUNDUP. Ah, you mean the lack of rounding in the first if: if ( ramend >= ram128mb + modsize && kernend < ram128mb ) ? Since ram128mb is 2MB aligned it doesn't really matter that kernend isn't. > In another point, I don't think 2MB-aligned is enough to ensure the > kernel won't decompress on the device tree. Yes, like I say I don't know if it actually worthwhile. Ian.
On 04/09/2014 02:28 PM, Ian Campbell wrote: > > Ah, you mean the lack of rounding in the first if: > if ( ramend >= ram128mb + modsize && kernend < ram128mb ) > ? > > Since ram128mb is 2MB aligned it doesn't really matter that kernend > isn't. Oh right, I was lost with all the arithmetic. >> In another point, I don't think 2MB-aligned is enough to ensure the >> kernel won't decompress on the device tree. > > Yes, like I say I don't know if it actually worthwhile. Ok. Let's stick with this solution and see if someone platform will complain: Acked-by: Julien Grall <julien.grall@linaro.org>
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index bc625a4..1102392 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -77,7 +77,7 @@ static void place_modules(struct kernel_info *info, const paddr_t rambase = info->mem.bank[0].start; const paddr_t ramsize = info->mem.bank[0].size; const paddr_t ramend = rambase + ramsize; - const paddr_t kernsize = kernend - kernbase; + const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase; const paddr_t ram128mb = rambase + MB(128); paddr_t modbase; @@ -95,16 +95,18 @@ static void place_modules(struct kernel_info *info, * If the bootloader provides an initrd, it will be loaded just * after the DTB. * - * We try to place dtb+initrd at 128MB, (or, if we have less RAM, - * as high as possible). If there is no space then fallback to - * just after the kernel, if there is room, otherwise just before. + * We try to place dtb+initrd at 128MB or if we have less RAM + * as high as possible. If there is no space then fallback to + * just before the kernel. + * + * If changing this then consider + * tools/libxc/xc_dom_arm.c:arch_setup_meminit as well. */ - - if ( kernend < MIN(ram128mb, ramend - modsize) ) - modbase = MIN(ram128mb, ramend - modsize); - else if ( ramend - ROUNDUP(kernend, MB(2)) >= modsize ) - modbase = ROUNDUP(kernend, MB(2)); - else if ( kernbase - rambase >= modsize ) + if ( ramend >= ram128mb + modsize && kernend < ram128mb ) + modbase = ram128mb; + else if ( ramend - modsize > ROUNDUP(kernend, MB(2)) ) + modbase = ramend - modsize; + else if ( kernbase - rambase > modsize ) modbase = kernbase - modsize; else {
This now uses the same decision tree as libxc (which is much easier to test). The main change is to explictly handle the placement at 128MB or end of RAM as two cases, rather than combining with MIN. The effect is the same but the code is clearer. Secondly the attempt to place the mopules right after the kernel is removed, since it is redundant with the case where placing them at the end of RAM ends up abutting the kernel. Also round the kernel size up to a 2MB boundary. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- I'm sure to regret playing with this yet again... --- xen/arch/arm/kernel.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)