[Qemu-devel] [PATCHv2 0/2] pc: Fix startup with memory hotplug enabled

[Qemu-devel] [PATCHv2 0/2] pc: Fix startup with memory hotplug enabled

[Peter Krempa]

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(-)

-- 
2.2.1

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

[Peter Krempa]

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 <pkrempa@redhat.com>
---

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(-)

diff --git a/vl.c b/vl.c
index 983259b..5a012f4 100644
--- a/vl.c
+++ b/vl.c
@@ -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) ||
-- 
2.2.1

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

[Igor Mammedov]

On Wed, 28 Jan 2015 09:35:03 +0100
Peter Krempa <pkrempa@redhat.com> 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 <pkrempa@redhat.com>
> ---
> 
> 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(-)
> 
> diff --git a/vl.c b/vl.c
> index 983259b..5a012f4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -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"

> +                         "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) ||

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

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

On Wed, Jan 28, 2015 at 13:29:41 +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 09:35:03 +0100
> Peter Krempa <pkrempa@redhat.com> 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 <pkrempa@redhat.com>
> > ---
> > 
> > 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(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 983259b..5a012f4 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -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"
> 

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

Peter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Igor Mammedov]

On Wed, 28 Jan 2015 17:21:28 +0100
Peter Krempa <pkrempa@redhat.com> wrote:

> On Wed, Jan 28, 2015 at 13:29:41 +0100, Igor Mammedov wrote:
> > On Wed, 28 Jan 2015 09:35:03 +0100
> > Peter Krempa <pkrempa@redhat.com> 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 <pkrempa@redhat.com>
> > > ---
> > > 
> > > 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(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 983259b..5a012f4 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -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"
> > 
> 
> 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"

> 
> Peter

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

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Wed, 28 Jan 2015 17:21:28 +0100
> Peter Krempa <pkrempa@redhat.com> wrote:
>
>> On Wed, Jan 28, 2015 at 13:29:41 +0100, Igor Mammedov wrote:
>> > On Wed, 28 Jan 2015 09:35:03 +0100
>> > Peter Krempa <pkrempa@redhat.com> 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 <pkrempa@redhat.com>
>> > > ---
>> > > 
>> > > 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(-)
>> > > 
>> > > diff --git a/vl.c b/vl.c
>> > > index 983259b..5a012f4 100644
>> > > --- a/vl.c
>> > > +++ b/vl.c
>> > > @@ -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"
>> > 
>> 
>> 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.

[Qemu-devel] [PATCHv2 2/2] pc: memory: Validate alignment of maxram_size to page size

[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 <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/i386/pc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c7af6aa..8cf405a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -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);

-- 
2.2.1