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

[Andrew Jones]

On Tue, Jun 18, 2019 at 12:58:30PM +0100, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 12:56, Andrew Jones <address@hidden> wrote:
> >
> > On Tue, Jun 18, 2019 at 12:02:37PM +0100, Peter Maydell wrote:
> > > On Tue, 18 Jun 2019 at 09:34, Andrew Jones <address@hidden> 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 <address@hidden>
> > > > ---
> > > >  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 <address@hidden>
> > >
> > > 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