| Message ID | 20240830105304.2547406-1-changbin.du@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] hw/arm/boot: Report error msg if loading elf/dtb failed | expand |
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.
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.
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
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.
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.
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); }
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(+)