[Qemu-devel] [PATCH v2 0/8] Guest memory allocation fixes & cleanup

[Qemu-devel] [PATCH v2 0/8] Guest memory allocation fixes & cleanup

[Markus Armbruster]

All I wanted to do is exit(1) instead of abort() on guest memory
allocation failure [07/08].  But that lead me into a minor #ifdef bog,
and here's what I brought back.  Enjoy!

Testing:
* Christian Borntraeger reports v1 works fine under LPAR (new S390
  KVM, i.e. generic allocation) and as second guest under z/VM (old
  S390 KVM, i.e. legacy S390 allocation).  Thanks for testing, and for
  catching a stupid mistake.  v2 differs from v1 only in code that
  isn't reachable on S390.

Changes since v1:
* 5/8: Fix assertion in qemu_ram_remap() (Paolo)
* All other patches unchanged except for Acked-by in commit messages
Changes since RFC:
* 1-3+8/8 unchanged except for commit message tweaks
* 4+6/8 rewritten to address Paolo's review
* 5/8 rewritten: don't fix dead code, just assert it's dead
* 7/8 fix mistakes caught by Richard Henderson and Peter Maydell

Markus Armbruster (8):
  exec: Fix Xen RAM allocation with unusual options
  exec: Clean up fall back when -mem-path allocation fails
  exec: Reduce ifdeffery around -mem-path
  exec: Simplify the guest physical memory allocation hook
  exec: Drop incorrect & dead S390 code in qemu_ram_remap()
  exec: Clean up unnecessary S390 ifdeffery
  exec: Don't abort when we can't allocate guest memory
  pc_sysfw: Fix ISA BIOS init for ridiculously big flash

 exec.c                  | 121 ++++++++++++++++++++++++++----------------------
 hw/block/pc_sysfw.c     |   5 +-
 include/exec/cpu-all.h  |   2 -
 include/exec/exec-all.h |   2 +
 include/sysemu/kvm.h    |   5 --
 kvm-all.c               |  13 ------
 target-s390x/kvm.c      |  23 +++------
 util/oslib-posix.c      |   4 +-
 util/oslib-win32.c      |   5 +-
 9 files changed, 78 insertions(+), 102 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH v2 1/8] exec: Fix Xen RAM allocation with unusual options

[Markus Armbruster]

Issues:

* We try to obey -mem-path even though it can't work with Xen.

* To implement -machine mem-merge, we call
  memory_try_enable_merging(new_block->host, size).  But with Xen,
  new_block->host remains null.  Oops.

Fix by separating Xen allocation from normal allocation.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index 5b8b40d..b424e12 100644
--- a/exec.c
+++ b/exec.c
@@ -1081,6 +1081,12 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
     if (host) {
         new_block->host = host;
         new_block->flags |= RAM_PREALLOC_MASK;
+    } else if (xen_enabled()) {
+        if (mem_path) {
+            fprintf(stderr, "-mem-path not supported with Xen\n");
+            exit(1);
+        }
+        xen_ram_alloc(new_block->offset, size, mr);
     } else {
         if (mem_path) {
 #if defined (__linux__) && !defined(TARGET_S390X)
@@ -1094,9 +1100,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
             exit(1);
 #endif
         } else {
-            if (xen_enabled()) {
-                xen_ram_alloc(new_block->offset, size, mr);
-            } else if (kvm_enabled()) {
+            if (kvm_enabled()) {
                 /* some s390/kvm configurations have special constraints */
                 new_block->host = kvm_ram_alloc(size);
             } else {
@@ -1174,6 +1178,8 @@ void qemu_ram_free(ram_addr_t addr)
             ram_list.version++;
             if (block->flags & RAM_PREALLOC_MASK) {
                 ;
+            } else if (xen_enabled()) {
+                xen_invalidate_map_cache_entry(block->host);
             } else if (mem_path) {
 #if defined (__linux__) && !defined(TARGET_S390X)
                 if (block->fd) {
@@ -1186,11 +1192,7 @@ void qemu_ram_free(ram_addr_t addr)
                 abort();
 #endif
             } else {
-                if (xen_enabled()) {
-                    xen_invalidate_map_cache_entry(block->host);
-                } else {
-                    qemu_anon_ram_free(block->host, block->length);
-                }
+                qemu_anon_ram_free(block->host, block->length);
             }
             g_free(block);
             break;
@@ -1214,6 +1216,8 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
             vaddr = block->host + offset;
             if (block->flags & RAM_PREALLOC_MASK) {
                 ;
+            } else if (xen_enabled()) {
+                abort();
             } else {
                 flags = MAP_FIXED;
                 munmap(vaddr, length);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 2/8] exec: Clean up fall back when -mem-path allocation fails

[Markus Armbruster]

With -mem-path, qemu_ram_alloc_from_ptr() first tries to allocate
accordingly, but when it fails, it falls back to normal allocation.

The fall back allocation code used to be effectively identical to the
"-mem-path not given" code, until it started to diverge in commit
432d268.  I believe the code still works, but clean it up anyway: drop
the special fall back allocation code, and fall back to the ordinary
"-mem-path not given" code instead.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/exec.c b/exec.c
index b424e12..56c31a9 100644
--- a/exec.c
+++ b/exec.c
@@ -1091,15 +1091,12 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
         if (mem_path) {
 #if defined (__linux__) && !defined(TARGET_S390X)
             new_block->host = file_ram_alloc(new_block, size, mem_path);
-            if (!new_block->host) {
-                new_block->host = qemu_anon_ram_alloc(size);
-                memory_try_enable_merging(new_block->host, size);
-            }
 #else
             fprintf(stderr, "-mem-path option unsupported\n");
             exit(1);
 #endif
-        } else {
+        }
+        if (!new_block->host) {
             if (kvm_enabled()) {
                 /* some s390/kvm configurations have special constraints */
                 new_block->host = kvm_ram_alloc(size);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 3/8] exec: Reduce ifdeffery around -mem-path

[Markus Armbruster]

Instead of spreading its ifdeffery everywhere, confine it to
qemu_ram_alloc_from_ptr().  Everywhere else, simply test block->fd,
which is non-negative exactly when block uses -mem-path.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c                 | 37 ++++++++++---------------------------
 include/exec/cpu-all.h |  2 --
 2 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/exec.c b/exec.c
index 56c31a9..4dbb0f1 100644
--- a/exec.c
+++ b/exec.c
@@ -1073,6 +1073,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
 
     size = TARGET_PAGE_ALIGN(size);
     new_block = g_malloc0(sizeof(*new_block));
+    new_block->fd = -1;
 
     /* This assumes the iothread lock is taken here too.  */
     qemu_mutex_lock_ramlist();
@@ -1177,17 +1178,9 @@ void qemu_ram_free(ram_addr_t addr)
                 ;
             } else if (xen_enabled()) {
                 xen_invalidate_map_cache_entry(block->host);
-            } else if (mem_path) {
-#if defined (__linux__) && !defined(TARGET_S390X)
-                if (block->fd) {
-                    munmap(block->host, block->length);
-                    close(block->fd);
-                } else {
-                    qemu_anon_ram_free(block->host, block->length);
-                }
-#else
-                abort();
-#endif
+            } else if (block->fd >= 0) {
+                munmap(block->host, block->length);
+                close(block->fd);
             } else {
                 qemu_anon_ram_free(block->host, block->length);
             }
@@ -1218,25 +1211,15 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
             } else {
                 flags = MAP_FIXED;
                 munmap(vaddr, length);
-                if (mem_path) {
-#if defined(__linux__) && !defined(TARGET_S390X)
-                    if (block->fd) {
+                if (block->fd >= 0) {
 #ifdef MAP_POPULATE
-                        flags |= mem_prealloc ? MAP_POPULATE | MAP_SHARED :
-                            MAP_PRIVATE;
+                    flags |= mem_prealloc ? MAP_POPULATE | MAP_SHARED :
+                        MAP_PRIVATE;
 #else
-                        flags |= MAP_PRIVATE;
-#endif
-                        area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
-                                    flags, block->fd, offset);
-                    } else {
-                        flags |= MAP_PRIVATE | MAP_ANONYMOUS;
-                        area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
-                                    flags, -1, 0);
-                    }
-#else
-                    abort();
+                    flags |= MAP_PRIVATE;
 #endif
+                    area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
+                                flags, block->fd, offset);
                 } else {
 #if defined(TARGET_S390X) && defined(CONFIG_KVM)
                     flags |= MAP_SHARED | MAP_ANONYMOUS;
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index e9c3717..c369b25 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -476,9 +476,7 @@ typedef struct RAMBlock {
      * Writes must take both locks.
      */
     QTAILQ_ENTRY(RAMBlock) next;
-#if defined(__linux__) && !defined(TARGET_S390X)
     int fd;
-#endif
 } RAMBlock;
 
 typedef struct RAMList {
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 4/8] exec: Simplify the guest physical memory allocation hook

[Markus Armbruster]

Make it a generic hook rather than a KVM hook.  Less code and
ifdeffery.

Since the only user of the hook is old S390 KVM, there's hope we can
get rid of it some day.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c                  | 20 ++++++++++++++------
 include/exec/exec-all.h |  2 ++
 include/sysemu/kvm.h    |  5 -----
 kvm-all.c               | 13 -------------
 target-s390x/kvm.c      | 17 ++++++-----------
 5 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/exec.c b/exec.c
index 4dbb0f1..c45eb33 100644
--- a/exec.c
+++ b/exec.c
@@ -685,6 +685,19 @@ typedef struct subpage_t {
 static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
                              uint16_t section);
 static subpage_t *subpage_init(hwaddr base);
+
+static void *(*phys_mem_alloc)(ram_addr_t size) = qemu_anon_ram_alloc;
+
+/*
+ * Set a custom physical guest memory alloator.
+ * Accelerators with unusual needs may need this.  Hopefully, we can
+ * get rid of it eventually.
+ */
+void phys_mem_set_alloc(void *(*alloc)(ram_addr_t))
+{
+    phys_mem_alloc = alloc;
+}
+
 static void destroy_page_desc(uint16_t section_index)
 {
     MemoryRegionSection *section = &phys_sections[section_index];
@@ -1098,12 +1111,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
 #endif
         }
         if (!new_block->host) {
-            if (kvm_enabled()) {
-                /* some s390/kvm configurations have special constraints */
-                new_block->host = kvm_ram_alloc(size);
-            } else {
-                new_block->host = qemu_anon_ram_alloc(size);
-            }
+            new_block->host = phys_mem_alloc(size);
             memory_try_enable_merging(new_block->host, size);
         }
     }
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index b2162a4..4921696 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -369,6 +369,8 @@ bool is_tcg_gen_code(uintptr_t pc_ptr);
 
 #if !defined(CONFIG_USER_ONLY)
 
+void phys_mem_set_alloc(void *(*alloc)(ram_addr_t));
+
 struct MemoryRegion *iotlb_to_region(hwaddr index);
 bool io_mem_read(struct MemoryRegion *mr, hwaddr addr,
                  uint64_t *pvalue, unsigned size);
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 8b19322..e722027 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -151,11 +151,6 @@ int kvm_init_vcpu(CPUState *cpu);
 #ifdef NEED_CPU_H
 int kvm_cpu_exec(CPUArchState *env);
 
-#if !defined(CONFIG_USER_ONLY)
-void *kvm_ram_alloc(ram_addr_t size);
-void *kvm_arch_ram_alloc(ram_addr_t size);
-#endif
-
 void kvm_setup_guest_memory(void *start, size_t size);
 void kvm_flush_coalesced_mmio_buffer(void);
 
diff --git a/kvm-all.c b/kvm-all.c
index 405480e..f88c4ec 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1816,19 +1816,6 @@ int kvm_has_intx_set_mask(void)
     return kvm_state->intx_set_mask;
 }
 
-void *kvm_ram_alloc(ram_addr_t size)
-{
-#ifdef TARGET_S390X
-    void *mem;
-
-    mem = kvm_arch_ram_alloc(size);
-    if (mem) {
-        return mem;
-    }
-#endif
-    return qemu_anon_ram_alloc(size);
-}
-
 void kvm_setup_guest_memory(void *start, size_t size)
 {
 #ifdef CONFIG_VALGRIND_H
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 4d9ac4a..e7863d7 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -92,9 +92,15 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
 
 static int cap_sync_regs;
 
+static void *legacy_s390_alloc(ram_addr_t size);
+
 int kvm_arch_init(KVMState *s)
 {
     cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
+    if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
+        || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
+        phys_mem_set_alloc(legacy_s390_alloc);
+    }
     return 0;
 }
 
@@ -332,17 +338,6 @@ static void *legacy_s390_alloc(ram_addr_t size)
     return mem;
 }
 
-void *kvm_arch_ram_alloc(ram_addr_t size)
-{
-    /* Can we use the standard allocation ? */
-    if (kvm_check_extension(kvm_state, KVM_CAP_S390_GMAP) &&
-        kvm_check_extension(kvm_state, KVM_CAP_S390_COW)) {
-        return NULL;
-    } else {
-        return legacy_s390_alloc(size);
-    }
-}
-
 int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
 {
     S390CPU *cpu = S390_CPU(cs);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 5/8] exec: Drop incorrect & dead S390 code in qemu_ram_remap()

[Markus Armbruster]

Old S390 KVM wants guest RAM mapped in a peculiar way.  Commit 6b02494
implemented that.

When qemu_ram_remap() got added in commit cd19cfa, its code carefully
mimicked the allocation code: peculiar way if defined(TARGET_S390X) &&
defined(CONFIG_KVM), else normal way.

For new S390 KVM, we actually want the normal way.  Commit fdec991
changed qemu_ram_alloc_from_ptr() accordingly, but forgot to update
qemu_ram_remap().  If qemu_ram_alloc_from_ptr() maps RAM the normal
way, but qemu_ram_remap() remaps it the peculiar way, remapping
changes protection and flags, which it shouldn't.

Fortunately, this can't happen, as we never remap on S390.

Replace the incorrect code with an assertion.

Thanks to Christian Borntraeger for help with assessing the bug's
(non-)impact.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/exec.c b/exec.c
index c45eb33..366ac6a 100644
--- a/exec.c
+++ b/exec.c
@@ -1229,15 +1229,16 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
                     area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
                                 flags, block->fd, offset);
                 } else {
-#if defined(TARGET_S390X) && defined(CONFIG_KVM)
-                    flags |= MAP_SHARED | MAP_ANONYMOUS;
-                    area = mmap(vaddr, length, PROT_EXEC|PROT_READ|PROT_WRITE,
-                                flags, -1, 0);
-#else
+                    /*
+                     * Remap needs to match alloc.  Accelerators that
+                     * set phys_mem_alloc never remap.  If they did,
+                     * we'd need a remap hook here.
+                     */
+                    assert(phys_mem_alloc == qemu_anon_ram_alloc);
+
                     flags |= MAP_PRIVATE | MAP_ANONYMOUS;
                     area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
                                 flags, -1, 0);
-#endif
                 }
                 if (area != vaddr) {
                     fprintf(stderr, "Could not remap addr: "
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 6/8] exec: Clean up unnecessary S390 ifdeffery

[Markus Armbruster]

Another issue missed in commit fdec991 is -mem-path: it needs to be
rejected only for old S390 KVM, not for any S390.  Not that I
personally care, but the ifdeffery in qemu_ram_alloc_from_ptr() annoys
me.

Note that this doesn't actually make -mem-path work, as the kernel
doesn't (yet?)  support large pages in the host for KVM guests.  Clean
it up anyway.

Thanks to Christian Borntraeger for pointing out the S390 kernel
limitations.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/exec.c b/exec.c
index 366ac6a..bf2a7d6 100644
--- a/exec.c
+++ b/exec.c
@@ -862,7 +862,7 @@ void qemu_mutex_unlock_ramlist(void)
     qemu_mutex_unlock(&ram_list.mutex);
 }
 
-#if defined(__linux__) && !defined(TARGET_S390X)
+#ifdef __linux__
 
 #include <sys/vfs.h>
 
@@ -965,6 +965,14 @@ static void *file_ram_alloc(RAMBlock *block,
     block->fd = fd;
     return area;
 }
+#else
+static void *file_ram_alloc(RAMBlock *block,
+                            ram_addr_t memory,
+                            const char *path)
+{
+    fprintf(stderr, "-mem-path not supported on this host\n");
+    exit(1);
+}
 #endif
 
 static ram_addr_t find_ram_offset(ram_addr_t size)
@@ -1103,12 +1111,17 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
         xen_ram_alloc(new_block->offset, size, mr);
     } else {
         if (mem_path) {
-#if defined (__linux__) && !defined(TARGET_S390X)
+            if (phys_mem_alloc != qemu_anon_ram_alloc) {
+                /*
+                 * file_ram_alloc() needs to allocate just like
+                 * phys_mem_alloc, but we haven't bothered to provide
+                 * a hook there.
+                 */
+                fprintf(stderr,
+                        "-mem-path not supported with this accelerator\n");
+                exit(1);
+            }
             new_block->host = file_ram_alloc(new_block, size, mem_path);
-#else
-            fprintf(stderr, "-mem-path option unsupported\n");
-            exit(1);
-#endif
         }
         if (!new_block->host) {
             new_block->host = phys_mem_alloc(size);
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 7/8] exec: Don't abort when we can't allocate guest memory

[Markus Armbruster]

We abort() on memory allocation failure.  abort() is appropriate for
programming errors.  Maybe most memory allocation failures are
programming errors, maybe not.  But guest memory allocation failure
isn't, and aborting when the user asks for more memory than we can
provide is not nice.  exit(1) instead, and do it in just one place, so
the error message is consistent.

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c             | 5 +++++
 target-s390x/kvm.c | 6 +-----
 util/oslib-posix.c | 4 +---
 util/oslib-win32.c | 5 +----
 4 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/exec.c b/exec.c
index bf2a7d6..3f7fe29 100644
--- a/exec.c
+++ b/exec.c
@@ -1125,6 +1125,11 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
         }
         if (!new_block->host) {
             new_block->host = phys_mem_alloc(size);
+            if (!new_block->host) {
+                fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
+                        new_block->mr->name, strerror(errno));
+                exit(1);
+            }
             memory_try_enable_merging(new_block->host, size);
         }
     }
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index e7863d7..b1ffcea 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -331,11 +331,7 @@ static void *legacy_s390_alloc(ram_addr_t size)
     mem = mmap((void *) 0x800000000ULL, size,
                PROT_EXEC|PROT_READ|PROT_WRITE,
                MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
-    if (mem == MAP_FAILED) {
-        fprintf(stderr, "Allocating RAM failed\n");
-        abort();
-    }
-    return mem;
+    return mem == MAP_FAILED ? NULL : mem;
 }
 
 int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 3dc8b1b..253bc3d 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -112,9 +112,7 @@ void *qemu_anon_ram_alloc(size_t size)
     size_t offset = QEMU_ALIGN_UP((uintptr_t)ptr, align) - (uintptr_t)ptr;
 
     if (ptr == MAP_FAILED) {
-        fprintf(stderr, "Failed to allocate %zu B: %s\n",
-                size, strerror(errno));
-        abort();
+        return NULL;
     }
 
     ptr += offset;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 961fbf5..983b7a2 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -65,10 +65,7 @@ void *qemu_anon_ram_alloc(size_t size)
     /* FIXME: this is not exactly optimal solution since VirtualAlloc
        has 64Kb granularity, but at least it guarantees us that the
        memory is page aligned. */
-    if (!size) {
-        abort();
-    }
-    ptr = qemu_oom_check(VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE));
+    ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
     trace_qemu_anon_ram_alloc(size, ptr);
     return ptr;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 8/8] pc_sysfw: Fix ISA BIOS init for ridiculously big flash

[Markus Armbruster]

pc_isa_bios_init() suffers integer overflow for flash larger than
INT_MAX.

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

diff --git a/hw/block/pc_sysfw.c b/hw/block/pc_sysfw.c
index 412d1b0..aebefc9 100644
--- a/hw/block/pc_sysfw.c
+++ b/hw/block/pc_sysfw.c
@@ -54,10 +54,7 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
     flash_size = memory_region_size(flash_mem);
 
     /* map the last 128KB of the BIOS in ISA space */
-    isa_bios_size = flash_size;
-    if (isa_bios_size > (128 * 1024)) {
-        isa_bios_size = 128 * 1024;
-    }
+    isa_bios_size = MIN(flash_size, 128 * 1024);
     isa_bios = g_malloc(sizeof(*isa_bios));
     memory_region_init_ram(isa_bios, "isa-bios", isa_bios_size);
     vmstate_register_ram_global(isa_bios);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v2 0/8] Guest memory allocation fixes & cleanup

[Martin Kletzander]

On 06/20/2013 09:54 AM, Markus Armbruster wrote:
> All I wanted to do is exit(1) instead of abort() on guest memory
> allocation failure [07/08].  But that lead me into a minor #ifdef bog,
> and here's what I brought back.  Enjoy!
> 
> Testing:
> * Christian Borntraeger reports v1 works fine under LPAR (new S390
>   KVM, i.e. generic allocation) and as second guest under z/VM (old
>   S390 KVM, i.e. legacy S390 allocation).  Thanks for testing, and for
>   catching a stupid mistake.  v2 differs from v1 only in code that
>   isn't reachable on S390.
> 
> Changes since v1:
> * 5/8: Fix assertion in qemu_ram_remap() (Paolo)
> * All other patches unchanged except for Acked-by in commit messages
> Changes since RFC:
> * 1-3+8/8 unchanged except for commit message tweaks
> * 4+6/8 rewritten to address Paolo's review
> * 5/8 rewritten: don't fix dead code, just assert it's dead
> * 7/8 fix mistakes caught by Richard Henderson and Peter Maydell
> 
> Markus Armbruster (8):
>   exec: Fix Xen RAM allocation with unusual options
>   exec: Clean up fall back when -mem-path allocation fails
>   exec: Reduce ifdeffery around -mem-path
>   exec: Simplify the guest physical memory allocation hook
>   exec: Drop incorrect & dead S390 code in qemu_ram_remap()
>   exec: Clean up unnecessary S390 ifdeffery
>   exec: Don't abort when we can't allocate guest memory
>   pc_sysfw: Fix ISA BIOS init for ridiculously big flash
> 
>  exec.c                  | 121 ++++++++++++++++++++++++++----------------------
>  hw/block/pc_sysfw.c     |   5 +-
>  include/exec/cpu-all.h  |   2 -
>  include/exec/exec-all.h |   2 +
>  include/sysemu/kvm.h    |   5 --
>  kvm-all.c               |  13 ------
>  target-s390x/kvm.c      |  23 +++------
>  util/oslib-posix.c      |   4 +-
>  util/oslib-win32.c      |   5 +-
>  9 files changed, 78 insertions(+), 102 deletions(-)
> 

I appreciate this.  Works great on x86 and arm architectures.  From
libvirt's POV there's only better error reporting to fix, but that's a
rare race with only a small impact, nothing to do on qemu's side, I guess.

This is what we desired to have, thanks,
Martin

Re: [Qemu-devel] [PATCH v2 0/8] Guest memory allocation fixes & cleanup

[Markus Armbruster]

Ping?

Markus Armbruster <armbru@redhat.com> writes:

> All I wanted to do is exit(1) instead of abort() on guest memory
> allocation failure [07/08].  But that lead me into a minor #ifdef bog,
> and here's what I brought back.  Enjoy!
>
> Testing:
> * Christian Borntraeger reports v1 works fine under LPAR (new S390
>   KVM, i.e. generic allocation) and as second guest under z/VM (old
>   S390 KVM, i.e. legacy S390 allocation).  Thanks for testing, and for
>   catching a stupid mistake.  v2 differs from v1 only in code that
>   isn't reachable on S390.
>
> Changes since v1:
> * 5/8: Fix assertion in qemu_ram_remap() (Paolo)
> * All other patches unchanged except for Acked-by in commit messages
> Changes since RFC:
> * 1-3+8/8 unchanged except for commit message tweaks
> * 4+6/8 rewritten to address Paolo's review
> * 5/8 rewritten: don't fix dead code, just assert it's dead
> * 7/8 fix mistakes caught by Richard Henderson and Peter Maydell
>
> Markus Armbruster (8):
>   exec: Fix Xen RAM allocation with unusual options
>   exec: Clean up fall back when -mem-path allocation fails
>   exec: Reduce ifdeffery around -mem-path
>   exec: Simplify the guest physical memory allocation hook
>   exec: Drop incorrect & dead S390 code in qemu_ram_remap()
>   exec: Clean up unnecessary S390 ifdeffery
>   exec: Don't abort when we can't allocate guest memory
>   pc_sysfw: Fix ISA BIOS init for ridiculously big flash
>
>  exec.c                  | 121 ++++++++++++++++++++++++++----------------------
>  hw/block/pc_sysfw.c     |   5 +-
>  include/exec/cpu-all.h  |   2 -
>  include/exec/exec-all.h |   2 +
>  include/sysemu/kvm.h    |   5 --
>  kvm-all.c               |  13 ------
>  target-s390x/kvm.c      |  23 +++------
>  util/oslib-posix.c      |   4 +-
>  util/oslib-win32.c      |   5 +-
>  9 files changed, 78 insertions(+), 102 deletions(-)

Re: [Qemu-devel] [PATCH v2 0/8] Guest memory allocation fixes & cleanup

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Ping?

Has been ignored for six weeks, and now it no longer applies.  Rebased
version coming.

*Sigh*

Re: [Qemu-devel] [PATCH v2 0/8] Guest memory allocation fixes & cleanup

[Laszlo Ersek]

On 07/31/13 10:50, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Ping?
> 
> Has been ignored for six weeks, and now it no longer applies.  Rebased
> version coming.
> 
> *Sigh*

Why haven't you been made a maintainer yet? You could have a
"cleanup-fubars" tree and you could send pull requests for cleanups.

What you're doing is (apparently) unpopular to review, but critically
important for long-term health. Everyone is overloaded and concentrating
on important features and bugfixes with high visibility. Refactoring
falls of the wagon. Which is exactly why you should be able to send pull
requests.

Laszlo

Re: [Qemu-devel] [PATCH v2 0/8] Guest memory allocation fixes & cleanup

[Andreas Färber]

Am 31.07.2013 14:33, schrieb Laszlo Ersek:
> On 07/31/13 10:50, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Ping?
>>
>> Has been ignored for six weeks, and now it no longer applies.  Rebased
>> version coming.
>>
>> *Sigh*
> 
> Why haven't you been made a maintainer yet? You could have a
> "cleanup-fubars" tree and you could send pull requests for cleanups.
> 
> What you're doing is (apparently) unpopular to review, but critically
> important for long-term health. Everyone is overloaded and concentrating
> on important features and bugfixes with high visibility. Refactoring
> falls of the wagon.

Paolo and me will disagree, but sadly whenever you do one refactoring,
you can't help but notice that there's more things that could use some
refactoring, too. Where we get back to priorities and time and the need
for spreading the load over more shoulders. ;)

Andreas

> Which is exactly why you should be able to send pull
> requests.
> 
> Laszlo
> 


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