You are right. I'm shocked by my mistakes... > The hex format is used mainly by Cortex-M boards. This code doesn't > support them, because armv7m uses its own load function. Could you add > support for armv7m? > > Best regards, Julia Suvorova.
Julia indeed have mentioned me about this. But at that time, I was thinking about "get this patch merged first, then add support for armv7m". I am wrong. SU Hang > -----Original Messages----- > From: "Peter Maydell" <peter.mayd...@linaro.org> > Sent Time: 2018-05-24 22:40:04 (Thursday) > To: "Su Hang" <suhan...@mails.ucas.ac.cn> > Cc: "Stefan Hajnoczi" <stefa...@gmail.com>, j...@groklearning.com, "Joel > Stanley" <j...@jms.id.au>, "QEMU Developers" <qemu-devel@nongnu.org> > Subject: Re: [Qemu-devel] [PULL 1/2] Implement .hex file loader > > On 24 May 2018 at 12:28, Su Hang <suhan...@mails.ucas.ac.cn> wrote: > > This patch adds Intel Hexadecimal Object File format support to > > the loader. The file format specification is available here: > > http://www.piclist.com/techref/fileext/hex/intel.htm > > > > The file format is mainly intended for embedded systems > > and microcontrollers, such as Micro:bit Arduino, ARM, STM32, etc. > > Could we have some more rationale here, please? For instance, > we could mention who ships binaries in this format, what other > boot loaders handle this, and so on. The idea is to explain > why it's worth our having this code, rather than just having > users convert their hex files to a format we already handle > (ie why there is a significant body of users with hex format > images they want to load). > > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > > index 9496f331a8fa..17fe1a8c287b 100644 > > --- a/hw/arm/boot.c > > +++ b/hw/arm/boot.c > > @@ -1038,12 +1038,17 @@ void arm_load_kernel(ARMCPU *cpu, struct > > arm_boot_info *info) > > kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL, > > &is_linux, NULL, NULL, as); > > } > > + if (kernel_size < 0) { > > + /* 32-bit ARM Intel HEX file */ > > + entry = 0; > > + kernel_size = load_targphys_hex_as(info->kernel_filename, &entry, > > as); > > + } > > if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) { > > kernel_size = load_aarch64_image(info->kernel_filename, > > info->loader_start, &entry, as); > > is_linux = 1; > > } else if (kernel_size < 0) { > > - /* 32-bit ARM */ > > + /* 32-bit ARM binary file */ > > entry = info->loader_start + KERNEL_LOAD_ADDR; > > kernel_size = load_image_targphys_as(info->kernel_filename, entry, > > info->ram_size - > > KERNEL_LOAD_ADDR, > > I don't think it makes sense to add support for this format here. > Who is using hex files to provide A-class Linux kernels? > > (Note that hw/arm/boot.c does *not* handle -kernel for M-class cores.) > > There might be an argument for wiring up hex file support > in the "generic loader" hw/misc/generic-loader.c > (documentation in docs/generic-loader.txt). > > It looks like your implementation calls rom_add_blob_fixed_as() > as it goes along to add chunks of data to guest memory, but > it doesn't do anything to undo that if it detects an error > in the input halfway through and returns a failure code. > That matters if you want to put it in a chain of "try this > format? if that didn't work, try this other format; last > resort, load as binary" calls like you have here. > > It's probably worth splitting the "add load_targphys_hex_as()" > patch from the one that wires it up for a specific loader. > > thanks > -- PMM