[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH] hw/arm/boot: fix direct kernel boot setup |
Date: |
Tue, 18 Jun 2019 14:08:13 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
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 <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.
>
>>
>> ?
>>
>> (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é <address@hidden>