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

[Qemu-devel] [PATCH 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 this version 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.

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

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

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..a0f18fe 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);
+
                     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 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 a0f18fe..14477ea 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 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 14477ea..b88698b 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 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 4/8] exec: Simplify the guest physical memory allocation hook

[Christian Borntraeger]

On 19/06/13 13:44, Markus Armbruster wrote:
> 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

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

[Christian Borntraeger]

On 19/06/13 13:44, Markus Armbruster wrote:
> 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

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

[Paolo Bonzini]

Il 19/06/2013 13:44, Markus Armbruster ha scritto:
> 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.
> 
> 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..a0f18fe 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);

Probably "assert(phys_mem_alloc == qemu_anon_ram_alloc)"?

Otherwise all looks fine.

Paolo

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

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 19/06/2013 13:44, Markus Armbruster ha scritto:
>> 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.
>> 
>> 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..a0f18fe 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);
>
> Probably "assert(phys_mem_alloc == qemu_anon_ram_alloc)"?

Of course.  Will fix.

> Otherwise all looks fine.

Thanks!