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

[Qemu-devel] [PATCH RFC 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!

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
  s390: Simplify the RAM allocation hook
  s390: Make qemu_ram_remap() consistent with allocation
  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                 | 117 ++++++++++++++++++++++++-------------------------
 hw/block/pc_sysfw.c    |   5 +--
 include/exec/cpu-all.h |   2 -
 include/sysemu/kvm.h   |   4 +-
 kvm-all.c              |  16 ++-----
 target-s390x/kvm.c     |  34 +++++++-------
 util/oslib-posix.c     |   4 +-
 util/oslib-win32.c     |   5 +--
 8 files changed, 81 insertions(+), 106 deletions(-)

-- 
1.7.11.7

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

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

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

[Stefano Stabellini]

On Thu, 13 Jun 2013, Markus Armbruster wrote:
> 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.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 RFC 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 incommit
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.

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

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

[Paolo Bonzini]

Il 13/06/2013 03:02, Markus Armbruster ha scritto:
> 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 incommit
> 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.
> 
> 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);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH RFC 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 valid 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 RFC 4/8] s390: Simplify the RAM allocation hook

[Markus Armbruster]

Less code and ifdeffery.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c               |  4 ++--
 include/sysemu/kvm.h |  3 +--
 kvm-all.c            | 15 ++-------------
 target-s390x/kvm.c   | 17 ++++++-----------
 4 files changed, 11 insertions(+), 28 deletions(-)

diff --git a/exec.c b/exec.c
index 4dbb0f1..7552e3c 100644
--- a/exec.c
+++ b/exec.c
@@ -1098,9 +1098,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
 #endif
         }
         if (!new_block->host) {
-            if (kvm_enabled()) {
+            if (kvm_enabled() && kvm_arch_ram_alloc) {
                 /* some s390/kvm configurations have special constraints */
-                new_block->host = kvm_ram_alloc(size);
+                new_block->host = kvm_arch_ram_alloc(size);
             } else {
                 new_block->host = qemu_anon_ram_alloc(size);
             }
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 8b19322..dd79914 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -152,8 +152,7 @@ int kvm_init_vcpu(CPUState *cpu);
 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);
+extern void *(*kvm_arch_ram_alloc)(ram_addr_t size);
 #endif
 
 void kvm_setup_guest_memory(void *start, size_t size);
diff --git a/kvm-all.c b/kvm-all.c
index 405480e..5e0cc9b 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -119,6 +119,8 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_LAST_INFO
 };
 
+void *(*kvm_arch_ram_alloc)(ram_addr_t size);
+
 static KVMSlot *kvm_alloc_slot(KVMState *s)
 {
     int i;
@@ -1816,19 +1818,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 862fb12..be802ab 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)) {
+        kvm_arch_ram_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

Re: [Qemu-devel] [PATCH RFC 4/8] s390: Simplify the RAM allocation hook

[Andreas Färber]

Am 13.06.2013 09:02, schrieb Markus Armbruster:
> Less code and ifdeffery.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

Andreas

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

Re: [Qemu-devel] [PATCH RFC 4/8] s390: Simplify the RAM allocation hook

[Paolo Bonzini]

Il 13/06/2013 03:02, Markus Armbruster ha scritto:
> Less code and ifdeffery.

Less ifdeffery is of course fine, but please add instead
kvm_arch_ram_alloc to the other target-*/kvm.c files.  It can just
return NULL.  I think this may affect the karma that comes from a
negative diffstat, but it is more similar to other kvm_arch_ hooks.

Paolo

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  exec.c               |  4 ++--
>  include/sysemu/kvm.h |  3 +--
>  kvm-all.c            | 15 ++-------------
>  target-s390x/kvm.c   | 17 ++++++-----------
>  4 files changed, 11 insertions(+), 28 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 4dbb0f1..7552e3c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1098,9 +1098,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>  #endif
>          }
>          if (!new_block->host) {
> -            if (kvm_enabled()) {
> +            if (kvm_enabled() && kvm_arch_ram_alloc) {
>                  /* some s390/kvm configurations have special constraints */
> -                new_block->host = kvm_ram_alloc(size);
> +                new_block->host = kvm_arch_ram_alloc(size);
>              } else {
>                  new_block->host = qemu_anon_ram_alloc(size);
>              }
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 8b19322..dd79914 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -152,8 +152,7 @@ int kvm_init_vcpu(CPUState *cpu);
>  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);
> +extern void *(*kvm_arch_ram_alloc)(ram_addr_t size);
>  #endif
>  
>  void kvm_setup_guest_memory(void *start, size_t size);
> diff --git a/kvm-all.c b/kvm-all.c
> index 405480e..5e0cc9b 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -119,6 +119,8 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_LAST_INFO
>  };
>  
> +void *(*kvm_arch_ram_alloc)(ram_addr_t size);
> +
>  static KVMSlot *kvm_alloc_slot(KVMState *s)
>  {
>      int i;
> @@ -1816,19 +1818,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 862fb12..be802ab 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)) {
> +        kvm_arch_ram_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);
> 

[Qemu-devel] [PATCH RFC 5/8] s390: Make qemu_ram_remap() consistent with allocation

[Markus Armbruster]

Old S390 KVM wants guest RAM mapped in a peculiar way.

Commit fdec991 changed qemu_ram_alloc_from_ptr() to do this only when
necessary, 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 right now, as we never remap on S390.
Fix it anyway.

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c               | 19 +++++++------------
 include/sysemu/kvm.h |  1 +
 kvm-all.c            |  1 +
 target-s390x/kvm.c   | 15 ++++++++++-----
 4 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/exec.c b/exec.c
index 7552e3c..8810d33 100644
--- a/exec.c
+++ b/exec.c
@@ -1209,27 +1209,22 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
             } else if (xen_enabled()) {
                 abort();
             } else {
-                flags = MAP_FIXED;
                 munmap(vaddr, length);
                 if (block->fd >= 0) {
 #ifdef MAP_POPULATE
-                    flags |= mem_prealloc ? MAP_POPULATE | MAP_SHARED :
+                    flags = mem_prealloc ? MAP_POPULATE | MAP_SHARED :
                         MAP_PRIVATE;
 #else
-                    flags |= MAP_PRIVATE;
+                    flags = MAP_PRIVATE;
 #endif
                     area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
-                                flags, block->fd, offset);
+                                MAP_FIXED | flags, block->fd, offset);
+                } else if (kvm_enabled() && kvm_arch_ram_remap) {
+                    area = kvm_arch_ram_remap(vaddr, length);
                 } 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
-                    flags |= MAP_PRIVATE | MAP_ANONYMOUS;
                     area = mmap(vaddr, length, PROT_READ | PROT_WRITE,
-                                flags, -1, 0);
-#endif
+                                MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS,
+                                -1, 0);
                 }
                 if (area != vaddr) {
                     fprintf(stderr, "Could not remap addr: "
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index dd79914..fdfa7ba 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -153,6 +153,7 @@ int kvm_cpu_exec(CPUArchState *env);
 
 #if !defined(CONFIG_USER_ONLY)
 extern void *(*kvm_arch_ram_alloc)(ram_addr_t size);
+extern void *(*kvm_arch_ram_remap)(void *vaddr, ram_addr_t size);
 #endif
 
 void kvm_setup_guest_memory(void *start, size_t size);
diff --git a/kvm-all.c b/kvm-all.c
index 5e0cc9b..daa4d40 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -120,6 +120,7 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = {
 };
 
 void *(*kvm_arch_ram_alloc)(ram_addr_t size);
+void *(*kvm_arch_ram_remap)(void *vaddr, ram_addr_t size);
 
 static KVMSlot *kvm_alloc_slot(KVMState *s)
 {
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index be802ab..aa45e06 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -93,6 +93,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
 static int cap_sync_regs;
 
 static void *legacy_s390_alloc(ram_addr_t size);
+static void *legacy_s390_mmap(void *vaddr, ram_addr_t length);
 
 int kvm_arch_init(KVMState *s)
 {
@@ -100,6 +101,7 @@ int kvm_arch_init(KVMState *s)
     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
         kvm_arch_ram_alloc = legacy_s390_alloc;
+        kvm_arch_ram_remap = legacy_s390_mmap;
     }
     return 0;
 }
@@ -324,13 +326,16 @@ int kvm_s390_get_registers_partial(CPUState *cs)
  * to grow. We also have to use MAP parameters that avoid
  * read-only mapping of guest pages.
  */
-static void *legacy_s390_alloc(ram_addr_t size)
+
+static void *legacy_s390_mmap(void *vaddr, ram_addr_t size)
 {
-    void *mem;
+    return mmap(vaddr, size, PROT_EXEC | PROT_READ | PROT_WRITE,
+                MAP_FIXED | MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+}
 
-    mem = mmap((void *) 0x800000000ULL, size,
-               PROT_EXEC|PROT_READ|PROT_WRITE,
-               MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
+static void *legacy_s390_alloc(ram_addr_t size)
+{
+    void *mem = legacy_s390_mmap((void *) 0x800000000ULL, size);
     if (mem == MAP_FAILED) {
         fprintf(stderr, "Allocating RAM failed\n");
         abort();
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH RFC 5/8] s390: Make qemu_ram_remap() consistent with allocation

[Paolo Bonzini]

Il 13/06/2013 03:02, Markus Armbruster ha scritto:
> -static void *legacy_s390_alloc(ram_addr_t size)
> +
> +static void *legacy_s390_mmap(void *vaddr, ram_addr_t size)
>  {
> -    void *mem;
> +    return mmap(vaddr, size, PROT_EXEC | PROT_READ | PROT_WRITE,
> +                MAP_FIXED | MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +}
>  
> -    mem = mmap((void *) 0x800000000ULL, size,
> -               PROT_EXEC|PROT_READ|PROT_WRITE,
> -               MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0);

You can just turn kvm_arch_mem_alloc to kvm_arch_ram_mmap, and call it
from both kvm_mem_alloc and a new kvm_mem_remap.  Then s390 can do

    mmap(vaddr ? vaddr : (void *) 0x800000000ULL,
         size, PROT_EXEC | PROT_READ | PROT_WRITE,
         MAP_FIXED | MAP_SHARED | MAP_ANONYMOUS, -1, 0);

and other arches can still return NULL.

Paolo

[Qemu-devel] [PATCH RFC 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 | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/exec.c b/exec.c
index 8810d33..9f0355b 100644
--- a/exec.c
+++ b/exec.c
@@ -849,7 +849,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>
 
@@ -952,6 +952,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)
@@ -1088,22 +1096,21 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
             exit(1);
         }
         xen_ram_alloc(new_block->offset, size, mr);
+    } else if (kvm_enabled() && kvm_arch_ram_alloc) {
+        /* some s390/kvm configurations have special constraints */
+        if (mem_path) {
+            fprintf(stderr,
+                    "-mem-path not supported with this version of KVM\n");
+            exit(1);
+        }
+        new_block->host = kvm_arch_ram_alloc(size);
+        memory_try_enable_merging(new_block->host, size);
     } else {
         if (mem_path) {
-#if defined (__linux__) && !defined(TARGET_S390X)
             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) {
-            if (kvm_enabled() && kvm_arch_ram_alloc) {
-                /* some s390/kvm configurations have special constraints */
-                new_block->host = kvm_arch_ram_alloc(size);
-            } else {
-                new_block->host = qemu_anon_ram_alloc(size);
-            }
+            new_block->host = qemu_anon_ram_alloc(size);
             memory_try_enable_merging(new_block->host, size);
         }
     }
-- 
1.7.11.7

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

[Paolo Bonzini]

Il 13/06/2013 03:02, Markus Armbruster ha scritto:
> +    } else if (kvm_enabled() && kvm_arch_ram_alloc) {
> +        /* some s390/kvm configurations have special constraints */
> +        if (mem_path) {
> +            fprintf(stderr,
> +                    "-mem-path not supported with this version of KVM\n");
> +            exit(1);
> +        }
> +        new_block->host = kvm_arch_ram_alloc(size);
> +        memory_try_enable_merging(new_block->host, size);

Uh oh, now I see why you wanted the hook as a function pointer.  Can you
instead pass the file descriptor to kvm_ram_alloc?

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 13/06/2013 03:02, Markus Armbruster ha scritto:
>> +    } else if (kvm_enabled() && kvm_arch_ram_alloc) {
>> +        /* some s390/kvm configurations have special constraints */
>> +        if (mem_path) {
>> +            fprintf(stderr,
>> +                    "-mem-path not supported with this version of KVM\n");
>> +            exit(1);
>> +        }
>> +        new_block->host = kvm_arch_ram_alloc(size);
>> +        memory_try_enable_merging(new_block->host, size);
>
> Uh oh, now I see why you wanted the hook as a function pointer.  Can you
> instead pass the file descriptor to kvm_ram_alloc?

I'm not sure I understand what you're suggesting.  Here's my best guess.

I made kvm_arch_ram_alloc() an *optional* hook, implemented as function
pointer that may be null.  It's actually non-null only with old S390
KVM.

Non-null value also suppresses -mem-path.  Admittedly not the cleanest
possible solution, but (1) it's better than what we have now, and (2)
the whole thing should go away forever once we stop supporting old S390
kernels.

You seem to suggest to factor the actual allocation of guest memory out
of this part of qemu_ram_alloc_from_ptr():

    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 (kvm_enabled() && kvm_arch_ram_alloc) {
        /* some s390/kvm configurations have special constraints */
        if (mem_path) {
            fprintf(stderr,
                    "-mem-path not supported with this version of KVM\n");
            exit(1);
        }
        new_block->host = kvm_arch_ram_alloc(size);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
        memory_try_enable_merging(new_block->host, size);
    } else {
        if (mem_path) {
            new_block->host = file_ram_alloc(new_block, size, mem_path);
        }
        if (!new_block->host) {
            new_block->host = qemu_anon_ram_alloc(size);
            if (!new_block->host) {
                no_guest_mem(new_block);
            }
            memory_try_enable_merging(new_block->host, size);
        }
    }

Three functions, one each for Xen, S390 KVM, and generic.  Except the
one for Xen has only one caller, so we better leave that one alone.

Put a trivial wrapper around generic into each target-*/kvm.c other than
s390: arm, i386, ppc.

qemu_ram_alloc_from_ptr() then looks roughly like this:

    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 (kvm_enabled()) {
        new->block->host = kvm_arch_ram_alloc(size, new_block->fd);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
    } else {
        new_block->host = qemu_guest_mem_alloc(size, new_block->fd);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
    }

Except new_block->fd still needs to be set.  Need to split
file_ram_alloc() into two: first part yields the file descriptor, to be
called by qemu_ram_alloc_from_ptr(), second part does the actual
allocation, to be called by the allocation functions.  The code in
qemu_ram_alloc_from_ptr() becomes:

    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 {
        new->block->fd = file_ram_fd(mem_path);
        new->block->host = kvm_enabled()
            ? kvm_arch_ram_alloc(size, new_block->fd);
            : qemu_guest_mem_alloc(size, new_block->fd);
        if (!new_block->host) {
            no_guest_mem(new_block);
        }
    }

Is that what you have in mind?

It's a lot of restructuring and notational overhead just for this stupid
hack to support old S390 kernels.

Here's another way to keep the KVM hooks regular: make my function
pointers exec.c hooks instead of KVM hooks ;-P

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

[Paolo Bonzini]

Il 14/06/2013 04:06, Markus Armbruster ha scritto:
> Here's another way to keep the KVM hooks regular: make my function
> pointers exec.c hooks instead of KVM hooks ;-P

I can buy this one!

Paolo

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

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c             | 15 ++++++++++++++-
 target-s390x/kvm.c |  6 +-----
 util/oslib-posix.c |  4 +---
 util/oslib-win32.c |  5 +----
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/exec.c b/exec.c
index 9f0355b..7432465 100644
--- a/exec.c
+++ b/exec.c
@@ -851,6 +851,13 @@ void qemu_mutex_unlock_ramlist(void)
 
 #ifdef __linux__
 
+static void no_guest_mem(RAMBlock *block)
+{
+    fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
+            block->mr->name, strerror(errno));
+    exit(1);
+}
+
 #include <sys/vfs.h>
 
 #define HUGETLBFS_MAGIC       0x958458f6
@@ -945,7 +952,7 @@ static void *file_ram_alloc(RAMBlock *block,
     area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 #endif
     if (area == MAP_FAILED) {
-        perror("file_ram_alloc: can't mmap RAM pages");
+        no_guest_mem(block);
         close(fd);
         return (NULL);
     }
@@ -1104,6 +1111,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
             exit(1);
         }
         new_block->host = kvm_arch_ram_alloc(size);
+        if (!new_block->host) {
+            no_guest_mem(new_block);
+        }
         memory_try_enable_merging(new_block->host, size);
     } else {
         if (mem_path) {
@@ -1111,6 +1121,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
         }
         if (!new_block->host) {
             new_block->host = qemu_anon_ram_alloc(size);
+            if (!new_block->host) {
+                no_guest_mem(new_block);
+            }
             memory_try_enable_merging(new_block->host, size);
         }
     }
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index aa45e06..472a66f 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -336,11 +336,7 @@ static void *legacy_s390_mmap(void *vaddr, ram_addr_t size)
 static void *legacy_s390_alloc(ram_addr_t size)
 {
     void *mem = legacy_s390_mmap((void *) 0x800000000ULL, size);
-    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 RFC 7/8] exec: Don't abort when we can't allocate guest memory

[Andreas Färber]

Am 13.06.2013 09:02, 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.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

Thanks for looking into this!

Andreas

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

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

[Richard Henderson]

On 06/13/2013 12:02 AM, Markus Armbruster wrote:
> @@ -945,7 +952,7 @@ static void *file_ram_alloc(RAMBlock *block,
>      area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>  #endif
>      if (area == MAP_FAILED) {
> -        perror("file_ram_alloc: can't mmap RAM pages");
> +        no_guest_mem(block);
>          close(fd);
>          return (NULL);
>      }

Dead code after no_guest_mem.


r~

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

[Markus Armbruster]

Richard Henderson <rth@twiddle.net> writes:

> On 06/13/2013 12:02 AM, Markus Armbruster wrote:
>> @@ -945,7 +952,7 @@ static void *file_ram_alloc(RAMBlock *block,
>>      area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>>  #endif
>>      if (area == MAP_FAILED) {
>> -        perror("file_ram_alloc: can't mmap RAM pages");
>> +        no_guest_mem(block);
>>          close(fd);
>>          return (NULL);
>>      }
>
> Dead code after no_guest_mem.

Right you are.

Before: if allocation obeying -mem-path fails, we retry without
-mem-path.  Some failures are silent.

After: if mmap() fails, we give up.  Functional change not mentioned in
commit message.  I'll either revert it or document it.

I'm not sure falling back to non-mem-path allocation is a good idea in
all failure cases.

Thanks!

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

[Peter Maydell]

On 13 June 2013 08:02, Markus Armbruster <armbru@redhat.com> wrote:
>  #ifdef __linux__
>
> +static void no_guest_mem(RAMBlock *block)
> +{
> +    fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
> +            block->mr->name, strerror(errno));
> +    exit(1);
> +}

This new error message is inside an #ifdef __linux__...

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

...but this error message which got removed is not.
Shouldn't we be reporting errors in a non-host-specific
bit of code?

thanks
-- PMM

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 June 2013 08:02, Markus Armbruster <armbru@redhat.com> wrote:
>>  #ifdef __linux__
>>
>> +static void no_guest_mem(RAMBlock *block)
>> +{
>> +    fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
>> +            block->mr->name, strerror(errno));
>> +    exit(1);
>> +}
>
> This new error message is inside an #ifdef __linux__...

I'm afraid this won't compile on non-linux hosts.  Will respin.  Thanks!

[...]

[Qemu-devel] [PATCH RFC 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 RFC 0/8] Guest memory allocation fixes & cleanup

[Markus Armbruster]

Err, the RFC is accidental.  Please review anyway.  I'll respin for
commit afterwards.