[Qemu-devel] [PULL 0/2] Implement Hex file loader and add test case

[Qemu-devel] [PULL 0/2] Implement Hex file loader and add test case

[Su Hang]

The following changes since commit 9cac60db4513f968b56e12720c1ea433be318cc3:

  Merge remote-tracking branch 'remotes/mwalle/tags/lm32-queue/20180521' into staging (2018-05-24 10:25:43 +0100)

are available in the git repository at:

  git://github.com/DarcySail/qemu.git foo.for-upstream

for you to fetch changes up to a9120811a5b9109d79f704e0a79fb851860e9f04:

  Add QTest testcase for the Intel Hexadecimal Object File Loader. (2018-05-24 18:54:02 +0800)

----------------------------------------------------------------
Su Hang (2):
      Implement .hex file loader
      Add QTest testcase for the Intel Hexadecimal Object File Loader.

 MAINTAINERS                          |   6 +
 configure                            |   4 +
 hw/arm/boot.c                        |   7 +-
 hw/core/loader.c                     | 246 +++++++++++++++++++++++++++++++++++
 include/hw/loader.h                  |  12 ++
 tests/Makefile.include               |   2 +
 tests/hex-loader-check-data/test.hex |  12 ++
 tests/hexloader-test.c               |  56 ++++++++
 8 files changed, 344 insertions(+), 1 deletion(-)
 create mode 100644 tests/hex-loader-check-data/test.hex
 create mode 100644 tests/hexloader-test.c

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

[Su Hang]

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.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 hw/arm/boot.c       |   7 +-
 hw/core/loader.c    | 246 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h |  12 +++
 3 files changed, 264 insertions(+), 1 deletion(-)

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,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 06bdbca53709..3c0202caa88f 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1286,3 +1286,249 @@ 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 {
+    uint8_t byte_count;
+    uint16_t address;
+    uint8_t record_type;
+    uint8_t data[DATA_FIELD_MAX_LEN];
+    uint8_t checksum;
+} HexLine;
+
+/* return 0 or -1 if error */
+static bool parse_record(HexLine *line, uint8_t *our_checksum, const uint8_t c,
+                         uint32_t *index, const bool in_process)
+{
+    /* +-------+---------------+-------+---------------------+--------+
+     * | byte  |               |record |                     |        |
+     * | count |    address    | type  |        data         |checksum|
+     * +-------+---------------+-------+---------------------+--------+
+     * ^       ^               ^       ^                     ^        ^
+     * |1 byte |    2 bytes    |1 byte |     0-255 bytes     | 1 byte |
+     */
+    uint8_t value = 0;
+    uint32_t idx = *index;
+    /* ignore space */
+    if (g_ascii_isspace(c)) {
+        return true;
+    }
+    if (!g_ascii_isxdigit(c) || !in_process) {
+        return false;
+    }
+    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 false;
+    }
+    *our_checksum += value;
+    ++(*index);
+    return true;
+}
+
+typedef struct {
+    const char *filename;
+    HexLine line;
+    uint8_t *bin_buf;
+    hwaddr *start_addr;
+    int total_size;
+    uint32_t next_address_to_write;
+    uint32_t current_address;
+    uint32_t current_rom_index;
+    uint32_t rom_start_address;
+    AddressSpace *as;
+} HexParser;
+
+/* return size or -1 if error */
+static int handle_record_type(HexParser *parser)
+{
+    HexLine *line = &(parser->line);
+    switch (line->record_type) {
+    case DATA_RECORD:
+        parser->current_address =
+            (parser->next_address_to_write & 0xffff0000) | line->address;
+        /* verify this is a contiguous block of memory */
+        if (parser->current_address != parser->next_address_to_write) {
+            if (parser->current_rom_index != 0) {
+                rom_add_blob_fixed_as(parser->filename, parser->bin_buf,
+                                      parser->current_rom_index,
+                                      parser->rom_start_address, parser->as);
+            }
+            parser->rom_start_address = parser->current_address;
+            parser->current_rom_index = 0;
+        }
+
+        /* copy from line buffer to output bin_buf */
+        memcpy(parser->bin_buf + parser->current_rom_index, line->data,
+               line->byte_count);
+        parser->current_rom_index += line->byte_count;
+        parser->total_size += line->byte_count;
+        /* save next address to write */
+        parser->next_address_to_write =
+            parser->current_address + line->byte_count;
+        break;
+
+    case EOF_RECORD:
+        if (parser->current_rom_index != 0) {
+            rom_add_blob_fixed_as(parser->filename, parser->bin_buf,
+                                  parser->current_rom_index,
+                                  parser->rom_start_address, parser->as);
+        }
+        return parser->total_size;
+    case EXT_SEG_ADDR_RECORD:
+    case EXT_LINEAR_ADDR_RECORD:
+        if (line->byte_count != 2 && line->address != 0) {
+            return -1;
+        }
+
+        if (parser->current_rom_index != 0) {
+            rom_add_blob_fixed_as(parser->filename, parser->bin_buf,
+                                  parser->current_rom_index,
+                                  parser->rom_start_address, parser->as);
+        }
+
+        /* save next address to write,
+         * in case of non-contiguous block of memory */
+        parser->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));
+        parser->rom_start_address = parser->next_address_to_write;
+        parser->current_rom_index = 0;
+        break;
+
+    case START_SEG_ADDR_RECORD:
+        /* TODO: START_SEG_ADDR_RECORD is x86-specific */
+        return -1;
+
+    case START_LINEAR_ADDR_RECORD:
+        if (line->byte_count != 4 && line->address != 0) {
+            return -1;
+        }
+
+        *(parser->start_addr) = (line->data[0] << 24) | (line->data[1] << 16) |
+                                (line->data[2] << 8)  | (line->data[3]);
+        break;
+
+    default:
+        return -1;
+    }
+
+    return parser->total_size;
+}
+
+/* return size or -1 if error */
+static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t *hex_blob,
+                          off_t hex_blob_size, uint8_t *bin_buf,
+                          AddressSpace *as)
+{
+    bool in_process = false; /* avoid re-enter and
+                              * check whether record begin with ':' */
+    uint8_t *end = hex_blob + hex_blob_size;
+    uint8_t our_checksum = 0;
+    uint32_t record_index = 0;
+    HexParser parser = {0};
+    parser.filename = filename;
+    parser.bin_buf = bin_buf;
+    parser.start_addr = addr;
+    parser.as = as;
+
+    for (; hex_blob < end; ++hex_blob) {
+        switch (*hex_blob) {
+        case '\r':
+        case '\n':
+            if (!in_process) {
+                break;
+            }
+
+            in_process = false;
+            if ((LEN_EXCEPT_DATA + parser.line.byte_count) * 2 !=
+                    record_index ||
+                our_checksum != 0) {
+                return -1;
+            }
+
+            if (handle_record_type(&parser) == -1) {
+                return -1;
+            }
+            break;
+
+        /* start of a new record. */
+        case ':':
+            memset(&parser.line, 0, sizeof(HexLine));
+            in_process = true;
+            record_index = 0;
+            break;
+
+        /* decoding lines */
+        default:
+            if (!parse_record(&parser.line, &our_checksum, *hex_blob,
+                              &record_index, in_process)) {
+                return -1;
+            }
+            break;
+        }
+    }
+    return parser.total_size;
+}
+
+/* return size or -1 if error */
+int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
+{
+    off_t hex_blob_size;
+    uint8_t *hex_blob;
+    uint8_t *bin_buf;
+    int fd;
+    int 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, entry, hex_blob, hex_blob_size, bin_buf, as);
+
+hex_parser_exit:
+    close(fd);
+    g_free(hex_blob);
+    g_free(bin_buf);
+    return total_size;
+}
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 5ed3fd8ae67a..bd9d7b380076 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -28,6 +28,18 @@ 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_targphys_hex_as:
+ * @filename: Path to the .hex file
+ * @entry: Store the entry point given by the .hex file
+ * @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 *entry, AddressSpace *as);
+
 /** load_image_targphys:
  * Same as load_image_targphys_as(), but doesn't allow the caller to specify
  * an AddressSpace.
--
2.7.4

[Qemu-devel] [PULL 2/2] Add QTest testcase for the Intel Hexadecimal

[Su Hang]

'test.hex' file is a bare metal ARM software stored in Hexadecimal
Object Format. When it's loaded by QEMU, it will print "Hello world!\n"
on console.

`pre_store` array in 'hexloader-test.c' file, stores the binary format
of 'test.hex' file, which is used to verify correctness.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Suggested-by: Steffen Gortz <qemu.ml@steffen-goertz.de>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 MAINTAINERS                          |  6 ++++
 configure                            |  4 +++
 tests/Makefile.include               |  2 ++
 tests/hex-loader-check-data/test.hex | 12 ++++++++
 tests/hexloader-test.c               | 56 ++++++++++++++++++++++++++++++++++++
 5 files changed, 80 insertions(+)
 create mode 100644 tests/hex-loader-check-data/test.hex
 create mode 100644 tests/hexloader-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e187b1f18f27..817ac7176b91 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1291,6 +1291,12 @@ F: hw/core/generic-loader.c
 F: include/hw/core/generic-loader.h
 F: docs/generic-loader.txt

+Intel Hexadecimal Object File Loader
+M: Su Hang <suhang16@mails.ucas.ac.cn>
+S: Maintained
+F: tests/hexloader-test.c
+F: tests/hex-loader-check-data/test.hex
+
 CHRP NVRAM
 M: Thomas Huth <thuth@redhat.com>
 S: Maintained
diff --git a/configure b/configure
index 59f91ab3f996..15cb143b17ec 100755
--- a/configure
+++ b/configure
@@ -7188,6 +7188,10 @@ for test_file in $(find $source_path/tests/acpi-test-data -type f)
 do
     FILES="$FILES tests/acpi-test-data$(echo $test_file | sed -e 's/.*acpi-test-data//')"
 done
+for test_file in $(find $source_path/tests/hex-loader-check-data -type f)
+do
+    FILES="$FILES tests/hex-loader-check-data$(echo $test_file | sed -e 's/.*hex-loader-check-data//')"
+done
 mkdir -p $DIRS
 for f in $FILES ; do
     if [ -e "$source_path/$f" ] && [ "$pwd_is_source_path" != "y" ]; then
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2c2..f4a3e71f34ee 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -380,6 +380,7 @@ check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 gcov-files-arm-y += hw/timer/arm_mptimer.c
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/sdhci-test$(EXESUF)
+check-qtest-arm-y += tests/hexloader-test$(EXESUF)

 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
@@ -755,6 +756,7 @@ tests/qmp-test$(EXESUF): tests/qmp-test.o
 tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
+tests/hexloader-test$(EXESUF): tests/hexloader-test.o
 tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
 tests/prom-env-test$(EXESUF): tests/prom-env-test.o $(libqos-obj-y)
diff --git a/tests/hex-loader-check-data/test.hex b/tests/hex-loader-check-data/test.hex
new file mode 100644
index 000000000000..7e99b452f5cc
--- /dev/null
+++ b/tests/hex-loader-check-data/test.hex
@@ -0,0 +1,12 @@
+:020000040001F9
+:1000000004D09FE5160000EBFEFFFFEA9810010008
+:1000100004B02DE500B08DE20CD04DE208000BE5F8
+:10002000060000EA08301BE50020D3E52C309FE5F0
+:10003000002083E508301BE5013083E208300BE542
+:1000400008301BE50030D3E5000053E3F4FFFF1A4E
+:100050000000A0E100D08BE204B09DE41EFF2FE180
+:1000600000101F1000482DE904B08DE208009FE544
+:10007000E6FFFFEB0000A0E10088BDE8840001007E
+:1000800000101F1048656C6C6F20776F726C6421D4
+:020090000A0064
+:00000001FF
diff --git a/tests/hexloader-test.c b/tests/hexloader-test.c
new file mode 100644
index 000000000000..a8b4cb78e06a
--- /dev/null
+++ b/tests/hexloader-test.c
@@ -0,0 +1,56 @@
+/*
+ * QTest testcase for the Intel Hexadecimal Object File Loader
+ *
+ * Authors:
+ *  Su Hang <suhang16@mails.ucas.ac.cn> 2018
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+
+#define BIN_SIZE 146
+
+static unsigned char pre_store[BIN_SIZE] = {
+    4,   208, 159, 229, 22,  0,   0,   235, 254, 255, 255, 234, 152, 16,  1,
+    0,   4,   176, 45,  229, 0,   176, 141, 226, 12,  208, 77,  226, 8,   0,
+    11,  229, 6,   0,   0,   234, 8,   48,  27,  229, 0,   32,  211, 229, 44,
+    48,  159, 229, 0,   32,  131, 229, 8,   48,  27,  229, 1,   48,  131, 226,
+    8,   48,  11,  229, 8,   48,  27,  229, 0,   48,  211, 229, 0,   0,   83,
+    227, 244, 255, 255, 26,  0,   0,   160, 225, 0,   208, 139, 226, 4,   176,
+    157, 228, 30,  255, 47,  225, 0,   16,  31,  16,  0,   72,  45,  233, 4,
+    176, 141, 226, 8,   0,   159, 229, 230, 255, 255, 235, 0,   0,   160, 225,
+    0,   136, 189, 232, 132, 0,   1,   0,   0,   16,  31,  16,  72,  101, 108,
+    108, 111, 32,  119, 111, 114, 108, 100, 33,  10,  0};
+
+/* success if no crash or abort */
+static void hex_loader_test(void)
+{
+    unsigned int i;
+    unsigned char memory_content[BIN_SIZE];
+    const unsigned int base_addr = 0x00010000;
+
+    QTestState *s = qtest_startf(
+        "-M versatilepb -m 128M -nographic -kernel ./tests/hex-loader-check-data/test.hex");
+
+    for (i = 0; i < BIN_SIZE; ++i) {
+        memory_content[i] = qtest_readb(s, base_addr + i);
+        g_assert_cmpuint(memory_content[i], ==, pre_store[i]);
+    }
+    qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/tmp/hex_loader", hex_loader_test);
+    ret = g_test_run();
+
+    return ret;
+}
--
2.7.4

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

[Peter Maydell]

On 24 May 2018 at 12:28, Su Hang <suhang16@mails.ucas.ac.cn> 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

Re: [Qemu-devel] [PULL 2/2] Add QTest testcase for the Intel Hexadecimal

[Eric Blake]

On 05/24/2018 06:29 AM, Su Hang wrote:
> 'test.hex' file is a bare metal ARM software stored in Hexadecimal
> Object Format. When it's loaded by QEMU, it will print "Hello world!\n"
> on console.
> 
> `pre_store` array in 'hexloader-test.c' file, stores the binary format
> of 'test.hex' file, which is used to verify correctness.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Suggested-by: Steffen Gortz <qemu.ml@steffen-goertz.de>
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> ---
>   MAINTAINERS                          |  6 ++++
>   configure                            |  4 +++
>   tests/Makefile.include               |  2 ++
>   tests/hex-loader-check-data/test.hex | 12 ++++++++
>   tests/hexloader-test.c               | 56 ++++++++++++++++++++++++++++++++++++

The previous patch also touched:

  hw/arm/boot.c       |   7 +-
  hw/core/loader.c    | 246 
++++++++++++++++++++++++++++++++++++++++++++++++++++
  include/hw/loader.h |  12 +++

> +++ b/MAINTAINERS
> @@ -1291,6 +1291,12 @@ F: hw/core/generic-loader.c
>   F: include/hw/core/generic-loader.h
>   F: docs/generic-loader.txt
> 
> +Intel Hexadecimal Object File Loader
> +M: Su Hang <suhang16@mails.ucas.ac.cn>
> +S: Maintained
> +F: tests/hexloader-test.c
> +F: tests/hex-loader-check-data/test.hex
> +

It looks odd having a maintainer that claims only test files; do you 
want to also list some of the other files touched by this patch so that 
you get notification if one of the implementation files has subsequent 
patches (rather than just the test files)?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PULL 2/2] Add QTest testcase for the Intel Hexadecimal

[Su Hang]

Sure, I will list other involved files. Thanks for you suggestion.

SU Hang

"Eric Blake" <eblake@redhat.com>wrote:
> On 05/24/2018 06:29 AM, Su Hang wrote:
> > 'test.hex' file is a bare metal ARM software stored in Hexadecimal
> > Object Format. When it's loaded by QEMU, it will print "Hello world!\n"
> > on console.
> > 
> > `pre_store` array in 'hexloader-test.c' file, stores the binary format
> > of 'test.hex' file, which is used to verify correctness.
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Suggested-by: Steffen Gortz <qemu.ml@steffen-goertz.de>
> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> > ---
> >   MAINTAINERS                          |  6 ++++
> >   configure                            |  4 +++
> >   tests/Makefile.include               |  2 ++
> >   tests/hex-loader-check-data/test.hex | 12 ++++++++
> >   tests/hexloader-test.c               | 56 ++++++++++++++++++++++++++++++++++++
> 
> The previous patch also touched:
> 
>   hw/arm/boot.c       |   7 +-
>   hw/core/loader.c    | 246 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/loader.h |  12 +++
> 
> > +++ b/MAINTAINERS
> > @@ -1291,6 +1291,12 @@ F: hw/core/generic-loader.c
> >   F: include/hw/core/generic-loader.h
> >   F: docs/generic-loader.txt
> > 
> > +Intel Hexadecimal Object File Loader
> > +M: Su Hang <suhang16@mails.ucas.ac.cn>
> > +S: Maintained
> > +F: tests/hexloader-test.c
> > +F: tests/hex-loader-check-data/test.hex
> > +
> 
> It looks odd having a maintainer that claims only test files; do you 
> want to also list some of the other files touched by this patch so that 
> you get notification if one of the implementation files has subsequent 
> patches (rather than just the test files)?
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

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" <peter.maydell@linaro.org>
> Sent Time: 2018-05-24 22:40:04 (Thursday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>
> Cc: "Stefan Hajnoczi" <stefanha@gmail.com>, jim@groklearning.com, "Joel Stanley" <joel@jms.id.au>, "QEMU Developers" <qemu-devel@nongnu.org>
> Subject: Re: [Qemu-devel] [PULL 1/2] Implement .hex file loader
> 
> On 24 May 2018 at 12:28, Su Hang <suhang16@mails.ucas.ac.cn> 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