[Qemu-devel] [PATCH 0/8] Fixes and cleanups around fw_cfg

[Qemu-devel] [PATCH 0/8] Fixes and cleanups around fw_cfg

[Markus Armbruster]

Markus Armbruster (8):
  fw_cfg: Replace debug prints by tracepoints
  fw_cfg: Dumb down fw_cfg_add_*() not to return success / failure
  fw_cfg: New fw_cfg_add_string()
  pc: Fix unchecked strdup() by switching to fw_cfg_add_string()
  sun4: Fix unchecked strdup() by switching to fw_cfg_add_string()
  pc: Clean up bochs_bios_init()'s (non-)use of sizeof
  fw_cfg: Use void *, size_t instead of uint8_t *, uint32_t for blobs
  vl: Use size_t for sizes in get_boot_devices_list()

 hw/fw_cfg.c             | 86 ++++++++++++++++++++-----------------------------
 hw/fw_cfg.h             | 17 +++++-----
 hw/pc.c                 | 21 ++++++------
 hw/sun4m.c              | 12 ++-----
 hw/sun4u.c              |  4 +--
 include/sysemu/sysemu.h |  2 +-
 trace-events            |  7 ++++
 vl.c                    |  6 ++--
 8 files changed, 68 insertions(+), 87 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/8] fw_cfg: Replace debug prints by tracepoints

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fw_cfg.c  | 25 ++++++-------------------
 trace-events |  7 +++++++
 2 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 7c9480c..2fadf36 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -26,19 +26,10 @@
 #include "isa.h"
 #include "fw_cfg.h"
 #include "sysbus.h"
+#include "trace.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-/* debug firmware config */
-//#define DEBUG_FW_CFG
-
-#ifdef DEBUG_FW_CFG
-#define FW_CFG_DPRINTF(fmt, ...)                        \
-    do { printf("FW_CFG: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define FW_CFG_DPRINTF(fmt, ...)
-#endif
-
 #define FW_CFG_SIZE 2
 #define FW_CFG_DATA_SIZE 1
 
@@ -213,7 +204,7 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value)
     int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
     FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
 
-    FW_CFG_DPRINTF("write %d\n", value);
+    trace_fw_cfg_write(s, value);
 
     if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
         s->cur_offset < e->len) {
@@ -238,8 +229,7 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
         ret = 1;
     }
 
-    FW_CFG_DPRINTF("select key %d (%sfound)\n", key, ret ? "" : "not ");
-
+    trace_fw_cfg_select(s, key, ret);
     return ret;
 }
 
@@ -254,8 +244,7 @@ static uint8_t fw_cfg_read(FWCfgState *s)
     else
         ret = e->data[s->cur_offset++];
 
-    FW_CFG_DPRINTF("read %d\n", ret);
-
+    trace_fw_cfg_read(s, ret);
     return ret;
 }
 
@@ -470,16 +459,14 @@ int fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
             filename);
     for (i = 0; i < index; i++) {
         if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
-            FW_CFG_DPRINTF("%s: skip duplicate: %s\n", __FUNCTION__,
-                           s->files->f[index].name);
+            trace_fw_cfg_add_file_dupe(s, s->files->f[index].name);
             return 1;
         }
     }
 
     s->files->f[index].size   = cpu_to_be32(len);
     s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
-    FW_CFG_DPRINTF("%s: #%d: %s (%d bytes)\n", __FUNCTION__,
-                   index, s->files->f[index].name, len);
+    trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
 
     s->files->count = cpu_to_be32(index+1);
     return 1;
diff --git a/trace-events b/trace-events
index 6eabbac..cf76a11 100644
--- a/trace-events
+++ b/trace-events
@@ -167,6 +167,13 @@ ecc_mem_readl_ecr1(uint32_t ret) "Read event count 2 %08x"
 ecc_diag_mem_writeb(uint64_t addr, uint32_t val) "Write diagnostic %"PRId64" = %02x"
 ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x"
 
+# hw/fw_cfg.c
+fw_cfg_write(void *s, uint8_t value) "%p %d"
+fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
+fw_cfg_read(void *s, uint8_t ret) "%p = %d"
+fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
+fw_cfg_add_file(void *s, int index, char *name, uint32_t len) "%p #%d: %s (%d bytes)"
+
 # hw/hd-geometry.c
 hd_geometry_lchs_guess(void *bs, int cyls, int heads, int secs) "bs %p LCHS %d %d %d"
 hd_geometry_guess(void *bs, uint32_t cyls, uint32_t heads, uint32_t secs, int trans) "bs %p CHS %u %u %u trans %d"
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/8] fw_cfg: Dumb down fw_cfg_add_*() not to return success / failure

[Markus Armbruster]

No caller is checking the value, so all errors get ignored, usually
silently.  assert() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fw_cfg.c | 43 ++++++++++++++++---------------------------
 hw/fw_cfg.h | 16 ++++++++--------
 2 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 2fadf36..0361f68 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -373,71 +373,64 @@ static const VMStateDescription vmstate_fw_cfg = {
     }
 };
 
-int fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len)
+void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len)
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
     key &= FW_CFG_ENTRY_MASK;
 
-    if (key >= FW_CFG_MAX_ENTRY)
-        return 0;
+    assert(key < FW_CFG_MAX_ENTRY);
 
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
-
-    return 1;
 }
 
-int fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
+void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
 {
     uint16_t *copy;
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le16(value);
-    return fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
+    fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
 }
 
-int fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
+void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
 {
     uint32_t *copy;
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le32(value);
-    return fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
+    fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
 }
 
-int fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
+void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
 {
     uint64_t *copy;
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le64(value);
-    return fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
+    fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
 }
 
-int fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                        void *callback_opaque, uint8_t *data, size_t len)
+void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
+                         void *callback_opaque, uint8_t *data, size_t len)
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
-    if (!(key & FW_CFG_WRITE_CHANNEL))
-        return 0;
+    assert(key & FW_CFG_WRITE_CHANNEL);
 
     key &= FW_CFG_ENTRY_MASK;
 
-    if (key >= FW_CFG_MAX_ENTRY || len > 65535)
-        return 0;
+    assert(key < FW_CFG_MAX_ENTRY && len <= 65535);
 
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
     s->entries[arch][key].callback_opaque = callback_opaque;
     s->entries[arch][key].callback = callback;
-
-    return 1;
 }
 
-int fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
-                    uint32_t len)
+void fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
+                     uint32_t len)
 {
     int i, index;
 
@@ -448,10 +441,7 @@ int fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
     }
 
     index = be32_to_cpu(s->files->count);
-    if (index == FW_CFG_FILE_SLOTS) {
-        fprintf(stderr, "fw_cfg: out of file slots\n");
-        return 0;
-    }
+    assert(index < FW_CFG_FILE_SLOTS);
 
     fw_cfg_add_bytes(s, FW_CFG_FILE_FIRST + index, data, len);
 
@@ -460,7 +450,7 @@ int fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
     for (i = 0; i < index; i++) {
         if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
             trace_fw_cfg_add_file_dupe(s, s->files->f[index].name);
-            return 1;
+            return;
         }
     }
 
@@ -469,7 +459,6 @@ int fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
     trace_fw_cfg_add_file(s, index, s->files->f[index].name, len);
 
     s->files->count = cpu_to_be32(index+1);
-    return 1;
 }
 
 static void fw_cfg_machine_ready(struct Notifier *n, void *data)
diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index 619a394..7d32f28 100644
--- a/hw/fw_cfg.h
+++ b/hw/fw_cfg.h
@@ -54,14 +54,14 @@ typedef struct FWCfgFiles {
 typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
 
 typedef struct FWCfgState FWCfgState;
-int fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len);
-int fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
-int fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
-int fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
-int fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                        void *callback_opaque, uint8_t *data, size_t len);
-int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
-                    uint32_t len);
+void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len);
+void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
+void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
+void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
+void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
+                         void *callback_opaque, uint8_t *data, size_t len);
+void fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
+                     uint32_t len);
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
                         hwaddr crl_addr, hwaddr data_addr);
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 3/8] fw_cfg: New fw_cfg_add_string()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fw_cfg.c | 7 +++++++
 hw/fw_cfg.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 0361f68..3d6dd5f 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -385,6 +385,13 @@ void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len)
     s->entries[arch][key].len = len;
 }
 
+void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
+{
+    size_t sz = strlen(value) + 1;
+
+    return fw_cfg_add_bytes(s, key, (uint8_t *)g_memdup(value, sz), sz);
+}
+
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
 {
     uint16_t *copy;
diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index 7d32f28..c2c57cd 100644
--- a/hw/fw_cfg.h
+++ b/hw/fw_cfg.h
@@ -55,6 +55,7 @@ typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
 
 typedef struct FWCfgState FWCfgState;
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len);
+void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
-- 
1.7.11.7

[Qemu-devel] [PATCH 4/8] pc: Fix unchecked strdup() by switching to fw_cfg_add_string()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index ba1f19d..bc5c33f 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -693,9 +693,7 @@ static void load_linux(void *fw_cfg,
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline)+1);
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                     (uint8_t*)strdup(kernel_cmdline),
-                     strlen(kernel_cmdline)+1);
+    fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
 
     if (protocol >= 0x202) {
 	stl_p(header+0x228, cmdline_addr);
-- 
1.7.11.7

[Qemu-devel] [PATCH 5/8] sun4: Fix unchecked strdup() by switching to fw_cfg_add_string()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sun4m.c | 12 +++---------
 hw/sun4u.c |  4 +---
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/sun4m.c b/hw/sun4m.c
index 6f5de44..4264efd 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -1030,9 +1030,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
     if (kernel_cmdline) {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, CMDLINE_ADDR);
         pstrcpy_targphys("cmdline", CMDLINE_ADDR, TARGET_PAGE_SIZE, kernel_cmdline);
-        fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
-                         strlen(kernel_cmdline) + 1);
+        fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
         fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
                        strlen(kernel_cmdline) + 1);
     } else {
@@ -1676,9 +1674,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
     if (kernel_cmdline) {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, CMDLINE_ADDR);
         pstrcpy_targphys("cmdline", CMDLINE_ADDR, TARGET_PAGE_SIZE, kernel_cmdline);
-        fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
-                         strlen(kernel_cmdline) + 1);
+        fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
     } else {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, 0);
     }
@@ -1878,9 +1874,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
     if (kernel_cmdline) {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, CMDLINE_ADDR);
         pstrcpy_targphys("cmdline", CMDLINE_ADDR, TARGET_PAGE_SIZE, kernel_cmdline);
-        fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
-                         strlen(kernel_cmdline) + 1);
+        fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
     } else {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, 0);
     }
diff --git a/hw/sun4u.c b/hw/sun4u.c
index cb75d03..d36acde 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -886,9 +886,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     if (kernel_cmdline) {
         fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE,
                        strlen(kernel_cmdline) + 1);
-        fw_cfg_add_bytes(fw_cfg, FW_CFG_CMDLINE_DATA,
-                         (uint8_t*)strdup(kernel_cmdline),
-                         strlen(kernel_cmdline) + 1);
+        fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
     } else {
         fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, 0);
     }
-- 
1.7.11.7

[Qemu-devel] [PATCH 6/8] pc: Clean up bochs_bios_init()'s (non-)use of sizeof

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index bc5c33f..34cf79d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -572,7 +572,7 @@ static void *bochs_bios_init(void)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, (uint8_t *)&e820_table,
-                     sizeof(struct e820_table));
+                     sizeof(e820_table));
 
     fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, (uint8_t *)&hpet_cfg,
                      sizeof(struct hpet_fw_config));
@@ -580,7 +580,7 @@ static void *bochs_bios_init(void)
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
      */
-    numa_fw_cfg = g_malloc0((1 + max_cpus + nb_numa_nodes) * 8);
+    numa_fw_cfg = g_new0(uint64_t, 1 + max_cpus + nb_numa_nodes);
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
     for (i = 0; i < max_cpus; i++) {
         for (j = 0; j < nb_numa_nodes; j++) {
@@ -594,7 +594,7 @@ static void *bochs_bios_init(void)
         numa_fw_cfg[max_cpus + 1 + i] = cpu_to_le64(node_mem[i]);
     }
     fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, (uint8_t *)numa_fw_cfg,
-                     (1 + max_cpus + nb_numa_nodes) * 8);
+                     (1 + max_cpus + nb_numa_nodes) * sizeof(*numa_fw_cfg));
 
     return fw_cfg;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 7/8] fw_cfg: Use void *, size_t instead of uint8_t *, uint32_t for blobs

[Markus Armbruster]

Many callers pass size_t, which gets silently truncated to uint32_t.
Harmless, because all practical sizes are well below 4GiB.  Clean it
up anyway.  Size overflow now fails assertions.

Bonus: saves a whole bunch of silly casts.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fw_cfg.c  | 31 ++++++++++++++++---------------
 hw/fw_cfg.h  |  8 ++++----
 hw/pc.c      | 13 ++++++-------
 trace-events |  2 +-
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 3d6dd5f..699383c 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -373,23 +373,23 @@ static const VMStateDescription vmstate_fw_cfg = {
     }
 };
 
-void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len)
+void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
     key &= FW_CFG_ENTRY_MASK;
 
-    assert(key < FW_CFG_MAX_ENTRY);
+    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
 
     s->entries[arch][key].data = data;
-    s->entries[arch][key].len = len;
+    s->entries[arch][key].len = (uint32_t)len;
 }
 
 void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
 {
     size_t sz = strlen(value) + 1;
 
-    return fw_cfg_add_bytes(s, key, (uint8_t *)g_memdup(value, sz), sz);
+    return fw_cfg_add_bytes(s, key, g_memdup(value, sz), sz);
 }
 
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
@@ -398,7 +398,7 @@ void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le16(value);
-    fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
+    fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
@@ -407,7 +407,7 @@ void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le32(value);
-    fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
+    fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
@@ -416,11 +416,11 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value)
 
     copy = g_malloc(sizeof(value));
     *copy = cpu_to_le64(value);
-    fw_cfg_add_bytes(s, key, (uint8_t *)copy, sizeof(value));
+    fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
 void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, uint8_t *data, size_t len)
+                         void *callback_opaque, void *data, size_t len)
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
@@ -428,23 +428,24 @@ void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
 
     key &= FW_CFG_ENTRY_MASK;
 
-    assert(key < FW_CFG_MAX_ENTRY && len <= 65535);
+    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
 
     s->entries[arch][key].data = data;
-    s->entries[arch][key].len = len;
+    s->entries[arch][key].len = (uint32_t)len;
     s->entries[arch][key].callback_opaque = callback_opaque;
     s->entries[arch][key].callback = callback;
 }
 
-void fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
-                     uint32_t len)
+void fw_cfg_add_file(FWCfgState *s,  const char *filename,
+                     void *data, size_t len)
 {
     int i, index;
+    size_t dsize;
 
     if (!s->files) {
-        int dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * FW_CFG_FILE_SLOTS;
+        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * FW_CFG_FILE_SLOTS;
         s->files = g_malloc0(dsize);
-        fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, (uint8_t*)s->files, dsize);
+        fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
     }
 
     index = be32_to_cpu(s->files->count);
@@ -498,7 +499,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     if (data_addr) {
         sysbus_mmio_map(d, 1, data_addr);
     }
-    fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (uint8_t *)"QEMU", 4);
+    fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index c2c57cd..05c8df1 100644
--- a/hw/fw_cfg.h
+++ b/hw/fw_cfg.h
@@ -54,15 +54,15 @@ typedef struct FWCfgFiles {
 typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
 
 typedef struct FWCfgState FWCfgState;
-void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, uint8_t *data, uint32_t len);
+void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
 void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
 void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, uint8_t *data, size_t len);
-void fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
-                     uint32_t len);
+                         void *callback_opaque, void *data, size_t len);
+void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
+                     size_t len);
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
                         hwaddr crl_addr, hwaddr data_addr);
 
diff --git a/hw/pc.c b/hw/pc.c
index 34cf79d..8fc69db 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -563,19 +563,18 @@ static void *bochs_bios_init(void)
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, (uint8_t *)acpi_tables,
-                     acpi_tables_len);
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
+                     acpi_tables, acpi_tables_len);
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
 
     smbios_table = smbios_get_table(&smbios_len);
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE, (uint8_t *)&e820_table,
-                     sizeof(e820_table));
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
+                     &e820_table, sizeof(e820_table));
 
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, (uint8_t *)&hpet_cfg,
-                     sizeof(struct hpet_fw_config));
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
     /* allocate memory for the NUMA channel: one (64bit) word for the number
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
@@ -593,7 +592,7 @@ static void *bochs_bios_init(void)
     for (i = 0; i < nb_numa_nodes; i++) {
         numa_fw_cfg[max_cpus + 1 + i] = cpu_to_le64(node_mem[i]);
     }
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, (uint8_t *)numa_fw_cfg,
+    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
                      (1 + max_cpus + nb_numa_nodes) * sizeof(*numa_fw_cfg));
 
     return fw_cfg;
diff --git a/trace-events b/trace-events
index cf76a11..7de9106 100644
--- a/trace-events
+++ b/trace-events
@@ -172,7 +172,7 @@ fw_cfg_write(void *s, uint8_t value) "%p %d"
 fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
 fw_cfg_read(void *s, uint8_t ret) "%p = %d"
 fw_cfg_add_file_dupe(void *s, char *name) "%p %s"
-fw_cfg_add_file(void *s, int index, char *name, uint32_t len) "%p #%d: %s (%d bytes)"
+fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
 
 # hw/hd-geometry.c
 hd_geometry_lchs_guess(void *bs, int cyls, int heads, int secs) "bs %p LCHS %d %d %d"
-- 
1.7.11.7

[Qemu-devel] [PATCH 8/8] vl: Use size_t for sizes in get_boot_devices_list()

[Markus Armbruster]

Code mixes uint32_t, int and size_t.  Very unlikely to go wrong in
practice, but clean it up anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fw_cfg.c             | 2 +-
 include/sysemu/sysemu.h | 2 +-
 vl.c                    | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 699383c..3b31d77 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -471,7 +471,7 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
 
 static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 {
-    uint32_t len;
+    size_t len;
     FWCfgState *s = container_of(n, FWCfgState, machine_ready);
     char *bootindex = get_boot_devices_list(&len);
 
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index c07d4ee..6047ef3 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -179,7 +179,7 @@ void register_devices(void);
 
 void add_boot_device_path(int32_t bootindex, DeviceState *dev,
                           const char *suffix);
-char *get_boot_devices_list(uint32_t *size);
+char *get_boot_devices_list(size_t *size);
 
 bool usb_enabled(bool default_usb);
 
diff --git a/vl.c b/vl.c
index 15e0280..395c85b 100644
--- a/vl.c
+++ b/vl.c
@@ -1198,15 +1198,15 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
  * memory pointed by "size" is assigned total length of the array in bytes
  *
  */
-char *get_boot_devices_list(uint32_t *size)
+char *get_boot_devices_list(size_t *size)
 {
     FWBootEntry *i;
-    uint32_t total = 0;
+    size_t total = 0;
     char *list = NULL;
 
     QTAILQ_FOREACH(i, &fw_boot_order, link) {
         char *devpath = NULL, *bootpath;
-        int len;
+        size_t len;
 
         if (i->dev) {
             devpath = qdev_get_fw_dev_path(i->dev);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 0/8] Fixes and cleanups around fw_cfg

[Gleb Natapov]

On Wed, Jan 16, 2013 at 02:50:21PM +0100, Markus Armbruster wrote:
> Markus Armbruster (8):
>   fw_cfg: Replace debug prints by tracepoints
>   fw_cfg: Dumb down fw_cfg_add_*() not to return success / failure
>   fw_cfg: New fw_cfg_add_string()
>   pc: Fix unchecked strdup() by switching to fw_cfg_add_string()
>   sun4: Fix unchecked strdup() by switching to fw_cfg_add_string()
>   pc: Clean up bochs_bios_init()'s (non-)use of sizeof
>   fw_cfg: Use void *, size_t instead of uint8_t *, uint32_t for blobs
>   vl: Use size_t for sizes in get_boot_devices_list()
> 
Reviewed-by: Gleb Natapov <gleb@redhat.com>

>  hw/fw_cfg.c             | 86 ++++++++++++++++++++-----------------------------
>  hw/fw_cfg.h             | 17 +++++-----
>  hw/pc.c                 | 21 ++++++------
>  hw/sun4m.c              | 12 ++-----
>  hw/sun4u.c              |  4 +--
>  include/sysemu/sysemu.h |  2 +-
>  trace-events            |  7 ++++
>  vl.c                    |  6 ++--
>  8 files changed, 68 insertions(+), 87 deletions(-)
> 
> -- 
> 1.7.11.7

--
			Gleb.

Re: [Qemu-devel] [PATCH 0/8] Fixes and cleanups around fw_cfg

[Blue Swirl]

Thanks, applied all.

On Thu, Jan 17, 2013 at 1:16 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Wed, Jan 16, 2013 at 02:50:21PM +0100, Markus Armbruster wrote:
>> Markus Armbruster (8):
>>   fw_cfg: Replace debug prints by tracepoints
>>   fw_cfg: Dumb down fw_cfg_add_*() not to return success / failure
>>   fw_cfg: New fw_cfg_add_string()
>>   pc: Fix unchecked strdup() by switching to fw_cfg_add_string()
>>   sun4: Fix unchecked strdup() by switching to fw_cfg_add_string()
>>   pc: Clean up bochs_bios_init()'s (non-)use of sizeof
>>   fw_cfg: Use void *, size_t instead of uint8_t *, uint32_t for blobs
>>   vl: Use size_t for sizes in get_boot_devices_list()
>>
> Reviewed-by: Gleb Natapov <gleb@redhat.com>

Sorry, forgot to apply this.

>
>>  hw/fw_cfg.c             | 86 ++++++++++++++++++++-----------------------------
>>  hw/fw_cfg.h             | 17 +++++-----
>>  hw/pc.c                 | 21 ++++++------
>>  hw/sun4m.c              | 12 ++-----
>>  hw/sun4u.c              |  4 +--
>>  include/sysemu/sysemu.h |  2 +-
>>  trace-events            |  7 ++++
>>  vl.c                    |  6 ++--
>>  8 files changed, 68 insertions(+), 87 deletions(-)
>>
>> --
>> 1.7.11.7
>
> --
>                         Gleb.