Re: [Qemu-devel] [PULL 1/2] Implement .hex file loader

[Su Hang]

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" <address@hidden>
> Sent Time: 2018-05-24 22:40:04 (Thursday)
> To: "Su Hang" <address@hidden>
> Cc: "Stefan Hajnoczi" <address@hidden>, address@hidden, "Joel Stanley" 
> <address@hidden>, "QEMU Developers" <address@hidden>
> Subject: Re: [Qemu-devel] [PULL 1/2] Implement .hex file loader
> 
> On 24 May 2018 at 12:28, Su Hang <address@hidden> 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