[v2] hw/arm/boot: Report error msg if loading elf/dtb failed

[PATCH v2] hw/arm/boot: Report error msg if loading elf/dtb failed

Posted by Changbin Du via 2 months, 3 weeks ago

Print errors before exit. Do not exit silently.

Signed-off-by: Changbin Du <changbin.du@huawei.com>

---
v2: remove msg for arm_load_dtb.
---
 hw/arm/boot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index d480a7da02cf..e15bf097a559 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -839,6 +839,7 @@ static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
                       1, data_swab, as);
     if (ret <= 0) {
         /* The header loaded but the image didn't */
+        error_report("could not load elf '%s'", info->kernel_filename);
         exit(1);
     }
 
-- 
2.34.1

Re: [PATCH v2] hw/arm/boot: Report error msg if loading elf/dtb failed

Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago

Hi Changbin,

On 30/8/24 12:53, Changbin Du via wrote:
> Print errors before exit. Do not exit silently.
> 
> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> 
> ---
> v2: remove msg for arm_load_dtb.
> ---
>   hw/arm/boot.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index d480a7da02cf..e15bf097a559 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -839,6 +839,7 @@ static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
>                         1, data_swab, as);
>       if (ret <= 0) {
>           /* The header loaded but the image didn't */
> +        error_report("could not load elf '%s'", info->kernel_filename);

"Could ..." (caps)

"hw/loader.h" is not well documented, but it seems load_elf*() returns:

   #define ELF_LOAD_FAILED       -1
   #define ELF_LOAD_NOT_ELF      -2
   #define ELF_LOAD_WRONG_ARCH   -3
   #define ELF_LOAD_WRONG_ENDIAN -4
   #define ELF_LOAD_TOO_BIG      -5

And we can display this error calling:

   const char *load_elf_strerror(ssize_t error);

So we can be more precise here using:

   error_report("Could not load elf '%s'", info->kernel_filename,
                load_elf_strerror(ret));

>           exit(1);
>       }
>   

Better (but out of scope of this patch) could be to pass an Error *errp
argument to the load_elf*() family of functions, and fill it with the
appropriate error message.

Regards,

Phil.

Re: [PATCH v2] hw/arm/boot: Report error msg if loading elf/dtb failed

Posted by duchangbin via 2 months, 3 weeks ago

On Mon, Sep 02, 2024 at 09:55:19PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Changbin,
> 
> On 30/8/24 12:53, Changbin Du via wrote:
> > Print errors before exit. Do not exit silently.
> > 
> > Signed-off-by: Changbin Du <changbin.du@huawei.com>
> > 
> > ---
> > v2: remove msg for arm_load_dtb.
> > ---
> >   hw/arm/boot.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index d480a7da02cf..e15bf097a559 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -839,6 +839,7 @@ static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> >                         1, data_swab, as);
> >       if (ret <= 0) {
> >           /* The header loaded but the image didn't */
> > +        error_report("could not load elf '%s'", info->kernel_filename);
> 
> "Could ..." (caps)
> 
> "hw/loader.h" is not well documented, but it seems load_elf*() returns:
> 
>   #define ELF_LOAD_FAILED       -1
>   #define ELF_LOAD_NOT_ELF      -2
>   #define ELF_LOAD_WRONG_ARCH   -3
>   #define ELF_LOAD_WRONG_ENDIAN -4
>   #define ELF_LOAD_TOO_BIG      -5
> 
> And we can display this error calling:
> 
>   const char *load_elf_strerror(ssize_t error);
> 
> So we can be more precise here using:
> 
>   error_report("Could not load elf '%s'", info->kernel_filename,
>                load_elf_strerror(ret));
> 
> >           exit(1);
> >       }
> 
> Better (but out of scope of this patch) could be to pass an Error *errp
> argument to the load_elf*() family of functions, and fill it with the
> appropriate error message.
>
Thanks for your suggestion. I changed it as below:
+        error_report("Couldn't load elf '%s': %s",
+                     info->kernel_filename, load_elf_strerror(ret));

$ qemu-system-aarch64 -M virt -kernel /work/linux/vmlinux
qemu-system-aarch64: Couldn't load elf '/work/linux/vmlinux': The image is from incompatible architecture

> Regards,
> 
> Phil.

-- 
Cheers,
Changbin Du

Re: [PATCH v2] hw/arm/boot: Report error msg if loading elf/dtb failed

Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago

On 2/9/24 21:55, Philippe Mathieu-Daudé wrote:
> Hi Changbin,
> 
> On 30/8/24 12:53, Changbin Du via wrote:
>> Print errors before exit. Do not exit silently.
>>
>> Signed-off-by: Changbin Du <changbin.du@huawei.com>
>>
>> ---
>> v2: remove msg for arm_load_dtb.
>> ---
>>   hw/arm/boot.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index d480a7da02cf..e15bf097a559 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -839,6 +839,7 @@ static ssize_t arm_load_elf(struct arm_boot_info 

Note that header error is also silently ignored and could be logged:

     load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
     if (err) {
-        error_free(err);
+        error_report_err(err);
         return ret;
     }

(untested)

>>       if (ret <= 0) {
>>           /* The header loaded but the image didn't */
>> +        error_report("could not load elf '%s'", info->kernel_filename);
> 
> "Could ..." (caps)
> 
> "hw/loader.h" is not well documented, but it seems load_elf*() returns:
> 
>    #define ELF_LOAD_FAILED       -1
>    #define ELF_LOAD_NOT_ELF      -2
>    #define ELF_LOAD_WRONG_ARCH   -3
>    #define ELF_LOAD_WRONG_ENDIAN -4
>    #define ELF_LOAD_TOO_BIG      -5
> 
> And we can display this error calling:
> 
>    const char *load_elf_strerror(ssize_t error);
> 
> So we can be more precise here using:
> 
>    error_report("Could not load elf '%s'", info->kernel_filename,
>                 load_elf_strerror(ret));
> 
>>           exit(1);
>>       }
> 
> Better (but out of scope of this patch) could be to pass an Error *errp
> argument to the load_elf*() family of functions, and fill it with the
> appropriate error message.
> 
> Regards,
> 
> Phil.

Re: [PATCH v2] hw/arm/boot: Report error msg if loading elf/dtb failed

Posted by Peter Maydell 2 months, 3 weeks ago

On Mon, 2 Sept 2024 at 21:00, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 2/9/24 21:55, Philippe Mathieu-Daudé wrote:
> > Hi Changbin,
> >
> > On 30/8/24 12:53, Changbin Du via wrote:
> >> Print errors before exit. Do not exit silently.
> >>
> >> Signed-off-by: Changbin Du <changbin.du@huawei.com>
> >>
> >> ---
> >> v2: remove msg for arm_load_dtb.
> >> ---
> >>   hw/arm/boot.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> >> index d480a7da02cf..e15bf097a559 100644
> >> --- a/hw/arm/boot.c
> >> +++ b/hw/arm/boot.c
> >> @@ -839,6 +839,7 @@ static ssize_t arm_load_elf(struct arm_boot_info
>
> Note that header error is also silently ignored and could be logged:
>
>      load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
>      if (err) {
> -        error_free(err);
> +        error_report_err(err);
>          return ret;
>      }
>
> (untested)

This one is deliberate -- if the file is not an ELF file
we want to silently fall back to trying it as a uimage or an
AArch64 Image file or as last resort a bare raw binary.

-- PMM

Re: [PATCH v2] hw/arm/boot: Report error msg if loading elf/dtb failed

Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago

On 3/9/24 11:20, Peter Maydell wrote:
> On Mon, 2 Sept 2024 at 21:00, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 2/9/24 21:55, Philippe Mathieu-Daudé wrote:
>>> Hi Changbin,
>>>
>>> On 30/8/24 12:53, Changbin Du via wrote:
>>>> Print errors before exit. Do not exit silently.
>>>>
>>>> Signed-off-by: Changbin Du <changbin.du@huawei.com>
>>>>
>>>> ---
>>>> v2: remove msg for arm_load_dtb.
>>>> ---
>>>>    hw/arm/boot.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>>> index d480a7da02cf..e15bf097a559 100644
>>>> --- a/hw/arm/boot.c
>>>> +++ b/hw/arm/boot.c
>>>> @@ -839,6 +839,7 @@ static ssize_t arm_load_elf(struct arm_boot_info
>>
>> Note that header error is also silently ignored and could be logged:
>>
>>       load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
>>       if (err) {
>> -        error_free(err);
>> +        error_report_err(err);
>>           return ret;
>>       }
>>
>> (untested)
> 
> This one is deliberate -- if the file is not an ELF file
> we want to silently fall back to trying it as a uimage or an
> AArch64 Image file or as last resort a bare raw binary.

Oh right. I'll add a comment about it to clarify, thanks.