[Qemu-devel] [PATCH v3 for 1.6 0/8] Guest memory allocation fixes & cleanup

[Qemu-devel] [PATCH v3 for 1.6 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 v2:
* Straightforward rebase, only 4/8 conflicted
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                  | 120 ++++++++++++++++++++++++++----------------------
 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, 77 insertions(+), 102 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH v3 for 1.6 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 c4f2894..5b1be92 100644
--- a/exec.c
+++ b/exec.c
@@ -1119,6 +1119,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)
@@ -1132,9 +1138,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 {
@@ -1212,6 +1216,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) {
@@ -1224,11 +1230,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;
@@ -1252,6 +1254,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 v3 for 1.6 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 5b1be92..957266c 100644
--- a/exec.c
+++ b/exec.c
@@ -1129,15 +1129,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 v3 for 1.6 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 957266c..4a825fa 100644
--- a/exec.c
+++ b/exec.c
@@ -1111,6 +1111,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();
@@ -1215,17 +1216,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);
             }
@@ -1256,25 +1249,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 a407b50..b6998f0 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -453,9 +453,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 v3 for 1.6 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                  | 19 +++++++++++++------
 include/exec/exec-all.h |  2 ++
 include/sysemu/kvm.h    |  5 -----
 kvm-all.c               | 13 -------------
 target-s390x/kvm.c      | 17 ++++++-----------
 5 files changed, 21 insertions(+), 35 deletions(-)

diff --git a/exec.c b/exec.c
index 4a825fa..dc87cce 100644
--- a/exec.c
+++ b/exec.c
@@ -761,6 +761,18 @@ static int subpage_register (subpage_t *mmio, uint32_t start, uint32_t end,
                              uint16_t section);
 static subpage_t *subpage_init(AddressSpace *as, 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 uint16_t phys_section_add(MemoryRegionSection *section)
 {
     /* The physical section number is ORed with a page-aligned
@@ -1136,12 +1148,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 5920f73..33b43d2 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -383,6 +383,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 de74411..a50a474 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -161,11 +161,6 @@ int kvm_cpu_exec(CPUState *cpu);
 
 #ifdef NEED_CPU_H
 
-#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 716860f..971bed8 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1820,19 +1820,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 ab0e2b5..b5351e6 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)
 {
     static const uint8_t diag_501[] = {0x83, 0x24, 0x05, 0x01};
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 for 1.6 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 dc87cce..dea4b1a 100644
--- a/exec.c
+++ b/exec.c
@@ -1266,15 +1266,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 v3 for 1.6 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 dea4b1a..231d04e 100644
--- a/exec.c
+++ b/exec.c
@@ -904,7 +904,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>
 
@@ -1007,6 +1007,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)
@@ -1140,12 +1148,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 v3 for 1.6 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 231d04e..0cfca3a 100644
--- a/exec.c
+++ b/exec.c
@@ -1162,6 +1162,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 b5351e6..c4a0fdd 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

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

[Andreas Färber]

Am 31.07.2013 15:11, schrieb 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 231d04e..0cfca3a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1162,6 +1162,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));

This could use error_report() while at it, but still

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thought I had ack'ed it long ago, but I guess something minor changed.

Cheers,
Andreas

> +                exit(1);
> +            }
>              memory_try_enable_merging(new_block->host, size);
>          }
>      }
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index b5351e6..c4a0fdd 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;
>  }
> 


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

[Qemu-devel] [PATCH v3 for 1.6 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 7db68f0..74a5364 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, NULL, "isa-bios", isa_bios_size);
     vmstate_register_ram_global(isa_bios);
-- 
1.7.11.7

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

[Laszlo Ersek]

On 07/31/13 15:11, 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 v2:
> * Straightforward rebase, only 4/8 conflicted
> 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                  | 120 ++++++++++++++++++++++++++----------------------
>  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, 77 insertions(+), 102 deletions(-)
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

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

[Markus Armbruster]

Series has been on list for more than 8 weeks (not counting the initial
PATCH RFC), and rebased twice.  Right now, it still applies.  Please
either merge or tell me what I need to do to get it merged.  Thanks!

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 v2:
> * Straightforward rebase, only 4/8 conflicted
> 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                  | 120 ++++++++++++++++++++++++++----------------------
>  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, 77 insertions(+), 102 deletions(-)

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Series has been on list for more than 8 weeks (not counting the initial
> PATCH RFC), and rebased twice.  Right now, it still applies.  Please
> either merge or tell me what I need to do to get it merged.  Thanks!

Twelve weeks now.

> 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 v2:
>> * Straightforward rebase, only 4/8 conflicted
>> 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                  | 120 ++++++++++++++++++++++++++----------------------
>>  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, 77 insertions(+), 102 deletions(-)

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

[Stefan Weil]

Am 31.07.2013 15:11, schrieb 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 v2:
> * Straightforward rebase, only 4/8 conflicted
> 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                  | 120 ++++++++++++++++++++++++++----------------------
>  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, 77 insertions(+), 102 deletions(-)


Two patches from this series seem to cause compiler errors
in latest QEMU ona 32 bit Ubuntu precise host:

  CC    arm-softmmu/exec.o
exec.c:752:51: error: initialization from incompatible pointer type
[-Werror]
exec.c: In function 'qemu_ram_alloc_from_ptr':
exec.c:1139:32: error: comparison of distinct pointer types lacks a cast
[-Werror]
exec.c: In function 'qemu_ram_remap':
exec.c:1283:21: error: comparison of distinct pointer types lacks a cast
[-Werror]

There is a mismatch of function prototypes (size_t <-> ram_addr_t):

void *qemu_anon_ram_alloc(size_t size);
static void *(*phys_mem_alloc)(ram_addr_t size) = qemu_anon_ram_alloc;

It's strange that the buildbots don't complain.

Stefan

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

[Markus Armbruster]

Stefan Weil <sw@weilnetz.de> writes:

> Am 31.07.2013 15:11, schrieb 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.
[...]
> Two patches from this series seem to cause compiler errors
> in latest QEMU ona 32 bit Ubuntu precise host:
>
>   CC    arm-softmmu/exec.o
> exec.c:752:51: error: initialization from incompatible pointer type
> [-Werror]
> exec.c: In function 'qemu_ram_alloc_from_ptr':
> exec.c:1139:32: error: comparison of distinct pointer types lacks a cast
> [-Werror]
> exec.c: In function 'qemu_ram_remap':
> exec.c:1283:21: error: comparison of distinct pointer types lacks a cast
> [-Werror]
>
> There is a mismatch of function prototypes (size_t <-> ram_addr_t):
>
> void *qemu_anon_ram_alloc(size_t size);
> static void *(*phys_mem_alloc)(ram_addr_t size) = qemu_anon_ram_alloc;
>
> It's strange that the buildbots don't complain.

Indeed.  Thanks for posting a fix!