[Xen-devel,v2] tools: implement initial ramdisk support for ARM.

On 04/08/2014 06:29 PM, Julien Grall wrote:
> Hi Ian & Ian,
> 
> On 04/08/2014 05:53 PM, Ian Jackson wrote:
>> Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
>>> On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
>>>> I think this is a viable backport candidate.  Can you let me know when
>>>> it's in-tree and I'll add it to my list
>>>
>>> It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b
>>
>> Noted, thanks.
> 
> I'm unable to boot a guest with this patch on Xen 4.5. Revert it works
> correctly. The guest is blocked without useful log.
> 
> My config:
> 
> kernel="/root/zImage"
> memory=128
> name="test"
> vcpus=2
> autoballon="off"
> root="/dev/xvda"
> extra="console=hvc0"
> disk=[ 'phy:/dev/loop0,xvda,w' ]
> 

This small changes fix boot of the guest with RAM < 128Mb:

I guess this is because the kernel is extracting on it. I think we
should follow the same "algorithm" as Xen (see place_modules) to decide
where the modules should be loaded.

Regards,

On Tue, 2014-04-08 at 19:20 +0100, Julien Grall wrote:
> On 04/08/2014 06:29 PM, Julien Grall wrote:
> > Hi Ian & Ian,
> > 
> > On 04/08/2014 05:53 PM, Ian Jackson wrote:
> >> Ian Campbell writes ("Re: [PATCH v2] tools: implement initial ramdisk support for ARM."):
> >>> On Fri, 2014-04-04 at 16:00 +0100, Ian Jackson wrote:
> >>>> I think this is a viable backport candidate.  Can you let me know when
> >>>> it's in-tree and I'll add it to my list
> >>>
> >>> It's in now, commit 314c9815e2f5dc8a9fec11e0cf9b49b16ed0e96b
> >>
> >> Noted, thanks.
> > 
> > I'm unable to boot a guest with this patch on Xen 4.5. Revert it works
> > correctly. The guest is blocked without useful log.
> > 
> > My config:
> > 
> > kernel="/root/zImage"
> > memory=128
> > name="test"
> > vcpus=2
> > autoballon="off"
> > root="/dev/xvda"
> > extra="console=hvc0"
> > disk=[ 'phy:/dev/loop0,xvda,w' ]
> > 
> 
> This small changes fix boot of the guest with RAM < 128Mb:
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index f051515..2228ba5 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>      if ( ramend >= ram128mb + modsize && kernend < ram128mb )
>          modbase = ram128mb;
>      else if ( ramend >= kernend + modsize )
> -        modbase = kernend;
> +        modbase = ramend - modsize;
>      else
>          return -1;
> 
> I guess this is because the kernel is extracting on it. I think we
> should follow the same "algorithm" as Xen (see place_modules) to decide
> where the modules should be loaded.

Yes, this fix is correct. The existing code is just bogus, it makes no
sense to place the modules exactly at the end of RAM since they will
spin over the end.

Can you resubmit with a changelog and an S-o-b please.

Ian.

On 09/04/14 09:12, Ian Campbell wrote:
>> This small changes fix boot of the guest with RAM < 128Mb:
>>
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index f051515..2228ba5 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>>       if ( ramend >= ram128mb + modsize && kernend < ram128mb )
>>           modbase = ram128mb;
>>       else if ( ramend >= kernend + modsize )
>> -        modbase = kernend;
>> +        modbase = ramend - modsize;
>>       else
>>           return -1;
>>
>> I guess this is because the kernel is extracting on it. I think we
>> should follow the same "algorithm" as Xen (see place_modules) to decide
>> where the modules should be loaded.
>
> Yes, this fix is correct. The existing code is just bogus, it makes no
> sense to place the modules exactly at the end of RAM since they will
> spin over the end.

I'm not sure to understand why you are talking about RAM... currently 
the modules are placed just after the kernel (which in this case is 
always before ramend).

> Can you resubmit with a changelog and an S-o-b please.

Sure.

Regards,

On Wed, 2014-04-09 at 09:17 +0100, Julien Grall wrote:
> 
> On 09/04/14 09:12, Ian Campbell wrote:
> >> This small changes fix boot of the guest with RAM < 128Mb:
> >>
> >> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> >> index f051515..2228ba5 100644
> >> --- a/tools/libxc/xc_dom_arm.c
> >> +++ b/tools/libxc/xc_dom_arm.c
> >> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >>       if ( ramend >= ram128mb + modsize && kernend < ram128mb )
> >>           modbase = ram128mb;
> >>       else if ( ramend >= kernend + modsize )
> >> -        modbase = kernend;
> >> +        modbase = ramend - modsize;
> >>       else
> >>           return -1;
> >>
> >> I guess this is because the kernel is extracting on it. I think we
> >> should follow the same "algorithm" as Xen (see place_modules) to decide
> >> where the modules should be loaded.
> >
> > Yes, this fix is correct. The existing code is just bogus, it makes no
> > sense to place the modules exactly at the end of RAM since they will
> > spin over the end.
> 
> I'm not sure to understand why you are talking about RAM... currently 
> the modules are placed just after the kernel (which in this case is 
> always before ramend).

I misread the change.

I think the current code matches the dom0 place_modules call (modulo the
condition being expressed differently). Perhaps that case is also wrong?

> 
> > Can you resubmit with a changelog and an S-o-b please.
> 
> Sure.
> 
> Regards,
>

On 09/04/14 09:37, Ian Campbell wrote:
> On Wed, 2014-04-09 at 09:17 +0100, Julien Grall wrote:
>>
>> On 09/04/14 09:12, Ian Campbell wrote:
>>>> This small changes fix boot of the guest with RAM < 128Mb:
>>>>
>>>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>>>> index f051515..2228ba5 100644
>>>> --- a/tools/libxc/xc_dom_arm.c
>>>> +++ b/tools/libxc/xc_dom_arm.c
>>>> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>>>>        if ( ramend >= ram128mb + modsize && kernend < ram128mb )
>>>>            modbase = ram128mb;
>>>>        else if ( ramend >= kernend + modsize )
>>>> -        modbase = kernend;
>>>> +        modbase = ramend - modsize;
>>>>        else
>>>>            return -1;
>>>>
>>>> I guess this is because the kernel is extracting on it. I think we
>>>> should follow the same "algorithm" as Xen (see place_modules) to decide
>>>> where the modules should be loaded.
>>>
>>> Yes, this fix is correct. The existing code is just bogus, it makes no
>>> sense to place the modules exactly at the end of RAM since they will
>>> spin over the end.
>>
>> I'm not sure to understand why you are talking about RAM... currently
>> the modules are placed just after the kernel (which in this case is
>> always before ramend).
>
> I misread the change.
>
> I think the current code matches the dom0 place_modules call (modulo the
> condition being expressed differently). Perhaps that case is also wrong?

The 2 if of libxc  express the first if of dom0 place_modules, right?

If dom0 as less than 128MB of RAM, we are trying to load the modules as 
high as possible.

In case of the guest, we are loading them just after kernel. But ... the 
kernel will likely decompress on it.

On Wed, 2014-04-09 at 10:10 +0100, Julien Grall wrote:
> 
> On 09/04/14 09:37, Ian Campbell wrote:
> > On Wed, 2014-04-09 at 09:17 +0100, Julien Grall wrote:
> >>
> >> On 09/04/14 09:12, Ian Campbell wrote:
> >>>> This small changes fix boot of the guest with RAM < 128Mb:
> >>>>
> >>>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> >>>> index f051515..2228ba5 100644
> >>>> --- a/tools/libxc/xc_dom_arm.c
> >>>> +++ b/tools/libxc/xc_dom_arm.c
> >>>> @@ -300,7 +300,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >>>>        if ( ramend >= ram128mb + modsize && kernend < ram128mb )
> >>>>            modbase = ram128mb;
> >>>>        else if ( ramend >= kernend + modsize )
> >>>> -        modbase = kernend;
> >>>> +        modbase = ramend - modsize;
> >>>>        else
> >>>>            return -1;
> >>>>
> >>>> I guess this is because the kernel is extracting on it. I think we
> >>>> should follow the same "algorithm" as Xen (see place_modules) to decide
> >>>> where the modules should be loaded.
> >>>
> >>> Yes, this fix is correct. The existing code is just bogus, it makes no
> >>> sense to place the modules exactly at the end of RAM since they will
> >>> spin over the end.
> >>
> >> I'm not sure to understand why you are talking about RAM... currently
> >> the modules are placed just after the kernel (which in this case is
> >> always before ramend).
> >
> > I misread the change.
> >
> > I think the current code matches the dom0 place_modules call (modulo the
> > condition being expressed differently). Perhaps that case is also wrong?
> 
> The 2 if of libxc  express the first if of dom0 place_modules, right?
> 
> If dom0 as less than 128MB of RAM, we are trying to load the modules as 
> high as possible.

In Xen we have:
    if ( kernel_end < MIN(mem_start + MB(128), mem_end - total) )
        addr = MIN(mem_start + MB(128), mem_end - total);
    else if ( mem_end - ROUNDUP(kernel_end, MB(2)) >= total )
        addr = ROUNDUP(kernel_end, MB(2));
    else if ( kernel_start - mem_start >= total )
        addr = kernel_start - total;
    else
    {
        panic("Unable to find suitable location for dtb+initrd");

In libxc we have:
    if ( ramend >= ram128mb + modsize && kernend < ram128mb )
        modbase = ram128mb;
    else if ( ramend >= kernend + modsize )
        modbase = kernend;
    else
        return -1;

I think the second if and assignments are equivalent. I think the
problem comes from the differences in the first if which are in fact
different. They also differ in that userspace won't try and place the
modules before the kernel as a last resort.

Switching the Xen side to use the  libxc variable names makes it less
confusing to compare IMO:

    if ( kernend < MIN(ram128mb, ramend - modsize) )
        addr = MIN(ram128mb, ramend - modsize);
    else if ( ramend - kernend >= modsize )
        addr = kernend;
    else if ( kernel_start - mem_start >= total )
        addr = kernel_start - total;
    else
    {
        panic("Unable to find suitable location for dtb+initrd");

I think we need to use that MIN(ram128mb, ramend - modsize) calculation
in libxc too.

I'm also wondering if the second statement actually make sense with that
change. If kernelend >= ramend - modsize (so the first test fails) then 
I'm not convinced that ramend - kernend >= modsize can ever be true.

Ian.