[Qemu-devel] [PATCH v4 0/6] arm: add Cortex M0 CPU model and hex file loader

[Qemu-devel] [PATCH v4 0/6] arm: add Cortex M0 CPU model and hex file loader

[Stefan Hajnoczi]

v4:
 * Drop ARMv7MState to ARMMProfileState rename because it causes a lot of code
   churn and is incomplete.  Other parts of QEMU (like NVIC emulation) still
   refer to "v7m" although they apply to other architecture versions too.
   [Peter]
 * Use the generic loader device (-device loader,file=program.hex,cpu-num=1)
   instead of -kernel.  -kernel is a legacy command-line option that is kept as
   a convenience, but it's not as flexible or predictable as the generic loader
   device.  New image formats should be added to the generic loader device.
   [Peter]
 * Use hex/ASCII literals instead of decimal literals in hexloader-test.c.
   This makes the test case slightly easier to understand. [Philippe]
v3:
 * Rename ARMv7MState to ARMMProfileState so a single class can cater for
   ARMv6-M, ARMv7-M, and ARMv8-M [Peter]
 * Make bitbanding optional via a qdev property [Peter]
 * Add hex file loader patches to reduce dependencies in upcoming patch series
 * Implement rollback if hex file loader fails partway through [Peter]
 * Update hex file loader test case to use an ARMv7-M board since hex file
   loading is only done for ARM M Profile boards

This series contains the prerequistes for the "microbit" machine type that Joel
Stanley has written.  I have combined the Cortex M0 CPU model and hex loader
work into one series to reduce the dependencies in upcoming patch series from
Joel, Julia, and Steffen.

This patch series:
 * Makes bitbanding optional on ARMv7MState.
 * Adds a "cortex-m0" cpu type which can be used with ARMMProfileState.  This
   works thanks to Julia's Cortex M0 emulation patches, which are now all
   merged.
 * Adds Su Hang's Intel HEX file format loader so that micro:bit and other
   microcontroller firmware images can be run using -kernel.  The micro:bit
   development tools typically only emit .hex files and not ELFs.

Stefan Hajnoczi (4):
  hw/arm: make bitbanded IO optional on ARMv7-M
  target/arm: add "cortex-m0" CPU model
  loader: extract rom_free() function
  loader: add rom transaction API

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

 MAINTAINERS                          |   6 +
 configure                            |   4 +
 tests/Makefile.include               |   2 +
 include/hw/arm/armv7m.h              |   2 +
 include/hw/loader.h                  |  31 +++
 hw/arm/armv7m.c                      |  37 ++--
 hw/arm/mps2.c                        |   1 +
 hw/arm/msf2-soc.c                    |   1 +
 hw/arm/stellaris.c                   |   1 +
 hw/arm/stm32f205_soc.c               |   1 +
 hw/core/generic-loader.c             |   4 +
 hw/core/loader.c                     | 296 ++++++++++++++++++++++++++-
 target/arm/cpu.c                     |  11 +
 tests/hexloader-test.c               |  60 ++++++
 tests/hex-loader-check-data/test.hex |  12 ++
 15 files changed, 443 insertions(+), 26 deletions(-)
 create mode 100644 tests/hexloader-test.c
 create mode 100644 tests/hex-loader-check-data/test.hex

-- 
2.17.1

[Qemu-devel] [PATCH v4 1/6] hw/arm: make bitbanded IO optional on ARMv7-M

[Stefan Hajnoczi]

Some ARM CPUs have bitbanded IO, a memory region that allows convenient
bit access via 32-bit memory loads/stores.  This eliminates the need for
read-modify-update instruction sequences.

This patch makes this optional feature an ARMv7MState qdev property,
allowing boards to choose whether they want bitbanding or not.

Status of boards:
 * iotkit (Cortex M33), no bitband
 * mps2 (Cortex M3), bitband
 * msf2 (Cortex M3), bitband
 * stellaris (Cortex M3), bitband
 * stm32f205 (Cortex M3), bitband

As a side-effect of this patch, Peter Maydell noted that the Ethernet
controller on mps2 board is now accessible.  Previously they were hidden
by the bitband region (which does not exist on the real board).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/arm/armv7m.h |  2 ++
 hw/arm/armv7m.c         | 37 ++++++++++++++++++++-----------------
 hw/arm/mps2.c           |  1 +
 hw/arm/msf2-soc.c       |  1 +
 hw/arm/stellaris.c      |  1 +
 hw/arm/stm32f205_soc.c  |  1 +
 6 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
index 78308d1484..2ba24953b6 100644
--- a/include/hw/arm/armv7m.h
+++ b/include/hw/arm/armv7m.h
@@ -43,6 +43,7 @@ typedef struct {
  *   devices will be automatically layered on top of this view.)
  * + Property "idau": IDAU interface (forwarded to CPU object)
  * + Property "init-svtor": secure VTOR reset value (forwarded to CPU object)
+ * + Property "enable-bitband": expose bitbanded IO
  */
 typedef struct ARMv7MState {
     /*< private >*/
@@ -63,6 +64,7 @@ typedef struct ARMv7MState {
     MemoryRegion *board_memory;
     Object *idau;
     uint32_t init_svtor;
+    bool enable_bitband;
 } ARMv7MState;
 
 #endif
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 6b07666057..878613994d 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -211,25 +211,27 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(&s->container, 0xe000e000,
                                 sysbus_mmio_get_region(sbd, 0));
 
-    for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
-        Object *obj = OBJECT(&s->bitband[i]);
-        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
+    if (s->enable_bitband) {
+        for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
+            Object *obj = OBJECT(&s->bitband[i]);
+            SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
 
-        object_property_set_int(obj, bitband_input_addr[i], "base", &err);
-        if (err != NULL) {
-            error_propagate(errp, err);
-            return;
-        }
-        object_property_set_link(obj, OBJECT(s->board_memory),
-                                 "source-memory", &error_abort);
-        object_property_set_bool(obj, true, "realized", &err);
-        if (err != NULL) {
-            error_propagate(errp, err);
-            return;
-        }
+            object_property_set_int(obj, bitband_input_addr[i], "base", &err);
+            if (err != NULL) {
+                error_propagate(errp, err);
+                return;
+            }
+            object_property_set_link(obj, OBJECT(s->board_memory),
+                                     "source-memory", &error_abort);
+            object_property_set_bool(obj, true, "realized", &err);
+            if (err != NULL) {
+                error_propagate(errp, err);
+                return;
+            }
 
-        memory_region_add_subregion(&s->container, bitband_output_addr[i],
-                                    sysbus_mmio_get_region(sbd, 0));
+            memory_region_add_subregion(&s->container, bitband_output_addr[i],
+                                        sysbus_mmio_get_region(sbd, 0));
+        }
     }
 }
 
@@ -239,6 +241,7 @@ static Property armv7m_properties[] = {
                      MemoryRegion *),
     DEFINE_PROP_LINK("idau", ARMv7MState, idau, TYPE_IDAU_INTERFACE, Object *),
     DEFINE_PROP_UINT32("init-svtor", ARMv7MState, init_svtor, 0),
+    DEFINE_PROP_BOOL("enable-bitband", ARMv7MState, enable_bitband, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index c3946da317..0a0ae867d9 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -186,6 +186,7 @@ static void mps2_common_init(MachineState *machine)
         g_assert_not_reached();
     }
     qdev_prop_set_string(armv7m, "cpu-type", machine->cpu_type);
+    qdev_prop_set_bit(armv7m, "enable-bitband", true);
     object_property_set_link(OBJECT(&mms->armv7m), OBJECT(system_memory),
                              "memory", &error_abort);
     object_property_set_bool(OBJECT(&mms->armv7m), true, "realized",
diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index dbefade644..2702e90b45 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -117,6 +117,7 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
     armv7m = DEVICE(&s->armv7m);
     qdev_prop_set_uint32(armv7m, "num-irq", 81);
     qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
+    qdev_prop_set_bit(armv7m, "enable-bitband", true);
     object_property_set_link(OBJECT(&s->armv7m), OBJECT(get_system_memory()),
                                      "memory", &error_abort);
     object_property_set_bool(OBJECT(&s->armv7m), true, "realized", &err);
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index dc521b4a5a..6c69ce79b2 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1304,6 +1304,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     nvic = qdev_create(NULL, TYPE_ARMV7M);
     qdev_prop_set_uint32(nvic, "num-irq", NUM_IRQ_LINES);
     qdev_prop_set_string(nvic, "cpu-type", ms->cpu_type);
+    qdev_prop_set_bit(nvic, "enable-bitband", true);
     object_property_set_link(OBJECT(nvic), OBJECT(get_system_memory()),
                                      "memory", &error_abort);
     /* This will exit with an error if the user passed us a bad cpu_type */
diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
index c486d06a8b..980e5af13c 100644
--- a/hw/arm/stm32f205_soc.c
+++ b/hw/arm/stm32f205_soc.c
@@ -109,6 +109,7 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
     armv7m = DEVICE(&s->armv7m);
     qdev_prop_set_uint32(armv7m, "num-irq", 96);
     qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
+    qdev_prop_set_bit(armv7m, "enable-bitband", true);
     object_property_set_link(OBJECT(&s->armv7m), OBJECT(get_system_memory()),
                                      "memory", &error_abort);
     object_property_set_bool(OBJECT(&s->armv7m), true, "realized", &err);
-- 
2.17.1

[Qemu-devel] [PATCH v4 2/6] target/arm: add "cortex-m0" CPU model

[Stefan Hajnoczi]

Define a "cortex-m0" ARMv6-M CPU model.

Most of the register reset values set by other CPU models are not
relevant for the cut-down ARMv6-M architecture.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 3848ef46aa..7e477c0d23 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1255,6 +1255,15 @@ static void arm11mpcore_initfn(Object *obj)
     cpu->reset_auxcr = 1;
 }
 
+static void cortex_m0_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    set_feature(&cpu->env, ARM_FEATURE_V6);
+    set_feature(&cpu->env, ARM_FEATURE_M);
+
+    cpu->midr = 0x410cc200;
+}
+
 static void cortex_m3_initfn(Object *obj)
 {
     ARMCPU *cpu = ARM_CPU(obj);
@@ -1845,6 +1854,8 @@ static const ARMCPUInfo arm_cpus[] = {
     { .name = "arm1136",     .initfn = arm1136_initfn },
     { .name = "arm1176",     .initfn = arm1176_initfn },
     { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
+    { .name = "cortex-m0",   .initfn = cortex_m0_initfn,
+                             .class_init = arm_v7m_class_init },
     { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
                              .class_init = arm_v7m_class_init },
     { .name = "cortex-m4",   .initfn = cortex_m4_initfn,
-- 
2.17.1

[Qemu-devel] [PATCH v4 3/6] loader: extract rom_free() function

[Stefan Hajnoczi]

The next patch will need to free a rom.  There is already code to do
this in rom_add_file().

Note that rom_add_file() uses:

  rom = g_malloc0(sizeof(*rom));
  ...
  if (rom->fw_dir) {
      g_free(rom->fw_dir);
      g_free(rom->fw_file);
  }

The conditional is unnecessary since g_free(NULL) is a no-op.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/core/loader.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index bbb6e65bb5..0c72e7c05a 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -847,6 +847,17 @@ struct Rom {
 static FWCfgState *fw_cfg;
 static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
 
+/* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */
+static void rom_free(Rom *rom)
+{
+    g_free(rom->data);
+    g_free(rom->path);
+    g_free(rom->name);
+    g_free(rom->fw_dir);
+    g_free(rom->fw_file);
+    g_free(rom);
+}
+
 static inline bool rom_order_compare(Rom *rom, Rom *item)
 {
     return ((uintptr_t)(void *)rom->as > (uintptr_t)(void *)item->as) ||
@@ -995,15 +1006,7 @@ err:
     if (fd != -1)
         close(fd);
 
-    g_free(rom->data);
-    g_free(rom->path);
-    g_free(rom->name);
-    if (fw_dir) {
-        g_free(rom->fw_dir);
-        g_free(rom->fw_file);
-    }
-    g_free(rom);
-
+    rom_free(rom);
     return -1;
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH v4 4/6] loader: add rom transaction API

[Stefan Hajnoczi]

Image file loaders may add a series of roms.  If an error occurs partway
through loading there is no easy way to drop previously added roms.

This patch adds a transaction mechanism that works like this:

  rom_transaction_begin();
  ...call rom_add_*()...
  rom_transaction_end(ok);

If ok is false then roms added in this transaction are dropped.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/loader.h | 19 +++++++++++++++++++
 hw/core/loader.c    | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index e98b84b8f9..5235f119a3 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -225,6 +225,25 @@ int rom_check_and_register_reset(void);
 void rom_set_fw(FWCfgState *f);
 void rom_set_order_override(int order);
 void rom_reset_order_override(void);
+
+/**
+ * rom_transaction_begin:
+ *
+ * Call this before of a series of rom_add_*() calls.  Call
+ * rom_transaction_end() afterwards to commit or abort.  These functions are
+ * useful for undoing a series of rom_add_*() calls if image file loading fails
+ * partway through.
+ */
+void rom_transaction_begin(void);
+
+/**
+ * rom_transaction_end:
+ * @commit: true to commit added roms, false to drop added roms
+ *
+ * Call this after a series of rom_add_*() calls.  See rom_transaction_begin().
+ */
+void rom_transaction_end(bool commit);
+
 int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
 void *rom_ptr(hwaddr addr, size_t size);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 0c72e7c05a..612420b870 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -840,6 +840,8 @@ struct Rom {
     char *fw_dir;
     char *fw_file;
 
+    bool committed;
+
     hwaddr addr;
     QTAILQ_ENTRY(Rom) next;
 };
@@ -877,6 +879,8 @@ static void rom_insert(Rom *rom)
         rom->as = &address_space_memory;
     }
 
+    rom->committed = false;
+
     /* List is ordered by load address in the same address space */
     QTAILQ_FOREACH(item, &roms, next) {
         if (rom_order_compare(rom, item)) {
@@ -1168,6 +1172,34 @@ void rom_reset_order_override(void)
     fw_cfg_reset_order_override(fw_cfg);
 }
 
+void rom_transaction_begin(void)
+{
+    Rom *rom;
+
+    /* Ignore ROMs added without the transaction API */
+    QTAILQ_FOREACH(rom, &roms, next) {
+        rom->committed = true;
+    }
+}
+
+void rom_transaction_end(bool commit)
+{
+    Rom *rom;
+    Rom *tmp;
+
+    QTAILQ_FOREACH_SAFE(rom, &roms, next, tmp) {
+        if (rom->committed) {
+            continue;
+        }
+        if (commit) {
+            rom->committed = true;
+        } else {
+            QTAILQ_REMOVE(&roms, rom, next);
+            rom_free(rom);
+        }
+    }
+}
+
 static Rom *find_rom(hwaddr addr, size_t size)
 {
     Rom *rom;
-- 
2.17.1

[Qemu-devel] [PATCH v4 5/6] loader: Implement .hex file loader

[Stefan Hajnoczi]

From: Su Hang <suhang16@mails.ucas.ac.cn>

This patch adds Intel Hexadecimal Object File format support to the
generic loader device.  The file format specification is available here:
http://www.piclist.com/techref/fileext/hex/intel.htm

This file format is often used with microcontrollers such as the
micro:bit, Arduino, STM32, etc.  Users expect to be able to run .hex
files directly with without first converting them to ELF.  Most
micro:bit code is developed in web-based IDEs without direct user access
to binutils so it is important for QEMU to handle this file format
natively.

Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/loader.h      |  12 ++
 hw/core/generic-loader.c |   4 +
 hw/core/loader.c         | 243 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 259 insertions(+)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 5235f119a3..3c112975f4 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.
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index cb0e68486d..fde32cbda1 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -147,6 +147,10 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
                 size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL,
                                       as);
             }
+
+            if (size < 0) {
+                size = load_targphys_hex_as(s->file, &entry, as);
+            }
         }
 
         if (size < 0 || s->force_raw) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 612420b870..072bf8b434 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1321,3 +1321,246 @@ 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:
+        if (line->byte_count != 4 && line->address != 0) {
+            return -1;
+        }
+
+        /* x86 16-bit CS:IP segmented addressing */
+        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
+                                (line->data[2] << 8) | line->data[3];
+        break;
+
+    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,
+                          size_t hex_blob_size, 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 = {
+        .filename = filename,
+        .bin_buf = g_malloc(hex_blob_size),
+        .start_addr = addr,
+        .as = as,
+    };
+
+    rom_transaction_begin();
+
+    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) {
+                parser.total_size = -1;
+                goto out;
+            }
+
+            if (handle_record_type(&parser) == -1) {
+                parser.total_size = -1;
+                goto out;
+            }
+            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)) {
+                parser.total_size = -1;
+                goto out;
+            }
+            break;
+        }
+    }
+
+out:
+    g_free(parser.bin_buf);
+    rom_transaction_end(parser.total_size != -1);
+    return parser.total_size;
+}
+
+/* return size or -1 if error */
+int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
+{
+    gsize hex_blob_size;
+    gchar *hex_blob;
+    int total_size = 0;
+
+    if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) {
+        return -1;
+    }
+
+    total_size = parse_hex_blob(filename, entry, (uint8_t *)hex_blob,
+                                hex_blob_size, as);
+
+    g_free(hex_blob);
+    return total_size;
+}
-- 
2.17.1

[Qemu-devel] [PATCH v4 6/6] Add QTest testcase for the Intel Hexadecimal

[Stefan Hajnoczi]

From: Su Hang <suhang16@mails.ucas.ac.cn>

'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>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 MAINTAINERS                          |  6 +++
 configure                            |  4 ++
 tests/Makefile.include               |  2 +
 tests/hexloader-test.c               | 60 ++++++++++++++++++++++++++++
 tests/hex-loader-check-data/test.hex | 12 ++++++
 5 files changed, 84 insertions(+)
 create mode 100644 tests/hexloader-test.c
 create mode 100644 tests/hex-loader-check-data/test.hex

diff --git a/MAINTAINERS b/MAINTAINERS
index 666e936812..c48d9271cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1323,6 +1323,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 2a7796ea80..db97930314 100755
--- a/configure
+++ b/configure
@@ -7382,6 +7382,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 a49282704e..760a0f18b6 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -386,6 +386,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)
@@ -773,6 +774,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 0000000000..8818bd45f8
--- /dev/null
+++ b/tests/hexloader-test.c
@@ -0,0 +1,60 @@
+/*
+ * 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] = {
+  0x04, 0xd0, 0x9f, 0xe5,   0x16, 0x00, 0x00, 0xeb,   0xfe, 0xff, 0xff, 0xea,
+  0x98, 0x10, 0x01, 0x00,   0x04, 0xb0, 0x2d, 0xe5,   0x00, 0xb0, 0x8d, 0xe2,
+  0x0c, 0xd0, 0x4d, 0xe2,   0x08, 0x00, 0x0b, 0xe5,   0x06, 0x00, 0x00, 0xea,
+  0x08, 0x30, 0x1b, 0xe5,   0x00, 0x20, 0xd3, 0xe5,   0x2c, 0x30, 0x9f, 0xe5,
+  0x00, 0x20, 0x83, 0xe5,   0x08, 0x30, 0x1b, 0xe5,   0x01, 0x30, 0x83, 0xe2,
+  0x08, 0x30, 0x0b, 0xe5,   0x08, 0x30, 0x1b, 0xe5,   0x00, 0x30, 0xd3, 0xe5,
+  0x00, 0x00, 0x53, 0xe3,   0xf4, 0xff, 0xff, 0x1a,   0x00, 0x00, 0xa0, 0xe1,
+  0x00, 0xd0, 0x8b, 0xe2,   0x04, 0xb0, 0x9d, 0xe4,   0x1e, 0xff, 0x2f, 0xe1,
+  0x00, 0x10, 0x1f, 0x10,   0x00, 0x48, 0x2d, 0xe9,   0x04, 0xb0, 0x8d, 0xe2,
+  0x08, 0x00, 0x9f, 0xe5,   0xe6, 0xff, 0xff, 0xeb,   0x00, 0x00, 0xa0, 0xe1,
+  0x00, 0x88, 0xbd, 0xe8,   0x84, 0x00, 0x01, 0x00,   0x00, 0x10, 0x1f, 0x10,
+  'H',  'e',  'l',  'l',    'o',  ' ',  'w',  'o',    'r',  'l',  'd',  '!',
+  '\n', '\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 emcraft-sf2 -nographic -device loader,file=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;
+}
diff --git a/tests/hex-loader-check-data/test.hex b/tests/hex-loader-check-data/test.hex
new file mode 100644
index 0000000000..7e99b452f5
--- /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
-- 
2.17.1

Re: [Qemu-devel] [PATCH v4 4/6] loader: add rom transaction API

[Alistair Francis]

On Fri, Aug 3, 2018 at 7:47 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Image file loaders may add a series of roms.  If an error occurs partway
> through loading there is no easy way to drop previously added roms.
>
> This patch adds a transaction mechanism that works like this:
>
>   rom_transaction_begin();
>   ...call rom_add_*()...
>   rom_transaction_end(ok);
>
> If ok is false then roms added in this transaction are dropped.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/loader.h | 19 +++++++++++++++++++
>  hw/core/loader.c    | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index e98b84b8f9..5235f119a3 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -225,6 +225,25 @@ int rom_check_and_register_reset(void);
>  void rom_set_fw(FWCfgState *f);
>  void rom_set_order_override(int order);
>  void rom_reset_order_override(void);
> +
> +/**
> + * rom_transaction_begin:
> + *
> + * Call this before of a series of rom_add_*() calls.  Call
> + * rom_transaction_end() afterwards to commit or abort.  These functions are
> + * useful for undoing a series of rom_add_*() calls if image file loading fails
> + * partway through.
> + */
> +void rom_transaction_begin(void);
> +
> +/**
> + * rom_transaction_end:
> + * @commit: true to commit added roms, false to drop added roms
> + *
> + * Call this after a series of rom_add_*() calls.  See rom_transaction_begin().
> + */
> +void rom_transaction_end(bool commit);
> +
>  int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
>  void *rom_ptr(hwaddr addr, size_t size);
>  void hmp_info_roms(Monitor *mon, const QDict *qdict);
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 0c72e7c05a..612420b870 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -840,6 +840,8 @@ struct Rom {
>      char *fw_dir;
>      char *fw_file;
>
> +    bool committed;
> +
>      hwaddr addr;
>      QTAILQ_ENTRY(Rom) next;
>  };
> @@ -877,6 +879,8 @@ static void rom_insert(Rom *rom)
>          rom->as = &address_space_memory;
>      }
>
> +    rom->committed = false;
> +
>      /* List is ordered by load address in the same address space */
>      QTAILQ_FOREACH(item, &roms, next) {
>          if (rom_order_compare(rom, item)) {
> @@ -1168,6 +1172,34 @@ void rom_reset_order_override(void)
>      fw_cfg_reset_order_override(fw_cfg);
>  }
>
> +void rom_transaction_begin(void)
> +{
> +    Rom *rom;
> +
> +    /* Ignore ROMs added without the transaction API */
> +    QTAILQ_FOREACH(rom, &roms, next) {
> +        rom->committed = true;

My only thought is that maybe this should produce a warning or error
if a ROM isn't committed.

Alistair

> +    }
> +}
> +
> +void rom_transaction_end(bool commit)
> +{
> +    Rom *rom;
> +    Rom *tmp;
> +
> +    QTAILQ_FOREACH_SAFE(rom, &roms, next, tmp) {
> +        if (rom->committed) {
> +            continue;
> +        }
> +        if (commit) {
> +            rom->committed = true;
> +        } else {
> +            QTAILQ_REMOVE(&roms, rom, next);
> +            rom_free(rom);
> +        }
> +    }
> +}
> +
>  static Rom *find_rom(hwaddr addr, size_t size)
>  {
>      Rom *rom;
> --
> 2.17.1
>
>

Re: [Qemu-devel] [PATCH v4 3/6] loader: extract rom_free() function

[Alistair Francis]

On Fri, Aug 3, 2018 at 7:47 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The next patch will need to free a rom.  There is already code to do
> this in rom_add_file().
>
> Note that rom_add_file() uses:
>
>   rom = g_malloc0(sizeof(*rom));
>   ...
>   if (rom->fw_dir) {
>       g_free(rom->fw_dir);
>       g_free(rom->fw_file);
>   }
>
> The conditional is unnecessary since g_free(NULL) is a no-op.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/core/loader.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index bbb6e65bb5..0c72e7c05a 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -847,6 +847,17 @@ struct Rom {
>  static FWCfgState *fw_cfg;
>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>
> +/* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */
> +static void rom_free(Rom *rom)
> +{
> +    g_free(rom->data);
> +    g_free(rom->path);
> +    g_free(rom->name);
> +    g_free(rom->fw_dir);
> +    g_free(rom->fw_file);
> +    g_free(rom);
> +}
> +
>  static inline bool rom_order_compare(Rom *rom, Rom *item)
>  {
>      return ((uintptr_t)(void *)rom->as > (uintptr_t)(void *)item->as) ||
> @@ -995,15 +1006,7 @@ err:
>      if (fd != -1)
>          close(fd);
>
> -    g_free(rom->data);
> -    g_free(rom->path);
> -    g_free(rom->name);
> -    if (fw_dir) {
> -        g_free(rom->fw_dir);
> -        g_free(rom->fw_file);
> -    }
> -    g_free(rom);
> -
> +    rom_free(rom);
>      return -1;
>  }
>
> --
> 2.17.1
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 1/6] hw/arm: make bitbanded IO optional on ARMv7-M

[Philippe Mathieu-Daudé]

On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> Some ARM CPUs have bitbanded IO, a memory region that allows convenient
> bit access via 32-bit memory loads/stores.  This eliminates the need for
> read-modify-update instruction sequences.
> 
> This patch makes this optional feature an ARMv7MState qdev property,
> allowing boards to choose whether they want bitbanding or not.
> 
> Status of boards:
>  * iotkit (Cortex M33), no bitband
>  * mps2 (Cortex M3), bitband
>  * msf2 (Cortex M3), bitband
>  * stellaris (Cortex M3), bitband
>  * stm32f205 (Cortex M3), bitband
> 
> As a side-effect of this patch, Peter Maydell noted that the Ethernet
> controller on mps2 board is now accessible.  Previously they were hidden
> by the bitband region (which does not exist on the real board).
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/hw/arm/armv7m.h |  2 ++
>  hw/arm/armv7m.c         | 37 ++++++++++++++++++++-----------------
>  hw/arm/mps2.c           |  1 +
>  hw/arm/msf2-soc.c       |  1 +
>  hw/arm/stellaris.c      |  1 +
>  hw/arm/stm32f205_soc.c  |  1 +
>  6 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
> index 78308d1484..2ba24953b6 100644
> --- a/include/hw/arm/armv7m.h
> +++ b/include/hw/arm/armv7m.h
> @@ -43,6 +43,7 @@ typedef struct {
>   *   devices will be automatically layered on top of this view.)
>   * + Property "idau": IDAU interface (forwarded to CPU object)
>   * + Property "init-svtor": secure VTOR reset value (forwarded to CPU object)
> + * + Property "enable-bitband": expose bitbanded IO
>   */
>  typedef struct ARMv7MState {
>      /*< private >*/
> @@ -63,6 +64,7 @@ typedef struct ARMv7MState {
>      MemoryRegion *board_memory;
>      Object *idau;
>      uint32_t init_svtor;
> +    bool enable_bitband;
>  } ARMv7MState;
>  
>  #endif
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 6b07666057..878613994d 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -211,25 +211,27 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(&s->container, 0xe000e000,
>                                  sysbus_mmio_get_region(sbd, 0));
>  
> -    for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
> -        Object *obj = OBJECT(&s->bitband[i]);
> -        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
> +    if (s->enable_bitband) {
> +        for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
> +            Object *obj = OBJECT(&s->bitband[i]);
> +            SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
>  
> -        object_property_set_int(obj, bitband_input_addr[i], "base", &err);
> -        if (err != NULL) {
> -            error_propagate(errp, err);
> -            return;
> -        }
> -        object_property_set_link(obj, OBJECT(s->board_memory),
> -                                 "source-memory", &error_abort);
> -        object_property_set_bool(obj, true, "realized", &err);
> -        if (err != NULL) {
> -            error_propagate(errp, err);
> -            return;
> -        }
> +            object_property_set_int(obj, bitband_input_addr[i], "base", &err);
> +            if (err != NULL) {
> +                error_propagate(errp, err);
> +                return;
> +            }
> +            object_property_set_link(obj, OBJECT(s->board_memory),
> +                                     "source-memory", &error_abort);
> +            object_property_set_bool(obj, true, "realized", &err);
> +            if (err != NULL) {
> +                error_propagate(errp, err);
> +                return;
> +            }
>  
> -        memory_region_add_subregion(&s->container, bitband_output_addr[i],
> -                                    sysbus_mmio_get_region(sbd, 0));
> +            memory_region_add_subregion(&s->container, bitband_output_addr[i],
> +                                        sysbus_mmio_get_region(sbd, 0));
> +        }
>      }
>  }
>  
> @@ -239,6 +241,7 @@ static Property armv7m_properties[] = {
>                       MemoryRegion *),
>      DEFINE_PROP_LINK("idau", ARMv7MState, idau, TYPE_IDAU_INTERFACE, Object *),
>      DEFINE_PROP_UINT32("init-svtor", ARMv7MState, init_svtor, 0),
> +    DEFINE_PROP_BOOL("enable-bitband", ARMv7MState, enable_bitband, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
> index c3946da317..0a0ae867d9 100644
> --- a/hw/arm/mps2.c
> +++ b/hw/arm/mps2.c
> @@ -186,6 +186,7 @@ static void mps2_common_init(MachineState *machine)
>          g_assert_not_reached();
>      }
>      qdev_prop_set_string(armv7m, "cpu-type", machine->cpu_type);
> +    qdev_prop_set_bit(armv7m, "enable-bitband", true);
>      object_property_set_link(OBJECT(&mms->armv7m), OBJECT(system_memory),
>                               "memory", &error_abort);
>      object_property_set_bool(OBJECT(&mms->armv7m), true, "realized",
> diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
> index dbefade644..2702e90b45 100644
> --- a/hw/arm/msf2-soc.c
> +++ b/hw/arm/msf2-soc.c
> @@ -117,6 +117,7 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
>      armv7m = DEVICE(&s->armv7m);
>      qdev_prop_set_uint32(armv7m, "num-irq", 81);
>      qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
> +    qdev_prop_set_bit(armv7m, "enable-bitband", true);
>      object_property_set_link(OBJECT(&s->armv7m), OBJECT(get_system_memory()),
>                                       "memory", &error_abort);
>      object_property_set_bool(OBJECT(&s->armv7m), true, "realized", &err);
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index dc521b4a5a..6c69ce79b2 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1304,6 +1304,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>      nvic = qdev_create(NULL, TYPE_ARMV7M);
>      qdev_prop_set_uint32(nvic, "num-irq", NUM_IRQ_LINES);
>      qdev_prop_set_string(nvic, "cpu-type", ms->cpu_type);
> +    qdev_prop_set_bit(nvic, "enable-bitband", true);
>      object_property_set_link(OBJECT(nvic), OBJECT(get_system_memory()),
>                                       "memory", &error_abort);
>      /* This will exit with an error if the user passed us a bad cpu_type */
> diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
> index c486d06a8b..980e5af13c 100644
> --- a/hw/arm/stm32f205_soc.c
> +++ b/hw/arm/stm32f205_soc.c
> @@ -109,6 +109,7 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
>      armv7m = DEVICE(&s->armv7m);
>      qdev_prop_set_uint32(armv7m, "num-irq", 96);
>      qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
> +    qdev_prop_set_bit(armv7m, "enable-bitband", true);
>      object_property_set_link(OBJECT(&s->armv7m), OBJECT(get_system_memory()),
>                                       "memory", &error_abort);
>      object_property_set_bool(OBJECT(&s->armv7m), true, "realized", &err);
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 2/6] target/arm: add "cortex-m0" CPU model

[Philippe Mathieu-Daudé]

On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> Define a "cortex-m0" ARMv6-M CPU model.
> 
> Most of the register reset values set by other CPU models are not
> relevant for the cut-down ARMv6-M architecture.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 3848ef46aa..7e477c0d23 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1255,6 +1255,15 @@ static void arm11mpcore_initfn(Object *obj)
>      cpu->reset_auxcr = 1;
>  }
>  
> +static void cortex_m0_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    set_feature(&cpu->env, ARM_FEATURE_V6);
> +    set_feature(&cpu->env, ARM_FEATURE_M);
> +
> +    cpu->midr = 0x410cc200;
> +}
> +
>  static void cortex_m3_initfn(Object *obj)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> @@ -1845,6 +1854,8 @@ static const ARMCPUInfo arm_cpus[] = {
>      { .name = "arm1136",     .initfn = arm1136_initfn },
>      { .name = "arm1176",     .initfn = arm1176_initfn },
>      { .name = "arm11mpcore", .initfn = arm11mpcore_initfn },
> +    { .name = "cortex-m0",   .initfn = cortex_m0_initfn,
> +                             .class_init = arm_v7m_class_init },
>      { .name = "cortex-m3",   .initfn = cortex_m3_initfn,
>                               .class_init = arm_v7m_class_init },
>      { .name = "cortex-m4",   .initfn = cortex_m4_initfn,
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 3/6] loader: extract rom_free() function

[Philippe Mathieu-Daudé]

On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> The next patch will need to free a rom.  There is already code to do
> this in rom_add_file().
> 
> Note that rom_add_file() uses:
> 
>   rom = g_malloc0(sizeof(*rom));
>   ...
>   if (rom->fw_dir) {
>       g_free(rom->fw_dir);
>       g_free(rom->fw_file);
>   }
> 
> The conditional is unnecessary since g_free(NULL) is a no-op.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/core/loader.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index bbb6e65bb5..0c72e7c05a 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -847,6 +847,17 @@ struct Rom {
>  static FWCfgState *fw_cfg;
>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>  
> +/* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */
> +static void rom_free(Rom *rom)
> +{
> +    g_free(rom->data);
> +    g_free(rom->path);
> +    g_free(rom->name);
> +    g_free(rom->fw_dir);
> +    g_free(rom->fw_file);
> +    g_free(rom);
> +}
> +
>  static inline bool rom_order_compare(Rom *rom, Rom *item)
>  {
>      return ((uintptr_t)(void *)rom->as > (uintptr_t)(void *)item->as) ||
> @@ -995,15 +1006,7 @@ err:
>      if (fd != -1)
>          close(fd);
>  
> -    g_free(rom->data);
> -    g_free(rom->path);
> -    g_free(rom->name);
> -    if (fw_dir) {
> -        g_free(rom->fw_dir);
> -        g_free(rom->fw_file);
> -    }
> -    g_free(rom);
> -
> +    rom_free(rom);
>      return -1;
>  }
>  
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 6/6] Add QTest testcase for the Intel Hexadecimal

[Philippe Mathieu-Daudé]

Hi Su,

On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> From: Su Hang <suhang16@mails.ucas.ac.cn>
> 
> '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.

... on the console of the Emcraft SmartFusion2 machine.

> 
> `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>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  MAINTAINERS                          |  6 +++
>  configure                            |  4 ++
>  tests/Makefile.include               |  2 +
>  tests/hexloader-test.c               | 60 ++++++++++++++++++++++++++++
>  tests/hex-loader-check-data/test.hex | 12 ++++++
>  5 files changed, 84 insertions(+)
>  create mode 100644 tests/hexloader-test.c
>  create mode 100644 tests/hex-loader-check-data/test.hex
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 666e936812..c48d9271cf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1323,6 +1323,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 2a7796ea80..db97930314 100755
> --- a/configure
> +++ b/configure
> @@ -7382,6 +7382,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 a49282704e..760a0f18b6 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -386,6 +386,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)
> @@ -773,6 +774,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 0000000000..8818bd45f8
> --- /dev/null
> +++ b/tests/hexloader-test.c
> @@ -0,0 +1,60 @@
> +/*
> + * 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

This size is not needed, just use sizeof().

> +
> +static unsigned char pre_store[BIN_SIZE] = {

I'd call this blob 'emcraft_sf2_uart_hello'.

It can also be const:

static const unsigned char emcraft_sf2_uart_hello[] = {
    ...

Can you add a comment providing the .S asm source and how to
build/generate this blob? (I think this is required to respect the GPLv2
license in your header).

> +  0x04, 0xd0, 0x9f, 0xe5,   0x16, 0x00, 0x00, 0xeb,   0xfe, 0xff, 0xff, 0xea,
> +  0x98, 0x10, 0x01, 0x00,   0x04, 0xb0, 0x2d, 0xe5,   0x00, 0xb0, 0x8d, 0xe2,
> +  0x0c, 0xd0, 0x4d, 0xe2,   0x08, 0x00, 0x0b, 0xe5,   0x06, 0x00, 0x00, 0xea,
> +  0x08, 0x30, 0x1b, 0xe5,   0x00, 0x20, 0xd3, 0xe5,   0x2c, 0x30, 0x9f, 0xe5,
> +  0x00, 0x20, 0x83, 0xe5,   0x08, 0x30, 0x1b, 0xe5,   0x01, 0x30, 0x83, 0xe2,
> +  0x08, 0x30, 0x0b, 0xe5,   0x08, 0x30, 0x1b, 0xe5,   0x00, 0x30, 0xd3, 0xe5,
> +  0x00, 0x00, 0x53, 0xe3,   0xf4, 0xff, 0xff, 0x1a,   0x00, 0x00, 0xa0, 0xe1,
> +  0x00, 0xd0, 0x8b, 0xe2,   0x04, 0xb0, 0x9d, 0xe4,   0x1e, 0xff, 0x2f, 0xe1,
> +  0x00, 0x10, 0x1f, 0x10,   0x00, 0x48, 0x2d, 0xe9,   0x04, 0xb0, 0x8d, 0xe2,
> +  0x08, 0x00, 0x9f, 0xe5,   0xe6, 0xff, 0xff, 0xeb,   0x00, 0x00, 0xa0, 0xe1,
> +  0x00, 0x88, 0xbd, 0xe8,   0x84, 0x00, 0x01, 0x00,   0x00, 0x10, 0x1f, 0x10,
> +  'H',  'e',  'l',  'l',    'o',  ' ',  'w',  'o',    'r',  'l',  'd',  '!',
> +  '\n', '\0'
> +};
> +
> +/* success if no crash or abort */
> +static void hex_loader_test(void)
> +{
> +    unsigned int i;
> +    unsigned char memory_content[BIN_SIZE];

Not needed.

> +    const unsigned int base_addr = 0x00010000;
> +
> +    QTestState *s = qtest_startf(
> +        "-M emcraft-sf2 -nographic -device loader,file=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]);
> +    }

Simply:

    for (i = 0; i < ARRAY_SIZE(emcraft_sf2_uart_hello); ++i) {
        g_assert_cmphex(qtest_readb(s, base_addr + i),
                        ==, emcraft_sf2_uart_hello[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);

       qtest_add_func("/loader/hex/emcraft-sf2/uart_hello", ...

> +    ret = g_test_run();
> +
> +    return ret;
> +}
> diff --git a/tests/hex-loader-check-data/test.hex b/tests/hex-loader-check-data/test.hex
> new file mode 100644
> index 0000000000..7e99b452f5
> --- /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
> 

With comment addressed:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 5/6] loader: Implement .hex file loader

[Philippe Mathieu-Daudé]

Hi Su,

On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> From: Su Hang <suhang16@mails.ucas.ac.cn>
> 
> This patch adds Intel Hexadecimal Object File format support to the
> generic loader device.  The file format specification is available here:
> http://www.piclist.com/techref/fileext/hex/intel.htm
> 
> This file format is often used with microcontrollers such as the
> micro:bit, Arduino, STM32, etc.  Users expect to be able to run .hex
> files directly with without first converting them to ELF.  Most
> micro:bit code is developed in web-based IDEs without direct user access
> to binutils so it is important for QEMU to handle this file format
> natively.
> 
> Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/loader.h      |  12 ++
>  hw/core/generic-loader.c |   4 +
>  hw/core/loader.c         | 243 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 259 insertions(+)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 5235f119a3..3c112975f4 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.
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index cb0e68486d..fde32cbda1 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -147,6 +147,10 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>                  size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL,
>                                        as);
>              }
> +
> +            if (size < 0) {
> +                size = load_targphys_hex_as(s->file, &entry, as);
> +            }
>          }
>  
>          if (size < 0 || s->force_raw) {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 612420b870..072bf8b434 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1321,3 +1321,246 @@ 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;

This construction is slightly easier to read as:

       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;

Can you add a #define for this 0xffff0000? Maybe NEXT_ADDR_MASK 0xffff
and use ~NEXT_ADDR_MASK.

> +        /* 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));

Hard to read IMHO, what about this?

           parser->next_address_to_write = (line->data[0] << 12)
                                         | (line->data[1] << 4);
           if (line->record_type != EXT_SEG_ADDR_RECORD) {
               parser->next_address_to_write <<= 12;
           }

> +        parser->rom_start_address = parser->next_address_to_write;
> +        parser->current_rom_index = 0;
> +        break;
> +
> +    case START_SEG_ADDR_RECORD:
> +        if (line->byte_count != 4 && line->address != 0) {
> +            return -1;
> +        }
> +
> +        /* x86 16-bit CS:IP segmented addressing */
> +        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
> +                                (line->data[2] << 8) | line->data[3];

Can you add a qtest for this case?
For the HEX loader I understand the specs as this is the same parsing as
the START_LINEAR_ADDR_RECORD case; so I disagree with data[0] and
data[1] shifts.
This is different for the consumer (i386 expects 2 16-bit registers but
HexParser->start_addr is a hwaddr, used as a single (at least) 32-bit
register.

> +        break;
> +
> +    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]);

You can use this helper:

           *(parser->start_addr) = ldl_be_p(line->data);

> +        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,
> +                          size_t hex_blob_size, 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 = {
> +        .filename = filename,
> +        .bin_buf = g_malloc(hex_blob_size),
> +        .start_addr = addr,
> +        .as = as,
> +    };
> +
> +    rom_transaction_begin();
> +
> +    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) {
> +                parser.total_size = -1;
> +                goto out;
> +            }
> +
> +            if (handle_record_type(&parser) == -1) {
> +                parser.total_size = -1;
> +                goto out;
> +            }
> +            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)) {
> +                parser.total_size = -1;
> +                goto out;
> +            }
> +            break;
> +        }
> +    }
> +
> +out:
> +    g_free(parser.bin_buf);
> +    rom_transaction_end(parser.total_size != -1);
> +    return parser.total_size;
> +}
> +
> +/* return size or -1 if error */
> +int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
> +{
> +    gsize hex_blob_size;
> +    gchar *hex_blob;
> +    int total_size = 0;
> +
> +    if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) {
> +        return -1;
> +    }
> +
> +    total_size = parse_hex_blob(filename, entry, (uint8_t *)hex_blob,
> +                                hex_blob_size, as);
> +
> +    g_free(hex_blob);
> +    return total_size;
> +}
> 

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 5/6] loader: Implement .hex file loader

[sail darcy]

Thanks for your suggestion, I will review your comments carefully and take
modification in the next patch.

Best,
SU Hang

Philippe Mathieu-Daudé <f4bug@amsat.org> 于2018年8月10日周五 下午3:19写道：

> Hi Su,
>
> On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> > From: Su Hang <suhang16@mails.ucas.ac.cn>
> >
> > This patch adds Intel Hexadecimal Object File format support to the
> > generic loader device.  The file format specification is available here:
> > http://www.piclist.com/techref/fileext/hex/intel.htm
> >
> > This file format is often used with microcontrollers such as the
> > micro:bit, Arduino, STM32, etc.  Users expect to be able to run .hex
> > files directly with without first converting them to ELF.  Most
> > micro:bit code is developed in web-based IDEs without direct user access
> > to binutils so it is important for QEMU to handle this file format
> > natively.
> >
> > Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  include/hw/loader.h      |  12 ++
> >  hw/core/generic-loader.c |   4 +
> >  hw/core/loader.c         | 243 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 259 insertions(+)
> >
> > diff --git a/include/hw/loader.h b/include/hw/loader.h
> > index 5235f119a3..3c112975f4 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.
> > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> > index cb0e68486d..fde32cbda1 100644
> > --- a/hw/core/generic-loader.c
> > +++ b/hw/core/generic-loader.c
> > @@ -147,6 +147,10 @@ static void generic_loader_realize(DeviceState
> *dev, Error **errp)
> >                  size = load_uimage_as(s->file, &entry, NULL, NULL,
> NULL, NULL,
> >                                        as);
> >              }
> > +
> > +            if (size < 0) {
> > +                size = load_targphys_hex_as(s->file, &entry, as);
> > +            }
> >          }
> >
> >          if (size < 0 || s->force_raw) {
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 612420b870..072bf8b434 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -1321,3 +1321,246 @@ 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;
>
> This construction is slightly easier to read as:
>
>        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;
>
> Can you add a #define for this 0xffff0000? Maybe NEXT_ADDR_MASK 0xffff
> and use ~NEXT_ADDR_MASK.
>
> > +        /* 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));
>
> Hard to read IMHO, what about this?
>
>            parser->next_address_to_write = (line->data[0] << 12)
>                                          | (line->data[1] << 4);
>            if (line->record_type != EXT_SEG_ADDR_RECORD) {
>                parser->next_address_to_write <<= 12;
>            }
>
> > +        parser->rom_start_address = parser->next_address_to_write;
> > +        parser->current_rom_index = 0;
> > +        break;
> > +
> > +    case START_SEG_ADDR_RECORD:
> > +        if (line->byte_count != 4 && line->address != 0) {
> > +            return -1;
> > +        }
> > +
> > +        /* x86 16-bit CS:IP segmented addressing */
> > +        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1])
> << 4) |
> > +                                (line->data[2] << 8) | line->data[3];
>
> Can you add a qtest for this case?
> For the HEX loader I understand the specs as this is the same parsing as
> the START_LINEAR_ADDR_RECORD case; so I disagree with data[0] and
> data[1] shifts.
> This is different for the consumer (i386 expects 2 16-bit registers but
> HexParser->start_addr is a hwaddr, used as a single (at least) 32-bit
> register.
>
> > +        break;
> > +
> > +    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]);
>
> You can use this helper:
>
>            *(parser->start_addr) = ldl_be_p(line->data);
>
> > +        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,
> > +                          size_t hex_blob_size, 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 = {
> > +        .filename = filename,
> > +        .bin_buf = g_malloc(hex_blob_size),
> > +        .start_addr = addr,
> > +        .as = as,
> > +    };
> > +
> > +    rom_transaction_begin();
> > +
> > +    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) {
> > +                parser.total_size = -1;
> > +                goto out;
> > +            }
> > +
> > +            if (handle_record_type(&parser) == -1) {
> > +                parser.total_size = -1;
> > +                goto out;
> > +            }
> > +            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)) {
> > +                parser.total_size = -1;
> > +                goto out;
> > +            }
> > +            break;
> > +        }
> > +    }
> > +
> > +out:
> > +    g_free(parser.bin_buf);
> > +    rom_transaction_end(parser.total_size != -1);
> > +    return parser.total_size;
> > +}
> > +
> > +/* return size or -1 if error */
> > +int load_targphys_hex_as(const char *filename, hwaddr *entry,
> AddressSpace *as)
> > +{
> > +    gsize hex_blob_size;
> > +    gchar *hex_blob;
> > +    int total_size = 0;
> > +
> > +    if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size,
> NULL)) {
> > +        return -1;
> > +    }
> > +
> > +    total_size = parse_hex_blob(filename, entry, (uint8_t *)hex_blob,
> > +                                hex_blob_size, as);
> > +
> > +    g_free(hex_blob);
> > +    return total_size;
> > +}
> >
>
> Regards,
>
> Phil.
>
>

Re: [Qemu-devel] [PATCH v4 4/6] loader: add rom transaction API

[Stefan Hajnoczi]

On Wed, Aug 8, 2018 at 10:32 PM Alistair Francis <alistair23@gmail.com> wrote:
> On Fri, Aug 3, 2018 at 7:47 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > @@ -1168,6 +1172,34 @@ void rom_reset_order_override(void)
> >      fw_cfg_reset_order_override(fw_cfg);
> >  }
> >
> > +void rom_transaction_begin(void)
> > +{
> > +    Rom *rom;
> > +
> > +    /* Ignore ROMs added without the transaction API */
> > +    QTAILQ_FOREACH(rom, &roms, next) {
> > +        rom->committed = true;
>
> My only thought is that maybe this should produce a warning or error
> if a ROM isn't committed.

Not all loaders use the transaction API.  Therefore it is likely that
some pre-existing ROMs will have ->committed = false.

For example, imagine a firmware ROM is loaded by the machine type and
then -kernel is used to load a file.  The -kernel loader shouldn't
worry about the firmware ROM, which was added without the transaction
API.

If we want to be strict I'd have to audit all ROM API users and wrap
them in add rom_transaction_begin/end() even if they cannot fail.
This is why I decided to simply ignore pre-existing ROMs.

Stefan

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 5/6] loader: Implement .hex file loader

[Stefan Hajnoczi]

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

On Fri, Aug 10, 2018 at 02:00:44AM -0300, Philippe Mathieu-Daudé wrote:
> On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> > +        parser->rom_start_address = parser->next_address_to_write;
> > +        parser->current_rom_index = 0;
> > +        break;
> > +
> > +    case START_SEG_ADDR_RECORD:
> > +        if (line->byte_count != 4 && line->address != 0) {
> > +            return -1;
> > +        }
> > +
> > +        /* x86 16-bit CS:IP segmented addressing */
> > +        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
> > +                                (line->data[2] << 8) | line->data[3];
> 
> Can you add a qtest for this case?
> For the HEX loader I understand the specs as this is the same parsing as
> the START_LINEAR_ADDR_RECORD case; so I disagree with data[0] and
> data[1] shifts.

x86 real-mode CS:IP addressing means (CS << 4) + IP.  It produces 24-bit
addresses on 80286 and later.  This is not the same as
START_LINEAR_ADDR_RECORD.

GNU bfd implements it as follows:

  abfd->start_address += (HEX4 (buf) << 4) + HEX4 (buf + 4);

https://sourceware.org/git/?p=binutils.git;a=blob;f=bfd/ihex.c;h=09f756a1c2c11e57c5da15a6ff96491275575b86;hb=HEAD#l415

Thanks for commenting on this, I had written (CS << 4) | IP instead of
(CS << 4) + IP.  This will be fixed in the next revision.

> This is different for the consumer (i386 expects 2 16-bit registers but
> HexParser->start_addr is a hwaddr, used as a single (at least) 32-bit
> register.

I think you're saying that on x86 the guest CS:IP registers should be
set.  This loader is never called from hw/i386/ so it doesn't matter at
this time.

GNU bfd uses START_SEG_ADDR_RECORD records on non-x86 architectures
where there are no segment registers and that's where we've encountered
them.

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

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 6/6] Add QTest testcase for the Intel Hexadecimal

[Stefan Hajnoczi]

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

On Fri, Aug 10, 2018 at 01:26:08AM -0300, Philippe Mathieu-Daudé wrote:
> On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> Can you add a comment providing the .S asm source and how to
> build/generate this blob? (I think this is required to respect the GPLv2
> license in your header).

I have resolved this by using a memory test pattern instead of an actual
program.  The test case uses -qtest anyway, so no guest code is
executed.

Stefan

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

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 5/6] loader: Implement .hex file loader

[Philippe Mathieu-Daudé]

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

On 08/13/2018 12:56 PM, Stefan Hajnoczi wrote:
> On Fri, Aug 10, 2018 at 02:00:44AM -0300, Philippe Mathieu-Daudé wrote:
>> On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
>>> +        parser->rom_start_address = parser->next_address_to_write;
>>> +        parser->current_rom_index = 0;
>>> +        break;
>>> +
>>> +    case START_SEG_ADDR_RECORD:
>>> +        if (line->byte_count != 4 && line->address != 0) {
>>> +            return -1;
>>> +        }
>>> +
>>> +        /* x86 16-bit CS:IP segmented addressing */
>>> +        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
>>> +                                (line->data[2] << 8) | line->data[3];
>>
>> Can you add a qtest for this case?
>> For the HEX loader I understand the specs as this is the same parsing as
>> the START_LINEAR_ADDR_RECORD case; so I disagree with data[0] and
>> data[1] shifts.
> 
> x86 real-mode CS:IP addressing means (CS << 4) + IP.  It produces 24-bit
> addresses on 80286 and later.  This is not the same as
> START_LINEAR_ADDR_RECORD.

OK!

> 
> GNU bfd implements it as follows:
> 
>   abfd->start_address += (HEX4 (buf) << 4) + HEX4 (buf + 4);

Hmm any idea why they use +4 ?

> 
> https://sourceware.org/git/?p=binutils.git;a=blob;f=bfd/ihex.c;h=09f756a1c2c11e57c5da15a6ff96491275575b86;hb=HEAD#l415
> 
> Thanks for commenting on this, I had written (CS << 4) | IP instead of
> (CS << 4) + IP.  This will be fixed in the next revision.
> 
>> This is different for the consumer (i386 expects 2 16-bit registers but
>> HexParser->start_addr is a hwaddr, used as a single (at least) 32-bit
>> register.
> 
> I think you're saying that on x86 the guest CS:IP registers should be
> set.  This loader is never called from hw/i386/ so it doesn't matter at
> this time.

OK.

> 
> GNU bfd uses START_SEG_ADDR_RECORD records on non-x86 architectures
> where there are no segment registers and that's where we've encountered
> them.
> 


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

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 5/6] loader: Implement .hex file loader

[Stefan Hajnoczi]

On Wed, Aug 15, 2018 at 3:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 08/13/2018 12:56 PM, Stefan Hajnoczi wrote:
> > On Fri, Aug 10, 2018 at 02:00:44AM -0300, Philippe Mathieu-Daudé wrote:
> >> On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> >>> +        parser->rom_start_address = parser->next_address_to_write;
> >>> +        parser->current_rom_index = 0;
> >>> +        break;
> >>> +
> >>> +    case START_SEG_ADDR_RECORD:
> >>> +        if (line->byte_count != 4 && line->address != 0) {
> >>> +            return -1;
> >>> +        }
> >>> +
> >>> +        /* x86 16-bit CS:IP segmented addressing */
> >>> +        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
> >>> +                                (line->data[2] << 8) | line->data[3];
> >>
> >> Can you add a qtest for this case?
> >> For the HEX loader I understand the specs as this is the same parsing as
> >> the START_LINEAR_ADDR_RECORD case; so I disagree with data[0] and
> >> data[1] shifts.
> >
> > x86 real-mode CS:IP addressing means (CS << 4) + IP.  It produces 24-bit
> > addresses on 80286 and later.  This is not the same as
> > START_LINEAR_ADDR_RECORD.
>
> OK!
>
> >
> > GNU bfd implements it as follows:
> >
> >   abfd->start_address += (HEX4 (buf) << 4) + HEX4 (buf + 4);
>
> Hmm any idea why they use +4 ?

buf is char* and HEX4("0123") produces 0x0123.  So this line evaluates
ASCII hex input buf="XXXXYYYY" to (0xXXXX << 4) + 0xYYYY.

Re: [Qemu-devel] [Qemu-arm] [PATCH v4 5/6] loader: Implement .hex file loader

[Philippe Mathieu-Daudé]

On 08/15/2018 02:52 PM, Stefan Hajnoczi wrote:
> On Wed, Aug 15, 2018 at 3:18 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 08/13/2018 12:56 PM, Stefan Hajnoczi wrote:
>>> On Fri, Aug 10, 2018 at 02:00:44AM -0300, Philippe Mathieu-Daudé wrote:
>>>> On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
>>>>> +        parser->rom_start_address = parser->next_address_to_write;
>>>>> +        parser->current_rom_index = 0;
>>>>> +        break;
>>>>> +
>>>>> +    case START_SEG_ADDR_RECORD:
>>>>> +        if (line->byte_count != 4 && line->address != 0) {
>>>>> +            return -1;
>>>>> +        }
>>>>> +
>>>>> +        /* x86 16-bit CS:IP segmented addressing */
>>>>> +        *(parser->start_addr) = (((line->data[0] << 8) | line->data[1]) << 4) |
>>>>> +                                (line->data[2] << 8) | line->data[3];
>>>>
>>>> Can you add a qtest for this case?
>>>> For the HEX loader I understand the specs as this is the same parsing as
>>>> the START_LINEAR_ADDR_RECORD case; so I disagree with data[0] and
>>>> data[1] shifts.
>>>
>>> x86 real-mode CS:IP addressing means (CS << 4) + IP.  It produces 24-bit
>>> addresses on 80286 and later.  This is not the same as
>>> START_LINEAR_ADDR_RECORD.
>>
>> OK!
>>
>>>
>>> GNU bfd implements it as follows:
>>>
>>>   abfd->start_address += (HEX4 (buf) << 4) + HEX4 (buf + 4);
>>
>> Hmm any idea why they use +4 ?
> 
> buf is char* and HEX4("0123") produces 0x0123.  So this line evaluates
> ASCII hex input buf="XXXXYYYY" to (0xXXXX << 4) + 0xYYYY.

Haha with this weird indentation (space before parenthesis) I quickly
thought 4 was added to the address, and probably than HEX4 was a
constant... I will avoid too late review ;) sorry =)