qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader
@ 2018-08-14 16:27 Stefan Hajnoczi
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 1/6] hw/arm: make bitbanded IO optional on ARMv7-M Stefan Hajnoczi
                   ` (6 more replies)
  0 siblings, 7 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-14 16:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Julia Suvorova, Peter Maydell, joel, qemu-arm, Subbaraya Sundeep,
	jim, Alistair Francis, Steffen Gortz, mail, Su Hang, ilg,
	Stefan Hajnoczi

v6:
 * Added comment to hexloader-test.c explaining contents of test.hex file
   [Phil]
v5:
 * Implemented Phil's hex loader suggestions
 * Replaced Su Hang's test hex file with a simple memory test pattern,
   eliminating the need to distribute source [Phil]
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                     | 302 ++++++++++++++++++++++++++-
 target/arm/cpu.c                     |  11 +
 tests/hexloader-test.c               |  45 ++++
 tests/hex-loader-check-data/test.hex |  18 ++
 15 files changed, 440 insertions(+), 26 deletions(-)
 create mode 100644 tests/hexloader-test.c
 create mode 100644 tests/hex-loader-check-data/test.hex

-- 
2.17.1

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v6 1/6] hw/arm: make bitbanded IO optional on ARMv7-M
  2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
@ 2018-08-14 16:27 ` Stefan Hajnoczi
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 2/6] target/arm: add "cortex-m0" CPU model Stefan Hajnoczi
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-14 16:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Julia Suvorova, Peter Maydell, joel, qemu-arm, Subbaraya Sundeep,
	jim, Alistair Francis, Steffen Gortz, mail, Su Hang, ilg,
	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>
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);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v6 2/6] target/arm: add "cortex-m0" CPU model
  2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 1/6] hw/arm: make bitbanded IO optional on ARMv7-M Stefan Hajnoczi
@ 2018-08-14 16:27 ` Stefan Hajnoczi
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 3/6] loader: extract rom_free() function Stefan Hajnoczi
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-14 16:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Julia Suvorova, Peter Maydell, joel, qemu-arm, Subbaraya Sundeep,
	jim, Alistair Francis, Steffen Gortz, mail, Su Hang, ilg,
	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>
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,
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v6 3/6] loader: extract rom_free() function
  2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 1/6] hw/arm: make bitbanded IO optional on ARMv7-M Stefan Hajnoczi
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 2/6] target/arm: add "cortex-m0" CPU model Stefan Hajnoczi
@ 2018-08-14 16:27 ` Stefan Hajnoczi
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 4/6] loader: add rom transaction API Stefan Hajnoczi
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-14 16:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Julia Suvorova, Peter Maydell, joel, qemu-arm, Subbaraya Sundeep,
	jim, Alistair Francis, Steffen Gortz, mail, Su Hang, ilg,
	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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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;
 }
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v6 4/6] loader: add rom transaction API
  2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
                   ` (2 preceding siblings ...)
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 3/6] loader: extract rom_free() function Stefan Hajnoczi
@ 2018-08-14 16:27 ` Stefan Hajnoczi
  2018-08-14 18:18   ` Alistair Francis
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 5/6] loader: Implement .hex file loader Stefan Hajnoczi
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-14 16:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Julia Suvorova, Peter Maydell, joel, qemu-arm, Subbaraya Sundeep,
	jim, Alistair Francis, Steffen Gortz, mail, Su Hang, ilg,
	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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v6 5/6] loader: Implement .hex file loader
  2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
                   ` (3 preceding siblings ...)
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 4/6] loader: add rom transaction API Stefan Hajnoczi
@ 2018-08-14 16:27 ` Stefan Hajnoczi
  2018-08-15 23:01   ` Alistair Francis
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 6/6] Add QTest testcase for the Intel Hexadecimal Stefan Hajnoczi
  2018-08-16 12:38 ` [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Peter Maydell
  6 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-14 16:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Julia Suvorova, Peter Maydell, joel, qemu-arm, Subbaraya Sundeep,
	jim, Alistair Francis, Steffen Gortz, mail, Su Hang, ilg,
	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         | 249 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 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..390987a05c 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1321,3 +1321,252 @@ 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,
+};
+
+/* Each record contains a 16-bit address which is combined with the upper 16
+ * bits of the implicit "next address" to form a 32-bit address.
+ */
+#define NEXT_ADDR_MASK 0xffff0000
+
+#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 & NEXT_ADDR_MASK) | 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->data[0] << 12) |
+                                        (line->data[1] << 4);
+        if (line->record_type == EXT_LINEAR_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]);
+        break;
+
+    case START_LINEAR_ADDR_RECORD:
+        if (line->byte_count != 4 && line->address != 0) {
+            return -1;
+        }
+
+        *(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;
+}
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v6 6/6] Add QTest testcase for the Intel Hexadecimal
  2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
                   ` (4 preceding siblings ...)
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 5/6] loader: Implement .hex file loader Stefan Hajnoczi
@ 2018-08-14 16:27 ` Stefan Hajnoczi
  2018-08-16 12:38 ` [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Peter Maydell
  6 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-14 16:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Julia Suvorova, Peter Maydell, joel, qemu-arm, Subbaraya Sundeep,
	jim, Alistair Francis, Steffen Gortz, mail, Su Hang, ilg,
	Stefan Hajnoczi

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

'test.hex' file is a memory test pattern stored in Hexadecimal Object
Format.  It loads at 0x10000 in RAM and contains values from 0 through
255.

The test case verifies that the expected memory test pattern was loaded.

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 MAINTAINERS                          |  6 ++++
 configure                            |  4 +++
 tests/Makefile.include               |  2 ++
 tests/hexloader-test.c               | 45 ++++++++++++++++++++++++++++
 tests/hex-loader-check-data/test.hex | 18 +++++++++++
 5 files changed, 75 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..afb620694d
--- /dev/null
+++ b/tests/hexloader-test.c
@@ -0,0 +1,45 @@
+/*
+ * 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"
+
+/* Load 'test.hex' and verify that the in-memory contents are as expected.
+ * 'test.hex' is a memory test pattern stored in Hexadecimal Object
+ * format.  It loads at 0x10000 in RAM and contains values from 0 through
+ * 255.
+ */
+static void hex_loader_test(void)
+{
+    unsigned int i;
+    const unsigned int base_addr = 0x00010000;
+
+    QTestState *s = qtest_startf(
+        "-M vexpress-a9 -nographic -device loader,file=tests/hex-loader-check-data/test.hex");
+
+    for (i = 0; i < 256; ++i) {
+        uint8_t val = qtest_readb(s, base_addr + i);
+        g_assert_cmpuint(i, ==, val);
+    }
+    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..008a90bd4d
--- /dev/null
+++ b/tests/hex-loader-check-data/test.hex
@@ -0,0 +1,18 @@
+:020000040001F9
+:10000000000102030405060708090a0b0c0d0e0f78
+:10001000101112131415161718191a1b1c1d1e1f68
+:10002000202122232425262728292a2b2c2d2e2f58
+:10003000303132333435363738393a3b3c3d3e3f48
+:10004000404142434445464748494a4b4c4d4e4f38
+:10005000505152535455565758595a5b5c5d5e5f28
+:10006000606162636465666768696a6b6c6d6e6f18
+:10007000707172737475767778797a7b7c7d7e7f08
+:10008000808182838485868788898a8b8c8d8e8ff8
+:10009000909192939495969798999a9b9c9d9e9fe8
+:1000a000a0a1a2a3a4a5a6a7a8a9aaabacadaeafd8
+:1000b000b0b1b2b3b4b5b6b7b8b9babbbcbdbebfc8
+:1000c000c0c1c2c3c4c5c6c7c8c9cacbcccdcecfb8
+:1000d000d0d1d2d3d4d5d6d7d8d9dadbdcdddedfa8
+:1000e000e0e1e2e3e4e5e6e7e8e9eaebecedeeef98
+:1000f000f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff88
+:00000001FF
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v6 4/6] loader: add rom transaction API
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 4/6] loader: add rom transaction API Stefan Hajnoczi
@ 2018-08-14 18:18   ` Alistair Francis
  0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2018-08-14 18:18 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell, jim, mail,
	Su Hang, Liviu Ionescu, Alistair Francis, Subbaraya Sundeep,
	Steffen Gortz, qemu-arm, Joel Stanley, Julia Suvorova

On Tue, Aug 14, 2018 at 9:27 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>

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

Alistair

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v6 5/6] loader: Implement .hex file loader
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 5/6] loader: Implement .hex file loader Stefan Hajnoczi
@ 2018-08-15 23:01   ` Alistair Francis
  0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2018-08-15 23:01 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell, jim, mail,
	Su Hang, Liviu Ionescu, Alistair Francis, Subbaraya Sundeep,
	Steffen Gortz, qemu-arm, Joel Stanley, Julia Suvorova

On Tue, Aug 14, 2018 at 9:27 AM, Stefan Hajnoczi <stefanha@redhat.com> 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         | 249 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 265 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..390987a05c 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1321,3 +1321,252 @@ 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,
> +};
> +
> +/* Each record contains a 16-bit address which is combined with the upper 16
> + * bits of the implicit "next address" to form a 32-bit address.
> + */
> +#define NEXT_ADDR_MASK 0xffff0000
> +
> +#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 |

I think it would be clearer if this followed the language used in the
documentation. Otherwise I don't see anything wrong here.

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

Alistair

> +     */
> +    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 & NEXT_ADDR_MASK) | 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->data[0] << 12) |
> +                                        (line->data[1] << 4);
> +        if (line->record_type == EXT_LINEAR_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]);
> +        break;
> +
> +    case START_LINEAR_ADDR_RECORD:
> +        if (line->byte_count != 4 && line->address != 0) {
> +            return -1;
> +        }
> +
> +        *(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;
> +}
> --
> 2.17.1
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader
  2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
                   ` (5 preceding siblings ...)
  2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 6/6] Add QTest testcase for the Intel Hexadecimal Stefan Hajnoczi
@ 2018-08-16 12:38 ` Peter Maydell
  2018-08-16 14:08   ` Stefan Hajnoczi
  6 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2018-08-16 12:38 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: QEMU Developers, Julia Suvorova, Joel Stanley, qemu-arm,
	Subbaraya Sundeep, Jim Mussared, Alistair Francis, Steffen Gortz,
	Steffen Görtz, Su Hang, Liviu Ionescu

On 14 August 2018 at 17:27, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 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.

Applied to target-arm.next, thanks.

I notice that the change to generic-loader doesn't include an
update to docs/generic-loader.txt -- could somebody do a followup
patch for that? Currently the docs say

# The loader device also allows files to be loaded into memory. It can load raw
# files and ELF executable files.  Raw files are loaded verbatim.  ELF
executable
# files are loaded by an ELF loader.

but looking at the code we also support uImage files and HEX files.
We should mention all of these, and be clear about which ones
require the <addr> to be specified, and what the PC will be set
to in the case where <cpu-num> is specified.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader
  2018-08-16 12:38 ` [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Peter Maydell
@ 2018-08-16 14:08   ` Stefan Hajnoczi
  0 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2018-08-16 14:08 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Stefan Hajnoczi, Jim Mussared, Steffen Görtz,
	苏航, Liviu Ionescu, Alistair Francis, qemu-devel,
	Subbaraya Sundeep, Steffen Görtz, qemu-arm, Joel Stanley,
	Julia Suvorova

On Thu, Aug 16, 2018 at 1:39 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 August 2018 at 17:27, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 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.
>
> Applied to target-arm.next, thanks.
>
> I notice that the change to generic-loader doesn't include an
> update to docs/generic-loader.txt -- could somebody do a followup
> patch for that? Currently the docs say
>
> # The loader device also allows files to be loaded into memory. It can load raw
> # files and ELF executable files.  Raw files are loaded verbatim.  ELF
> executable
> # files are loaded by an ELF loader.
>
> but looking at the code we also support uImage files and HEX files.
> We should mention all of these, and be clear about which ones
> require the <addr> to be specified, and what the PC will be set
> to in the case where <cpu-num> is specified.

Thanks, I will take a look and send a patch.

Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-08-16 14:08 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-14 16:27 [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Stefan Hajnoczi
2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 1/6] hw/arm: make bitbanded IO optional on ARMv7-M Stefan Hajnoczi
2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 2/6] target/arm: add "cortex-m0" CPU model Stefan Hajnoczi
2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 3/6] loader: extract rom_free() function Stefan Hajnoczi
2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 4/6] loader: add rom transaction API Stefan Hajnoczi
2018-08-14 18:18   ` Alistair Francis
2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 5/6] loader: Implement .hex file loader Stefan Hajnoczi
2018-08-15 23:01   ` Alistair Francis
2018-08-14 16:27 ` [Qemu-devel] [PATCH v6 6/6] Add QTest testcase for the Intel Hexadecimal Stefan Hajnoczi
2018-08-16 12:38 ` [Qemu-devel] [PATCH v6 0/6] arm: add Cortex M0 CPU model and hex file loader Peter Maydell
2018-08-16 14:08   ` Stefan Hajnoczi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).