Re: [Xen-devel] [PATCH 3/3] xen: arm: rework dom0 initrd and dtb placement

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel