diff mbox

[v5,1/2] Implement .hex file loader

Message ID 1524206732-7729-2-git-send-email-suhang16@mails.ucas.ac.cn (mailing list archive)
State New, archived
Headers show

Commit Message

Su Hang April 20, 2018, 6:45 a.m. UTC
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 Arduino, ARM, STM32, etc.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 hw/arm/boot.c       |  18 ++--
 hw/core/loader.c    | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h |  14 +++
 3 files changed, 280 insertions(+), 5 deletions(-)

Comments

Stefan Hajnoczi April 23, 2018, 1:02 p.m. UTC | #1
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.
diff mbox

Patch

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) {
+        /* 32-bit ARM .hex file */
+        entry = info->loader_start + KERNEL_LOAD_ADDR;
+        kernel_size =
+            load_targphys_hex_as(info->kernel_filename, entry,
+                                 info->ram_size - KERNEL_LOAD_ADDR, as);
+    }
+    if (kernel_size < 0) {
+        /* 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,
-                                             as);
+        kernel_size =
+            load_image_targphys_as(info->kernel_filename, entry,
+                                   info->ram_size - KERNEL_LOAD_ADDR, as);
         is_linux = 1;
     }
     if (kernel_size < 0) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 06bdbca53709..4e3cb141aca4 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1286,3 +1286,256 @@  void hmp_info_roms(Monitor *mon, const QDict *qdict)
         }
     }
 }
+
+typedef enum HexRecord HexRecord;
+enum HexRecord {
+    DATA_RECORD = 0,
+    EOF_RECORD,
+    EXT_SEG_ADDR_RECORD,
+    START_SEG_ADDR_RECORD,
+    EXT_LINEAR_ADDR_RECORD,
+    START_LINEAR_ADDR_RECORD,
+};
+
+#define DATA_FIELD_MAX_LEN 0xff
+#define LEN_EXCEPT_DATA 0x5
+/* 0x5 = sizeof(byte_count) + sizeof(address) + sizeof(record_type) +
+ *       sizeof(checksum) */
+typedef struct HexLine HexLine;
+struct HexLine {
+    uint8_t byte_count;
+    uint16_t address;
+    uint8_t record_type;
+    uint8_t data[DATA_FIELD_MAX_LEN];
+    uint8_t checksum;
+};
+
+/* 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)
+{
+    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;
+        }
+
+        /* 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);
+            memset(line, 0, sizeof(HexLine));
+        }
+
+        *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
+                ? ((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;
+
+    default:
+        return -1;
+    }
+    return *total_size;
+}
+
+/* return 0 or -1 if error */
+static size_t parse_record(HexLine *line, uint8_t c, const uint32_t idx,
+                           uint8_t *our_checksum, const uint8_t in_process)
+{
+    /*+-------+---------------+-------+---------------------+--------+
+     *| byte  |               |record |                     |        |
+     *| count |    address    | type  |        data         |checksum|
+     *+-------+---------------+-------+---------------------+--------+
+     *^       ^               ^       ^                     ^        ^
+     *|1 byte |    2 bytes    |1 byte |     0-255 bytes     | 1 byte |
+     */
+    uint8_t value = 0;
+
+    /* ignore space */
+    if (g_ascii_isspace(c)) {
+        return 0;
+    }
+    if (!g_ascii_isxdigit(c) || in_process == 0) {
+        return -1;
+    }
+    value = g_ascii_xdigit_value(c);
+    value = idx & 0x1 ? value & 0xf : value << 4;
+    if (idx < 2) {
+        line->byte_count |= value;
+    } else if (2 <= idx && idx < 6) {
+        line->address <<= 4;
+        line->address += g_ascii_xdigit_value(c);
+    } else if (6 <= idx && idx < 8) {
+        line->record_type |= value;
+    } else if (8 <= idx && idx < 8 + 2 * line->byte_count) {
+        line->data[(idx - 8) >> 1] |= value;
+    } else if (8 + 2 * line->byte_count <= idx &&
+               idx < 10 + 2 * line->byte_count) {
+        line->checksum |= value;
+    } else {
+        return -1;
+    }
+    *our_checksum += value;
+    return 0;
+}
+
+/* return size or -1 if error */
+static size_t __parse_hex_blob(const char *filename, const hwaddr addr,
+                               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 */
+    uint8_t in_process = 0;        /* avoid re-enter and
+                                    * check whether record begin with ':' */
+    uint8_t *end = (uint8_t *)hex_blob + hex_blob_size;
+    uint8_t our_checksum = 0;
+    uint32_t record_index = 0;
+    uint32_t next_address_to_write = 0;
+    uint32_t current_address = 0;
+    uint32_t current_rom_index = 0;
+    uint32_t rom_start_address = 0;
+    size_t total_size = 0;
+    HexLine line = {0};
+
+    for (; hex_blob < end; ++hex_blob) {
+        switch (*hex_blob) {
+        case '\r':
+        case '\n':
+            if (!in_process) {
+                break;
+            }
+
+            in_process = 0;
+            /* uint8_t line.byte_count <= DATA_FIELD_MAX_LEN */
+            if ((record_index >> 1) - LEN_EXCEPT_DATA != line.byte_count ||
+                our_checksum != 0) {
+                return -1;
+            }
+
+            if (handle_record_type(filename, &line, bin_buf, addr, &total_size,
+                                   &ext_linear_record, &next_address_to_write,
+                                   &current_address, &current_rom_index,
+                                   &rom_start_address, as) == -1) {
+                return -1;
+            }
+            break;
+
+        /* start of a new record. */
+        case ':':
+            memset(&line, 0, sizeof(HexLine));
+            in_process = 1;
+            record_index = 0;
+            break;
+
+        /* decoding lines */
+        default:
+            if (parse_record(&line, *hex_blob, record_index, &our_checksum,
+                             in_process) != 0) {
+                return -1;
+            }
+            ++record_index;
+            break;
+        }
+    }
+    return total_size;
+}
+
+/* return size or -1 if error */
+static size_t parse_hex_blob(const char *filename, const hwaddr addr,
+                             AddressSpace *as)
+{
+    int fd;
+    off_t hex_blob_size;
+    uint8_t *hex_blob;
+    uint8_t *bin_buf;
+    size_t total_size = 0;
+
+    fd = open(filename, O_RDONLY);
+    if (fd < 0) {
+        return -1;
+    }
+    hex_blob_size = lseek(fd, 0, SEEK_END);
+    if (hex_blob_size < 0) {
+        close(fd);
+        return -1;
+    }
+    hex_blob = g_malloc(hex_blob_size);
+    bin_buf = g_malloc(hex_blob_size);
+    lseek(fd, 0, SEEK_SET);
+    if (read(fd, hex_blob, hex_blob_size) != hex_blob_size) {
+        total_size = -1;
+        goto hex_parser_exit;
+    }
+
+    total_size =
+        __parse_hex_blob(filename, addr, hex_blob, hex_blob_size, bin_buf, as);
+
+hex_parser_exit:
+    close(fd);
+    g_free(hex_blob);
+    g_free(bin_buf);
+    return total_size;
+}
+
+/* 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) {
+        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
+ * @max_sz: The maximum size of the .hex file to load
+ * @as: The AddressSpace to load the .hex file to. The value of
+ *      address_space_memory is used if nothing is supplied here.
+ *
+ * Load a fixed .hex file into memory.
+ *
+ * Returns the size of the loaded .hex file on success, -1 otherwise.
+ */
+int load_targphys_hex_as(const char *filename,
+                         hwaddr addr, uint64_t max_sz, AddressSpace *as);
+
 /** load_image_targphys:
  * Same as load_image_targphys_as(), but doesn't allow the caller to specify
  * an AddressSpace.