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

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

[Peter Maydell]

On Mon, 3 Dec 2018 at 14:39, 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 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>
> CC: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>

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

thanks
-- PMM

[Qemu-devel] [PATCH for-4.0 v3 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:
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 bootparam.h
  i386: allow to load initrd below 4G for recent linux

 exec.c                    | 47 +++++++++++++++++++++++------------------------
 hw/core/loader.c          | 11 +++++------
 hw/i386/pc.c              | 25 ++++++++++---------------
 include/exec/cpu-all.h    |  2 +-
 include/exec/cpu-common.h |  8 ++++----
 include/exec/memory.h     | 22 +++++++++++-----------
 6 files changed, 54 insertions(+), 61 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH for-4.0 v3 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 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>
CC: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

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

[Qemu-devel] [PATCH for-4.0 v3 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>
---
 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

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

[Li Zhijian]

It provides setup_data struct and header fields

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

---
V3: new patch
---
 hw/i386/pc.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 067d23a..8db7417 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 <asm/bootparam.h>
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -820,20 +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;
-    uint32_t len;
-    uint8_t data[0];
-} __attribute__((packed));
-
 static void load_linux(PCMachineState *pcms,
                        FWCfgState *fw_cfg)
 {
-- 
2.7.4

[Qemu-devel] [PATCH for-4.0 v3 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 8db7417..ebaa65f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -897,7 +897,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 v3 1/4] unify len and addr type for memory/address APIs

[Richard Henderson]

On 12/3/18 8:48 AM, Li Zhijian wrote:
> Some address/memory APIs have different type between
> 'hwaddr/target_ulong 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>
> CC: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> 
> ---
> 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: Richard Henderson <richard.henderson@linaro.org>


r~

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

[Richard Henderson]

On 12/3/18 8:48 AM, 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>
> ---
>  hw/core/loader.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

[Richard Henderson]

On 12/3/18 8:48 AM, Li Zhijian wrote:
>  #include "hw/net/ne2000-isa.h"
> +#include <asm/bootparam.h>
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -820,20 +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;
> -    uint32_t len;
> -    uint8_t data[0];
> -} __attribute__((packed));
> -

No, this will fail to build for a non-x86 host, which will not have
<asm/bootparam.h>.  Or, perhaps, have a *different* asm/bootparam.h that will
be specific to a different host cpu.


r~

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

[Michael S. Tsirkin]

On Mon, Dec 03, 2018 at 10:48:51PM +0800, Li Zhijian wrote:
> It provides setup_data struct and header fields
> 
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>


Sorry I was unclear when I said "import it from Linux".

The way to import it is to copy the header
from the linux kernel by adding it to ./scripts/update-linux-headers.sh
and placing a copy into include/standard-headers/

Hope that helps.

> ---
> V3: new patch
> ---
>  hw/i386/pc.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 067d23a..8db7417 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 <asm/bootparam.h>
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -820,20 +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;
> -    uint32_t len;
> -    uint8_t data[0];
> -} __attribute__((packed));
> -
>  static void load_linux(PCMachineState *pcms,
>                         FWCfgState *fw_cfg)
>  {
> -- 
> 2.7.4

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

[Li Zhijian]

On 12/04/2018 09:06 AM, Michael S. Tsirkin wrote:
> On Mon, Dec 03, 2018 at 10:48:51PM +0800, Li Zhijian wrote:
>> It provides setup_data struct and header fields
>>
>> CC: Michael S. Tsirkin<mst@redhat.com>
>> Signed-off-by: Li Zhijian<lizhijian@cn.fujitsu.com>
> Sorry I was unclear when I said "import it from Linux".

Sorry, it's my fault.

>
> The way to import it is to copy the header
> from the linux kernel by adding it to ./scripts/update-linux-headers.sh
> and placing a copy into include/standard-headers/
>
> Hope that helps.

that's really helpful.

i wonder which type copy should i use 'cp' or 'cp_portable'
this header includes several sub-headers

$ grep include  /tmp/tmp.MBF10Z9MeS/include/asm/bootparam.h
#include <linux/types.h>
#include <linux/screen_info.h>
#include <linux/apm_bios.h>
#include <linux/edd.h>
#include <asm/ist.h>
#include <video/edid.h>

Thanks
Zhijian

>
>
>

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

[Philippe Mathieu-Daudé]

Hi Li,

On 3/12/18 15:48, Li Zhijian wrote:
> Some address/memory APIs have different type between
> 'hwaddr/target_ulong addr' and 'int len'. It is very unsafety, espcially

I'm not native English speaker, but I think this should be spell:

... "very unsafe, 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>
> 
> ---
> 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)) {
> 

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

[Li Zhijian]

Hi Michael

I cooked a draft with cp_portable to import bootparam.h, could you have a look.

diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index 0a964fe..1beeceb 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -44,6 +44,12 @@ cp_portable() {
                                       -e 'linux/kernel' \
                                       -e 'linux/sysinfo' \
                                       -e 'asm-generic/kvm_para' \
+                                     -e 'linux/screen_info.h' \
+                                     -e 'linux/apm_bios.h' \
+                                     -e 'linux/edd.h' \
+                                     -e 'video/edid.h' \
+                                     -e 'asm/ist.h' \
+                                     -e 'linux/ioctl.h' \
                                       > /dev/null
      then
          echo "Unexpected #include in input file $f".
@@ -59,6 +65,8 @@ cp_portable() {
          -e 's/__be\([0-9][0-9]*\)/uint\1_t/g' \
          -e 's/"\(input-event-codes\.h\)"/"standard-headers\/linux\/\1"/' \
          -e 's/<linux\/\([^>]*\)>/"standard-headers\/linux\/\1"/' \
+        -e "s/<asm\/\([^>]*\)>/\"standard-headers\/asm-$arch\/\1\"/" \
+        -e 's/<video\/\([^>]*\)>/"standard-headers\/video\/\1"/' \
          -e 's/__bitwise//' \
          -e 's/__attribute__((packed))/QEMU_PACKED/' \
          -e 's/__inline__/inline/' \
@@ -74,6 +82,23 @@ cp_portable() {
          "$f" > "$to/$header";
  }
  
+rm -rf "$output/include/standard-headers/linux"
+mkdir -p "$output/include/standard-headers/linux"
+
+cp_bootparam()
+{
+    mkdir -p $output/include/standard-headers/video
+    cp "$tmpdir"/include/linux/ioctl.h "$output/include/standard-headers/linux"
+    cp_portable "$tmpdir"/include/linux/screen_info.h "$output/include/standard-headers/linux"
+    cp_portable "$tmpdir/include/linux/apm_bios.h" "$output/include/standard-headers/linux"
+    cp_portable "$tmpdir/include/linux/edd.h" "$output/include/standard-headers/linux"
+    cp_portable "$tmpdir/include/asm/ist.h" $output/include/standard-headers/asm-$arch
+    cp_portable "$tmpdir/include/video/edid.h" $output/include/standard-headers/video
+
+    # bootparam.h includes above headers
+    cp_portable "$tmpdir/include/asm/bootparam.h" "$output/include/standard-headers/asm-$arch"
+}
+
  # This will pick up non-directories too (eg "Kconfig") but we will
  # ignore them in the next loop.
  ARCHLIST=$(cd "$linux/arch" && echo *)
@@ -120,6 +145,7 @@ 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"
+        cp_bootparam
      fi
  done
  
@@ -163,8 +189,6 @@ cat <<EOF >$output/linux-headers/linux/virtio_ring.h
  #include "standard-headers/linux/virtio_ring.h"
  EOF
  
-rm -rf "$output/include/standard-headers/linux"
-mkdir -p "$output/include/standard-headers/linux"
  for i in "$tmpdir"/include/linux/*virtio*.h \
           "$tmpdir/include/linux/qemu_fw_cfg.h" \
           "$tmpdir/include/linux/input.h" \

Thanks
Zhijian


On 12/04/2018 04:09 PM, Li Zhijian wrote:
>
>
>
> On 12/04/2018 09:06 AM, Michael S. Tsirkin wrote:
>> On Mon, Dec 03, 2018 at 10:48:51PM +0800, Li Zhijian wrote:
>>> It provides setup_data struct and header fields
>>>
>>> CC: Michael S. Tsirkin<mst@redhat.com>
>>> Signed-off-by: Li Zhijian<lizhijian@cn.fujitsu.com>
>> Sorry I was unclear when I said "import it from Linux".
> Sorry, it's my fault.
>
>> The way to import it is to copy the header
>> from the linux kernel by adding it to ./scripts/update-linux-headers.sh
>> and placing a copy into include/standard-headers/
>>
>> Hope that helps.
> that's really helpful.
>
> i wonder which type copy should i use 'cp' or 'cp_portable'
> this header includes several sub-headers
>
> $ grep include  /tmp/tmp.MBF10Z9MeS/include/asm/bootparam.h
> #include <linux/types.h>
> #include <linux/screen_info.h>
> #include <linux/apm_bios.h>
> #include <linux/edd.h>
> #include <asm/ist.h>
> #include <video/edid.h>
>
> Thanks
> Zhijian
>
>>
>      
>    
>
>
>
>

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

[Michael S. Tsirkin]

On Wed, Dec 05, 2018 at 06:28:11PM +0800, Li Zhijian wrote:
> Hi Michael
> 
> I cooked a draft with cp_portable to import bootparam.h, could you have a look.
> 
> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
> index 0a964fe..1beeceb 100755
> --- a/scripts/update-linux-headers.sh
> +++ b/scripts/update-linux-headers.sh
> @@ -44,6 +44,12 @@ cp_portable() {
>                                       -e 'linux/kernel' \
>                                       -e 'linux/sysinfo' \
>                                       -e 'asm-generic/kvm_para' \
> +                                     -e 'linux/screen_info.h' \
> +                                     -e 'linux/apm_bios.h' \
> +                                     -e 'linux/edd.h' \
> +                                     -e 'video/edid.h' \
> +                                     -e 'asm/ist.h' \
> +                                     -e 'linux/ioctl.h' \
>                                       > /dev/null
>      then
>          echo "Unexpected #include in input file $f".
> @@ -59,6 +65,8 @@ cp_portable() {
>          -e 's/__be\([0-9][0-9]*\)/uint\1_t/g' \
>          -e 's/"\(input-event-codes\.h\)"/"standard-headers\/linux\/\1"/' \
>          -e 's/<linux\/\([^>]*\)>/"standard-headers\/linux\/\1"/' \
> +        -e "s/<asm\/\([^>]*\)>/\"standard-headers\/asm-$arch\/\1\"/" \
> +        -e 's/<video\/\([^>]*\)>/"standard-headers\/video\/\1"/' \
>          -e 's/__bitwise//' \
>          -e 's/__attribute__((packed))/QEMU_PACKED/' \
>          -e 's/__inline__/inline/' \
> @@ -74,6 +82,23 @@ cp_portable() {
>          "$f" > "$to/$header";
>  }
> 
> +rm -rf "$output/include/standard-headers/linux"
> +mkdir -p "$output/include/standard-headers/linux"
> +
> +cp_bootparam()
> +{
> +    mkdir -p $output/include/standard-headers/video
> +    cp "$tmpdir"/include/linux/ioctl.h "$output/include/standard-headers/linux"
> +    cp_portable "$tmpdir"/include/linux/screen_info.h "$output/include/standard-headers/linux"
> +    cp_portable "$tmpdir/include/linux/apm_bios.h" "$output/include/standard-headers/linux"
> +    cp_portable "$tmpdir/include/linux/edd.h" "$output/include/standard-headers/linux"
> +    cp_portable "$tmpdir/include/asm/ist.h" $output/include/standard-headers/asm-$arch
> +    cp_portable "$tmpdir/include/video/edid.h" $output/include/standard-headers/video
> +
> +    # bootparam.h includes above headers
> +    cp_portable "$tmpdir/include/asm/bootparam.h" "$output/include/standard-headers/asm-$arch"
> +}
> +
>  # This will pick up non-directories too (eg "Kconfig") but we will
>  # ignore them in the next loop.
>  ARCHLIST=$(cd "$linux/arch" && echo *)
> @@ -120,6 +145,7 @@ 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"
> +        cp_bootparam
>      fi
>  done
> 
> @@ -163,8 +189,6 @@ cat <<EOF >$output/linux-headers/linux/virtio_ring.h
>  #include "standard-headers/linux/virtio_ring.h"
>  EOF
> 
> -rm -rf "$output/include/standard-headers/linux"
> -mkdir -p "$output/include/standard-headers/linux"
>  for i in "$tmpdir"/include/linux/*virtio*.h \
>           "$tmpdir/include/linux/qemu_fw_cfg.h" \
>           "$tmpdir/include/linux/input.h" \
> 
> Thanks
> Zhijian
> 

So arch specific asm including asm doesn't work well right now :(
You can either fix the path to ist to pull it from asm-x86,
or if you don't actually need anything in that header the
macros, you can just cut out everything around __ASSEMBLY__
with a bit of e.g. sed magic. E.g. pvrdma does this.

Something like:

# Remove everything except the macros from bootparam.h avoiding the unnecessary
# import of several video/ist/etc headers
sed  -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' arch/x86/include/uapi/asm/bootparam.h

should do the job.


> 
> On 12/04/2018 04:09 PM, Li Zhijian wrote:
> 
> 
> 
> 
>     On 12/04/2018 09:06 AM, Michael S. Tsirkin wrote:
> 
>         On Mon, Dec 03, 2018 at 10:48:51PM +0800, Li Zhijian wrote:
> 
>             It provides setup_data struct and header fields
> 
>             CC: Michael S. Tsirkin <mst@redhat.com>
>             Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> 
>         Sorry I was unclear when I said "import it from Linux".
> 
>     Sorry, it's my fault.
> 
> 
>         The way to import it is to copy the header
>         from the linux kernel by adding it to ./scripts/update-linux-headers.sh
>         and placing a copy into include/standard-headers/
> 
>         Hope that helps.
> 
>     that's really helpful.
> 
>     i wonder which type copy should i use 'cp' or 'cp_portable'
>     this header includes several sub-headers
> 
>     $ grep include  /tmp/tmp.MBF10Z9MeS/include/asm/bootparam.h
>     #include <linux/types.h>
>     #include <linux/screen_info.h>
>     #include <linux/apm_bios.h>
>     #include <linux/edd.h>
>     #include <asm/ist.h>
>     #include <video/edid.h>
> 
>     Thanks
>     Zhijian
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

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

[Li Zhijian]

On 12/05/2018 01:40 AM, Philippe Mathieu-Daudé wrote:
> Hi Li,
>
> On 3/12/18 15:48, Li Zhijian wrote:
>> Some address/memory APIs have different type between
>> 'hwaddr/target_ulong addr' and 'int len'. It is very unsafety, espcially
> I'm not native English speaker, but I think this should be spell:
>
> ... "very unsafe, especially" ...

thanks, Google said so.

Thanks
Zhijian

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

[Li Zhijian]

On 12/05/2018 11:33 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 05, 2018 at 06:28:11PM +0800, Li Zhijian wrote:
>> Hi Michael
>>
>> I cooked a draft with cp_portable to import bootparam.h, could you have a look.
>>
>> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
>> index 0a964fe..1beeceb 100755
>> --- a/scripts/update-linux-headers.sh
>> +++ b/scripts/update-linux-headers.sh
>> @@ -44,6 +44,12 @@ cp_portable() {
>>                                        -e 'linux/kernel' \
>>                                        -e 'linux/sysinfo' \
>>                                        -e 'asm-generic/kvm_para' \
>> +                                     -e 'linux/screen_info.h' \
>> +                                     -e 'linux/apm_bios.h' \
>> +                                     -e 'linux/edd.h' \
>> +                                     -e 'video/edid.h' \
>> +                                     -e 'asm/ist.h' \
>> +                                     -e 'linux/ioctl.h' \
>>                                        > /dev/null
>>       then
>>           echo "Unexpected #include in input file $f".
>> @@ -59,6 +65,8 @@ cp_portable() {
>>           -e 's/__be\([0-9][0-9]*\)/uint\1_t/g' \
>>           -e 's/"\(input-event-codes\.h\)"/"standard-headers\/linux\/\1"/' \
>>           -e 's/<linux\/\([^>]*\)>/"standard-headers\/linux\/\1"/' \
>> +        -e "s/<asm\/\([^>]*\)>/\"standard-headers\/asm-$arch\/\1\"/" \
>> +        -e 's/<video\/\([^>]*\)>/"standard-headers\/video\/\1"/' \
>>           -e 's/__bitwise//' \
>>           -e 's/__attribute__((packed))/QEMU_PACKED/' \
>>           -e 's/__inline__/inline/' \
>> @@ -74,6 +82,23 @@ cp_portable() {
>>           "$f" > "$to/$header";
>>   }
>>
>> +rm -rf "$output/include/standard-headers/linux"
>> +mkdir -p "$output/include/standard-headers/linux"
>> +
>> +cp_bootparam()
>> +{
>> +    mkdir -p $output/include/standard-headers/video
>> +    cp "$tmpdir"/include/linux/ioctl.h "$output/include/standard-headers/linux"
>> +    cp_portable "$tmpdir"/include/linux/screen_info.h "$output/include/standard-headers/linux"
>> +    cp_portable "$tmpdir/include/linux/apm_bios.h" "$output/include/standard-headers/linux"
>> +    cp_portable "$tmpdir/include/linux/edd.h" "$output/include/standard-headers/linux"
>> +    cp_portable "$tmpdir/include/asm/ist.h" $output/include/standard-headers/asm-$arch
>> +    cp_portable "$tmpdir/include/video/edid.h" $output/include/standard-headers/video
>> +
>> +    # bootparam.h includes above headers
>> +    cp_portable "$tmpdir/include/asm/bootparam.h" "$output/include/standard-headers/asm-$arch"
>> +}
>> +
>>   # This will pick up non-directories too (eg "Kconfig") but we will
>>   # ignore them in the next loop.
>>   ARCHLIST=$(cd "$linux/arch" && echo *)
>> @@ -120,6 +145,7 @@ 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"
>> +        cp_bootparam
>>       fi
>>   done
>>
>> @@ -163,8 +189,6 @@ cat <<EOF >$output/linux-headers/linux/virtio_ring.h
>>   #include "standard-headers/linux/virtio_ring.h"
>>   EOF
>>
>> -rm -rf "$output/include/standard-headers/linux"
>> -mkdir -p "$output/include/standard-headers/linux"
>>   for i in "$tmpdir"/include/linux/*virtio*.h \
>>            "$tmpdir/include/linux/qemu_fw_cfg.h" \
>>            "$tmpdir/include/linux/input.h" \
>>
>> Thanks
>> Zhijian
>>
> So arch specific asm including asm doesn't work well right now :(
> You can either fix the path to ist to pull it from asm-x86,

> +        -e "s/<asm\/\([^>]*\)>/\"standard-headers\/asm-$arch\/\1\"/" \
> +        -e 's/<video\/\([^>]*\)>/"standard-headers\/video\/\1"/' \


Actually above changes fix the path with asm as well.
But I'd like below solution which is simpler and clearer


> or if you don't actually need anything in that header the
> macros, you can just cut out everything around __ASSEMBLY__
> with a bit of e.g. sed magic. E.g. pvrdma does this.
>
> Something like:
>
> # Remove everything except the macros from bootparam.h avoiding the unnecessary
> # import of several video/ist/etc headers
> sed  -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' arch/x86/include/uapi/asm/bootparam.h
>
> should do the job.

Thanks
Zhijian