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

[Stefan Hajnoczi]

On Thu, May 10, 2018 at 03:18:30PM +0800, Su Hang wrote:

Great, this is getting close.  A few more comments below...

>      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 */
> +    }
> +    if (kernel_size < 0) {
> +        /* 32-bit ARM binary file */

Why is this change necessary?  A 64-bit machine previously didn't
attempt a 32-bit kernel load.  I'm not sure why you changed this
behavior.

> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 5ed3fd8ae67a..ba82c9686ba3 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -28,6 +28,19 @@ ssize_t load_image_size(const char *filename, void *addr, 
> size_t size);
>  int load_image_targphys_as(const char *filename,
>                             hwaddr addr, uint64_t max_sz, AddressSpace *as);
>  
> +/**load_image_targphys_hex_as:

s/load_image_targphys_hex_as/load_targphys_hex_as/

> + * @filename: Path to the .hex file
> + * @entry: Store the entry point get from .hex file

s/get from/given by the/g

> + * @max_sz: The maximum size of the .hex file to load

This argument does not exist, please drop it.

> @@ -241,6 +254,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
>      rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
>  #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
>      rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
> +#define rom_add_hex_blob_as(_f, _b, _l, _a, _as)      \
> +    rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)

This macro is equivalent to rom_add_blob_fixed_as().  Why not use
rom_add_blob_fixed_as()?

signature.asc
Description: PGP signature