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

[Peter Maydell]

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