[Qemu-devel] [PATCH v2 0/5] support loading initrd below 4G for recent kernel

[Qemu-devel] [PATCH v2 0/5] support loading initrd below 4G for recent kernel

[Li Zhijian]

Long long ago, linux kernel have supported up to 4G initrd, but it's header
still hard code to allow loading initrd below 2G only.
 cutting from arch/x86/head.S:
 # (Header version 0x0203 or later) the highest safe address for the contents
 # of an initrd. The current kernel allows up to 4 GB, but leave it at 2 GB to
 # avoid possible bootloader bugs.

In order to supporting more than 2G initrd, qemu must allow loading initrd
above 2G address. Luckly, recent kernel introduced a new field to linux header
named xloadflags:XLF_CAN_BE_LOADED_ABOVE_4G which tells bootload an optional
and safe address to load initrd.

Current QEMU always load initrd below below_4g_mem_size which always
less than 4G, so here limit initrd_max to 4G - 1 simply is enough if
this bit is set.

Default roms(Seabios + optionrom(linuxboot_dma)) works as expected with those
patch set.

Patch 1/5 make a huge changes to address/memory APIs, but if it's not a good
idea, below is an specfic patch to avoid dma overflow

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 946f765..5c9607c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -338,7 +338,7 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
     while (dma.length > 0 && !(dma.control & FW_CFG_DMA_CTL_ERROR)) {
         if (s->cur_entry == FW_CFG_INVALID || !e->data ||
                                 s->cur_offset >= e->len) {
-            len = dma.length;
+            len = dma.length > INT32_MAX ? INT32_MAX : dma.length;
 
             /* If the access is not a read access, it will be a skip access,
              * tested before.
@@ -358,6 +358,8 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
                 len = (e->len - s->cur_offset);
             }
 
+            len = len > INT32_MAX ? INT32_MAX : len;
+
             /* If the access is not a read access, it will be a skip access,
              * tested before.
              */

changes:
V2: add 2 patches(3/5, 4/5) to fix potential loading issue.

Li Zhijian (5):
  unify len and addr type for memory/address APIs
  change load_image() reture type to ssize_t
  refactor load_image/load_image_size
  x86: exit qemu if load_image fails
  x86: allow load initrd below 4G for recent linux

 exec.c                    | 49 ++++++++++++++++++++++++-----------------------
 hw/core/loader.c          | 27 +++++++++++++++-----------
 hw/i386/pc.c              | 17 +++++++++++++++-
 include/exec/cpu-all.h    |  2 +-
 include/exec/cpu-common.h | 10 +++++-----
 include/exec/memory.h     | 20 +++++++++----------
 include/hw/loader.h       |  2 +-
 7 files changed, 74 insertions(+), 53 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/5] unify len and addr type for memory/address APIs

[Li Zhijian]

Some address/memory APIs have different type between 'hwaddr addr' and
'int len'. It is very unsafety, espcially some APIs will be passed a non-int
len by caller which might cause overflow quietly.
Below is an potential overflow case:
    dma_memory_read(uint32_t len)
      -> dma_memory_rw(uint32_t len)
        -> dma_memory_rw_relaxed(uint32_t len)
          -> address_space_rw(int len) # len overflow

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Peter Crosthwaite <crosthwaite.peter@gmail.com>
CC: Richard Henderson <rth@twiddle.net>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 exec.c                    | 49 ++++++++++++++++++++++++-----------------------
 include/exec/cpu-all.h    |  2 +-
 include/exec/cpu-common.h | 10 +++++-----
 include/exec/memory.h     | 20 +++++++++----------
 4 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/exec.c b/exec.c
index bb6170d..05823ae 100644
--- a/exec.c
+++ b/exec.c
@@ -2719,7 +2719,8 @@ static const MemoryRegionOps notdirty_mem_ops = {
 };
 
 /* Generate a debug exception if a watchpoint has been hit.  */
-static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
+static void check_watchpoint(hwaddr offset,  unsigned len,
+                             MemTxAttrs attrs, int flags)
 {
     CPUState *cpu = current_cpu;
     CPUClass *cc = CPU_GET_CLASS(cpu);
@@ -2848,10 +2849,10 @@ static const MemoryRegionOps watch_mem_ops = {
 };
 
 static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
-                                      MemTxAttrs attrs, uint8_t *buf, int len);
+                                      MemTxAttrs attrs, uint8_t *buf, hwaddr len);
 static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
-                                  const uint8_t *buf, int len);
-static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
+                                  const uint8_t *buf, hwaddr len);
+static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
                                   bool is_write, MemTxAttrs attrs);
 
 static MemTxResult subpage_read(void *opaque, hwaddr addr, uint64_t *data,
@@ -3099,9 +3100,10 @@ MemoryRegion *get_system_io(void)
 /* physical memory access (slow version, mainly for debug) */
 #if defined(CONFIG_USER_ONLY)
 int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
-                        uint8_t *buf, int len, int is_write)
+                        uint8_t *buf, hwaddr len, int is_write)
 {
-    int l, flags;
+    hwaddr l;
+    int flags;
     target_ulong page;
     void * p;
 
@@ -3215,7 +3217,7 @@ static bool prepare_mmio_access(MemoryRegion *mr)
 static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
                                            MemTxAttrs attrs,
                                            const uint8_t *buf,
-                                           int len, hwaddr addr1,
+                                           hwaddr len, hwaddr addr1,
                                            hwaddr l, MemoryRegion *mr)
 {
     uint8_t *ptr;
@@ -3260,7 +3262,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
 
 /* Called from RCU critical section.  */
 static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
-                                  const uint8_t *buf, int len)
+                                  const uint8_t *buf, hwaddr len)
 {
     hwaddr l;
     hwaddr addr1;
@@ -3278,7 +3280,7 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
 /* Called within RCU critical section.  */
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    MemTxAttrs attrs, uint8_t *buf,
-                                   int len, hwaddr addr1, hwaddr l,
+                                   hwaddr len, hwaddr addr1, hwaddr l,
                                    MemoryRegion *mr)
 {
     uint8_t *ptr;
@@ -3321,7 +3323,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
 
 /* Called from RCU critical section.  */
 static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
-                                 MemTxAttrs attrs, uint8_t *buf, int len)
+                                 MemTxAttrs attrs, uint8_t *buf, hwaddr len)
 {
     hwaddr l;
     hwaddr addr1;
@@ -3334,7 +3336,7 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
 }
 
 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
-                                    MemTxAttrs attrs, uint8_t *buf, int len)
+                                    MemTxAttrs attrs, uint8_t *buf, hwaddr len)
 {
     MemTxResult result = MEMTX_OK;
     FlatView *fv;
@@ -3351,7 +3353,7 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
 
 MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs,
-                                const uint8_t *buf, int len)
+                                const uint8_t *buf, hwaddr len)
 {
     MemTxResult result = MEMTX_OK;
     FlatView *fv;
@@ -3367,7 +3369,7 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
 }
 
 MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
-                             uint8_t *buf, int len, bool is_write)
+                             uint8_t *buf, hwaddr len, bool is_write)
 {
     if (is_write) {
         return address_space_write(as, addr, attrs, buf, len);
@@ -3377,7 +3379,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
 }
 
 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
-                            int len, int is_write)
+                            hwaddr len, int is_write)
 {
     address_space_rw(&address_space_memory, addr, MEMTXATTRS_UNSPECIFIED,
                      buf, len, is_write);
@@ -3389,7 +3391,7 @@ enum write_rom_type {
 };
 
 static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
-    hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
+    hwaddr addr, const uint8_t *buf, hwaddr len, enum write_rom_type type)
 {
     hwaddr l;
     uint8_t *ptr;
@@ -3427,12 +3429,12 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
 
 /* used for ROM loading : can write in RAM and ROM */
 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len)
+                                   const uint8_t *buf, hwaddr len)
 {
     cpu_physical_memory_write_rom_internal(as, addr, buf, len, WRITE_DATA);
 }
 
-void cpu_flush_icache_range(hwaddr start, int len)
+void cpu_flush_icache_range(hwaddr start, hwaddr len)
 {
     /*
      * This function should do the same thing as an icache flush that was
@@ -3534,7 +3536,7 @@ static void cpu_notify_map_clients(void)
     qemu_mutex_unlock(&map_client_list_lock);
 }
 
-static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
+static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
                                   bool is_write, MemTxAttrs attrs)
 {
     MemoryRegion *mr;
@@ -3557,7 +3559,7 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
 }
 
 bool address_space_access_valid(AddressSpace *as, hwaddr addr,
-                                int len, bool is_write,
+                                hwaddr len, bool is_write,
                                 MemTxAttrs attrs)
 {
     FlatView *fv;
@@ -3810,7 +3812,7 @@ static inline MemoryRegion *address_space_translate_cached(
  */
 void
 address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
-                                   void *buf, int len)
+                                   void *buf, hwaddr len)
 {
     hwaddr addr1, l;
     MemoryRegion *mr;
@@ -3828,7 +3830,7 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
  */
 void
 address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
-                                    const void *buf, int len)
+                                    const void *buf, hwaddr len)
 {
     hwaddr addr1, l;
     MemoryRegion *mr;
@@ -3851,10 +3853,9 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
 
 /* virtual memory access for debug (includes writing to ROM) */
 int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
-                        uint8_t *buf, int len, int is_write)
+                        uint8_t *buf, hwaddr len, int is_write)
 {
-    int l;
-    hwaddr phys_addr;
+    hwaddr l, phys_addr;
     target_ulong page;
 
     cpu_synchronize_state(cpu);
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 117d2fb..4b56672 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -367,7 +367,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
 #endif /* !CONFIG_USER_ONLY */
 
 int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
-                        uint8_t *buf, int len, int is_write);
+                        uint8_t *buf, hwaddr len, int is_write);
 
 int cpu_exec(CPUState *cpu);
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 18b40d6..44b3554 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -83,14 +83,14 @@ size_t qemu_ram_pagesize(RAMBlock *block);
 size_t qemu_ram_pagesize_largest(void);
 
 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
-                            int len, int is_write);
+                            hwaddr len, int is_write);
 static inline void cpu_physical_memory_read(hwaddr addr,
-                                            void *buf, int len)
+                                            void *buf, hwaddr len)
 {
     cpu_physical_memory_rw(addr, buf, len, 0);
 }
 static inline void cpu_physical_memory_write(hwaddr addr,
-                                             const void *buf, int len)
+                                             const void *buf, hwaddr len)
 {
     cpu_physical_memory_rw(addr, (void *)buf, len, 1);
 }
@@ -112,8 +112,8 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr);
 void qemu_flush_coalesced_mmio_buffer(void);
 
 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len);
-void cpu_flush_icache_range(hwaddr start, int len);
+                                   const uint8_t *buf, hwaddr len);
+void cpu_flush_icache_range(hwaddr start, hwaddr len);
 
 extern struct MemoryRegion io_mem_rom;
 extern struct MemoryRegion io_mem_notdirty;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 8e61450..841fa6f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1773,7 +1773,7 @@ void address_space_destroy(AddressSpace *as);
  */
 MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
                              MemTxAttrs attrs, uint8_t *buf,
-                             int len, bool is_write);
+                             hwaddr len, bool is_write);
 
 /**
  * address_space_write: write to address space.
@@ -1790,7 +1790,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
  */
 MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
                                 MemTxAttrs attrs,
-                                const uint8_t *buf, int len);
+                                const uint8_t *buf, hwaddr len);
 
 /* address_space_ld*: load from an address space
  * address_space_st*: store to an address space
@@ -1991,7 +1991,7 @@ static inline MemoryRegion *address_space_translate(AddressSpace *as,
  * @is_write: indicates the transfer direction
  * @attrs: memory attributes
  */
-bool address_space_access_valid(AddressSpace *as, hwaddr addr, int len,
+bool address_space_access_valid(AddressSpace *as, hwaddr addr, hwaddr len,
                                 bool is_write, MemTxAttrs attrs);
 
 /* address_space_map: map a physical memory region into a host virtual address
@@ -2028,19 +2028,19 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
 
 /* Internal functions, part of the implementation of address_space_read.  */
 MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
-                                    MemTxAttrs attrs, uint8_t *buf, int len);
+                                    MemTxAttrs attrs, uint8_t *buf, hwaddr len);
 MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
                                    MemTxAttrs attrs, uint8_t *buf,
-                                   int len, hwaddr addr1, hwaddr l,
+                                   hwaddr len, hwaddr addr1, hwaddr l,
                                    MemoryRegion *mr);
 void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
 
 /* Internal functions, part of the implementation of address_space_read_cached
  * and address_space_write_cached.  */
 void address_space_read_cached_slow(MemoryRegionCache *cache,
-                                    hwaddr addr, void *buf, int len);
+                                    hwaddr addr, void *buf, hwaddr len);
 void address_space_write_cached_slow(MemoryRegionCache *cache,
-                                     hwaddr addr, const void *buf, int len);
+                                     hwaddr addr, const void *buf, hwaddr len);
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
@@ -2068,7 +2068,7 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 static inline __attribute__((__always_inline__))
 MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
                                MemTxAttrs attrs, uint8_t *buf,
-                               int len)
+                               hwaddr len)
 {
     MemTxResult result = MEMTX_OK;
     hwaddr l, addr1;
@@ -2107,7 +2107,7 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
  */
 static inline void
 address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
-                          void *buf, int len)
+                          void *buf, hwaddr len)
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
@@ -2127,7 +2127,7 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
  */
 static inline void
 address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
-                           void *buf, int len)
+                           void *buf, hwaddr len)
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/5] change load_image() reture type to ssize_t

[Li Zhijian]

This patch allow load_iamge to load >=2G file

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hw/core/loader.c    | 5 +++--
 include/hw/loader.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index aa0b3fc..0d53229 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -75,9 +75,10 @@ int64_t get_image_size(const char *filename)
 
 /* return the size or -1 if error */
 /* deprecated, because caller does not specify buffer size! */
-int load_image(const char *filename, uint8_t *addr)
+ssize_t load_image(const char *filename, uint8_t *addr)
 {
-    int fd, size;
+    int fd;
+    ssize_t size;
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0)
         return -1;
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 67a0af8..49bb189 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -11,7 +11,7 @@
  * On error, errno is also set as appropriate.
  */
 int64_t get_image_size(const char *filename);
-int load_image(const char *filename, uint8_t *addr); /* deprecated */
+ssize_t load_image(const char *filename, uint8_t *addr); /* deprecated */
 ssize_t load_image_size(const char *filename, void *addr, size_t size);
 
 /**load_image_targphys_as:
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/5] refactor load_image/load_image_size

[Li Zhijian]

Don't expect read(2) can always read as many as it's told.

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hw/core/loader.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 0d53229..5f891e2 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -78,7 +78,7 @@ int64_t get_image_size(const char *filename)
 ssize_t load_image(const char *filename, uint8_t *addr)
 {
     int fd;
-    ssize_t size;
+    ssize_t size, r = 0, l = 0;
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0)
         return -1;
@@ -91,11 +91,16 @@ ssize_t load_image(const char *filename, uint8_t *addr)
     }
 
     lseek(fd, 0, SEEK_SET);
-    if (read(fd, addr, size) != size) {
-        close(fd);
-        return -1;
+    while ((r = read(fd, addr + l, size - l)) > 0 ) {
+        l += r;
     }
     close(fd);
+
+    if (l != size) {
+        fprintf(stderr, "expect read %ld, actual read %ld\n", size, l);
+        return -1;
+    }
+
     return size;
 }
 
@@ -103,21 +108,20 @@ ssize_t load_image(const char *filename, uint8_t *addr)
 ssize_t load_image_size(const char *filename, void *addr, size_t size)
 {
     int fd;
-    ssize_t actsize;
+    ssize_t actsize, l = 0;
 
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0) {
         return -1;
     }
 
-    actsize = read(fd, addr, size);
-    if (actsize < 0) {
-        close(fd);
-        return -1;
+    while ((actsize = read(fd, addr + l, size - l)) > 0) {
+        l += actsize;
     }
+
     close(fd);
 
-    return actsize;
+    return actsize < 0 ? -1 : l;
 }
 
 /* read()-like version */
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/5] x86: exit qemu if load_image fails

[Li Zhijian]

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hw/i386/pc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f095725..2ffe6fb 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -984,7 +984,10 @@ static void load_linux(PCMachineState *pcms,
         initrd_addr = (initrd_max-initrd_size) & ~4095;
 
         initrd_data = g_malloc(initrd_size);
-        load_image(initrd_filename, initrd_data);
+        if (load_image(initrd_filename, initrd_data) < 0) {
+            fprintf(stderr, "failed to load initrd\n");
+            exit(1);
+        }
 
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr);
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
-- 
2.7.4

[Qemu-devel] [PATCH v2 5/5] x86: allow load initrd below 4G for recent linux

[Li Zhijian]

a new field xloadflags was added to recent x86 linux, and BIT 1:
XLF_CAN_BE_LOADED_ABOVE_4G is used to tell bootload that where initrd can be
loaded saftly.

Current QEMU always load initrd below below_4g_mem_size which always
less than 4G, so here limit initrd_max to 4G - 1 simply is enough if
this bit is set.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hw/i386/pc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2ffe6fb..6d4b973 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -92,6 +92,7 @@
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
 #define E820_NR_ENTRIES		16
+#define XLF_CAN_BE_LOADED_ABOVE_4G_MASK (1 << 1)
 
 struct e820_entry {
     uint64_t address;
@@ -916,6 +917,17 @@ static void load_linux(PCMachineState *pcms,
     } else {
         initrd_max = 0x37ffffff;
     }
+    if (protocol >= 0x20c) {
+        unsigned int xloadflags = lduw_p(header+0x236);
+        if (xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G_MASK) {
+                /*
+                 * Although kernel allow initrd loading to above 4G, here we
+                 * limit initrd_max to 4G -1 due to current QEMU always loads
+                 * initrd below pcms->below_4g_mem_size
+                 */
+                initrd_max = UINT32_MAX;
+        }
+    }
 
     if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
         initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 5/5] x86: allow load initrd below 4G for recent linux

[Michael S. Tsirkin]

On Wed, Nov 21, 2018 at 10:06:06AM +0800, Li Zhijian wrote:
> a new field xloadflags was added to recent x86 linux, and BIT 1:
> XLF_CAN_BE_LOADED_ABOVE_4G is used to tell bootload that where initrd can be
> loaded saftly.

safely

> 
> Current QEMU always load

loads

> initrd below below_4g_mem_size which

is

> always
> less than 4G, so here limit


limiting

> initrd_max to 4G - 1 simply is enough if
> this bit is set.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2ffe6fb..6d4b973 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -92,6 +92,7 @@
>  #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
>  
>  #define E820_NR_ENTRIES		16
> +#define XLF_CAN_BE_LOADED_ABOVE_4G_MASK (1 << 1)

why not XLF_CAN_BE_LOADED_ABOVE_4G to be consistent
with Linux?
In fact let's import include/uapi/asm/bootparam.h
into standard-headers, and use the macro from there?


>  
>  struct e820_entry {
>      uint64_t address;
> @@ -916,6 +917,17 @@ static void load_linux(PCMachineState *pcms,
>      } else {
>          initrd_max = 0x37ffffff;
>      }
> +    if (protocol >= 0x20c) {

Let's move it above so we have

if (protocol >= 20c)
else if 0x203
else


> +        unsigned int xloadflags = lduw_p(header+0x236);
> +        if (xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G_MASK) {
> +                /*
> +                 * Although kernel allow

allows

> initrd loading to above 4G, here we
> +                 * limit initrd_max to 4G -1


Well not really, it just makes it as large as possible
while still staying below 4G.

> due to

since

> current QEMU always loads
> +                 * initrd

It's not QEMU, is it? It's actually the bios ...

> below pcms->below_4g_mem_size
> +                 */
> +                initrd_max = UINT32_MAX;
> +        }
> +    }
>  
>      if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
>          initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v2 5/5] x86: allow load initrd below 4G for recent linux

[Li Zhijian]

On 11/30/2018 10:53 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 21, 2018 at 10:06:06AM +0800, Li Zhijian wrote:
>> a new field xloadflags was added to recent x86 linux, and BIT 1:
>> XLF_CAN_BE_LOADED_ABOVE_4G is used to tell bootload that where initrd can be
>> loaded saftly.
> safely
>
>> Current QEMU always load
> loads
>
>> initrd below below_4g_mem_size which
> is
>
>> always
>> less than 4G, so here limit
>
> limiting

okay, thanks


>
>> initrd_max to 4G - 1 simply is enough if
>> this bit is set.
>>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Richard Henderson <rth@twiddle.net>
>> CC: Eduardo Habkost <ehabkost@redhat.com>
>> CC: "Michael S. Tsirkin" <mst@redhat.com>
>> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> ---
>>   hw/i386/pc.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 2ffe6fb..6d4b973 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -92,6 +92,7 @@
>>   #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
>>   
>>   #define E820_NR_ENTRIES		16
>> +#define XLF_CAN_BE_LOADED_ABOVE_4G_MASK (1 << 1)
> why not XLF_CAN_BE_LOADED_ABOVE_4G to be consistent
> with Linux?
> In fact let's import include/uapi/asm/bootparam.h
> into standard-headers, and use the macro from there?

Okay, i will import this header at V3


>
>
>>   
>>   struct e820_entry {
>>       uint64_t address;
>> @@ -916,6 +917,17 @@ static void load_linux(PCMachineState *pcms,
>>       } else {
>>           initrd_max = 0x37ffffff;
>>       }
>> +    if (protocol >= 0x20c) {
> Let's move it above so we have
>
> if (protocol >= 20c)
> else if 0x203
> else

Okay


>
>> +        unsigned int xloadflags = lduw_p(header+0x236);
>> +        if (xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G_MASK) {
>> +                /*
>> +                 * Although kernel allow
> allows
>
>> initrd loading to above 4G, here we
>> +                 * limit initrd_max to 4G -1
>
> Well not really, it just makes it as large as possible
> while still staying below 4G.
>
>> due to
> since
>
>> current QEMU always loads
>> +                 * initrd
> It's not QEMU, is it? It's actually the bios ...

Got it, thanks again


Thanks
Zhijian


>
>> below pcms->below_4g_mem_size
>> +                 */
>> +                initrd_max = UINT32_MAX;
>> +        }
>> +    }
>>   
>>       if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
>>           initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
>> -- 
>> 2.7.4
>
> .
>

Re: [Qemu-devel] [PATCH v2 4/5] x86: exit qemu if load_image fails

[Peter Maydell]

On Wed, 21 Nov 2018 at 02:11, Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f095725..2ffe6fb 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -984,7 +984,10 @@ static void load_linux(PCMachineState *pcms,
>          initrd_addr = (initrd_max-initrd_size) & ~4095;
>
>          initrd_data = g_malloc(initrd_size);
> -        load_image(initrd_filename, initrd_data);
> +        if (load_image(initrd_filename, initrd_data) < 0) {
> +            fprintf(stderr, "failed to load initrd\n");
> +            exit(1);
> +        }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] change load_image() reture type to ssize_t

[Peter Maydell]

On Wed, 21 Nov 2018 at 02:07, Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>
> This patch allow load_iamge to load >=2G file
>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hw/core/loader.c    | 5 +++--
>  include/hw/loader.h | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa0b3fc..0d53229 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -75,9 +75,10 @@ int64_t get_image_size(const char *filename)
>
>  /* return the size or -1 if error */
>  /* deprecated, because caller does not specify buffer size! */
> -int load_image(const char *filename, uint8_t *addr)
> +ssize_t load_image(const char *filename, uint8_t *addr)
>  {
> -    int fd, size;
> +    int fd;
> +    ssize_t size;
>      fd = open(filename, O_RDONLY | O_BINARY);
>      if (fd < 0)
>          return -1;

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

As the comment says, we should ideally move all the callers
to load_image_size(), though...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] change load_image() reture type to ssize_t

[Peter Maydell]

On Fri, 30 Nov 2018 at 13:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 21 Nov 2018 at 02:07, Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
> >
> > This patch allow load_iamge to load >=2G file
> >
> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> > ---
> >  hw/core/loader.c    | 5 +++--
> >  include/hw/loader.h | 2 +-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index aa0b3fc..0d53229 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -75,9 +75,10 @@ int64_t get_image_size(const char *filename)
> >
> >  /* return the size or -1 if error */
> >  /* deprecated, because caller does not specify buffer size! */
> > -int load_image(const char *filename, uint8_t *addr)
> > +ssize_t load_image(const char *filename, uint8_t *addr)
> >  {
> > -    int fd, size;
> > +    int fd;
> > +    ssize_t size;
> >      fd = open(filename, O_RDONLY | O_BINARY);
> >      if (fd < 0)
> >          return -1;
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> As the comment says, we should ideally move all the callers
> to load_image_size(), though...

I'm just about to send out a patchset which removes all the
load_image() callers; that would make patches 2 and 4 in this
set unnecessary.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] change load_image() reture type to ssize_t

[Li Zhijian]

On 11/30/2018 11:14 PM, Peter Maydell wrote:
> I'm just about to send out a patchset which removes all the
> load_image() callers; that would make patches 2 and 4 in this
> set unnecessary.

Got it, i will remove them at next version basing on your patch set.

Thanks
Zhijian

Re: [Qemu-devel] [PATCH v2 1/5] unify len and addr type for memory/address APIs

[Peter Maydell]

On Wed, 21 Nov 2018 at 02:07, Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>
> Some address/memory APIs have different type between 'hwaddr addr' and
> 'int len'. It is very unsafety, espcially some APIs will be passed a non-int
> len by caller which might cause overflow quietly.
> Below is an potential overflow case:
>     dma_memory_read(uint32_t len)
>       -> dma_memory_rw(uint32_t len)
>         -> dma_memory_rw_relaxed(uint32_t len)
>           -> address_space_rw(int len) # len overflow
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> CC: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

Hi; this patch is almost all good -- there's just a couple of
functions here which either don't need a change or should
be using something other than hwaddr, which I've commented on
below.

> ---
>  exec.c                    | 49 ++++++++++++++++++++++++-----------------------
>  include/exec/cpu-all.h    |  2 +-
>  include/exec/cpu-common.h | 10 +++++-----
>  include/exec/memory.h     | 20 +++++++++----------
>  4 files changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index bb6170d..05823ae 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2719,7 +2719,8 @@ static const MemoryRegionOps notdirty_mem_ops = {
>  };
>
>  /* Generate a debug exception if a watchpoint has been hit.  */
> -static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
> +static void check_watchpoint(hwaddr offset,  unsigned len,
> +                             MemTxAttrs attrs, int flags)

This one doesn't need to change -- the offset is the offset within
the page, so it is always going to fit easily within an "int".

>  {
>      CPUState *cpu = current_cpu;
>      CPUClass *cc = CPU_GET_CLASS(cpu);


> @@ -3099,9 +3100,10 @@ MemoryRegion *get_system_io(void)
>  /* physical memory access (slow version, mainly for debug) */
>  #if defined(CONFIG_USER_ONLY)
>  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> -                        uint8_t *buf, int len, int is_write)
> +                        uint8_t *buf, hwaddr len, int is_write)
>  {
> -    int l, flags;
> +    hwaddr l;
> +    int flags;
>      target_ulong page;
>      void * p;

This one is operating on guest virtual addresses, so len should
be a target_ulong, like the addr parameter, as should the "l" variable.

> @@ -3389,7 +3391,7 @@ enum write_rom_type {
>  };
>
>  static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
> -    hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
> +    hwaddr addr, const uint8_t *buf, hwaddr len, enum write_rom_type type)
>  {
>      hwaddr l;
>      uint8_t *ptr;

Just a note that I have a patchset on-list which renames this
function and cpu_physical_memory_write_rom(), so depending on
what order your patch and mine get into master one of us will
have to rebase (or Paolo might fix up the conflict for us). It's
not a difficult one to fix, so no big deal.

https://patchew.org/QEMU/20181122133507.30950-1-peter.maydell@linaro.org/

>  /* virtual memory access for debug (includes writing to ROM) */
>  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> -                        uint8_t *buf, int len, int is_write)
> +                        uint8_t *buf, hwaddr len, int is_write)
>  {
> -    int l;
> -    hwaddr phys_addr;
> +    hwaddr l, phys_addr;
>      target_ulong page;

Here again l and len should be target_ulong, as these are
virtual addresses.

>      cpu_synchronize_state(cpu);
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 117d2fb..4b56672 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -367,7 +367,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
>  #endif /* !CONFIG_USER_ONLY */
>
>  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> -                        uint8_t *buf, int len, int is_write);
> +                        uint8_t *buf, hwaddr len, int is_write);

target_ulong.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] unify len and addr type for memory/address APIs

[Li Zhijian]

On 11/30/2018 09:40 PM, Peter Maydell wrote:
> On Wed, 21 Nov 2018 at 02:07, Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>> Some address/memory APIs have different type between 'hwaddr addr' and
>> 'int len'. It is very unsafety, espcially some APIs will be passed a non-int
>> len by caller which might cause overflow quietly.
>> Below is an potential overflow case:
>>      dma_memory_read(uint32_t len)
>>        -> dma_memory_rw(uint32_t len)
>>          -> dma_memory_rw_relaxed(uint32_t len)
>>            -> address_space_rw(int len) # len overflow
>>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> CC: Richard Henderson <rth@twiddle.net>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Hi; this patch is almost all good -- there's just a couple of
> functions here which either don't need a change or should
> be using something other than hwaddr, which I've commented on
> below.
>
>> ---
>>   exec.c                    | 49 ++++++++++++++++++++++++-----------------------
>>   include/exec/cpu-all.h    |  2 +-
>>   include/exec/cpu-common.h | 10 +++++-----
>>   include/exec/memory.h     | 20 +++++++++----------
>>   4 files changed, 41 insertions(+), 40 deletions(-)
>>
>>
>> This one doesn't need to change -- the offset is the offset within
>> the page, so it is always going to fit easily within an "int".

Got it

>
>> @@ -3099,9 +3100,10 @@ MemoryRegion *get_system_io(void)
>>   /* physical memory access (slow version, mainly for debug) */
>>   #if defined(CONFIG_USER_ONLY)
>>   int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>> -                        uint8_t *buf, int len, int is_write)
>> +                        uint8_t *buf, hwaddr len, int is_write)
>>   {
>> -    int l, flags;
>> +    hwaddr l;
>> +    int flags;
>>       target_ulong page;
>>       void * p;
> This one is operating on guest virtual addresses, so len should
> be a target_ulong, like the addr parameter, as should the "l" variable.

Okay, i will update it

>
>> @@ -3389,7 +3391,7 @@ enum write_rom_type {
>>   };
>>
>>   static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
>> -    hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
>> +    hwaddr addr, const uint8_t *buf, hwaddr len, enum write_rom_type type)
>>   {
>>       hwaddr l;
>>       uint8_t *ptr;
> Just a note that I have a patchset on-list which renames this
> function and cpu_physical_memory_write_rom(), so depending on
> what order your patch and mine get into master one of us will
> have to rebase (or Paolo might fix up the conflict for us). It's
> not a difficult one to fix, so no big deal.
>
> https://patchew.org/QEMU/20181122133507.30950-1-peter.maydell@linaro.org/

thanks a lot for the information.

>
>>   /* virtual memory access for debug (includes writing to ROM) */
>>   int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>> -                        uint8_t *buf, int len, int is_write)
>> +                        uint8_t *buf, hwaddr len, int is_write)
>>   {
>> -    int l;
>> -    hwaddr phys_addr;
>> +    hwaddr l, phys_addr;
>>       target_ulong page;
> Here again l and len should be target_ulong, as these are
> virtual addresses.

Got it



>>       cpu_synchronize_state(cpu);
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index 117d2fb..4b56672 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -367,7 +367,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf);
>>   #endif /* !CONFIG_USER_ONLY */
>>
>>   int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>> -                        uint8_t *buf, int len, int is_write);
>> +                        uint8_t *buf, hwaddr len, int is_write);
> target_ulong.

Got it

Thanks
Zhijian



> thanks
> -- PMM
>
>
> .
>