hw/arm/boot: fix direct kernel boot setup

[Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Andrew Jones 6 years, 4 months ago

We need to check ram_end, not ram_size.

Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
DTB off the end of RAM")
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b2f93f6beff6..8a280ab3ed49 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
                              info->initrd_filename);
                 exit(1);
             }
-            if (info->initrd_start + initrd_size > info->ram_size) {
+            if (info->initrd_start + initrd_size > ram_end) {
                 error_report("could not load initrd '%s': "
                              "too big to fit into RAM after the kernel",
                              info->initrd_filename);
-- 
2.20.1

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Peter Maydell 6 years, 4 months ago

On Tue, 18 Jun 2019 at 09:34, Andrew Jones <drjones@redhat.com> wrote:
>
> We need to check ram_end, not ram_size.
>
> Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
> DTB off the end of RAM")
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/boot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index b2f93f6beff6..8a280ab3ed49 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>                               info->initrd_filename);
>                  exit(1);
>              }
> -            if (info->initrd_start + initrd_size > info->ram_size) {
> +            if (info->initrd_start + initrd_size > ram_end) {
>                  error_report("could not load initrd '%s': "
>                               "too big to fit into RAM after the kernel",
>                               info->initrd_filename);
> --
> 2.20.1

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I think I missed this because my test case doesn't have an
initrd -- direct kernel boot works fine if all you're
passing to QEMU is the kernel... I think we could clarify
the commit message a little:

hw/arm/boot: fix direct kernel boot with initrd

Fix the condition used to check whether the initrd fits
into RAM; this meant we were spuriously refusing to do
a direct boot of a kernel in some cases if an initrd
was also passed on the command line.

?

(if you agree I can just fix up the commit message when I apply it.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Philippe Mathieu-Daudé 6 years, 4 months ago

On 6/18/19 1:02 PM, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 09:34, Andrew Jones <drjones@redhat.com> wrote:
>>
>> We need to check ram_end, not ram_size.
>>
>> Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
>> DTB off the end of RAM")
>> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>> ---
>>  hw/arm/boot.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index b2f93f6beff6..8a280ab3ed49 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>>                               info->initrd_filename);
>>                  exit(1);
>>              }
>> -            if (info->initrd_start + initrd_size > info->ram_size) {
>> +            if (info->initrd_start + initrd_size > ram_end) {
>>                  error_report("could not load initrd '%s': "
>>                               "too big to fit into RAM after the kernel",
>>                               info->initrd_filename);
>> --
>> 2.20.1
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I think I missed this because my test case doesn't have an
> initrd -- direct kernel boot works fine if all you're
> passing to QEMU is the kernel... I think we could clarify
> the commit message a little:
> 
> hw/arm/boot: fix direct kernel boot with initrd
> 
> Fix the condition used to check whether the initrd fits
> into RAM; this meant we were spuriously refusing to do
> a direct boot of a kernel in some cases if an initrd
> was also passed on the command line.
> 
> ?
> 
> (if you agree I can just fix up the commit message when I apply it.)
> 
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Andrew Jones 6 years, 4 months ago

On Tue, Jun 18, 2019 at 12:02:37PM +0100, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 09:34, Andrew Jones <drjones@redhat.com> wrote:
> >
> > We need to check ram_end, not ram_size.
> >
> > Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
> > DTB off the end of RAM")
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  hw/arm/boot.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index b2f93f6beff6..8a280ab3ed49 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> >                               info->initrd_filename);
> >                  exit(1);
> >              }
> > -            if (info->initrd_start + initrd_size > info->ram_size) {
> > +            if (info->initrd_start + initrd_size > ram_end) {
> >                  error_report("could not load initrd '%s': "
> >                               "too big to fit into RAM after the kernel",
> >                               info->initrd_filename);
> > --
> > 2.20.1
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I think I missed this because my test case doesn't have an
> initrd -- direct kernel boot works fine if all you're
> passing to QEMU is the kernel... I think we could clarify
> the commit message a little:

I found it using kvm-unit-tests which uses a small initrd to
pass environment variables to the guest. All the tests started
to report FAIL.

> 
> hw/arm/boot: fix direct kernel boot with initrd
> 
> Fix the condition used to check whether the initrd fits
> into RAM; this meant we were spuriously refusing to do
> a direct boot of a kernel in some cases if an initrd
> was also passed on the command line.

Actually I think we need another fix for this error too. We
weren't actually refusing do direct boot the kernel, but we
should have been. We're missing the 'exit(1)' after the error
message.

> 
> ?
> 
> (if you agree I can just fix up the commit message when I apply it.)

I agree with the improved commit message if we also add the
'exit(1)', otherwise "refusing to boot" isn't the right thing
to say.

Thanks,
drew

> 
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Philippe Mathieu-Daudé 6 years, 4 months ago

On 6/18/19 1:55 PM, Andrew Jones wrote:
> On Tue, Jun 18, 2019 at 12:02:37PM +0100, Peter Maydell wrote:
>> On Tue, 18 Jun 2019 at 09:34, Andrew Jones <drjones@redhat.com> wrote:
>>>
>>> We need to check ram_end, not ram_size.
>>>
>>> Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
>>> DTB off the end of RAM")
>>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>>  hw/arm/boot.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index b2f93f6beff6..8a280ab3ed49 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>>>                               info->initrd_filename);
>>>                  exit(1);
>>>              }
>>> -            if (info->initrd_start + initrd_size > info->ram_size) {
>>> +            if (info->initrd_start + initrd_size > ram_end) {
>>>                  error_report("could not load initrd '%s': "
>>>                               "too big to fit into RAM after the kernel",
>>>                               info->initrd_filename);
>>> --
>>> 2.20.1
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> I think I missed this because my test case doesn't have an
>> initrd -- direct kernel boot works fine if all you're
>> passing to QEMU is the kernel... I think we could clarify
>> the commit message a little:
> 
> I found it using kvm-unit-tests which uses a small initrd to
> pass environment variables to the guest. All the tests started
> to report FAIL.
> 
>>
>> hw/arm/boot: fix direct kernel boot with initrd
>>
>> Fix the condition used to check whether the initrd fits
>> into RAM; this meant we were spuriously refusing to do
>> a direct boot of a kernel in some cases if an initrd
>> was also passed on the command line.
> 
> Actually I think we need another fix for this error too. We
> weren't actually refusing do direct boot the kernel, but we
> should have been. We're missing the 'exit(1)' after the error
> message.
> 
>>
>> ?
>>
>> (if you agree I can just fix up the commit message when I apply it.)
> 
> I agree with the improved commit message if we also add the
> 'exit(1)', otherwise "refusing to boot" isn't the right thing
> to say.

Indeed. So for this patch + Peter comment + exit():

Fixes: 852dc64d665
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Peter Maydell 6 years, 4 months ago

On Tue, 18 Jun 2019 at 12:56, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Jun 18, 2019 at 12:02:37PM +0100, Peter Maydell wrote:
> > On Tue, 18 Jun 2019 at 09:34, Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > We need to check ram_end, not ram_size.
> > >
> > > Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
> > > DTB off the end of RAM")
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  hw/arm/boot.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > index b2f93f6beff6..8a280ab3ed49 100644
> > > --- a/hw/arm/boot.c
> > > +++ b/hw/arm/boot.c
> > > @@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> > >                               info->initrd_filename);
> > >                  exit(1);
> > >              }
> > > -            if (info->initrd_start + initrd_size > info->ram_size) {
> > > +            if (info->initrd_start + initrd_size > ram_end) {
> > >                  error_report("could not load initrd '%s': "
> > >                               "too big to fit into RAM after the kernel",
> > >                               info->initrd_filename);
> > > --
> > > 2.20.1
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > I think I missed this because my test case doesn't have an
> > initrd -- direct kernel boot works fine if all you're
> > passing to QEMU is the kernel... I think we could clarify
> > the commit message a little:
>
> I found it using kvm-unit-tests which uses a small initrd to
> pass environment variables to the guest. All the tests started
> to report FAIL.
>
> >
> > hw/arm/boot: fix direct kernel boot with initrd
> >
> > Fix the condition used to check whether the initrd fits
> > into RAM; this meant we were spuriously refusing to do
> > a direct boot of a kernel in some cases if an initrd
> > was also passed on the command line.
>
> Actually I think we need another fix for this error too. We
> weren't actually refusing do direct boot the kernel, but we
> should have been. We're missing the 'exit(1)' after the error
> message.

Hmm, so we are. Do you want to send a v2 then, which fixes
that too and fixes up the commit message?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Andrew Jones 6 years, 4 months ago

On Tue, Jun 18, 2019 at 12:58:30PM +0100, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 12:56, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 12:02:37PM +0100, Peter Maydell wrote:
> > > On Tue, 18 Jun 2019 at 09:34, Andrew Jones <drjones@redhat.com> wrote:
> > > >
> > > > We need to check ram_end, not ram_size.
> > > >
> > > > Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
> > > > DTB off the end of RAM")
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > >  hw/arm/boot.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > > index b2f93f6beff6..8a280ab3ed49 100644
> > > > --- a/hw/arm/boot.c
> > > > +++ b/hw/arm/boot.c
> > > > @@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> > > >                               info->initrd_filename);
> > > >                  exit(1);
> > > >              }
> > > > -            if (info->initrd_start + initrd_size > info->ram_size) {
> > > > +            if (info->initrd_start + initrd_size > ram_end) {
> > > >                  error_report("could not load initrd '%s': "
> > > >                               "too big to fit into RAM after the kernel",
> > > >                               info->initrd_filename);
> > > > --
> > > > 2.20.1
> > >
> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > >
> > > I think I missed this because my test case doesn't have an
> > > initrd -- direct kernel boot works fine if all you're
> > > passing to QEMU is the kernel... I think we could clarify
> > > the commit message a little:
> >
> > I found it using kvm-unit-tests which uses a small initrd to
> > pass environment variables to the guest. All the tests started
> > to report FAIL.
> >
> > >
> > > hw/arm/boot: fix direct kernel boot with initrd
> > >
> > > Fix the condition used to check whether the initrd fits
> > > into RAM; this meant we were spuriously refusing to do
> > > a direct boot of a kernel in some cases if an initrd
> > > was also passed on the command line.
> >
> > Actually I think we need another fix for this error too. We
> > weren't actually refusing do direct boot the kernel, but we
> > should have been. We're missing the 'exit(1)' after the error
> > message.
> 
> Hmm, so we are. Do you want to send a v2 then, which fixes
> that too and fixes up the commit message?

On it's way.

Thanks,
drew

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup

Posted by Andrew Jones 6 years, 4 months ago

On Tue, Jun 18, 2019 at 02:50:15PM +0200, Andrew Jones wrote:
> On Tue, Jun 18, 2019 at 12:58:30PM +0100, Peter Maydell wrote:
> > On Tue, 18 Jun 2019 at 12:56, Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 12:02:37PM +0100, Peter Maydell wrote:
> > > > On Tue, 18 Jun 2019 at 09:34, Andrew Jones <drjones@redhat.com> wrote:
> > > > >
> > > > > We need to check ram_end, not ram_size.
> > > > >
> > > > > Fixes: 852dc64d665f ("hw/arm/boot: Diagnose layouts that put initrd or
> > > > > DTB off the end of RAM")
> > > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > > ---
> > > > >  hw/arm/boot.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > > > > index b2f93f6beff6..8a280ab3ed49 100644
> > > > > --- a/hw/arm/boot.c
> > > > > +++ b/hw/arm/boot.c
> > > > > @@ -1109,7 +1109,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> > > > >                               info->initrd_filename);
> > > > >                  exit(1);
> > > > >              }
> > > > > -            if (info->initrd_start + initrd_size > info->ram_size) {
> > > > > +            if (info->initrd_start + initrd_size > ram_end) {
> > > > >                  error_report("could not load initrd '%s': "
> > > > >                               "too big to fit into RAM after the kernel",
> > > > >                               info->initrd_filename);
> > > > > --
> > > > > 2.20.1
> > > >
> > > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > > >
> > > > I think I missed this because my test case doesn't have an
> > > > initrd -- direct kernel boot works fine if all you're
> > > > passing to QEMU is the kernel... I think we could clarify
> > > > the commit message a little:
> > >
> > > I found it using kvm-unit-tests which uses a small initrd to
> > > pass environment variables to the guest. All the tests started
> > > to report FAIL.
> > >
> > > >
> > > > hw/arm/boot: fix direct kernel boot with initrd
> > > >
> > > > Fix the condition used to check whether the initrd fits
> > > > into RAM; this meant we were spuriously refusing to do
> > > > a direct boot of a kernel in some cases if an initrd
> > > > was also passed on the command line.
> > >
> > > Actually I think we need another fix for this error too. We
> > > weren't actually refusing do direct boot the kernel, but we
> > > should have been. We're missing the 'exit(1)' after the error
> > > message.
> > 
> > Hmm, so we are. Do you want to send a v2 then, which fixes
> > that too and fixes up the commit message?
> 
> On it's way.
>

Argh, just fired it off by forgot the '-v2' on my format-patch so
the subject tag won't be right. Sorry about that.

drew