On Fri, Apr 20, 2018 at 02:45:31PM +0800, Su Hang wrote:
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 26184bcd7c26..898a7af114ea 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -1070,12 +1070,20 @@ static void arm_load_kernel_notify(Notifier
> *notifier, void *data)
> 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) {
Please try the Intel HEX loader right after load_uimage_as(). In theory
it should be usable on aarch64 too, so the Intel HEX loader needs to be
called earlier.
> + /* 32-bit ARM .hex file */
> + entry = info->loader_start + KERNEL_LOAD_ADDR;
KERNEL_LOAD_ADDR is for Linux, can you explain why you used it for this
non-Linux image format?
> + kernel_size =
> + load_targphys_hex_as(info->kernel_filename, entry,
> + info->ram_size - KERNEL_LOAD_ADDR, as);
The Intel HEX file may provide the entrypoint address, so the argument
should be &entry.
> +/* return size or -1 if error */
> +static size_t handle_record_type(const char *filename, HexLine *line,
> + uint8_t *bin_buf, const hwaddr addr,
> + size_t *total_size, uint8_t
> *ext_linear_record,
> + uint32_t *next_address_to_write,
> + uint32_t *current_address,
> + uint32_t *current_rom_index,
> + uint32_t *rom_start_address, AddressSpace
> *as)
There are a lot of arguments. A struct makes the code easier to read
and prevents bugs if arguments are accidentally swapped (e.g.
next_address_to_write and current_address have the same type so the
compiler cannot warn if they are swapped):
typedef struct {
HexLine line;
uint8_t *bin_buf;
...
} HexParser;
static size_t handle_record_type(HexParser *parser);
> +{
> + switch (line->record_type) {
> + case DATA_RECORD:
> + *current_address =
> + (*next_address_to_write & 0xffff0000) | line->address;
> + /* verify this is a contiguous block of memory */
> + if (*current_address != *next_address_to_write ||
> + *ext_linear_record == 1) {
> + if (*ext_linear_record != 1) {
> + return -1;
> + }
> + *ext_linear_record = 0;
> + *rom_start_address = *current_address;
> + }
What is the purpose of ext_linear_record?
Given this input file:
:10000000C0070000D1060000D1000000B1060000CA
<--- address 0010 is skipped
:100020000000000000000000000000005107000078
<--- address 0030 is skipped
:10004000EF000000F9000000030100000D010000B6
:00000001FF
ext_linear_record is 0 when the final data record is handled, so
handle_record_type() returns -1. I don't understand why the parser
should reject this input file.
I think a rom blob should be added if data records are discontiguous.
> +
> + /* copy from line buffer to output bin_buf */
> + memcpy(bin_buf + *current_rom_index, line->data, line->byte_count);
> + *current_rom_index += line->byte_count;
> + *total_size += line->byte_count;
> + /* Save next address to write */
> + *next_address_to_write = *current_address + line->byte_count;
> + break;
> +
> + case EOF_RECORD:
> + if (*current_rom_index != 0) {
> + rom_add_blob(filename, bin_buf, *current_rom_index,
> + *current_rom_index, addr + *rom_start_address, NULL,
> + NULL, NULL, as, true);
> + }
> + return *total_size;
> + case EXT_SEG_ADDR_RECORD:
> + case EXT_LINEAR_ADDR_RECORD:
> + if (line->byte_count != 2) {
> + return -1;
> + }
> +
> + if (*current_rom_index != 0) {
> + rom_add_blob(filename, bin_buf, *current_rom_index,
> + *current_rom_index, addr + *rom_start_address, NULL,
> + NULL, NULL, as, true);
> + memset(bin_buf, 0, *current_rom_index);
This is unnecessary since bin_buf is overwritten from index 0
(*current_rom_index = 0 below) before it is read again.
> + memset(line, 0, sizeof(HexLine));
This is unnecessary since in_process = 0 and the next ':' input
character will clear line.
> + }
> +
> + *current_rom_index = 0;
> + *ext_linear_record = 1;
> +
> + /* save next address to write,
> + * in case of non-contiguous block of memory */
> + *next_address_to_write =
> + line->record_type == EXT_SEG_ADDR_RECORD
This is broken because of the memset(line, 0, sizeof(HexLine)) above.
> + ? ((line->data[0] << 12) | (line->data[1] << 4))
> + : ((line->data[0] << 24) | (line->data[1] << 16));
> + break;
> +
> + case START_SEG_ADDR_RECORD:
> + /* nothing to do */
> + break;
> +
> + case START_LINEAR_ADDR_RECORD:
> + /* nothing to do */
> + break;
These record types specify the entry point (the starting address).
Please store the entry point into arm_boot_info->entry so that
do_cpu_reset() starts the CPU at the right address.
> +/* return size or -1 if error */
> +static size_t __parse_hex_blob(const char *filename, const hwaddr addr,
QEMU does not use double underscore function names because they are
reserved by the C standard (see 2.4 in ./HACKING and 7.1.3 in the C
standard). Please rename this function.
> + uint8_t *hex_blob, off_t hex_blob_size,
> + uint8_t *bin_buf, AddressSpace *as)
> +{
> + uint8_t ext_linear_record = 1; /* record non-constitutes block */
s/constitutes/contiguous/ ?
> + uint8_t in_process = 0; /* avoid re-enter and
> + * check whether record begin with ':' */
As mentioned in previous review comments, please use bool for boolean
flags and not integer types.
> +/* return size or -1 if error */
> +int load_targphys_hex_as(const char *filename, hwaddr addr, uint64_t max_sz,
> + AddressSpace *as)
> +{
> + int size;
> +
> + size = get_image_size(filename);
> + if (size < 0 || size > max_sz) {
Why check the size of the file? I'm not sure why the size of the file
matters here.
> + return -1;
> + }
> +
> + return parse_hex_blob(filename, addr, as);
> +}
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 5ed3fd8ae67a..2b0cdfe56e70 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -28,6 +28,20 @@ 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:
> + * @filename: Path to the .hex file
> + * @addr: Address to load the .hex file to
Do you have a use in mind for this argument? It seems like most callers
would pass 0 and treat the Intel HEX file addresses as absolute
addresses.
If there is no real need for this then please drop it.
signature.asc
Description: PGP signature
