[Qemu-devel] [PATCH for-4.0 v4 0/4] allow to load initrd below 4G for recent kernel

[Qemu-devel] [PATCH for-4.0 v4 0/4] allow to load initrd below 4G for recent kernel

[Li Zhijian]

Long long ago, linux kernel has 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 support 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/BIOS always loads initrd below below_4g_mem_size which always
less than 4G, so here limiting initrd_max to 4G - 1 simply is enough if
this bit is set.

Default roms(Seabios + optionrom(linuxboot_dma)) works as expected with this
patchset.

changes:
V4:
  - add Reviwed-by tag to 1/4 and 2/4
  - use scripts/update-linux-headers.sh to import bootparam.h
  - minor fix at commit log
V3:
 - rebase code basing on http://patchwork.ozlabs.org/cover/1005990 and
   https://patchew.org/QEMU/20181122133507.30950-1-peter.maydell@linaro.org
 - add new patch 3/4 to import header bootparam.h (Michael S. Tsirkin)

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

Li Zhijian (4):
  unify len and addr type for memory/address APIs
  refactor load_image_size
  i386: import & use bootparam.h
  i386: allow to load initrd below 4G for recent linux

 exec.c                                       | 47 ++++++++++++++--------------
 hw/core/loader.c                             | 11 +++----
 hw/i386/pc.c                                 | 18 ++++++-----
 include/exec/cpu-all.h                       |  2 +-
 include/exec/cpu-common.h                    |  8 ++---
 include/exec/memory.h                        | 22 ++++++-------
 include/standard-headers/asm-x86/bootparam.h | 34 ++++++++++++++++++++
 scripts/update-linux-headers.sh              |  4 +++
 8 files changed, 92 insertions(+), 54 deletions(-)
 create mode 100644 include/standard-headers/asm-x86/bootparam.h

-- 
2.7.4

[Qemu-devel] [PATCH for-4.0 v4 1/4] unify len and addr type for memory/address APIs

[Li Zhijian]

Some address/memory APIs have different type between
'hwaddr/target_ulong addr' and 'int len'. It is very unsafe, 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>
CC: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
V4: minor fix at commit message and add Reviewed-by tag
V3: use the same type between len and addr(Peter Maydell)
    rebase code basing on https://patchew.org/QEMU/20181122133507.30950-1-peter.maydell@linaro.org/
---
 exec.c                    | 47 +++++++++++++++++++++++------------------------
 include/exec/cpu-all.h    |  2 +-
 include/exec/cpu-common.h |  8 ++++----
 include/exec/memory.h     | 22 +++++++++++-----------
 4 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/exec.c b/exec.c
index 6e875f0..f475974 100644
--- a/exec.c
+++ b/exec.c
@@ -2848,10 +2848,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,10 +3099,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, target_ulong len, int is_write)
 {
-    int l, flags;
-    target_ulong page;
+    int flags;
+    target_ulong l, page;
     void * p;
 
     while (len > 0) {
@@ -3215,7 +3215,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 +3260,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 +3278,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 +3321,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 +3334,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 +3351,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 +3367,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 +3377,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);
@@ -3392,7 +3392,7 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
                                                            hwaddr addr,
                                                            MemTxAttrs attrs,
                                                            const uint8_t *buf,
-                                                           int len,
+                                                           hwaddr len,
                                                            enum write_rom_type type)
 {
     hwaddr l;
@@ -3432,13 +3432,13 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
 /* used for ROM loading : can write in RAM and ROM */
 MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
                                     MemTxAttrs attrs,
-                                    const uint8_t *buf, int len)
+                                    const uint8_t *buf, hwaddr len)
 {
     return address_space_write_rom_internal(as, addr, attrs,
                                             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
@@ -3541,7 +3541,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;
@@ -3564,7 +3564,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;
@@ -3817,7 +3817,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;
@@ -3835,7 +3835,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;
@@ -3858,11 +3858,10 @@ 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, target_ulong len, int is_write)
 {
-    int l;
     hwaddr phys_addr;
-    target_ulong page;
+    target_ulong l, page;
 
     cpu_synchronize_state(cpu);
     while (len > 0) {
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 117d2fb..b16c9ec 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, target_ulong len, int is_write);
 
 int cpu_exec(CPUState *cpu);
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 2ad2d6d..63ec1f9 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);
 }
@@ -111,7 +111,7 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr);
  */
 void qemu_flush_coalesced_mmio_buffer(void);
 
-void cpu_flush_icache_range(hwaddr start, int 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 ffd23ed..6235f77 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_write_rom: write to address space, including ROM.
@@ -1816,7 +1816,7 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
  */
 MemTxResult address_space_write_rom(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
@@ -2017,7 +2017,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
@@ -2054,19 +2054,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)
 {
@@ -2094,7 +2094,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;
@@ -2133,7 +2133,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)) {
@@ -2153,7 +2153,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

Re: [Qemu-devel] [PATCH for-4.0 v4 1/4] unify len and addr type for memory/address APIs

[Stefano Garzarella]

Hi Li,

On Thu, Dec 6, 2018 at 3:26 AM Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>
> Some address/memory APIs have different type between
> 'hwaddr/target_ulong addr' and 'int len'. It is very unsafe, espcially


As Philippe already suggested,
s/espcially/especially


> 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>
> CC: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> ---
> V4: minor fix at commit message and add Reviewed-by tag
> V3: use the same type between len and addr(Peter Maydell)
>     rebase code basing on https://patchew.org/QEMU/20181122133507.30950-1-peter.maydell@linaro.org/
> ---
>  exec.c                    | 47 +++++++++++++++++++++++------------------------
>  include/exec/cpu-all.h    |  2 +-
>  include/exec/cpu-common.h |  8 ++++----
>  include/exec/memory.h     | 22 +++++++++++-----------
>  4 files changed, 39 insertions(+), 40 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [Qemu-devel] [PATCH for-4.0 v4 1/4] unify len and addr type for memory/address APIs

[Li Zhijian]

On 1/7/19 18:04, Stefano Garzarella wrote:
> As Philippe already suggested,
> s/espcially/especially

Hi Stefano，

thanks a lot, will update it at next version.

Thanks

[Qemu-devel] [PATCH for-4.0 v4 2/4] refactor 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
V4: add reviewed-by tag
---
 hw/core/loader.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index fa41842..9cbceab 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -77,21 +77,20 @@ int64_t get_image_size(const char *filename)
 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

Re: [Qemu-devel] [PATCH for-4.0 v4 2/4] refactor load_image_size

[Michael S. Tsirkin]

On Thu, Dec 06, 2018 at 10:32:11AM +0800, Li Zhijian wrote:
> Don't expect read(2) can always read as many as it's told.
> 
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

This is more a theoretical bugfix than a refactoring right?

> ---
> V4: add reviewed-by tag
> ---
>  hw/core/loader.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index fa41842..9cbceab 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -77,21 +77,20 @@ int64_t get_image_size(const char *filename)
>  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

Re: [Qemu-devel] [PATCH for-4.0 v4 2/4] refactor load_image_size

[Li Zhijian]

On 12/22/18 00:12, Michael S. Tsirkin wrote:
> On Thu, Dec 06, 2018 at 10:32:11AM +0800, Li Zhijian wrote:
>> Don't expect read(2) can always read as many as it's told.
>>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> This is more a theoretical bugfix than a refactoring right?

Yes， it does.

how about change the title to : "enhance reading on load_image_size()" or such

Thanks
Zhijian


>
>> ---
>> V4: add reviewed-by tag
>> ---
>>   hw/core/loader.c | 11 +++++------
>>   1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index fa41842..9cbceab 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -77,21 +77,20 @@ int64_t get_image_size(const char *filename)
>>   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
>
>
>
-- 
Best regards.
Li Zhijian (8528)

Re: [Qemu-devel] [PATCH for-4.0 v4 2/4] refactor load_image_size

[Stefano Garzarella]

On Mon, Dec 24, 2018 at 3:16 AM Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>
>
> On 12/22/18 00:12, Michael S. Tsirkin wrote:
> > On Thu, Dec 06, 2018 at 10:32:11AM +0800, Li Zhijian wrote:
> >> Don't expect read(2) can always read as many as it's told.
> >>
> >> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > This is more a theoretical bugfix than a refactoring right?
>
> Yes， it does.
>
> how about change the title to : "enhance reading on load_image_size()" or such

Maybe something like this: "hw/core/loader.c: Read as long as possible
in load_image_size()"

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
> Thanks
> Zhijian
>
>
> >
> >> ---
> >> V4: add reviewed-by tag
> >> ---
> >>   hw/core/loader.c | 11 +++++------
> >>   1 file changed, 5 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/core/loader.c b/hw/core/loader.c
> >> index fa41842..9cbceab 100644
> >> --- a/hw/core/loader.c
> >> +++ b/hw/core/loader.c
> >> @@ -77,21 +77,20 @@ int64_t get_image_size(const char *filename)
> >>   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
> >
> >
> >
> --
> Best regards.
> Li Zhijian (8528)
>
>
>


-- 
Stefano Garzarella
Red Hat

Re: [Qemu-devel] [PATCH for-4.0 v4 2/4] refactor load_image_size

[Li Zhijian]

On 1/7/19 18:33, Stefano Garzarella wrote:
> On Mon, Dec 24, 2018 at 3:16 AM Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>>
>> On 12/22/18 00:12, Michael S. Tsirkin wrote:
>>> On Thu, Dec 06, 2018 at 10:32:11AM +0800, Li Zhijian wrote:
>>>> Don't expect read(2) can always read as many as it's told.
>>>>
>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> This is more a theoretical bugfix than a refactoring right?
>> Yes， it does.
>>
>> how about change the title to : "enhance reading on load_image_size()" or such
> Maybe something like this: "hw/core/loader.c: Read as long as possible
> in load_image_size()"

It really helps, i will take your suggestion to the subject.

Thanks
Zhijian




>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>

[Qemu-devel] [PATCH for-4.0 v4 3/4] i386: import & use bootparam.h

[Li Zhijian]

it's from v4.20-rc5.

CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

---
V4: use scirpt to import bootparam.h (Michael S. Tsirkin)
V3: new patch

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hw/i386/pc.c                                 |  8 +------
 include/standard-headers/asm-x86/bootparam.h | 34 ++++++++++++++++++++++++++++
 scripts/update-linux-headers.sh              |  4 ++++
 3 files changed, 39 insertions(+), 7 deletions(-)
 create mode 100644 include/standard-headers/asm-x86/bootparam.h

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 067d23a..3b10726 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -74,6 +74,7 @@
 #include "hw/nmi.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
+#include "standard-headers/asm-x86/bootparam.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -820,13 +821,6 @@ static long get_file_size(FILE *f)
     return size;
 }
 
-/* setup_data types */
-#define SETUP_NONE     0
-#define SETUP_E820_EXT 1
-#define SETUP_DTB      2
-#define SETUP_PCI      3
-#define SETUP_EFI      4
-
 struct setup_data {
     uint64_t next;
     uint32_t type;
diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h
new file mode 100644
index 0000000..67d4f01
--- /dev/null
+++ b/include/standard-headers/asm-x86/bootparam.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _ASM_X86_BOOTPARAM_H
+#define _ASM_X86_BOOTPARAM_H
+
+/* setup_data types */
+#define SETUP_NONE			0
+#define SETUP_E820_EXT			1
+#define SETUP_DTB			2
+#define SETUP_PCI			3
+#define SETUP_EFI			4
+#define SETUP_APPLE_PROPERTIES		5
+#define SETUP_JAILHOUSE			6
+
+/* ram_size flags */
+#define RAMDISK_IMAGE_START_MASK	0x07FF
+#define RAMDISK_PROMPT_FLAG		0x8000
+#define RAMDISK_LOAD_FLAG		0x4000
+
+/* loadflags */
+#define LOADED_HIGH	(1<<0)
+#define KASLR_FLAG	(1<<1)
+#define QUIET_FLAG	(1<<5)
+#define KEEP_SEGMENTS	(1<<6)
+#define CAN_USE_HEAP	(1<<7)
+
+/* xloadflags */
+#define XLF_KERNEL_64			(1<<0)
+#define XLF_CAN_BE_LOADED_ABOVE_4G	(1<<1)
+#define XLF_EFI_HANDOVER_32		(1<<2)
+#define XLF_EFI_HANDOVER_64		(1<<3)
+#define XLF_EFI_KEXEC			(1<<4)
+
+
+#endif /* _ASM_X86_BOOTPARAM_H */
diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index 0a964fe..77ec108 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -120,6 +120,10 @@ for arch in $ARCHLIST; do
         cp "$tmpdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/"
         cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/"
         cp_portable "$tmpdir/include/asm/kvm_para.h" "$output/include/standard-headers/asm-$arch"
+        # Remove everything except the macros from bootparam.h avoiding the
+        # unnecessary import of several video/ist/etc headers
+        sed -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' $tmpdir/include/asm/bootparam.h > $tmpdir/bootparam.h
+        cp_portable $tmpdir/bootparam.h "$output/include/standard-headers/asm-$arch"
     fi
 done
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-4.0 v4 3/4] i386: import & use bootparam.h

[Michael S. Tsirkin]

On Thu, Dec 06, 2018 at 10:32:12AM +0800, Li Zhijian wrote:
> it's from v4.20-rc5.
> 
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> V4: use scirpt to import bootparam.h (Michael S. Tsirkin)
> V3: new patch
> 
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hw/i386/pc.c                                 |  8 +------
>  include/standard-headers/asm-x86/bootparam.h | 34 ++++++++++++++++++++++++++++
>  scripts/update-linux-headers.sh              |  4 ++++
>  3 files changed, 39 insertions(+), 7 deletions(-)
>  create mode 100644 include/standard-headers/asm-x86/bootparam.h
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 067d23a..3b10726 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -74,6 +74,7 @@
>  #include "hw/nmi.h"
>  #include "hw/i386/intel_iommu.h"
>  #include "hw/net/ne2000-isa.h"
> +#include "standard-headers/asm-x86/bootparam.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -820,13 +821,6 @@ static long get_file_size(FILE *f)
>      return size;
>  }
>  
> -/* setup_data types */
> -#define SETUP_NONE     0
> -#define SETUP_E820_EXT 1
> -#define SETUP_DTB      2
> -#define SETUP_PCI      3
> -#define SETUP_EFI      4
> -
>  struct setup_data {
>      uint64_t next;
>      uint32_t type;
> diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h
> new file mode 100644
> index 0000000..67d4f01
> --- /dev/null
> +++ b/include/standard-headers/asm-x86/bootparam.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _ASM_X86_BOOTPARAM_H
> +#define _ASM_X86_BOOTPARAM_H
> +
> +/* setup_data types */
> +#define SETUP_NONE			0
> +#define SETUP_E820_EXT			1
> +#define SETUP_DTB			2
> +#define SETUP_PCI			3
> +#define SETUP_EFI			4
> +#define SETUP_APPLE_PROPERTIES		5
> +#define SETUP_JAILHOUSE			6
> +
> +/* ram_size flags */
> +#define RAMDISK_IMAGE_START_MASK	0x07FF
> +#define RAMDISK_PROMPT_FLAG		0x8000
> +#define RAMDISK_LOAD_FLAG		0x4000
> +
> +/* loadflags */
> +#define LOADED_HIGH	(1<<0)
> +#define KASLR_FLAG	(1<<1)
> +#define QUIET_FLAG	(1<<5)
> +#define KEEP_SEGMENTS	(1<<6)
> +#define CAN_USE_HEAP	(1<<7)
> +
> +/* xloadflags */
> +#define XLF_KERNEL_64			(1<<0)
> +#define XLF_CAN_BE_LOADED_ABOVE_4G	(1<<1)
> +#define XLF_EFI_HANDOVER_32		(1<<2)
> +#define XLF_EFI_HANDOVER_64		(1<<3)
> +#define XLF_EFI_KEXEC			(1<<4)
> +
> +
> +#endif /* _ASM_X86_BOOTPARAM_H */
> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> index 0a964fe..77ec108 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -120,6 +120,10 @@ for arch in $ARCHLIST; do
>          cp "$tmpdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/"
>          cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/"
>          cp_portable "$tmpdir/include/asm/kvm_para.h" "$output/include/standard-headers/asm-$arch"
> +        # Remove everything except the macros from bootparam.h avoiding the
> +        # unnecessary import of several video/ist/etc headers
> +        sed -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' $tmpdir/include/asm/bootparam.h > $tmpdir/bootparam.h
> +        cp_portable $tmpdir/bootparam.h "$output/include/standard-headers/asm-$arch"
>      fi
>  done
>  
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH for-4.0 v4 3/4] i386: import & use bootparam.h

[Stefano Garzarella]

Hi Li,

On Thu, Dec 6, 2018 at 3:24 AM Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
>
> it's from v4.20-rc5.
>
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>
> ---
> V4: use scirpt to import bootparam.h (Michael S. Tsirkin)
> V3: new patch
>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hw/i386/pc.c                                 |  8 +------
>  include/standard-headers/asm-x86/bootparam.h | 34 ++++++++++++++++++++++++++++
>  scripts/update-linux-headers.sh              |  4 ++++
>  3 files changed, 39 insertions(+), 7 deletions(-)
>  create mode 100644 include/standard-headers/asm-x86/bootparam.h
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 067d23a..3b10726 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -74,6 +74,7 @@
>  #include "hw/nmi.h"
>  #include "hw/i386/intel_iommu.h"
>  #include "hw/net/ne2000-isa.h"
> +#include "standard-headers/asm-x86/bootparam.h"
>
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -820,13 +821,6 @@ static long get_file_size(FILE *f)
>      return size;
>  }
>
> -/* setup_data types */
> -#define SETUP_NONE     0
> -#define SETUP_E820_EXT 1
> -#define SETUP_DTB      2
> -#define SETUP_PCI      3
> -#define SETUP_EFI      4
> -
>  struct setup_data {
>      uint64_t next;
>      uint32_t type;
> diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h
> new file mode 100644
> index 0000000..67d4f01
> --- /dev/null
> +++ b/include/standard-headers/asm-x86/bootparam.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _ASM_X86_BOOTPARAM_H
> +#define _ASM_X86_BOOTPARAM_H
> +
> +/* setup_data types */
> +#define SETUP_NONE                     0
> +#define SETUP_E820_EXT                 1
> +#define SETUP_DTB                      2
> +#define SETUP_PCI                      3
> +#define SETUP_EFI                      4
> +#define SETUP_APPLE_PROPERTIES         5
> +#define SETUP_JAILHOUSE                        6
> +
> +/* ram_size flags */
> +#define RAMDISK_IMAGE_START_MASK       0x07FF
> +#define RAMDISK_PROMPT_FLAG            0x8000
> +#define RAMDISK_LOAD_FLAG              0x4000
> +
> +/* loadflags */
> +#define LOADED_HIGH    (1<<0)
> +#define KASLR_FLAG     (1<<1)
> +#define QUIET_FLAG     (1<<5)
> +#define KEEP_SEGMENTS  (1<<6)
> +#define CAN_USE_HEAP   (1<<7)
> +
> +/* xloadflags */
> +#define XLF_KERNEL_64                  (1<<0)
> +#define XLF_CAN_BE_LOADED_ABOVE_4G     (1<<1)
> +#define XLF_EFI_HANDOVER_32            (1<<2)
> +#define XLF_EFI_HANDOVER_64            (1<<3)
> +#define XLF_EFI_KEXEC                  (1<<4)
> +
> +
> +#endif /* _ASM_X86_BOOTPARAM_H */
> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> index 0a964fe..77ec108 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -120,6 +120,10 @@ for arch in $ARCHLIST; do
>          cp "$tmpdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/"
>          cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/"
>          cp_portable "$tmpdir/include/asm/kvm_para.h" "$output/include/standard-headers/asm-$arch"
> +        # Remove everything except the macros from bootparam.h avoiding the
> +        # unnecessary import of several video/ist/etc headers
> +        sed -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' $tmpdir/include/asm/bootparam.h > $tmpdir/bootparam.h
> +        cp_portable $tmpdir/bootparam.h "$output/include/standard-headers/asm-$arch"

Maybe is better to use the double quotes for all paths.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Stefano

>      fi
>  done
>
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH for-4.0 v4 3/4] i386: import & use bootparam.h

[Li Zhijian]

Hi Stefano,


On 1/5/19 00:41, Stefano Garzarella wrote:
>> +        # Remove everything except the macros from bootparam.h avoiding the
>> +        # unnecessary import of several video/ist/etc headers
>> +        sed -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' $tmpdir/include/asm/bootparam.h > $tmpdir/bootparam.h
>> +        cp_portable $tmpdir/bootparam.h "$output/include/standard-headers/asm-$arch"
> Maybe is better to use the double quotes for all paths.

Sure, i will update it at next version.

Thanks
Zhijian


>
> Reviewed-by: Stefano Garzarella<sgarzare@redhat.com>
>
> Thanks,
> Stefano
>

[Qemu-devel] [PATCH for-4.0 v4 4/4] i386: allow to 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 safely.

Current QEMU/BIOS always loads initrd below below_4g_mem_size which is always
less than 4G, so here 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>

---
V3: correct grammar and check XLF_CAN_BE_LOADED_ABOVE_4G first (Michael S. Tsirkin)

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hw/i386/pc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3b10726..baa99c0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -904,7 +904,15 @@ static void load_linux(PCMachineState *pcms,
 #endif
 
     /* highest address for loading the initrd */
-    if (protocol >= 0x203) {
+    if (protocol >= 0x20c &&
+        lduw_p(header+0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
+        /*
+         * Although kernel allows initrd loading to above 4G,
+         * it just makes it as large as possible while still staying below 4G
+         * since current BIOS always loads initrd below pcms->below_4g_mem_size
+         */
+        initrd_max = UINT32_MAX;
+    } else if (protocol >= 0x203) {
         initrd_max = ldl_p(header+0x22c);
     } else {
         initrd_max = 0x37ffffff;
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-4.0 v4 4/4] i386: allow to load initrd below 4G for recent linux

[Michael S. Tsirkin]

On Thu, Dec 06, 2018 at 10:32:13AM +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 safely.
> 
> Current QEMU/BIOS always loads initrd below below_4g_mem_size which is always
> less than 4G, so here 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>
> 
> ---
> V3: correct grammar and check XLF_CAN_BE_LOADED_ABOVE_4G first (Michael S. Tsirkin)
> 
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hw/i386/pc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3b10726..baa99c0 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -904,7 +904,15 @@ static void load_linux(PCMachineState *pcms,
>  #endif
>  
>      /* highest address for loading the initrd */
> -    if (protocol >= 0x203) {
> +    if (protocol >= 0x20c &&
> +        lduw_p(header+0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
> +        /*
> +         * Although kernel allows initrd loading to above 4G,
> +         * it just makes it as large as possible while still staying below 4G
> +         * since current BIOS always loads initrd below pcms->below_4g_mem_size
> +         */
> +        initrd_max = UINT32_MAX;
> +    } else if (protocol >= 0x203) {
>          initrd_max = ldl_p(header+0x22c);
>      } else {
>          initrd_max = 0x37ffffff;


I still have trouble understanding the above.
Anyone else wants to comment / help rephrase the comment
and commit log so it's readable?

> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH for-4.0 v4 4/4] i386: allow to load initrd below 4G for recent linux

[Eduardo Habkost]

On Fri, Dec 21, 2018 at 11:10:30AM -0500, Michael S. Tsirkin wrote:
> On Thu, Dec 06, 2018 at 10:32:13AM +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 safely.
> > 
> > Current QEMU/BIOS always loads initrd below below_4g_mem_size which is always
> > less than 4G, so here 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>
> > 
> > ---
> > V3: correct grammar and check XLF_CAN_BE_LOADED_ABOVE_4G first (Michael S. Tsirkin)
> > 
> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> > ---
> >  hw/i386/pc.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 3b10726..baa99c0 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -904,7 +904,15 @@ static void load_linux(PCMachineState *pcms,
> >  #endif
> >  
> >      /* highest address for loading the initrd */
> > -    if (protocol >= 0x203) {
> > +    if (protocol >= 0x20c &&
> > +        lduw_p(header+0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
> > +        /*
> > +         * Although kernel allows initrd loading to above 4G,
> > +         * it just makes it as large as possible while still staying below 4G
> > +         * since current BIOS always loads initrd below pcms->below_4g_mem_size
> > +         */
> > +        initrd_max = UINT32_MAX;
> > +    } else if (protocol >= 0x203) {
> >          initrd_max = ldl_p(header+0x22c);
> >      } else {
> >          initrd_max = 0x37ffffff;
> 
> 
> I still have trouble understanding the above.
> Anyone else wants to comment / help rephrase the comment
> and commit log so it's readable?


The comment seems to contradict what I see on the code:

| Although kernel allows initrd loading to above 4G,

Sounds correct.


| it just makes it as large as possible while still staying below 4G

I'm not a native English speaker, but I believe "it" here should
be interpreted as "the kernel", which would be incorrect.  It's
this QEMU function that limits initrd_max to a uint32 value, not
the kernel.


| since current BIOS always loads initrd below pcms->below_4g_mem_size

I don't know why the BIOS is mentioned here.  The
below_4g_mem_size limit comes from these 2 lines inside
load_linux():

    if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
        initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
    }
In addition to that, initrd_max is uint32_t simply because QEMU
doesn't support the 64-bit boot protocol (specifically the
ext_ramdisk_image field), so all talk about below_4g_mem_size
seems to be just a distraction.

All that said, I miss one piece of information here: is
XLF_CAN_BE_LOADED_ABOVE_4G really supposed to override
header+0x22c?  linux/Documentation/x86/boot.txt isn't clear about
that.  Is there any reference that can help us confirm this?

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-4.0 v4 4/4] i386: allow to load initrd below 4G for recent linux

[Li Zhijian]

On 12/28/2018 4:31 AM, Eduardo Habkost wrote:
> On Fri, Dec 21, 2018 at 11:10:30AM -0500, Michael S. Tsirkin wrote:
>> On Thu, Dec 06, 2018 at 10:32:13AM +0800, Li Zhijian wrote:
>>>   
>>>       /* highest address for loading the initrd */
>>> -    if (protocol >= 0x203) {
>>> +    if (protocol >= 0x20c &&
>>> +        lduw_p(header+0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
>>> +        /*
>>> +         * Although kernel allows initrd loading to above 4G,
>>> +         * it just makes it as large as possible while still staying below 4G
>>> +         * since current BIOS always loads initrd below pcms->below_4g_mem_size
>>> +         */
>>> +        initrd_max = UINT32_MAX;
>>> +    } else if (protocol >= 0x203) {
>>>           initrd_max = ldl_p(header+0x22c);
>>>       } else {
>>>           initrd_max = 0x37ffffff;
>>
>> I still have trouble understanding the above.
>> Anyone else wants to comment / help rephrase the comment
>> and commit log so it's readable?
>
> The comment seems to contradict what I see on the code:
>
> | Although kernel allows initrd loading to above 4G,
>
> Sounds correct.
>
>
> | it just makes it as large as possible while still staying below 4G
>
> I'm not a native English speaker, but I believe "it" here should
> be interpreted as "the kernel", which would be incorrect.  It's
> this QEMU function that limits initrd_max to a uint32 value, not
> the kernel.
>
>
> | since current BIOS always loads initrd below pcms->below_4g_mem_size
>
> I don't know why the BIOS is mentioned here.  The
> below_4g_mem_size limit comes from these 2 lines inside
> load_linux():
>
>      if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
>          initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
>      }
> In addition to that, initrd_max is uint32_t simply because QEMU
> doesn't support the 64-bit boot protocol (specifically the
> ext_ramdisk_image field),

Thanks for explaining this :), i'm not clear before.



> so all talk about below_4g_mem_size
> seems to be just a distraction.
>
> All that said, I miss one piece of information here: is
> XLF_CAN_BE_LOADED_ABOVE_4G really supposed to override
> header+0x22c?  linux/Documentation/x86/boot.txt isn't clear about
> that.  Is there any reference that can help us confirm this?

Good question.

Since i'm not familiar with boot protocol, at the beginning, i also CCed to LKML for helps
https://lkml.org/lkml/2018/11/10/82

Ingo said:
>> > If XLF_CAN_BE_LOADED_ABOVE_4G is not > set, then you most likely 
>> are on a 32-bit kernel and there are more > fundamental limits (even 
>> if you were to load it above the 2 GB mark, you > would be limited by 
>> the size of kernel memory.) > > So, in case you are wondering: the 
>> bootloader that broke when setting > the initrd_max field above 2 GB 
>> was, of course, Grub. > > So just use XLF_CAN_BE_LOADED_ABOVE_4G. 
>> There is no need for a new flag > or new field. That's nice, and 
>> that's the best solution!

that make me to believe that if XLF_CAN_BE_LOADED_ABOVE_4G is set, BELOW_4G is allowed too.

if above is credible, might be we can update the comments like:
-------
QEMU doesn't support the 64-bit boot protocol (specifically the
ext_ramdisk_image field).

In addition, kernel allows to load initrd above 4G if XLF_CAN_BE_LOADED_ABOVE_4G is set,
so we believe that load initrd below 4G is allowed too.

For simplicity, so just set initrd_max to UINT32_MAX is enough and safe.
-------
  
Thanks
Zhijian

Re: [Qemu-devel] [PATCH for-4.0 v4 4/4] i386: allow to load initrd below 4G for recent linux

[Stefano Garzarella]

Hi,

On Thu, Dec 27, 2018 at 9:32 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Fri, Dec 21, 2018 at 11:10:30AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Dec 06, 2018 at 10:32:13AM +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 safely.
> > >
> > > Current QEMU/BIOS always loads initrd below below_4g_mem_size which is always
> > > less than 4G, so here 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>
> > >
> > > ---
> > > V3: correct grammar and check XLF_CAN_BE_LOADED_ABOVE_4G first (Michael S. Tsirkin)
> > >
> > > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> > > ---
> > >  hw/i386/pc.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 3b10726..baa99c0 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -904,7 +904,15 @@ static void load_linux(PCMachineState *pcms,
> > >  #endif
> > >
> > >      /* highest address for loading the initrd */
> > > -    if (protocol >= 0x203) {
> > > +    if (protocol >= 0x20c &&
> > > +        lduw_p(header+0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
> > > +        /*
> > > +         * Although kernel allows initrd loading to above 4G,
> > > +         * it just makes it as large as possible while still staying below 4G
> > > +         * since current BIOS always loads initrd below pcms->below_4g_mem_size
> > > +         */
> > > +        initrd_max = UINT32_MAX;
> > > +    } else if (protocol >= 0x203) {
> > >          initrd_max = ldl_p(header+0x22c);
> > >      } else {
> > >          initrd_max = 0x37ffffff;
> >
> >
> > I still have trouble understanding the above.
> > Anyone else wants to comment / help rephrase the comment
> > and commit log so it's readable?
>
>
> The comment seems to contradict what I see on the code:
>
> | Although kernel allows initrd loading to above 4G,
>
> Sounds correct.
>
>
> | it just makes it as large as possible while still staying below 4G
>
> I'm not a native English speaker, but I believe "it" here should
> be interpreted as "the kernel", which would be incorrect.  It's
> this QEMU function that limits initrd_max to a uint32 value, not
> the kernel.
>
>
> | since current BIOS always loads initrd below pcms->below_4g_mem_size
>
> I don't know why the BIOS is mentioned here.  The
> below_4g_mem_size limit comes from these 2 lines inside
> load_linux():
>
>     if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
>         initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
>     }
> In addition to that, initrd_max is uint32_t simply because QEMU
> doesn't support the 64-bit boot protocol (specifically the
> ext_ramdisk_image field), so all talk about below_4g_mem_size
> seems to be just a distraction.
>
> All that said, I miss one piece of information here: is
> XLF_CAN_BE_LOADED_ABOVE_4G really supposed to override
> header+0x22c?  linux/Documentation/x86/boot.txt isn't clear about
> that.  Is there any reference that can help us confirm this?

Looking at the following patch seems that we can confirm the assumption:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=4bf7111f50167133a71c23530ca852a41355e739

Note: the patch was reverted due to bugs in some firmwares, but IMHO
the assumption is correct.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=47226ad4f4cfd1e91ded7f2ec42f83ff1c624663

Cheers,
Stefano

>
> --
> Eduardo
>

Re: [Qemu-devel] [PATCH for-4.0 v4 4/4] i386: allow to load initrd below 4G for recent linux

[Li Zhijian]

On 1/7/19 20:11, Stefano Garzarella wrote:
> Hi,
>
> On Thu, Dec 27, 2018 at 9:32 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
>> On Fri, Dec 21, 2018 at 11:10:30AM -0500, Michael S. Tsirkin wrote:
>>> On Thu, Dec 06, 2018 at 10:32:13AM +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 safely.
>>>>
>>>> Current QEMU/BIOS always loads initrd below below_4g_mem_size which is always
>>>> less than 4G, so here 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>
>>>>
>>>> ---
>>>> V3: correct grammar and check XLF_CAN_BE_LOADED_ABOVE_4G first (Michael S. Tsirkin)
>>>>
>>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>>> ---
>>>>   hw/i386/pc.c | 10 +++++++++-
>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 3b10726..baa99c0 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -904,7 +904,15 @@ static void load_linux(PCMachineState *pcms,
>>>>   #endif
>>>>
>>>>       /* highest address for loading the initrd */
>>>> -    if (protocol >= 0x203) {
>>>> +    if (protocol >= 0x20c &&
>>>> +        lduw_p(header+0x236) & XLF_CAN_BE_LOADED_ABOVE_4G) {
>>>> +        /*
>>>> +         * Although kernel allows initrd loading to above 4G,
>>>> +         * it just makes it as large as possible while still staying below 4G
>>>> +         * since current BIOS always loads initrd below pcms->below_4g_mem_size
>>>> +         */
>>>> +        initrd_max = UINT32_MAX;
>>>> +    } else if (protocol >= 0x203) {
>>>>           initrd_max = ldl_p(header+0x22c);
>>>>       } else {
>>>>           initrd_max = 0x37ffffff;
>>>
>>> I still have trouble understanding the above.
>>> Anyone else wants to comment / help rephrase the comment
>>> and commit log so it's readable?
>>
>> The comment seems to contradict what I see on the code:
>>
>> | Although kernel allows initrd loading to above 4G,
>>
>> Sounds correct.
>>
>>
>> | it just makes it as large as possible while still staying below 4G
>>
>> I'm not a native English speaker, but I believe "it" here should
>> be interpreted as "the kernel", which would be incorrect.  It's
>> this QEMU function that limits initrd_max to a uint32 value, not
>> the kernel.
>>
>>
>> | since current BIOS always loads initrd below pcms->below_4g_mem_size
>>
>> I don't know why the BIOS is mentioned here.  The
>> below_4g_mem_size limit comes from these 2 lines inside
>> load_linux():
>>
>>      if (initrd_max >= pcms->below_4g_mem_size - pcmc->acpi_data_size) {
>>          initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
>>      }
>> In addition to that, initrd_max is uint32_t simply because QEMU
>> doesn't support the 64-bit boot protocol (specifically the
>> ext_ramdisk_image field), so all talk about below_4g_mem_size
>> seems to be just a distraction.
>>
>> All that said, I miss one piece of information here: is
>> XLF_CAN_BE_LOADED_ABOVE_4G really supposed to override
>> header+0x22c?  linux/Documentation/x86/boot.txt isn't clear about
>> that.  Is there any reference that can help us confirm this?
> Looking at the following patch seems that we can confirm the assumption:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=4bf7111f50167133a71c23530ca852a41355e739
>
> Note: the patch was reverted due to bugs in some firmwares, but IMHO
> the assumption is correct.
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=47226ad4f4cfd1e91ded7f2ec42f83ff1c624663

thanks for you info.

When use '-kernel vmlinux -initrd initrd.cgz' to launch a VM,
the firmware(it could be linuxboot_dma.bin) helps to read initrd
contents into guest memory(below ramdisk_max) and jump to kernel.
that's similar with what bootloader does, like grub.

And firmware can work well with some fixes in this patchset.

Zhijian


>
> Cheers,
> Stefano
>
>> --
>> Eduardo
>>
>
> .
>

Re: [Qemu-devel] [PATCH for-4.0 v4 4/4] i386: allow to load initrd below 4G for recent linux

[Paolo Bonzini]

On 27/12/18 21:31, Eduardo Habkost wrote:
> All that said, I miss one piece of information here: is
> XLF_CAN_BE_LOADED_ABOVE_4G really supposed to override
> header+0x22c?  linux/Documentation/x86/boot.txt isn't clear about
> that.  Is there any reference that can help us confirm this?

Linux has supported initrd up to 4 GB for a very long time (2007, long
before XLF_CAN_BE_LOADED_ABOVE_4G which was added in 2013), though it
only sets initrd_max to 2 GB to "work around bootloader bugs".  So I
guess the flag can be taken as a hint that you can load at any address,
and perhaps could be renamed.

Paolo

Re: [Qemu-devel] [PATCH for-4.0 v4 0/4] allow to load initrd below 4G for recent kernel

[Li Zhijian]

ping

On 12/6/18 10:32, Li Zhijian wrote:
> Long long ago, linux kernel has 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 support 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/BIOS always loads initrd below below_4g_mem_size which always
> less than 4G, so here limiting initrd_max to 4G - 1 simply is enough if
> this bit is set.
>
> Default roms(Seabios + optionrom(linuxboot_dma)) works as expected with this
> patchset.
>
> changes:
> V4:
>    - add Reviwed-by tag to 1/4 and 2/4
>    - use scripts/update-linux-headers.sh to import bootparam.h
>    - minor fix at commit log
> V3:
>   - rebase code basing on http://patchwork.ozlabs.org/cover/1005990 and
>     https://patchew.org/QEMU/20181122133507.30950-1-peter.maydell@linaro.org
>   - add new patch 3/4 to import header bootparam.h (Michael S. Tsirkin)
>
> V2: add 2 patches(3/5, 4/5) to fix potential loading issue.
>
> Li Zhijian (4):
>    unify len and addr type for memory/address APIs
>    refactor load_image_size
>    i386: import & use bootparam.h
>    i386: allow to load initrd below 4G for recent linux
>
>   exec.c                                       | 47 ++++++++++++++--------------
>   hw/core/loader.c                             | 11 +++----
>   hw/i386/pc.c                                 | 18 ++++++-----
>   include/exec/cpu-all.h                       |  2 +-
>   include/exec/cpu-common.h                    |  8 ++---
>   include/exec/memory.h                        | 22 ++++++-------
>   include/standard-headers/asm-x86/bootparam.h | 34 ++++++++++++++++++++
>   scripts/update-linux-headers.sh              |  4 +++
>   8 files changed, 92 insertions(+), 54 deletions(-)
>   create mode 100644 include/standard-headers/asm-x86/bootparam.h
>