pc: Fix startup with memory hotplug enabled
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 | 22 +++++++--------------- 2 files changed, 14 insertions(+), 15 deletions(-)
Signature · 1 line
2.2.1
PATCH 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.
--- vl.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)
vl.c +7 -15
@@ -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) ||
Signature · 1 line
2.2.1
Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters
Quoted 15 lines · 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 <[email protected]>--- vl.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)vl.c +0
-0
vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2697
Quoted 8 lines
vl.c +1
-4@@ -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]
vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2700
Quoted 3 lines
vl.c +3
-0+ 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.
Quoted 12 lines
vl.c +3
-4+ 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" );
vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2719
Quoted 10 lines
vl.c +0
-7exit (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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters
Quoted 71 lines · Eric Blake
Quoted 15 lines · 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 <[email protected]>--- vl.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)vl.c +0
-0vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2697
Quoted 8 lines
vl.c +1
-4@@ -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]
vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2700
Quoted 3 lines
vl.c +3
-0+ 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.
Quoted 12 lines
vl.c +3
-4+ 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" );vl.c static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) · line 2719
Quoted 10 lines
vl.c +0
-7exit (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) {
Quoted 4 lines
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
PATCH 2/2 pc: memory: Validate alignment of maxram_size to page size
In reply to Peter KrempaIf 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.
--- hw/i386/pc.c | 7 +++++++ 1 file changed, 7 insertions(+)
hw/i386/pc.c +7 -0
@@ -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
Re: [Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size
Quoted 6 lines · Peter Krempa
If the
maxram_sizeis 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)
Quoted 7 lines
In case no dimm device was originally added on the commandline qemu exits on the assertion failure.
Signed-off-by Peter Krempa <[email protected]>--- hw/i386/pc.c | 7 +++++++ 1 file changed, 7 insertions(+)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org