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

[Qemu-devel] [PATCH v2 RFC 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 to
suppress code style complain.

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

 hw/arm/boot.c          |   9 +-
 hw/core/loader.c       | 280 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h    |  17 +++
 tests/Makefile.include |   2 +
 tests/hexloader-test.c |  56 ++++++++++
 tests/test.hex         |  11 ++
 6 files changed, 374 insertions(+), 1 deletion(-)
 create mode 100644 tests/hexloader-test.c
 create mode 100644 tests/test.hex

--
2.7.4

[Qemu-devel] [PATCH v2 RFC 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 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       |   9 +-
 hw/core/loader.c    | 280 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/loader.h |  17 ++++
 3 files changed, 305 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 9319b12fcd2a..07ce54e5936d 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1060,8 +1060,15 @@ 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 && strstr(info->kernel_filename, ".hex")) {
+        /* 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);
+        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..41d714520be4 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1286,3 +1286,283 @@ 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,
+};
+
+typedef union HexLine HexLine;
+union HexLine {
+    uint8_t buf[0x25];
+    struct __attribute__((packed)) {
+        uint8_t byte_count;
+        uint16_t address;
+        uint8_t record_type;
+        uint8_t data[0x25 - 0x5];
+        uint8_t checksum;
+    };
+};
+
+static uint8_t ctoh(char c)
+{
+    return (c & 0x10) ? /*0-9*/ c & 0xf : /*A-F, a-f*/ (c & 0xf) + 9;
+}
+
+static uint8_t validate_checksum(HexLine *record)
+{
+    uint8_t result = 0, i = 0;
+
+    for (; i < (record->byte_count + 5); ++i) {
+        result += record->buf[i];
+    }
+
+    return result == 0;
+}
+
+/* return pointer of bin_blob or NULL if error */
+static uint8_t *parse_hex_blob(char *filename, size_t *p_size)
+{
+    int fd;
+    off_t hex_blob_size;
+    uint8_t *p_data = NULL;
+    uint8_t *hex_blob;
+    uint8_t *hex_blob_ori;         /* used to free temporary memory */
+    uint8_t *bin_buf;
+    uint8_t *end;
+    uint8_t idx = 0;
+    uint8_t in_process = 0;        /* avoid re-enter */
+    uint8_t low_nibble = 0;        /* process two hex char into 8-bits */
+    uint8_t ext_linear_record = 0; /* record non-constitutes block */
+    uint32_t next_address_to_write = 0;
+    uint32_t current_address = 0;
+    uint32_t last_address = 0;
+    HexLine line = {0};
+
+    fd = open(filename, O_RDONLY);
+    if (fd < 0) {
+        return NULL;
+    }
+    hex_blob_size = lseek(fd, 0, SEEK_END);
+    if (hex_blob_size < 0) {
+        close(fd);
+        return NULL;
+    }
+    hex_blob = g_malloc(hex_blob_size);
+    hex_blob_ori = hex_blob;
+    bin_buf = g_malloc(hex_blob_size * 2);
+    lseek(fd, 0, SEEK_SET);
+    if (read(fd, hex_blob, hex_blob_size) != hex_blob_size) {
+        close(fd);
+        goto hex_parser_exit;
+    }
+    close(fd);
+
+    memset(line.buf, 0, sizeof(HexLine));
+    end = (uint8_t *)hex_blob + hex_blob_size;
+
+    for (; hex_blob != end; ++hex_blob) {
+        switch ((uint8_t)(*hex_blob)) {
+        case '\r':
+        case '\n':
+            if (!in_process) {
+                break;
+            }
+
+            in_process = 0;
+            if (validate_checksum(&line) == 0) {
+                p_data = NULL;
+                goto hex_parser_exit;
+            }
+
+            line.address = bswap16(line.address);
+            switch (line.record_type) {
+            case DATA_RECORD:
+                current_address =
+                    (next_address_to_write & 0xffff0000) | line.address;
+                /* verify this is a continous block of memory */
+                if (current_address != next_address_to_write ||
+                    ext_linear_record) {
+                    if (!ext_linear_record) {
+                        /* Store next address to write */
+                        last_address = next_address_to_write;
+                        next_address_to_write = current_address;
+                    }
+                    ext_linear_record = 0;
+                    memset(bin_buf + last_address, 0x0,
+                           current_address - last_address);
+                }
+
+                /* copy from line buffer to output bin_buf */
+                memcpy((uint8_t *)bin_buf + current_address,
+                       (uint8_t *)line.data, line.byte_count);
+                /* Save next address to write */
+                last_address = current_address;
+                next_address_to_write = current_address + line.byte_count;
+                break;
+
+            case EOF_RECORD:
+                /* nothing to do */
+                break;
+            case EXT_SEG_ADDR_RECORD:
+                /* save next address to write,
+                 * in case of non-continous block of memory */
+                ext_linear_record = 1;
+                last_address = next_address_to_write;
+                next_address_to_write =
+                    ((line.data[0] << 12) | (line.data[1] << 4));
+                break;
+            case START_SEG_ADDR_RECORD:
+                /* TODO */
+                break;
+
+            case EXT_LINEAR_ADDR_RECORD:
+                /* save next address to write,
+                 * in case of non-continous block of memory */
+                ext_linear_record = 1;
+                last_address = next_address_to_write;
+                next_address_to_write =
+                    ((line.data[0] << 24) | (line.data[1] << 16));
+                break;
+            case START_LINEAR_ADDR_RECORD:
+                /* TODO */
+                break;
+
+            default:
+                p_data = NULL;
+                goto hex_parser_exit;
+            }
+            break;
+
+        /* start of a new record. */
+        case ':':
+            memset(line.buf, 0, sizeof(HexLine));
+            in_process = 1;
+            low_nibble = 0;
+            idx = 0;
+            break;
+
+        /* decoding lines */
+        default:
+            if (low_nibble) {
+                line.buf[idx] |= ctoh((uint8_t)(*hex_blob)) & 0xf;
+                ++idx;
+            } else {
+                line.buf[idx] = ctoh((uint8_t)(*hex_blob)) << 4;
+            }
+
+            low_nibble = !low_nibble;
+            break;
+        }
+
+    }
+
+    *p_size = (size_t)next_address_to_write;
+    p_data = g_malloc(next_address_to_write);
+
+    memcpy(p_data, bin_buf, next_address_to_write);
+hex_parser_exit:
+    g_free(hex_blob_ori);
+    g_free(bin_buf);
+    return p_data;
+}
+
+/* return size or -1 if error */
+int load_targphys_hex_as(const char *filename, hwaddr addr, uint64_t max_sz,
+                         AddressSpace *as)
+{
+    return rom_add_hex_file(filename, NULL, addr, -1, false, NULL, as);
+}
+
+/* return size -1 if error */
+int rom_add_hex_file(const char *file, const char *fw_dir, hwaddr addr,
+                     int32_t bootindex, bool option_rom, MemoryRegion *mr,
+                     AddressSpace *as)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+    Rom *rom;
+    char devpath[100];
+    size_t datasize = 0;
+
+    if (as && mr) {
+        fprintf(stderr, "Specifying an Address Space and Memory Region is "
+                        "not valid when loading a rom\n");
+        /* We haven't allocated anything so we don't need any cleanup */
+        return -1;
+    }
+
+    rom = g_malloc0(sizeof(*rom));
+    rom->name = g_strdup(file);
+    rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
+    rom->as = as;
+    if (rom->path == NULL) {
+        rom->path = g_strdup(file);
+    }
+
+    rom->data = parse_hex_blob(rom->path, &datasize);
+    if (rom->data == NULL) {
+        fprintf(stderr, "failed to parse hex file '%s': %s\n", rom->path,
+                strerror(errno));
+        goto err;
+    }
+    rom->datasize = datasize;
+
+    if (fw_dir) {
+        rom->fw_dir = g_strdup(fw_dir);
+        rom->fw_file = g_strdup(file);
+    }
+    rom->addr = addr;
+    rom->romsize = rom->datasize;
+
+    rom_insert(rom);
+
+    if (rom->fw_file && fw_cfg) {
+        const char *basename;
+        char fw_file_name[FW_CFG_MAX_FILE_PATH];
+        void *data;
+
+        basename = strrchr(rom->fw_file, '/');
+        if (basename) {
+            ++basename;
+        } else {
+            basename = rom->fw_file;
+        }
+        snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
+                 basename);
+        snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
+
+        if ((!option_rom || mc->option_rom_has_mr) && mc->rom_file_has_mr) {
+            data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, true);
+        } else {
+            data = rom->data;
+        }
+
+        fw_cfg_add_file(fw_cfg, fw_file_name, data, rom->romsize);
+    } else {
+        if (mr) {
+            rom->mr = mr;
+            snprintf(devpath, sizeof(devpath), "/rom@%s", file);
+        } else {
+            snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
+        }
+    }
+
+    add_boot_device_path(bootindex, NULL, devpath);
+    return rom->datasize;
+
+err:
+    g_free(rom->path);
+    g_free(rom->name);
+    if (fw_dir) {
+        g_free(rom->fw_dir);
+        g_free(rom->fw_file);
+    }
+    g_free(rom);
+
+    return -1;
+}
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 5ed3fd8ae67a..176b11221a27 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.
@@ -210,6 +224,9 @@ void pstrcpy_targphys(const char *name,
 extern bool option_rom_has_mr;
 extern bool rom_file_has_mr;

+int rom_add_hex_file(const char *file, const char *fw_dir,
+                 hwaddr addr, int32_t bootindex,
+                 bool option_rom, MemoryRegion *mr, AddressSpace *as);
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex,
                  bool option_rom, MemoryRegion *mr, AddressSpace *as);
--
2.7.4

[Qemu-devel] [PATCH v2 RFC 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>
---
 tests/Makefile.include |  2 ++
 tests/hexloader-test.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/test.hex         | 11 ++++++++++
 3 files changed, 69 insertions(+)
 create mode 100644 tests/hexloader-test.c
 create mode 100644 tests/test.hex

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/hexloader-test.c b/tests/hexloader-test.c
new file mode 100644
index 000000000000..b1a491dcd9de
--- /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/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;
+}
diff --git a/tests/test.hex b/tests/test.hex
new file mode 100644
index 000000000000..59b96e3e6fa7
--- /dev/null
+++ b/tests/test.hex
@@ -0,0 +1,11 @@
+:1000000004D09FE5160000EBFEFFFFEA9810010008
+:1000100004B02DE500B08DE20CD04DE208000BE5F8
+:10002000060000EA08301BE50020D3E52C309FE5F0
+:10003000002083E508301BE5013083E208300BE542
+:1000400008301BE50030D3E5000053E3F4FFFF1A4E
+:100050000000A0E100D08BE204B09DE41EFF2FE180
+:1000600000101F1000482DE904B08DE208009FE544
+:10007000E6FFFFEB0000A0E10088BDE8840001007E
+:1000800000101F1048656C6C6F20776F726C6421D4
+:020090000A0064
+:00000001FF
--
2.7.4

Re: [Qemu-devel] [PATCH v2 RFC 0/2] Implement Hex file loader and add test case

[Eric Blake]

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

On 04/04/2018 10:19 AM, Su Hang wrote:
> 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 to
> suppress code style complain.

Better than rewriting code to work around a false positive is to fix the
false positive in the first place.  And I see you have a patch proposed
for that, but it was still marked RFC; unfortunately, during freeze,
patches that are marked RFC tend to be deferred in favor of patches
marked 'for-2.12' (RFC means the patch may be incomplete, and there's no
rush to get an incomplete patch in before the release, after all).  I'll
reply on that patch, now that it's got my attention; but for this
particular series, I'm more comfortable leaving the review to the
maintainer, modulo one comment:

> 
> Su Hang (2):
>   Implement .hex file loader
>   Add QTest testcase for the Intel Hexadecimal Object File Loader.
> 
>  hw/arm/boot.c          |   9 +-
>  hw/core/loader.c       | 280 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/loader.h    |  17 +++
>  tests/Makefile.include |   2 +
>  tests/hexloader-test.c |  56 ++++++++++
>  tests/test.hex         |  11 ++
>  6 files changed, 374 insertions(+), 1 deletion(-)
>  create mode 100644 tests/hexloader-test.c
>  create mode 100644 tests/test.hex

This creates new files that are not covered by MAINTAINERS.  Please be
sure to patch MAINTAINERS as well in your v3.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 RFC 0/2] Implement Hex file loader and add test case

[Su Hang]

Thanks for your reply! Owning to cultural difference， I used to regard “RFC” as a way of show modest...
I will apply your suggestion in v3.

Best,
Su Hang

"Eric Blake" <eblake@redhat.com>wrote:
> On 04/04/2018 10:19 AM, Su Hang wrote:
> > 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 to
> > suppress code style complain.
> 
> Better than rewriting code to work around a false positive is to fix the
> false positive in the first place.  And I see you have a patch proposed
> for that, but it was still marked RFC; unfortunately, during freeze,
> patches that are marked RFC tend to be deferred in favor of patches
> marked 'for-2.12' (RFC means the patch may be incomplete, and there's no
> rush to get an incomplete patch in before the release, after all).  I'll
> reply on that patch, now that it's got my attention; but for this
> particular series, I'm more comfortable leaving the review to the
> maintainer, modulo one comment:
> 
> > 
> > Su Hang (2):
> >   Implement .hex file loader
> >   Add QTest testcase for the Intel Hexadecimal Object File Loader.
> > 
> >  hw/arm/boot.c          |   9 +-
> >  hw/core/loader.c       | 280 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/loader.h    |  17 +++
> >  tests/Makefile.include |   2 +
> >  tests/hexloader-test.c |  56 ++++++++++
> >  tests/test.hex         |  11 ++
> >  6 files changed, 374 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/hexloader-test.c
> >  create mode 100644 tests/test.hex
> 
> This creates new files that are not covered by MAINTAINERS.  Please be
> sure to patch MAINTAINERS as well in your v3.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>