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 <drjo...@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 <drjo...@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.mayd...@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é <phi...@redhat.com>