pc: Fix startup with memory hotplug enabled

PATCH Superseded Peter Krempa 3 participants 7 messages Jan 28, 2015

Share & tools

Peter Krempa

author

2015-01-28 08:37 UTC #0 lore raw

v2 cover pc: Fix startup with memory hotplug enabled

Tweak error messages to make sense and add check to verify that maxmem_size is properly aligned right away rather than just crashing afterwards.

Peter Krempa (2):
vl.c: Fix error messages when parsing maxmem parameters
pc: memory: Validate alignment of maxram_size to page size

 hw/i386/pc.c |  7 +++++++
 vl.c         | 34 ++++++++++++++++------------------
 2 files changed, 23 insertions(+), 18 deletions(-)

Signature · 1 line

2.2.1

Peter Krempa

author

2015-01-28 08:37 UTC #1 lore raw

PATCH v2 1/2 vl.c: Fix error messages when parsing maxmem parameters

Produce more human readable error messages and fix few spelling mistakes.

Also remove a redundant check for the max memory size.

Signed-off-by Peter Krempa <[email protected]>

---

Notes:
Version 2:
- fixed spacing in error message
- changed control flow to allow maxmem == ram_size in case slots == 0

 vl.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

vl.c +16 ~~-18~~

index 983259b..5a012f4 100644
@@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
         uint64_t slots;
 
         sz = qemu_opt_get_size(opts, "maxmem", 0);
+        slots = qemu_opt_get_number(opts, "slots", 0);
         if (sz < ram_size) {
-            error_report("invalid -m option value: maxmem "
-                    "(0x%" PRIx64 ") <= initial memory (0x"
-                    RAM_ADDR_FMT ")", sz, ram_size);
+            error_report("invalid value of -m option maxmem: "
+                         "maximum memory size (0x%" PRIx64 ") must at least "
+                         "the initial memory size (0x" RAM_ADDR_FMT ")",
+                         sz, ram_size);
             exit(EXIT_FAILURE);
-        }
-
-        slots = qemu_opt_get_number(opts, "slots", 0);
-        if ((sz > ram_size) && !slots) {
-            error_report("invalid -m option value: maxmem "
-                    "(0x%" PRIx64 ") more than initial memory (0x"
-                    RAM_ADDR_FMT ") but no hotplug slots where "
-                    "specified", sz, ram_size);
+        } else if (sz > ram_size) {
+            if (!slots) {
+                error_report("invalid value of -m option: maxmem was "
+                             "specified, but no hotplug slots were specified");
+                exit(EXIT_FAILURE);
+            }
+        } else if (slots) {
+            error_report("invalid value of -m option maxmem: "
+                         "memory slots were specified but maximum memory size "
+                         "(0x%" PRIx64 ") is equal to the initial memory size "
+                         "(0x" RAM_ADDR_FMT ")", sz, ram_size);
             exit(EXIT_FAILURE);
         }
 
-        if ((sz <= ram_size) && slots) {
-            error_report("invalid -m option value:  %"
-                    PRIu64 " hotplug slots where specified but "
-                    "maxmem (0x%" PRIx64 ") <= initial memory (0x"
-                    RAM_ADDR_FMT ")", slots, sz, ram_size);
-            exit(EXIT_FAILURE);
-        }
         *maxram_size = sz;
         *ram_slots = slots;
     } else if ((!maxmem_str && slots_str) ||

Signature · 1 line

2.2.1

Igor Mammedov

2015-01-28 12:29 UTC #2 lore raw

Re: [Qemu-devel] [PATCHv2 1/2] vl.c: Fix error messages when parsing maxmem parameters

Quoted 21 lines · On Wed, 28 Jan 2015 09:35:03 +0100 Peter Krempa <[email protected]> wrote:

Produce more human readable error messages and fix few spelling mistakes.
Also remove a redundant check for the max memory size.
Signed-off-by Peter Krempa <[email protected]>
---
Notes:
Version 2:
- fixed spacing in error message
- changed control flow to allow maxmem == ram_size in case slots == 0
 vl.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
vl.c +0 -0
index 983259b..5a012f4 100644

vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2700

Quoted 10 lines

vl.c +3 -3

@@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
         uint64_t slots;
 
         sz = qemu_opt_get_size(opts, "maxmem", 0);
+        slots = qemu_opt_get_number(opts, "slots", 0);
         if (sz < ram_size) {
-            error_report("invalid -m option value: maxmem "
-                    "(0x%" PRIx64 ") <= initial memory (0x"
-                    RAM_ADDR_FMT ")", sz, ram_size);
+            error_report("invalid value of -m option maxmem: "
+                         "maximum memory size (0x%" PRIx64 ") must at least "

typo??
"must be at least"

Quoted 35 lines

vl.c +13 ~~-15~~

+                         "the initial memory size (0x" RAM_ADDR_FMT ")",
+                         sz, ram_size);
             exit(EXIT_FAILURE);
-        }
-
-        slots = qemu_opt_get_number(opts, "slots", 0);
-        if ((sz > ram_size) && !slots) {
-            error_report("invalid -m option value: maxmem "
-                    "(0x%" PRIx64 ") more than initial memory (0x"
-                    RAM_ADDR_FMT ") but no hotplug slots where "
-                    "specified", sz, ram_size);
+        } else if (sz > ram_size) {
+            if (!slots) {
+                error_report("invalid value of -m option: maxmem was "
+                             "specified, but no hotplug slots were specified");
+                exit(EXIT_FAILURE);
+            }
+        } else if (slots) {
+            error_report("invalid value of -m option maxmem: "
+                         "memory slots were specified but maximum memory size "
+                         "(0x%" PRIx64 ") is equal to the initial memory size "
+                         "(0x" RAM_ADDR_FMT ")", sz, ram_size);
             exit(EXIT_FAILURE);
         }
 
-        if ((sz <= ram_size) && slots) {
-            error_report("invalid -m option value:  %"
-                    PRIu64 " hotplug slots where specified but "
-                    "maxmem (0x%" PRIx64 ") <= initial memory (0x"
-                    RAM_ADDR_FMT ")", slots, sz, ram_size);
-            exit(EXIT_FAILURE);
-        }
         *maxram_size = sz;
         *ram_slots = slots;
     } else if ((!maxmem_str && slots_str) ||

Peter Krempa

author

2015-01-28 16:23 UTC #3 lore raw

Re: [Qemu-devel] [PATCHv2 1/2] vl.c: Fix error messages when parsing maxmem parameters

Quoted 26 lines · Igor Mammedov

Quoted 23 lines · On Wed, 28 Jan 2015 09:35:03 +0100 Peter Krempa <[email protected]> wrote:
Produce more human readable error messages and fix few spelling mistakes.
Also remove a redundant check for the max memory size.
Signed-off-by Peter Krempa <[email protected]>
---
Notes:
Version 2:
- fixed spacing in error message
- changed control flow to allow maxmem == ram_size in case slots == 0
 vl.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
vl.c +0 -0
index 983259b..5a012f4 100644
@@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
         uint64_t slots;
 

Quoted 11 lines

vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2700

Quoted 8 lines

vl.c +3 -3

         sz = qemu_opt_get_size(opts, "maxmem", 0);
+        slots = qemu_opt_get_number(opts, "slots", 0);
         if (sz < ram_size) {
-            error_report("invalid -m option value: maxmem "
-                    "(0x%" PRIx64 ") <= initial memory (0x"
-                    RAM_ADDR_FMT ")", sz, ram_size);
+            error_report("invalid value of -m option maxmem: "
+                         "maximum memory size (0x%" PRIx64 ") must at least "

typo??
"must be at least"

Hmm, right. Should I respin the series to fix it?

Peter

Igor Mammedov

2015-01-28 17:01 UTC #4 lore raw

Re: [Qemu-devel] [PATCHv2 1/2] vl.c: Fix error messages when parsing maxmem parameters

Quoted 39 lines · On Wed, 28 Jan 2015 17:21:28 +0100 Peter Krempa <[email protected]> wrote:

Quoted 26 lines · Igor Mammedov
Quoted 23 lines · On Wed, 28 Jan 2015 09:35:03 +0100 Peter Krempa <[email protected]> wrote:
Produce more human readable error messages and fix few spelling mistakes.
Also remove a redundant check for the max memory size.
Signed-off-by Peter Krempa <[email protected]>
---
Notes:
Version 2:
- fixed spacing in error message
- changed control flow to allow maxmem == ram_size in case slots == 0
 vl.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
vl.c +0 -0
index 983259b..5a012f4 100644
@@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
         uint64_t slots;
 
Quoted 11 lines
vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2700
Quoted 8 lines
vl.c +3 -3
         sz = qemu_opt_get_size(opts, "maxmem", 0);
+        slots = qemu_opt_get_number(opts, "slots", 0);
         if (sz < ram_size) {
-            error_report("invalid -m option value: maxmem "
-                    "(0x%" PRIx64 ") <= initial memory (0x"
-                    RAM_ADDR_FMT ")", sz, ram_size);
+            error_report("invalid value of -m option maxmem: "
+                         "maximum memory size (0x%" PRIx64 ") must at least "
typo??
"must be at least"

Quoted 1 line

Hmm, right. Should I respin the series to fix it?

Send an extra incremental patch with subject:
"fixup! vl.c: Fix error messages when parsing maxmem parameters"

Quoted 2 lines

Peter

Markus Armbruster

2015-01-28 18:32 UTC #5 lore raw

Re: [Qemu-devel] [PATCHv2 1/2] vl.c: Fix error messages when parsing maxmem parameters

Quoted 42 lines · Igor Mammedov

Quoted 39 lines · On Wed, 28 Jan 2015 17:21:28 +0100 Peter Krempa <[email protected]> wrote:
Quoted 37 lines · Igor Mammedov
Quoted 21 lines · On Wed, 28 Jan 2015 09:35:03 +0100 Peter Krempa <[email protected]> wrote:
Produce more human readable error messages and fix few spelling mistakes.
Also remove a redundant check for the max memory size.
Signed-off-by Peter Krempa <[email protected]>
---
Notes:
Version 2:
- fixed spacing in error message
- changed control flow to allow maxmem == ram_size in case slots == 0
 vl.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
vl.c +0 -0
index 983259b..5a012f4 100644
vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2700
Quoted 10 lines
vl.c +3 -3
@@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
         uint64_t slots;
 
         sz = qemu_opt_get_size(opts, "maxmem", 0);
+        slots = qemu_opt_get_number(opts, "slots", 0);
         if (sz < ram_size) {
-            error_report("invalid -m option value: maxmem "
-                    "(0x%" PRIx64 ") <= initial memory (0x"
-                    RAM_ADDR_FMT ")", sz, ram_size);
+            error_report("invalid value of -m option maxmem: "
+                         "maximum memory size (0x%" PRIx64 ") must at least "
typo??
"must be at least"

Quoted 3 lines

Quoted 1 line
Hmm, right. Should I respin the series to fix it?
Send an extra incremental patch with subject:
"fixup! vl.c: Fix error messages when parsing maxmem parameters"

I'd simply respin, especially a small series like this one.

Peter Krempa

author

2015-01-28 08:37 UTC #6 lore raw

PATCH v2 2/2 pc: memory: Validate alignment of maxram_size to page size

In reply to Peter Krempa

If the maxram_size is not aligned and dimm devices were added on the command line qemu would terminate with a rather unhelpful message:

ERROR:hw/mem/pc-dimm.c:150:pc_dimm_get_free_addr: assertion failed:
(QEMU_ALIGN_UP(address_space_size, align) == address_space_size)

In case no dimm device was originally added on the commandline qemu exits on the assertion failure.

Signed-off-by Peter Krempa <[email protected]>

Reviewed-by Eric Blake <[email protected]>

---
 hw/i386/pc.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/i386/pc.c +7 -0

index c7af6aa..8cf405a 100644
@@ -1246,6 +1246,13 @@ FWCfgState *pc_memory_init(MachineState *machine,
             exit(EXIT_FAILURE);
         }
 
+        if (QEMU_ALIGN_UP(machine->maxram_size,
+                          TARGET_PAGE_SIZE) != machine->maxram_size) {
+            error_report("maximum memory size must by aligned to multiple of "
+                         "%d bytes", TARGET_PAGE_SIZE);
+            exit(EXIT_FAILURE);
+        }
+
         pcms->hotplug_memory_base =
             ROUND_UP(0x100000000ULL + above_4g_mem_size, 1ULL << 30);

Signature · 1 line

2.2.1