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

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.

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

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.

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

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.

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