* [Qemu-devel] [PATCH 0/2] pc: Fix startup with memory hotplug enabled @ 2015-01-26 15:31 Peter Krempa 2015-01-26 15:31 ` [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters Peter Krempa 2015-01-26 15:31 ` [Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size Peter Krempa 0 siblings, 2 replies; 6+ messages in thread From: Peter Krempa @ 2015-01-26 15:31 UTC (permalink / raw) To: qemu-devel; +Cc: Igor Mammedov, 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 | 22 +++++++--------------- 2 files changed, 14 insertions(+), 15 deletions(-) -- 2.2.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters 2015-01-26 15:31 [Qemu-devel] [PATCH 0/2] pc: Fix startup with memory hotplug enabled Peter Krempa @ 2015-01-26 15:31 ` Peter Krempa 2015-01-27 18:15 ` Eric Blake 2015-01-26 15:31 ` [Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size Peter Krempa 1 sibling, 1 reply; 6+ messages in thread From: Peter Krempa @ 2015-01-26 15:31 UTC (permalink / raw) To: qemu-devel; +Cc: Igor Mammedov, 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> --- vl.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/vl.c b/vl.c index 983259b..cdc920c 100644 --- a/vl.c +++ b/vl.c @@ -2694,29 +2694,21 @@ 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); - if (sz < ram_size) { - error_report("invalid -m option value: maxmem " - "(0x%" PRIx64 ") <= initial memory (0x" - RAM_ADDR_FMT ")", sz, ram_size); + if (sz <= ram_size) { + error_report("invalid value of -m option maxmem: " + "maximum memory size (0x%" PRIx64 ") must be greater " + "than 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); + error_report("invalid value of -m option: maxmem was specified, " + "but no hotplug slots were specified"); 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters 2015-01-26 15:31 ` [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters Peter Krempa @ 2015-01-27 18:15 ` Eric Blake 2015-01-28 7:18 ` Peter Krempa 0 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2015-01-27 18:15 UTC (permalink / raw) To: Peter Krempa, qemu-devel; +Cc: Igor Mammedov [-- Attachment #1: Type: text/plain, Size: 3019 bytes --] On 01/26/2015 08:31 AM, Peter Krempa 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> > --- > vl.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/vl.c b/vl.c > index 983259b..cdc920c 100644 > --- a/vl.c > +++ b/vl.c > @@ -2694,29 +2694,21 @@ 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); > - if (sz < ram_size) { > - error_report("invalid -m option value: maxmem " > - "(0x%" PRIx64 ") <= initial memory (0x" > - RAM_ADDR_FMT ")", sz, ram_size); > + if (sz <= ram_size) { Why are we changing from '<' to '<='? I think the error was in the message, not in the code, and that setting max == size should be allowed. [1] > + error_report("invalid value of -m option maxmem: " > + "maximum memory size (0x%" PRIx64 ") must be greater " > + "than initial memory size (0x" RAM_ADDR_FMT ")", Why two spaces? If I'm correct that we want '<' in the condition, then the wording 'must be at least the initial memory size' would be better. > + 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); > + error_report("invalid value of -m option: maxmem was specified, " > + "but no hotplug slots were specified"); > 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); > - } Okay, I see. This is dead if condition [1] is changed to '<=', but still reachable (sz == ram_size) if condition [1] is left at '<'. Maybe better logic would be: if (sz < ram_size) { max cannot be less than memory } if (sz > ram_size) { if (!slots) { max cannot be larger than size without hotplug slots } } else if (slots) { max must be larger than size to support hotplug slots } to allow max==ram_size when slots is not present. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters 2015-01-27 18:15 ` Eric Blake @ 2015-01-28 7:18 ` Peter Krempa 0 siblings, 0 replies; 6+ messages in thread From: Peter Krempa @ 2015-01-28 7:18 UTC (permalink / raw) To: Eric Blake; +Cc: Igor Mammedov, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3484 bytes --] On Tue, Jan 27, 2015 at 11:15:41 -0700, Eric Blake wrote: > On 01/26/2015 08:31 AM, Peter Krempa 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> > > --- > > vl.c | 22 +++++++--------------- > > 1 file changed, 7 insertions(+), 15 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index 983259b..cdc920c 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2694,29 +2694,21 @@ 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); > > - if (sz < ram_size) { > > - error_report("invalid -m option value: maxmem " > > - "(0x%" PRIx64 ") <= initial memory (0x" > > - RAM_ADDR_FMT ")", sz, ram_size); > > + if (sz <= ram_size) { > > Why are we changing from '<' to '<='? I think the error was in the > message, not in the code, and that setting max == size should be > allowed. [1] > > > + error_report("invalid value of -m option maxmem: " > > + "maximum memory size (0x%" PRIx64 ") must be greater " > > + "than initial memory size (0x" RAM_ADDR_FMT ")", > > Why two spaces? If I'm correct that we want '<' in the condition, then > the wording 'must be at least the initial memory size' would be better. > > > + 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); > > + error_report("invalid value of -m option: maxmem was specified, " > > + "but no hotplug slots were specified"); > > 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); > > - } > > Okay, I see. This is dead if condition [1] is changed to '<=', but > still reachable (sz == ram_size) if condition [1] is left at '<'. Maybe > better logic would be: > > if (sz < ram_size) { > max cannot be less than memory > } > if (sz > ram_size) { > if (!slots) { > max cannot be larger than size without hotplug slots > } > } else if (slots) { > max must be larger than size to support hotplug slots > } > > to allow max==ram_size when slots is not present. This will only allow to specify ram_size equal to max if the slot parameter _is_ present and set to 0. If the string is not present a different branch is taken and the config will be rejected, as the two parameters have to be specified together. I'll post an updated version though, the control flow in your suggestion seems more clear. Thanks. Peter [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size 2015-01-26 15:31 [Qemu-devel] [PATCH 0/2] pc: Fix startup with memory hotplug enabled Peter Krempa 2015-01-26 15:31 ` [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters Peter Krempa @ 2015-01-26 15:31 ` Peter Krempa 2015-01-27 18:16 ` Eric Blake 1 sibling, 1 reply; 6+ messages in thread From: Peter Krempa @ 2015-01-26 15:31 UTC (permalink / raw) To: qemu-devel; +Cc: Igor Mammedov, 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> --- hw/i386/pc.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index e07f1fa..157eefe 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size 2015-01-26 15:31 ` [Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size Peter Krempa @ 2015-01-27 18:16 ` Eric Blake 0 siblings, 0 replies; 6+ messages in thread From: Eric Blake @ 2015-01-27 18:16 UTC (permalink / raw) To: Peter Krempa, qemu-devel; +Cc: Igor Mammedov [-- Attachment #1: Type: text/plain, Size: 726 bytes --] On 01/26/2015 08:31 AM, Peter Krempa wrote: > 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> > --- > hw/i386/pc.c | 7 +++++++ > 1 file changed, 7 insertions(+) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-01-28 7:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-26 15:31 [Qemu-devel] [PATCH 0/2] pc: Fix startup with memory hotplug enabled Peter Krempa 2015-01-26 15:31 ` [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters Peter Krempa 2015-01-27 18:15 ` Eric Blake 2015-01-28 7:18 ` Peter Krempa 2015-01-26 15:31 ` [Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size Peter Krempa 2015-01-27 18:16 ` Eric Blake
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.