[Qemu-devel] [0/3] Various code cleanups

[Qemu-devel] [0/3] Various code cleanups

[David Gibson]

These 3 patches make several cleanups to the qemu code; they affect
files across the tree, particularly 2/3.

Please apply.

[Qemu-devel] [PATCH 1/3] pci: Factor out bounds checking on config space accesses

[David Gibson]

There are several paths into the code to emulate PCI config space accesses:
one for MMIO to a plain old PCI bridge one for MMIO to a PCIe bridge and
one for the pseries machine which provides para-virtualized access to PCI
config space.  Each of these functions does their own bounds checking
against the size of config space to check for addresses outside the
size of config space.  The pci_host_config_{read,write}_common() (sort
of) checks for partial overruns, that is where the address is within
the size of config space, but address + length is not, it takes a
limit parameter for this purpose.

As well as being a small code duplication, and it being weird to
separate the checks for partial and total overruns, this checking
currently has a few buglets:

    * For non PCI-Express we assume that the size of config space is
      PCI_CONFIG_SPACE_SIZE.  That's true for everything we emulate
      now, but is not necessarily true (e.g. PCI-X devices can have
      extended config space)

    * The limit parameter is not necessary, since the size of config
      space can be obtained using pci_config_size()

    * Partial overruns could only occur with a misaligned access,
      which should have already been dealt with by this point

    * Partial overruns are handled as a partial read or write, which
      is very unlikely behaviour for real hardware

    * Furthermore, partial reads are 0x0 padded, whereas returning
      0xff for unimplemented addresses us much more common.

    * The partial reads/writes only work correctly by assuming
      little-endian byte layout.  While that is always true for PCI
      config space, it's an awfully subtle thing to rely on without
      comment.

This patch, therefore, moves the bounds checking wholly into
pci_host_config_{read,write}_common().  No partial reads or writes are
performed, instead any out-of-bounds write is simply ignored and an
out-of-bounds read returns 0xff.

This simplifies all the callers, and makes the overall semantics saner
for edge cases.

Cc: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci_host.c  |   26 ++++++++++++++------------
 hw/pci_host.h  |    4 ++--
 hw/pcie_host.c |   18 ++----------------
 hw/spapr_pci.c |   27 ++++-----------------------
 4 files changed, 22 insertions(+), 53 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..829d797 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -48,48 +48,50 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 }
 
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len)
+                                  uint32_t val, uint32_t len)
 {
     assert(len <= 4);
-    pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
+    if ((addr + len) <= pci_config_size(pci_dev)) {
+        pci_dev->config_write(pci_dev, addr, val, len);
+    }
 }
 
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len)
+                                     uint32_t len)
 {
     assert(len <= 4);
-    return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
+    if ((addr + len) <= pci_config_size(pci_dev)) {
+        return pci_dev->config_read(pci_dev, addr, len);
+    } else {
+        return ~0x0;
+    }
 }
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 {
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
-    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
     if (!pci_dev) {
         return;
     }
 
     PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n",
-                __func__, pci_dev->name, config_addr, val, len);
-    pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
-                                 val, len);
+                __func__, pci_dev->name, addr, val, len);
+    pci_host_config_write_common(pci_dev, addr, val, len);
 }
 
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 {
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
-    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
 
     if (!pci_dev) {
         return ~0x0;
     }
 
-    val = pci_host_config_read_common(pci_dev, config_addr,
-                                      PCI_CONFIG_SPACE_SIZE, len);
+    val = pci_host_config_read_common(pci_dev, addr, len);
     PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
-                __func__, pci_dev->name, config_addr, val, len);
+                __func__, pci_dev->name, addr, val, len);
 
     return val;
 }
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 359e38f..4bb0838 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -42,9 +42,9 @@ struct PCIHostState {
 
 /* common internal helpers for PCI/PCIe hosts, cut off overflows */
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len);
+                                  uint32_t val, uint32_t len);
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len);
+                                     uint32_t len);
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index 28bbe72..3af8610 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -60,19 +60,12 @@ static void pcie_mmcfg_data_write(void *opaque, target_phys_addr_t mmcfg_addr,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return;
-    }
-    pci_host_config_write_common(pci_dev, addr, limit, val, len);
+    pci_host_config_write_common(pci_dev, addr, val, len);
 }
 
 static uint64_t pcie_mmcfg_data_read(void *opaque,
@@ -83,19 +76,12 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return ~0x0;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return ~0x0;
-    }
-    return pci_host_config_read_common(pci_dev, addr, limit, len);
+    return pci_host_config_read_common(pci_dev, addr, len);
 }
 
 static const MemoryRegionOps pcie_mmcfg_ops = {
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index cfdd9dd..1e8d03e 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -67,25 +67,6 @@ static uint32_t rtas_pci_cfgaddr(uint32_t arg)
     return ((arg >> 20) & 0xf00) | (arg & 0xff);
 }
 
-static uint32_t rtas_read_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                        uint32_t limit, uint32_t len)
-{
-    if ((addr + len) <= limit) {
-        return pci_host_config_read_common(pci_dev, addr, limit, len);
-    } else {
-        return ~0x0;
-    }
-}
-
-static void rtas_write_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t val,
-                                     uint32_t len)
-{
-    if ((addr + len) <= limit) {
-        pci_host_config_write_common(pci_dev, addr, limit, val, len);
-    }
-}
-
 static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
                                      uint32_t token, uint32_t nargs,
                                      target_ulong args,
@@ -101,7 +82,7 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
     }
     size = rtas_ld(args, 3);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
+    val = pci_host_config_read_common(dev, addr, size);
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, val);
 }
@@ -120,7 +101,7 @@ static void rtas_read_pci_config(sPAPREnvironment *spapr,
     }
     size = rtas_ld(args, 1);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
+    val = pci_host_config_read_common(dev, addr, size);
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, val);
 }
@@ -141,7 +122,7 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
     val = rtas_ld(args, 4);
     size = rtas_ld(args, 3);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
+    pci_host_config_write_common(dev, addr, val, size);
     rtas_st(rets, 0, 0);
 }
 
@@ -160,7 +141,7 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr,
     val = rtas_ld(args, 2);
     size = rtas_ld(args, 1);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
+    pci_host_config_write_common(dev, addr, val, size);
     rtas_st(rets, 0, 0);
 }
 
-- 
1.7.9

[Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[David Gibson]

Currently get_image_size(), used to find the size of files, returns an int.
But for modern systems, int may be only 32-bit and we can have files
larger than that.

This patch, therefore, changes the return type of get_image_size() to off_t
(the same as the return type from lseek() itself).  It also audits all the
callers of get_image_size() to make sure they process the new unsigned
return type correctly.

This leaves load_image_targphys() with a limited return type, but one thing
at a time (that function has far more callers to be audited, so it will
take longer to fix).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 device_tree.c      |    4 ++--
 hw/alpha_dp264.c   |    5 +++--
 hw/highbank.c      |    2 +-
 hw/leon3.c         |    6 +++---
 hw/loader.c        |   12 +++++++-----
 hw/loader.h        |    2 +-
 hw/mips_fulong2e.c |   12 ++++++------
 hw/mips_malta.c    |   12 ++++++------
 hw/mips_mipssim.c  |    6 +++---
 hw/mips_r4k.c      |   17 +++++++++--------
 hw/multiboot.c     |    4 ++--
 hw/palm.c          |   11 ++++++-----
 hw/pc.c            |    5 +++--
 hw/pc_sysfw.c      |    6 +++---
 hw/pci.c           |    4 ++--
 hw/ppc_prep.c      |    7 ++++---
 hw/smbios.c        |    2 +-
 17 files changed, 62 insertions(+), 55 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 86a694c..5817b2d 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -27,14 +27,14 @@
 
 void *load_device_tree(const char *filename_path, int *sizep)
 {
-    int dt_size;
+    off_t dt_size;
     int dt_file_load_size;
     int ret;
     void *fdt = NULL;
 
     *sizep = 0;
     dt_size = get_image_size(filename_path);
-    if (dt_size < 0) {
+    if (dt_size == -1) {
         printf("Unable to get size of device tree file '%s'\n",
             filename_path);
         goto fail;
diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
index ea0fd95..01ec9c7 100644
--- a/hw/alpha_dp264.c
+++ b/hw/alpha_dp264.c
@@ -144,10 +144,11 @@ static void clipper_init(ram_addr_t ram_size,
         }
 
         if (initrd_filename) {
-            long initrd_base, initrd_size;
+            long initrd_base;
+            off_t initrd_size;
 
             initrd_size = get_image_size(initrd_filename);
-            if (initrd_size < 0) {
+            if (initrd_size != -1) {
                 hw_error("could not load initial ram disk '%s'\n",
                          initrd_filename);
                 exit(1);
diff --git a/hw/highbank.c b/hw/highbank.c
index 489c00e..f4de4c6 100644
--- a/hw/highbank.c
+++ b/hw/highbank.c
@@ -235,7 +235,7 @@ static void highbank_init(ram_addr_t ram_size,
     if (bios_name != NULL) {
         sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
         if (sysboot_filename != NULL) {
-            uint32_t filesize = get_image_size(sysboot_filename);
+            off_t filesize = get_image_size(sysboot_filename);
             if (load_image_targphys("sysram.bin", 0xfff88000, filesize) < 0) {
                 hw_error("Unable to load %s\n", bios_name);
             }
diff --git a/hw/leon3.c b/hw/leon3.c
index 71d79a6..f7d4860 100644
--- a/hw/leon3.c
+++ b/hw/leon3.c
@@ -108,7 +108,7 @@ static void leon3_generic_hw_init(ram_addr_t  ram_size,
     int         ret;
     char       *filename;
     qemu_irq   *cpu_irqs = NULL;
-    int         bios_size;
+    off_t       bios_size;
     int         prom_size;
     ResetData  *reset_info;
 
@@ -168,7 +168,7 @@ static void leon3_generic_hw_init(ram_addr_t  ram_size,
         exit(1);
     }
 
-    if (bios_size > 0) {
+    if (bios_size != -1) {
         ret = load_image_targphys(filename, 0x00000000, bios_size);
         if (ret < 0 || ret > prom_size) {
             fprintf(stderr, "qemu: could not load prom '%s'\n", filename);
@@ -191,7 +191,7 @@ static void leon3_generic_hw_init(ram_addr_t  ram_size,
                     kernel_filename);
             exit(1);
         }
-        if (bios_size <= 0) {
+        if (bios_size == 0 || bios_size == -1) {
             /* If there is no bios/monitor, start the application.  */
             env->pc = entry;
             env->npc = entry + 4;
diff --git a/hw/loader.c b/hw/loader.c
index 415cdce..372f6f8 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -57,12 +57,14 @@
 static int roms_loaded;
 
 /* return the size or -1 if error */
-int get_image_size(const char *filename)
+off_t get_image_size(const char *filename)
 {
-    int fd, size;
+    int fd;
+    off_t size;
+
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0)
-        return -1;
+        return (off_t)-1;
     size = lseek(fd, 0, SEEK_END);
     close(fd);
     return size;
@@ -105,13 +107,13 @@ ssize_t read_targphys(const char *name,
 int load_image_targphys(const char *filename,
 			target_phys_addr_t addr, int max_sz)
 {
-    int size;
+    off_t size;
 
     size = get_image_size(filename);
     if (size > max_sz) {
         return -1;
     }
-    if (size > 0) {
+    if (size != -1) {
         rom_add_file_fixed(filename, addr, -1);
     }
     return size;
diff --git a/hw/loader.h b/hw/loader.h
index fbcaba9..1fbdbcf 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -2,7 +2,7 @@
 #define LOADER_H
 
 /* loader.c */
-int get_image_size(const char *filename);
+off_t get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
 int load_image_targphys(const char *filename, target_phys_addr_t, int max_sz);
 int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
index e3ba9dd..1e7b49a 100644
--- a/hw/mips_fulong2e.c
+++ b/hw/mips_fulong2e.c
@@ -107,7 +107,7 @@ static int64_t load_kernel (CPUState *env)
 {
     int64_t kernel_entry, kernel_low, kernel_high;
     int index = 0;
-    long initrd_size;
+    off_t initrd_size;
     ram_addr_t initrd_offset;
     uint32_t *prom_buf;
     long prom_size;
@@ -125,7 +125,7 @@ static int64_t load_kernel (CPUState *env)
     initrd_offset = 0;
     if (loaderparams.initrd_filename) {
         initrd_size = get_image_size (loaderparams.initrd_filename);
-        if (initrd_size > 0) {
+        if (initrd_size != -1) {
             initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
             if (initrd_offset + initrd_size > ram_size) {
                 fprintf(stderr,
@@ -136,7 +136,7 @@ static int64_t load_kernel (CPUState *env)
             initrd_size = load_image_targphys(loaderparams.initrd_filename,
                                      initrd_offset, ram_size - initrd_offset);
         }
-        if (initrd_size == (target_ulong) -1) {
+        if (initrd_size == -1) {
             fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
                     loaderparams.initrd_filename);
             exit(1);
@@ -148,10 +148,10 @@ static int64_t load_kernel (CPUState *env)
     prom_buf = g_malloc(prom_size);
 
     prom_set(prom_buf, index++, "%s", loaderparams.kernel_filename);
-    if (initrd_size > 0) {
+    if (initrd_size != -1) {
         prom_set(prom_buf, index++, "rd_start=0x%" PRIx64 " rd_size=%li %s",
-                 cpu_mips_phys_to_kseg0(NULL, initrd_offset), initrd_size,
-                 loaderparams.kernel_cmdline);
+                 cpu_mips_phys_to_kseg0(NULL, initrd_offset),
+                 (long)initrd_size, loaderparams.kernel_cmdline);
     } else {
         prom_set(prom_buf, index++, "%s", loaderparams.kernel_cmdline);
     }
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index 86a5fbb..50e800a 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -667,7 +667,7 @@ static void GCC_FMT_ATTR(3, 4) prom_set(uint32_t* prom_buf, int index,
 static int64_t load_kernel (void)
 {
     int64_t kernel_entry, kernel_high;
-    long initrd_size;
+    off_t initrd_size;
     ram_addr_t initrd_offset;
     int big_endian;
     uint32_t *prom_buf;
@@ -693,7 +693,7 @@ static int64_t load_kernel (void)
     initrd_offset = 0;
     if (loaderparams.initrd_filename) {
         initrd_size = get_image_size (loaderparams.initrd_filename);
-        if (initrd_size > 0) {
+        if (initrd_size != -1) {
             initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
             if (initrd_offset + initrd_size > ram_size) {
                 fprintf(stderr,
@@ -705,7 +705,7 @@ static int64_t load_kernel (void)
                                               initrd_offset,
                                               ram_size - initrd_offset);
         }
-        if (initrd_size == (target_ulong) -1) {
+        if (initrd_size == -1) {
             fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
                     loaderparams.initrd_filename);
             exit(1);
@@ -717,10 +717,10 @@ static int64_t load_kernel (void)
     prom_buf = g_malloc(prom_size);
 
     prom_set(prom_buf, prom_index++, "%s", loaderparams.kernel_filename);
-    if (initrd_size > 0) {
+    if (initrd_size != -1) {
         prom_set(prom_buf, prom_index++, "rd_start=0x%" PRIx64 " rd_size=%li %s",
-                 cpu_mips_phys_to_kseg0(NULL, initrd_offset), initrd_size,
-                 loaderparams.kernel_cmdline);
+                 cpu_mips_phys_to_kseg0(NULL, initrd_offset),
+                 (long)initrd_size, loaderparams.kernel_cmdline);
     } else {
         prom_set(prom_buf, prom_index++, "%s", loaderparams.kernel_cmdline);
     }
diff --git a/hw/mips_mipssim.c b/hw/mips_mipssim.c
index 76c95b2..89de6ca 100644
--- a/hw/mips_mipssim.c
+++ b/hw/mips_mipssim.c
@@ -54,7 +54,7 @@ static int64_t load_kernel(void)
 {
     int64_t entry, kernel_high;
     long kernel_size;
-    long initrd_size;
+    off_t initrd_size;
     ram_addr_t initrd_offset;
     int big_endian;
 
@@ -82,7 +82,7 @@ static int64_t load_kernel(void)
     initrd_offset = 0;
     if (loaderparams.initrd_filename) {
         initrd_size = get_image_size (loaderparams.initrd_filename);
-        if (initrd_size > 0) {
+        if (initrd_size != -1) {
             initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
             if (initrd_offset + initrd_size > loaderparams.ram_size) {
                 fprintf(stderr,
@@ -93,7 +93,7 @@ static int64_t load_kernel(void)
             initrd_size = load_image_targphys(loaderparams.initrd_filename,
                 initrd_offset, loaderparams.ram_size - initrd_offset);
         }
-        if (initrd_size == (target_ulong) -1) {
+        if (initrd_size == -1) {
             fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
                     loaderparams.initrd_filename);
             exit(1);
diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
index 83401f0..067a793 100644
--- a/hw/mips_r4k.c
+++ b/hw/mips_r4k.c
@@ -72,7 +72,8 @@ typedef struct ResetData {
 static int64_t load_kernel(void)
 {
     int64_t entry, kernel_high;
-    long kernel_size, initrd_size, params_size;
+    long kernel_size, params_size;
+    off_t initrd_size;
     ram_addr_t initrd_offset;
     uint32_t *params_buf;
     int big_endian;
@@ -100,7 +101,7 @@ static int64_t load_kernel(void)
     initrd_offset = 0;
     if (loaderparams.initrd_filename) {
         initrd_size = get_image_size (loaderparams.initrd_filename);
-        if (initrd_size > 0) {
+        if (initrd_size != -1) {
             initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
             if (initrd_offset + initrd_size > ram_size) {
                 fprintf(stderr,
@@ -112,7 +113,7 @@ static int64_t load_kernel(void)
                                               initrd_offset,
                                               ram_size - initrd_offset);
         }
-        if (initrd_size == (target_ulong) -1) {
+        if (initrd_size == -1) {
             fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
                     loaderparams.initrd_filename);
             exit(1);
@@ -126,10 +127,10 @@ static int64_t load_kernel(void)
     params_buf[0] = tswap32(ram_size);
     params_buf[1] = tswap32(0x12345678);
 
-    if (initrd_size > 0) {
+    if (initrd_size != -1) {
         snprintf((char *)params_buf + 8, 256, "rd_start=0x%" PRIx64 " rd_size=%li %s",
                  cpu_mips_phys_to_kseg0(NULL, initrd_offset),
-                 initrd_size, loaderparams.kernel_cmdline);
+                 (long)initrd_size, loaderparams.kernel_cmdline);
     } else {
         snprintf((char *)params_buf + 8, 256, "%s", loaderparams.kernel_cmdline);
     }
@@ -161,7 +162,7 @@ void mips_r4k_init (ram_addr_t ram_size,
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *bios;
     MemoryRegion *iomem = g_new(MemoryRegion, 1);
-    int bios_size;
+    off_t bios_size;
     CPUState *env;
     ResetData *reset_info;
     int i;
@@ -214,14 +215,14 @@ void mips_r4k_init (ram_addr_t ram_size,
     if (filename) {
         bios_size = get_image_size(filename);
     } else {
-        bios_size = -1;
+        bios_size = (off_t)-1;
     }
 #ifdef TARGET_WORDS_BIGENDIAN
     be = 1;
 #else
     be = 0;
 #endif
-    if ((bios_size > 0) && (bios_size <= BIOS_SIZE)) {
+    if ((bios_size != -1) && (bios_size <= BIOS_SIZE)) {
         bios = g_new(MemoryRegion, 1);
         memory_region_init_ram(bios, "mips_r4k.bios", BIOS_SIZE);
         vmstate_register_ram_global(bios);
diff --git a/hw/multiboot.c b/hw/multiboot.c
index b4484a3..37e8af5 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -262,7 +262,7 @@ int load_multiboot(void *fw_cfg,
 
         do {
             char *next_space;
-            int mb_mod_length;
+            off_t mb_mod_length;
             uint32_t offs = mbs.mb_buf_size;
 
             next_initrd = (char *)get_opt_value(NULL, 0, initrd_filename);
@@ -275,7 +275,7 @@ int load_multiboot(void *fw_cfg,
                 *next_space = '\0';
             mb_debug("multiboot loading module: %s\n", initrd_filename);
             mb_mod_length = get_image_size(initrd_filename);
-            if (mb_mod_length < 0) {
+            if (mb_mod_length == -1) {
                 fprintf(stderr, "Failed to open file '%s'\n", initrd_filename);
                 exit(1);
             }
diff --git a/hw/palm.c b/hw/palm.c
index b1252ab..176ad4b 100644
--- a/hw/palm.c
+++ b/hw/palm.c
@@ -203,7 +203,8 @@ static void palmte_init(ram_addr_t ram_size,
     static uint32_t cs1val = 0x0000e1a0;
     static uint32_t cs2val = 0x0000e1a0;
     static uint32_t cs3val = 0xe1a0e1a0;
-    int rom_size, rom_loaded = 0;
+    int rom_loaded = 0;
+    off_t rom_size;
     DisplayState *ds = get_displaystate();
     MemoryRegion *flash = g_new(MemoryRegion, 1);
     MemoryRegion *cs = g_new(MemoryRegion, 4);
@@ -240,16 +241,16 @@ static void palmte_init(ram_addr_t ram_size,
     if (nb_option_roms) {
         rom_size = get_image_size(option_rom[0].name);
         if (rom_size > flash_size) {
-            fprintf(stderr, "%s: ROM image too big (%x > %x)\n",
-                            __FUNCTION__, rom_size, flash_size);
+            fprintf(stderr, "%s: ROM image too big (%lx > %x)\n",
+                    __FUNCTION__, (unsigned long)rom_size, flash_size);
             rom_size = 0;
         }
-        if (rom_size > 0) {
+        if (rom_size != -1) {
             rom_size = load_image_targphys(option_rom[0].name, OMAP_CS0_BASE,
                                            flash_size);
             rom_loaded = 1;
         }
-        if (rom_size < 0) {
+        if (rom_size == -1) {
             fprintf(stderr, "%s: error loading '%s'\n",
                             __FUNCTION__, option_rom[0].name);
         }
diff --git a/hw/pc.c b/hw/pc.c
index b9f4bc7..cb41955 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
                        target_phys_addr_t max_ram_size)
 {
     uint16_t protocol;
-    int setup_size, kernel_size, initrd_size = 0, cmdline_size;
+    int setup_size, kernel_size, cmdline_size;
+    off_t initrd_size = 0;
     uint32_t initrd_max;
     uint8_t header[8192], *setup, *kernel, *initrd_data;
     target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
@@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
 	}
 
 	initrd_size = get_image_size(initrd_filename);
-        if (initrd_size < 0) {
+        if (initrd_size == -1) {
             fprintf(stderr, "qemu: error reading initrd %s\n",
                     initrd_filename);
             exit(1);
diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index abf9004..7e2f715 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
@@ -132,7 +132,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
 {
     char *filename;
     MemoryRegion *bios, *isa_bios;
-    int bios_size, isa_bios_size;
+    off_t bios_size, isa_bios_size;
     int ret;
 
     /* BIOS load */
@@ -143,9 +143,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
     if (filename) {
         bios_size = get_image_size(filename);
     } else {
-        bios_size = -1;
+        bios_size = (off_t)-1;
     }
-    if (bios_size <= 0 ||
+    if ((bios_size == 0) || (bios_size == -1) ||
         (bios_size % 65536) != 0) {
         goto bios_error;
     }
diff --git a/hw/pci.c b/hw/pci.c
index bf046bf..5720a8c 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1672,7 +1672,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size)
 /* Add an option rom for the device */
 static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
 {
-    int size;
+    off_t size;
     char *path;
     void *ptr;
     char name[32];
@@ -1703,7 +1703,7 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
     }
 
     size = get_image_size(path);
-    if (size < 0) {
+    if (size == -1) {
         error_report("%s: failed to find romfile \"%s\"",
                      __FUNCTION__, pdev->romfile);
         g_free(path);
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index eb43fb5..c41e13c 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -489,7 +489,8 @@ static void ppc_prep_init (ram_addr_t ram_size,
 #if 0
     MemoryRegion *xcsr = g_new(MemoryRegion, 1);
 #endif
-    int linux_boot, i, nb_nics1, bios_size;
+    int linux_boot, i, nb_nics1;
+    off_t bios_size;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *bios = g_new(MemoryRegion, 1);
     uint32_t kernel_base, initrd_base;
@@ -546,13 +547,13 @@ static void ppc_prep_init (ram_addr_t ram_size,
     } else {
         bios_size = -1;
     }
-    if (bios_size > 0 && bios_size <= BIOS_SIZE) {
+    if (bios_size != -1 && bios_size <= BIOS_SIZE) {
         target_phys_addr_t bios_addr;
         bios_size = (bios_size + 0xfff) & ~0xfff;
         bios_addr = (uint32_t)(-bios_size);
         bios_size = load_image_targphys(filename, bios_addr, bios_size);
     }
-    if (bios_size < 0 || bios_size > BIOS_SIZE) {
+    if (bios_size == -1 || bios_size > BIOS_SIZE) {
         hw_error("qemu: could not load PPC PREP bios '%s'\n", bios_name);
     }
     if (filename) {
diff --git a/hw/smbios.c b/hw/smbios.c
index c57237d..85fad0e 100644
--- a/hw/smbios.c
+++ b/hw/smbios.c
@@ -185,7 +185,7 @@ int smbios_entry_add(const char *t)
     if (get_param_value(buf, sizeof(buf), "file", t)) {
         struct smbios_structure_header *header;
         struct smbios_table *table;
-        int size = get_image_size(buf);
+        off_t size = get_image_size(buf);
 
         if (size == -1 || size < sizeof(struct smbios_structure_header)) {
             fprintf(stderr, "Cannot read smbios file %s\n", buf);
-- 
1.7.9

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[Michael S. Tsirkin]

On Fri, Feb 24, 2012 at 11:36:45AM +1100, David Gibson wrote:
> Currently get_image_size(), used to find the size of files, returns an int.
> But for modern systems, int may be only 32-bit and we can have files
> larger than that.
> 
> This patch, therefore, changes the return type of get_image_size() to off_t
> (the same as the return type from lseek() itself).  It also audits all the
> callers of get_image_size() to make sure they process the new unsigned
> return type correctly.
> 
> This leaves load_image_targphys() with a limited return type, but one thing
> at a time (that function has far more callers to be audited, so it will
> take longer to fix).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Hmm, this can still be 32 bit.
We probably should set 

#define_FILE_OFFSET_BITS 64

before including any standard headers.
This will make it 64 bit consistently.

> ---
>  device_tree.c      |    4 ++--
>  hw/alpha_dp264.c   |    5 +++--
>  hw/highbank.c      |    2 +-
>  hw/leon3.c         |    6 +++---
>  hw/loader.c        |   12 +++++++-----
>  hw/loader.h        |    2 +-
>  hw/mips_fulong2e.c |   12 ++++++------
>  hw/mips_malta.c    |   12 ++++++------
>  hw/mips_mipssim.c  |    6 +++---
>  hw/mips_r4k.c      |   17 +++++++++--------
>  hw/multiboot.c     |    4 ++--
>  hw/palm.c          |   11 ++++++-----
>  hw/pc.c            |    5 +++--
>  hw/pc_sysfw.c      |    6 +++---
>  hw/pci.c           |    4 ++--
>  hw/ppc_prep.c      |    7 ++++---
>  hw/smbios.c        |    2 +-
>  17 files changed, 62 insertions(+), 55 deletions(-)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 86a694c..5817b2d 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -27,14 +27,14 @@
>  
>  void *load_device_tree(const char *filename_path, int *sizep)
>  {
> -    int dt_size;
> +    off_t dt_size;
>      int dt_file_load_size;
>      int ret;
>      void *fdt = NULL;
>  
>      *sizep = 0;
>      dt_size = get_image_size(filename_path);
> -    if (dt_size < 0) {
> +    if (dt_size == -1) {
>          printf("Unable to get size of device tree file '%s'\n",
>              filename_path);
>          goto fail;
> diff --git a/hw/alpha_dp264.c b/hw/alpha_dp264.c
> index ea0fd95..01ec9c7 100644
> --- a/hw/alpha_dp264.c
> +++ b/hw/alpha_dp264.c
> @@ -144,10 +144,11 @@ static void clipper_init(ram_addr_t ram_size,
>          }
>  
>          if (initrd_filename) {
> -            long initrd_base, initrd_size;
> +            long initrd_base;
> +            off_t initrd_size;
>  
>              initrd_size = get_image_size(initrd_filename);
> -            if (initrd_size < 0) {
> +            if (initrd_size != -1) {
>                  hw_error("could not load initial ram disk '%s'\n",
>                           initrd_filename);
>                  exit(1);
> diff --git a/hw/highbank.c b/hw/highbank.c
> index 489c00e..f4de4c6 100644
> --- a/hw/highbank.c
> +++ b/hw/highbank.c
> @@ -235,7 +235,7 @@ static void highbank_init(ram_addr_t ram_size,
>      if (bios_name != NULL) {
>          sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>          if (sysboot_filename != NULL) {
> -            uint32_t filesize = get_image_size(sysboot_filename);
> +            off_t filesize = get_image_size(sysboot_filename);
>              if (load_image_targphys("sysram.bin", 0xfff88000, filesize) < 0) {
>                  hw_error("Unable to load %s\n", bios_name);
>              }
> diff --git a/hw/leon3.c b/hw/leon3.c
> index 71d79a6..f7d4860 100644
> --- a/hw/leon3.c
> +++ b/hw/leon3.c
> @@ -108,7 +108,7 @@ static void leon3_generic_hw_init(ram_addr_t  ram_size,
>      int         ret;
>      char       *filename;
>      qemu_irq   *cpu_irqs = NULL;
> -    int         bios_size;
> +    off_t       bios_size;
>      int         prom_size;
>      ResetData  *reset_info;
>  
> @@ -168,7 +168,7 @@ static void leon3_generic_hw_init(ram_addr_t  ram_size,
>          exit(1);
>      }
>  
> -    if (bios_size > 0) {
> +    if (bios_size != -1) {
>          ret = load_image_targphys(filename, 0x00000000, bios_size);
>          if (ret < 0 || ret > prom_size) {
>              fprintf(stderr, "qemu: could not load prom '%s'\n", filename);
> @@ -191,7 +191,7 @@ static void leon3_generic_hw_init(ram_addr_t  ram_size,
>                      kernel_filename);
>              exit(1);
>          }
> -        if (bios_size <= 0) {
> +        if (bios_size == 0 || bios_size == -1) {
>              /* If there is no bios/monitor, start the application.  */
>              env->pc = entry;
>              env->npc = entry + 4;
> diff --git a/hw/loader.c b/hw/loader.c
> index 415cdce..372f6f8 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -57,12 +57,14 @@
>  static int roms_loaded;
>  
>  /* return the size or -1 if error */
> -int get_image_size(const char *filename)
> +off_t get_image_size(const char *filename)
>  {
> -    int fd, size;
> +    int fd;
> +    off_t size;
> +
>      fd = open(filename, O_RDONLY | O_BINARY);
>      if (fd < 0)
> -        return -1;
> +        return (off_t)-1;
>      size = lseek(fd, 0, SEEK_END);
>      close(fd);
>      return size;
> @@ -105,13 +107,13 @@ ssize_t read_targphys(const char *name,
>  int load_image_targphys(const char *filename,
>  			target_phys_addr_t addr, int max_sz)
>  {
> -    int size;
> +    off_t size;
>  
>      size = get_image_size(filename);
>      if (size > max_sz) {
>          return -1;
>      }
> -    if (size > 0) {
> +    if (size != -1) {
>          rom_add_file_fixed(filename, addr, -1);
>      }
>      return size;
> diff --git a/hw/loader.h b/hw/loader.h
> index fbcaba9..1fbdbcf 100644
> --- a/hw/loader.h
> +++ b/hw/loader.h
> @@ -2,7 +2,7 @@
>  #define LOADER_H
>  
>  /* loader.c */
> -int get_image_size(const char *filename);
> +off_t get_image_size(const char *filename);
>  int load_image(const char *filename, uint8_t *addr); /* deprecated */
>  int load_image_targphys(const char *filename, target_phys_addr_t, int max_sz);
>  int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
> index e3ba9dd..1e7b49a 100644
> --- a/hw/mips_fulong2e.c
> +++ b/hw/mips_fulong2e.c
> @@ -107,7 +107,7 @@ static int64_t load_kernel (CPUState *env)
>  {
>      int64_t kernel_entry, kernel_low, kernel_high;
>      int index = 0;
> -    long initrd_size;
> +    off_t initrd_size;
>      ram_addr_t initrd_offset;
>      uint32_t *prom_buf;
>      long prom_size;
> @@ -125,7 +125,7 @@ static int64_t load_kernel (CPUState *env)
>      initrd_offset = 0;
>      if (loaderparams.initrd_filename) {
>          initrd_size = get_image_size (loaderparams.initrd_filename);
> -        if (initrd_size > 0) {
> +        if (initrd_size != -1) {
>              initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
>              if (initrd_offset + initrd_size > ram_size) {
>                  fprintf(stderr,
> @@ -136,7 +136,7 @@ static int64_t load_kernel (CPUState *env)
>              initrd_size = load_image_targphys(loaderparams.initrd_filename,
>                                       initrd_offset, ram_size - initrd_offset);
>          }
> -        if (initrd_size == (target_ulong) -1) {
> +        if (initrd_size == -1) {
>              fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
>                      loaderparams.initrd_filename);
>              exit(1);
> @@ -148,10 +148,10 @@ static int64_t load_kernel (CPUState *env)
>      prom_buf = g_malloc(prom_size);
>  
>      prom_set(prom_buf, index++, "%s", loaderparams.kernel_filename);
> -    if (initrd_size > 0) {
> +    if (initrd_size != -1) {
>          prom_set(prom_buf, index++, "rd_start=0x%" PRIx64 " rd_size=%li %s",
> -                 cpu_mips_phys_to_kseg0(NULL, initrd_offset), initrd_size,
> -                 loaderparams.kernel_cmdline);
> +                 cpu_mips_phys_to_kseg0(NULL, initrd_offset),
> +                 (long)initrd_size, loaderparams.kernel_cmdline);
>      } else {
>          prom_set(prom_buf, index++, "%s", loaderparams.kernel_cmdline);
>      }
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index 86a5fbb..50e800a 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -667,7 +667,7 @@ static void GCC_FMT_ATTR(3, 4) prom_set(uint32_t* prom_buf, int index,
>  static int64_t load_kernel (void)
>  {
>      int64_t kernel_entry, kernel_high;
> -    long initrd_size;
> +    off_t initrd_size;
>      ram_addr_t initrd_offset;
>      int big_endian;
>      uint32_t *prom_buf;
> @@ -693,7 +693,7 @@ static int64_t load_kernel (void)
>      initrd_offset = 0;
>      if (loaderparams.initrd_filename) {
>          initrd_size = get_image_size (loaderparams.initrd_filename);
> -        if (initrd_size > 0) {
> +        if (initrd_size != -1) {
>              initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
>              if (initrd_offset + initrd_size > ram_size) {
>                  fprintf(stderr,
> @@ -705,7 +705,7 @@ static int64_t load_kernel (void)
>                                                initrd_offset,
>                                                ram_size - initrd_offset);
>          }
> -        if (initrd_size == (target_ulong) -1) {
> +        if (initrd_size == -1) {
>              fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
>                      loaderparams.initrd_filename);
>              exit(1);
> @@ -717,10 +717,10 @@ static int64_t load_kernel (void)
>      prom_buf = g_malloc(prom_size);
>  
>      prom_set(prom_buf, prom_index++, "%s", loaderparams.kernel_filename);
> -    if (initrd_size > 0) {
> +    if (initrd_size != -1) {
>          prom_set(prom_buf, prom_index++, "rd_start=0x%" PRIx64 " rd_size=%li %s",
> -                 cpu_mips_phys_to_kseg0(NULL, initrd_offset), initrd_size,
> -                 loaderparams.kernel_cmdline);
> +                 cpu_mips_phys_to_kseg0(NULL, initrd_offset),
> +                 (long)initrd_size, loaderparams.kernel_cmdline);
>      } else {
>          prom_set(prom_buf, prom_index++, "%s", loaderparams.kernel_cmdline);
>      }
> diff --git a/hw/mips_mipssim.c b/hw/mips_mipssim.c
> index 76c95b2..89de6ca 100644
> --- a/hw/mips_mipssim.c
> +++ b/hw/mips_mipssim.c
> @@ -54,7 +54,7 @@ static int64_t load_kernel(void)
>  {
>      int64_t entry, kernel_high;
>      long kernel_size;
> -    long initrd_size;
> +    off_t initrd_size;
>      ram_addr_t initrd_offset;
>      int big_endian;
>  
> @@ -82,7 +82,7 @@ static int64_t load_kernel(void)
>      initrd_offset = 0;
>      if (loaderparams.initrd_filename) {
>          initrd_size = get_image_size (loaderparams.initrd_filename);
> -        if (initrd_size > 0) {
> +        if (initrd_size != -1) {
>              initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
>              if (initrd_offset + initrd_size > loaderparams.ram_size) {
>                  fprintf(stderr,
> @@ -93,7 +93,7 @@ static int64_t load_kernel(void)
>              initrd_size = load_image_targphys(loaderparams.initrd_filename,
>                  initrd_offset, loaderparams.ram_size - initrd_offset);
>          }
> -        if (initrd_size == (target_ulong) -1) {
> +        if (initrd_size == -1) {
>              fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
>                      loaderparams.initrd_filename);
>              exit(1);
> diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
> index 83401f0..067a793 100644
> --- a/hw/mips_r4k.c
> +++ b/hw/mips_r4k.c
> @@ -72,7 +72,8 @@ typedef struct ResetData {
>  static int64_t load_kernel(void)
>  {
>      int64_t entry, kernel_high;
> -    long kernel_size, initrd_size, params_size;
> +    long kernel_size, params_size;
> +    off_t initrd_size;
>      ram_addr_t initrd_offset;
>      uint32_t *params_buf;
>      int big_endian;
> @@ -100,7 +101,7 @@ static int64_t load_kernel(void)
>      initrd_offset = 0;
>      if (loaderparams.initrd_filename) {
>          initrd_size = get_image_size (loaderparams.initrd_filename);
> -        if (initrd_size > 0) {
> +        if (initrd_size != -1) {
>              initrd_offset = (kernel_high + ~TARGET_PAGE_MASK) & TARGET_PAGE_MASK;
>              if (initrd_offset + initrd_size > ram_size) {
>                  fprintf(stderr,
> @@ -112,7 +113,7 @@ static int64_t load_kernel(void)
>                                                initrd_offset,
>                                                ram_size - initrd_offset);
>          }
> -        if (initrd_size == (target_ulong) -1) {
> +        if (initrd_size == -1) {
>              fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
>                      loaderparams.initrd_filename);
>              exit(1);
> @@ -126,10 +127,10 @@ static int64_t load_kernel(void)
>      params_buf[0] = tswap32(ram_size);
>      params_buf[1] = tswap32(0x12345678);
>  
> -    if (initrd_size > 0) {
> +    if (initrd_size != -1) {
>          snprintf((char *)params_buf + 8, 256, "rd_start=0x%" PRIx64 " rd_size=%li %s",
>                   cpu_mips_phys_to_kseg0(NULL, initrd_offset),
> -                 initrd_size, loaderparams.kernel_cmdline);
> +                 (long)initrd_size, loaderparams.kernel_cmdline);
>      } else {
>          snprintf((char *)params_buf + 8, 256, "%s", loaderparams.kernel_cmdline);
>      }
> @@ -161,7 +162,7 @@ void mips_r4k_init (ram_addr_t ram_size,
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      MemoryRegion *bios;
>      MemoryRegion *iomem = g_new(MemoryRegion, 1);
> -    int bios_size;
> +    off_t bios_size;
>      CPUState *env;
>      ResetData *reset_info;
>      int i;
> @@ -214,14 +215,14 @@ void mips_r4k_init (ram_addr_t ram_size,
>      if (filename) {
>          bios_size = get_image_size(filename);
>      } else {
> -        bios_size = -1;
> +        bios_size = (off_t)-1;
>      }
>  #ifdef TARGET_WORDS_BIGENDIAN
>      be = 1;
>  #else
>      be = 0;
>  #endif
> -    if ((bios_size > 0) && (bios_size <= BIOS_SIZE)) {
> +    if ((bios_size != -1) && (bios_size <= BIOS_SIZE)) {
>          bios = g_new(MemoryRegion, 1);
>          memory_region_init_ram(bios, "mips_r4k.bios", BIOS_SIZE);
>          vmstate_register_ram_global(bios);
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index b4484a3..37e8af5 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -262,7 +262,7 @@ int load_multiboot(void *fw_cfg,
>  
>          do {
>              char *next_space;
> -            int mb_mod_length;
> +            off_t mb_mod_length;
>              uint32_t offs = mbs.mb_buf_size;
>  
>              next_initrd = (char *)get_opt_value(NULL, 0, initrd_filename);
> @@ -275,7 +275,7 @@ int load_multiboot(void *fw_cfg,
>                  *next_space = '\0';
>              mb_debug("multiboot loading module: %s\n", initrd_filename);
>              mb_mod_length = get_image_size(initrd_filename);
> -            if (mb_mod_length < 0) {
> +            if (mb_mod_length == -1) {
>                  fprintf(stderr, "Failed to open file '%s'\n", initrd_filename);
>                  exit(1);
>              }
> diff --git a/hw/palm.c b/hw/palm.c
> index b1252ab..176ad4b 100644
> --- a/hw/palm.c
> +++ b/hw/palm.c
> @@ -203,7 +203,8 @@ static void palmte_init(ram_addr_t ram_size,
>      static uint32_t cs1val = 0x0000e1a0;
>      static uint32_t cs2val = 0x0000e1a0;
>      static uint32_t cs3val = 0xe1a0e1a0;
> -    int rom_size, rom_loaded = 0;
> +    int rom_loaded = 0;
> +    off_t rom_size;
>      DisplayState *ds = get_displaystate();
>      MemoryRegion *flash = g_new(MemoryRegion, 1);
>      MemoryRegion *cs = g_new(MemoryRegion, 4);
> @@ -240,16 +241,16 @@ static void palmte_init(ram_addr_t ram_size,
>      if (nb_option_roms) {
>          rom_size = get_image_size(option_rom[0].name);
>          if (rom_size > flash_size) {
> -            fprintf(stderr, "%s: ROM image too big (%x > %x)\n",
> -                            __FUNCTION__, rom_size, flash_size);
> +            fprintf(stderr, "%s: ROM image too big (%lx > %x)\n",
> +                    __FUNCTION__, (unsigned long)rom_size, flash_size);
>              rom_size = 0;
>          }
> -        if (rom_size > 0) {
> +        if (rom_size != -1) {
>              rom_size = load_image_targphys(option_rom[0].name, OMAP_CS0_BASE,
>                                             flash_size);
>              rom_loaded = 1;
>          }
> -        if (rom_size < 0) {
> +        if (rom_size == -1) {
>              fprintf(stderr, "%s: error loading '%s'\n",
>                              __FUNCTION__, option_rom[0].name);
>          }
> diff --git a/hw/pc.c b/hw/pc.c
> index b9f4bc7..cb41955 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
>                         target_phys_addr_t max_ram_size)
>  {
>      uint16_t protocol;
> -    int setup_size, kernel_size, initrd_size = 0, cmdline_size;
> +    int setup_size, kernel_size, cmdline_size;
> +    off_t initrd_size = 0;
>      uint32_t initrd_max;
>      uint8_t header[8192], *setup, *kernel, *initrd_data;
>      target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
>  	}
>  
>  	initrd_size = get_image_size(initrd_filename);
> -        if (initrd_size < 0) {
> +        if (initrd_size == -1) {
>              fprintf(stderr, "qemu: error reading initrd %s\n",
>                      initrd_filename);
>              exit(1);
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index abf9004..7e2f715 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> @@ -132,7 +132,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>  {
>      char *filename;
>      MemoryRegion *bios, *isa_bios;
> -    int bios_size, isa_bios_size;
> +    off_t bios_size, isa_bios_size;
>      int ret;
>  
>      /* BIOS load */
> @@ -143,9 +143,9 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory)
>      if (filename) {
>          bios_size = get_image_size(filename);
>      } else {
> -        bios_size = -1;
> +        bios_size = (off_t)-1;
>      }
> -    if (bios_size <= 0 ||
> +    if ((bios_size == 0) || (bios_size == -1) ||
>          (bios_size % 65536) != 0) {
>          goto bios_error;
>      }
> diff --git a/hw/pci.c b/hw/pci.c
> index bf046bf..5720a8c 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1672,7 +1672,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, int size)
>  /* Add an option rom for the device */
>  static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>  {
> -    int size;
> +    off_t size;
>      char *path;
>      void *ptr;
>      char name[32];
> @@ -1703,7 +1703,7 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>      }
>  
>      size = get_image_size(path);
> -    if (size < 0) {
> +    if (size == -1) {
>          error_report("%s: failed to find romfile \"%s\"",
>                       __FUNCTION__, pdev->romfile);
>          g_free(path);
> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
> index eb43fb5..c41e13c 100644
> --- a/hw/ppc_prep.c
> +++ b/hw/ppc_prep.c
> @@ -489,7 +489,8 @@ static void ppc_prep_init (ram_addr_t ram_size,
>  #if 0
>      MemoryRegion *xcsr = g_new(MemoryRegion, 1);
>  #endif
> -    int linux_boot, i, nb_nics1, bios_size;
> +    int linux_boot, i, nb_nics1;
> +    off_t bios_size;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      MemoryRegion *bios = g_new(MemoryRegion, 1);
>      uint32_t kernel_base, initrd_base;
> @@ -546,13 +547,13 @@ static void ppc_prep_init (ram_addr_t ram_size,
>      } else {
>          bios_size = -1;
>      }
> -    if (bios_size > 0 && bios_size <= BIOS_SIZE) {
> +    if (bios_size != -1 && bios_size <= BIOS_SIZE) {
>          target_phys_addr_t bios_addr;
>          bios_size = (bios_size + 0xfff) & ~0xfff;
>          bios_addr = (uint32_t)(-bios_size);
>          bios_size = load_image_targphys(filename, bios_addr, bios_size);
>      }
> -    if (bios_size < 0 || bios_size > BIOS_SIZE) {
> +    if (bios_size == -1 || bios_size > BIOS_SIZE) {
>          hw_error("qemu: could not load PPC PREP bios '%s'\n", bios_name);
>      }
>      if (filename) {
> diff --git a/hw/smbios.c b/hw/smbios.c
> index c57237d..85fad0e 100644
> --- a/hw/smbios.c
> +++ b/hw/smbios.c
> @@ -185,7 +185,7 @@ int smbios_entry_add(const char *t)
>      if (get_param_value(buf, sizeof(buf), "file", t)) {
>          struct smbios_structure_header *header;
>          struct smbios_table *table;
> -        int size = get_image_size(buf);
> +        off_t size = get_image_size(buf);
>  
>          if (size == -1 || size < sizeof(struct smbios_structure_header)) {
>              fprintf(stderr, "Cannot read smbios file %s\n", buf);
> -- 
> 1.7.9
> 

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[Andreas Färber]

Am 24.02.2012 01:36, schrieb David Gibson:
> Currently get_image_size(), used to find the size of files, returns an int.
> But for modern systems, int may be only 32-bit and we can have files
> larger than that.
> 
> This patch, therefore, changes the return type of get_image_size() to off_t
> (the same as the return type from lseek() itself).  It also audits all the
> callers of get_image_size() to make sure they process the new unsigned
> return type correctly.

It's a size, so why off_t and not size_t?

Andreas

> 
> This leaves load_image_targphys() with a limited return type, but one thing
> at a time (that function has far more callers to be audited, so it will
> take longer to fix).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[David Gibson]

On Fri, Feb 24, 2012 at 10:15:51AM +0100, Andreas Färber wrote:
> Am 24.02.2012 01:36, schrieb David Gibson:
> > Currently get_image_size(), used to find the size of files, returns an int.
> > But for modern systems, int may be only 32-bit and we can have files
> > larger than that.
> > 
> > This patch, therefore, changes the return type of get_image_size() to off_t
> > (the same as the return type from lseek() itself).  It also audits all the
> > callers of get_image_size() to make sure they process the new unsigned
> > return type correctly.
> 
> It's a size, so why off_t and not size_t?

Because it's a file size, rather than an in-memory size.  Or more
specifically, off_t is the return type of lseek() which is where the
information is coming from in the first place.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> Currently get_image_size(), used to find the size of files, returns an int.
> But for modern systems, int may be only 32-bit and we can have files
> larger than that.
>
> This patch, therefore, changes the return type of get_image_size() to off_t
> (the same as the return type from lseek() itself).  It also audits all the
> callers of get_image_size() to make sure they process the new unsigned
> return type correctly.
>
> This leaves load_image_targphys() with a limited return type, but one thing
> at a time (that function has far more callers to be audited, so it will
> take longer to fix).

I'm afraid this replaces the single, well-known integer overflow in
get_image_size()'s conversion of lseek() value to int by many unknown
overflows in get_image_size()'s users.  One example below.  Didn't look
for more.

If you need a wider get_image_size(), please make sure its users are
prepared for it!

Is the any use for image sizes exceeding size_t?  Arent such images
impossible to load?

[...]
> diff --git a/hw/pc.c b/hw/pc.c
> index b9f4bc7..cb41955 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
>                         target_phys_addr_t max_ram_size)
>  {
>      uint16_t protocol;
> -    int setup_size, kernel_size, initrd_size = 0, cmdline_size;
> +    int setup_size, kernel_size, cmdline_size;
> +    off_t initrd_size = 0;
>      uint32_t initrd_max;
>      uint8_t header[8192], *setup, *kernel, *initrd_data;
>      target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
>  	}
>  
>  	initrd_size = get_image_size(initrd_filename);
> -        if (initrd_size < 0) {
> +        if (initrd_size == -1) {

Needless churn.

>              fprintf(stderr, "qemu: error reading initrd %s\n",
>                      initrd_filename);
>              exit(1);
           }

           initrd_addr = (initrd_max-initrd_size) & ~4095;

           initrd_data = g_malloc(initrd_size);

Integer overflow in conversion from off_t initrd_size to the argument
type size_t[*].

           load_image(initrd_filename, initrd_data);

You could try replacing load_image() by a function that returns a
pointer to the malloced image contents.

[...]


[*] Technically some glib-defined type, because they're into making up
their own "portable" types for everything.

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[David Gibson]

On Mon, Feb 27, 2012 at 09:21:25AM +0100, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > Currently get_image_size(), used to find the size of files, returns an int.
> > But for modern systems, int may be only 32-bit and we can have files
> > larger than that.
> >
> > This patch, therefore, changes the return type of get_image_size() to off_t
> > (the same as the return type from lseek() itself).  It also audits all the
> > callers of get_image_size() to make sure they process the new unsigned
> > return type correctly.
> >
> > This leaves load_image_targphys() with a limited return type, but one thing
> > at a time (that function has far more callers to be audited, so it will
> > take longer to fix).
> 
> I'm afraid this replaces the single, well-known integer overflow in
> get_image_size()'s conversion of lseek() value to int by many unknown
> overflows in get_image_size()'s users.  One example below.  Didn't look
> for more.
> 
> If you need a wider get_image_size(), please make sure its users are
> prepared for it!

Actually, I have no such need at all, but when I fixed another bug in
loader.c, someone whinged about me not changing get_image_size(), so
here it is.

> Is the any use for image sizes exceeding size_t?  Arent such images
> impossible to load?

Well, possibly not.

> 
> [...]
> > diff --git a/hw/pc.c b/hw/pc.c
> > index b9f4bc7..cb41955 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
> >                         target_phys_addr_t max_ram_size)
> >  {
> >      uint16_t protocol;
> > -    int setup_size, kernel_size, initrd_size = 0, cmdline_size;
> > +    int setup_size, kernel_size, cmdline_size;
> > +    off_t initrd_size = 0;
> >      uint32_t initrd_max;
> >      uint8_t header[8192], *setup, *kernel, *initrd_data;
> >      target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> > @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
> >  	}
> >  
> >  	initrd_size = get_image_size(initrd_filename);
> > -        if (initrd_size < 0) {
> > +        if (initrd_size == -1) {
> 
> Needless churn.

No, it's not.  Now that initrd_size is unsigned initrd_size < 0 would
return false always (and give a "comparison is always false due to
limited range of data type" warning).

> 
> >              fprintf(stderr, "qemu: error reading initrd %s\n",
> >                      initrd_filename);
> >              exit(1);
>            }
> 
>            initrd_addr = (initrd_max-initrd_size) & ~4095;
> 
>            initrd_data = g_malloc(initrd_size);
> 
> Integer overflow in conversion from off_t initrd_size to the argument
> type size_t[*].

Hm, true.

Ok, well, I give up.  Someone who actually needs it can fix it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Mon, Feb 27, 2012 at 09:21:25AM +0100, Markus Armbruster wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > Currently get_image_size(), used to find the size of files, returns an int.
>> > But for modern systems, int may be only 32-bit and we can have files
>> > larger than that.
>> >
>> > This patch, therefore, changes the return type of get_image_size() to off_t
>> > (the same as the return type from lseek() itself).  It also audits all the
>> > callers of get_image_size() to make sure they process the new unsigned
>> > return type correctly.
>> >
>> > This leaves load_image_targphys() with a limited return type, but one thing
>> > at a time (that function has far more callers to be audited, so it will
>> > take longer to fix).
>> 
>> I'm afraid this replaces the single, well-known integer overflow in
>> get_image_size()'s conversion of lseek() value to int by many unknown
>> overflows in get_image_size()'s users.  One example below.  Didn't look
>> for more.
>> 
>> If you need a wider get_image_size(), please make sure its users are
>> prepared for it!
>
> Actually, I have no such need at all, but when I fixed another bug in
> loader.c, someone whinged about me not changing get_image_size(), so
> here it is.

Heh.

>> Is the any use for image sizes exceeding size_t?  Arent such images
>> impossible to load?
>
> Well, possibly not.
>
>> 
>> [...]
>> > diff --git a/hw/pc.c b/hw/pc.c
>> > index b9f4bc7..cb41955 100644
>> > --- a/hw/pc.c
>> > +++ b/hw/pc.c
>> > @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
>> >                         target_phys_addr_t max_ram_size)
>> >  {
>> >      uint16_t protocol;
>> > -    int setup_size, kernel_size, initrd_size = 0, cmdline_size;
>> > +    int setup_size, kernel_size, cmdline_size;
>> > +    off_t initrd_size = 0;
>> >      uint32_t initrd_max;
>> >      uint8_t header[8192], *setup, *kernel, *initrd_data;
>> >      target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
>> > @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
>> >  	}
>> >  
>> >  	initrd_size = get_image_size(initrd_filename);
>> > -        if (initrd_size < 0) {
>> > +        if (initrd_size == -1) {
>> 
>> Needless churn.
>
> No, it's not.  Now that initrd_size is unsigned initrd_size < 0 would
> return false always (and give a "comparison is always false due to
> limited range of data type" warning).

off_t is signed:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html

>> >              fprintf(stderr, "qemu: error reading initrd %s\n",
>> >                      initrd_filename);
>> >              exit(1);
>>            }
>> 
>>            initrd_addr = (initrd_max-initrd_size) & ~4095;
>> 
>>            initrd_data = g_malloc(initrd_size);
>> 
>> Integer overflow in conversion from off_t initrd_size to the argument
>> type size_t[*].
>
> Hm, true.
>
> Ok, well, I give up.  Someone who actually needs it can fix it.

Pity.  Thanks anyway!

[Qemu-devel] [PATCH 3/3] .gitignore update

[David Gibson]

This adds a few previously missing generated files to .gitignore: the
qemu-bridge-helper binary, and the linuxboot and multiboot images from
pc-bios/optionrom.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 .gitignore                   |    1 +
 pc-bios/optionrom/.gitignore |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)
 create mode 100644 pc-bios/optionrom/.gitignore

diff --git a/.gitignore b/.gitignore
index c72955a..f74e95b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -39,6 +39,7 @@ qemu-img-cmds.texi
 qemu-img-cmds.h
 qemu-io
 qemu-ga
+qemu-bridge-helper
 qemu-monitor.texi
 QMP/qmp-commands.txt
 test-coroutine
diff --git a/pc-bios/optionrom/.gitignore b/pc-bios/optionrom/.gitignore
new file mode 100644
index 0000000..c97a411
--- /dev/null
+++ b/pc-bios/optionrom/.gitignore
@@ -0,0 +1,3 @@
+linuxboot.img
+linuxboot.raw
+multiboot.img
-- 
1.7.9