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

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

[Su Hang]

These series of patchs implement Intel Hexadecimal File loader and
add QTest testcase to verify the correctness of Loader.

v1: 
--  Basic version.

v2: 
--  Replace `do{}while(cond);` block with `for(;;)` block.

v3: 
--  Add two new files information in MAINTAINERS.

v4: 
--  Correct the 'test.hex' path in hexloader-test.c.

v5: 
--  Split huge parse_hex_blob() function into four small function;
--  Add check for memory bounds;
--  Check validation for Record type;
--  Replace function ctoh() with glib function g_ascii_xdigit_value();
--  Remove check for '.hex' suffix;
--  Remove unnecessary type cast;
--  Remove duplicate zero-initialization;
--  Correct typos;

v6: 
--  Call Intel HEX loader after load_uimage_as();
--  Remove use of KERNEL_LOAD_ADDR;
--  Change (hwaddr) type argument to (hwaddr *) type;
--  Use new struct HexParser to contain all arguments needed by
        handle_record_type();
--  Enable discontiguous data records (again);
--  Remove unnecessary memory clean: bin_buf and HexLine line;
--  Correct typo;
--  Remove unnecessary check for hex file size;
--  Add entry point handle for START_SEG_ADDR_RECORD and
        START_LINEAR_ADDR_RECORD record type;
--  Use hwaddr * type to store entry point;

v7: 
--  Remove unnecessary code style changes;
--  Replace `bool` with `size_t` for function `parse_record` return type;
--  Replace `int` with `size_t` for struct `HexParser` member field
        `total_size` type;
--  Replace `int` with `size_t` for function `handle_record_type` return type;
--  Return an error when encountered unimplemented data type
        `START_SEG_ADDR_RECORD`;
--  Add check for `START_LINEAR_ADDR_RECORD` data type:
        byte_count == 4 and address == 0;
--  Rename struct `HexParser` member field `addr` to `start_addr`;
--  Replace `int` with `size_t` for function `parse_hex_blob` return type;
--  Stop incrementing `record_index` when encountered whitespace;
--  Replace
    `if ((record_index >> 1) - LEN_EXCEPT_DATA != parser.line.byte_count)`
    with
    `if ((LEN_EXCEPT_DATA + parser.line.byte_count) * 2 != record_index)`
--  Remove unused field `max_sz`;
--  Replace `int` with `size_t` for local variable `total_size`
        in function `parse_hex_blob`
--  Rename function `load_image_targphys_hex_as` argument `addr` to `entry`;

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

 MAINTAINERS                          |   6 +
 hw/arm/boot.c                        |  10 +-
 hw/core/loader.c                     | 246 +++++++++++++++++++++++++++++++++++
 include/hw/loader.h                  |  15 +++
 tests/Makefile.include               |   2 +
 tests/hex-loader-check-data/test.hex |  12 ++
 tests/hexloader-test.c               |  56 ++++++++
 7 files changed, 345 insertions(+), 2 deletions(-)
 create mode 100644 tests/hex-loader-check-data/test.hex
 create mode 100644 tests/hexloader-test.c

--
2.7.4

[Qemu-devel] [PATCH v7 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.

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

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 9ae6ab2689e6..4acd5259d5fe 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1066,12 +1066,18 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
                                      &is_linux, NULL, NULL, as);
     }
+    if (kernel_size < 0) {
+        entry = 0;
+        /* 32-bit ARM Intel HEX file */
+        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 */
+    }
+    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,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 06bdbca53709..c02a734743e2 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_hex_blob_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_hex_blob_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_hex_blob_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..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:
+ * @filename: Path to the .hex file
+ * @entry: Store the entry point get from .hex file
+ * @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 *entry, AddressSpace *as);
+
 /** load_image_targphys:
  * Same as load_image_targphys_as(), but doesn't allow the caller to specify
  * an AddressSpace.
@@ -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)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000
-- 
2.7.4

[Qemu-devel] [PATCH v7 2/2] Add QTest testcase for the Intel Hexadecimal Object File Loader.

[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.

Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
---
 MAINTAINERS                          |  6 ++++
 tests/Makefile.include               |  2 ++
 tests/hex-loader-check-data/test.hex | 12 ++++++++
 tests/hexloader-test.c               | 56 ++++++++++++++++++++++++++++++++++++
 4 files changed, 76 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 24b70169bc37..3d37d04c3345 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/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..70f99ac03c6b
--- /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] [PATCH v7 2/2] Add QTest testcase for the Intel Hexadecimal Object File Loader.

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Thu, May 10, 2018 at 03:18:31PM +0800, 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.
> 
> Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> ---
>  MAINTAINERS                          |  6 ++++
>  tests/Makefile.include               |  2 ++
>  tests/hex-loader-check-data/test.hex | 12 ++++++++
>  tests/hexloader-test.c               | 56 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+)
>  create mode 100644 tests/hex-loader-check-data/test.hex
>  create mode 100644 tests/hexloader-test.c

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

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()?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Julia Suvorova]

> 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.
> 

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.

Re: [Qemu-devel] [PATCH v7 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.
> > 
> 
> 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?

Sure, I'm willing to help, but in the next patch, not in this patch series.

Best,
Su Hang

Re: [Qemu-devel] [PATCH v7 2/2] Add QTest testcase for the Intel Hexadecimal Object File Loader.

[Steffen Görtz]

On 10.05.2018 09:18, Su Hang wrote:

Hi,
this will be my first comment on devel as part of my GSoC participation
this year.

> +
> +    QTestState *s = qtest_startf(
> +        "-M versatilepb -m 128M -nographic -kernel ../tests/hex-loader-check-data/test.hex");
> +

The test binary "text.hex" is not being copied to out-of-tree builds at
the moment and the test run will therefore fail.
The acpi tests also use a fixture (acpi-test-data) which is being copied
to the out-of-tree build in ./configure:L7181 .
This might also be an option for your test.

Re: [Qemu-devel] [PATCH v7 2/2] Add QTest testcase for the Intel Hexadecimal Object File Loader.

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

On Mon, May 14, 2018 at 08:57:48AM +0200, Steffen Görtz wrote:
> On 10.05.2018 09:18, Su Hang wrote:
> > +
> > +    QTestState *s = qtest_startf(
> > +        "-M versatilepb -m 128M -nographic -kernel ../tests/hex-loader-check-data/test.hex");
> > +
> 
> The test binary "text.hex" is not being copied to out-of-tree builds at
> the moment and the test run will therefore fail.
> The acpi tests also use a fixture (acpi-test-data) which is being copied
> to the out-of-tree build in ./configure:L7181 .
> This might also be an option for your test.

To explain how out-of-tree builds work for anyone who hasn't seen them
before:

  $ mkdir /var/tmp/build-dir
  $ cd /var/tmp/build-dir
  $ ~/qemu/configure ... && make

The .o files and other build output will be located in
/var/tmp/build-dir (away from the source code in ~/qemu/).

The build system must ensure that data files are available to the test
program in /var/tmp/build-dir/ if an out-of-tree build is being done.
In the acpi-test-data case that Steffen mentioned the ./configure script
sets up symlinks:

  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
  ...
  symlink "$source_path/$f" "$f"

Please add hex-loader-check-data in the same way.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v7 2/2] Add QTest testcase for the Intel Hexadecimal Object File Loader.

[Su Hang]

Great! Thanks for your suggestion!

Best,
SU Hang

"Steffen Görtz" <qemu.ml@steffen-goertz.de>wrote:
> On 10.05.2018 09:18, Su Hang wrote:
> 
> Hi,
> this will be my first comment on devel as part of my GSoC participation
> this year.
> 
> > +
> > +    QTestState *s = qtest_startf(
> > +        "-M versatilepb -m 128M -nographic -kernel ../tests/hex-loader-check-data/test.hex");
> > +
> 
> The test binary "text.hex" is not being copied to out-of-tree builds at
> the moment and the test run will therefore fail.
> The acpi tests also use a fixture (acpi-test-data) which is being copied
> to the out-of-tree build in ./configure:L7181 .
> This might also be an option for your test.