On Wed, May 21, 2014 at 1:52 PM, Igor Mammedov <imamm...@redhat.com> wrote:
> On Wed, 21 May 2014 13:12:13 +0400
> Andrey Korolyov <and...@xdel.ru> wrote:
>
>> On Wed, May 21, 2014 at 12:55 PM, Igor Mammedov <imamm...@redhat.com> wrote:
>> > On Wed, 21 May 2014 12:27:05 +0400
>> > Andrey Korolyov <and...@xdel.ru> wrote:
>> >
>> >> On Wed, May 21, 2014 at 12:10 PM, Michael S. Tsirkin <m...@redhat.com>
>> >> wrote:
>> >> > On Tue, May 20, 2014 at 05:15:08PM +0200, Igor Mammedov wrote:
>> >> >> Add following parameters:
>> >> >> "slots" - total number of hotplug memory slots
>> >> >> "maxmem" - maximum possible memory
>> >> >>
>> >> >> "slots" and "maxmem" should go in pair and "maxmem" should be greater
>> >> >> than "mem" for memory hotplug to be enabled.
>> >> >>
>> >> >> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
>> >> >
>> >> > Also, it's a bug to mix this with a compat machine type, right?
>> >> > Maybe best to fail initialization if users try this.
>> >> >
>> >> >> ---
>> >> >> v4:
>> >> >> - store maxmem & slots values in MachineState
>> >> >> v3:
>> >> >> - store maxmem & slots values in QEMUMachineInitArgs
>> >> >> v2:
>> >> >> - rebased on top of the latest "vl: convert -m to QemuOpts"
>> >> >> ---
>> >> >> include/hw/boards.h | 3 ++-
>> >> >> qemu-options.hx | 9 ++++++---
>> >> >> vl.c | 51
>> >> >> +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >> 3 files changed, 59 insertions(+), 4 deletions(-)
>> >> >>
>> >> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> >> >> index b62de4a..f6fbbe1 100644
>> >> >> --- a/include/hw/boards.h
>> >> >> +++ b/include/hw/boards.h
>> >> >> @@ -8,7 +8,6 @@
>> >> >> #include "hw/qdev.h"
>> >> >> #include "qom/object.h"
>> >> >>
>> >> >> -
>> >> >> typedef struct MachineState MachineState;
>> >> >>
>> >> >> typedef void QEMUMachineInitFunc(MachineState *ms);
>> >> >> @@ -113,6 +112,8 @@ struct MachineState {
>> >> >> char *firmware;
>> >> >>
>> >> >> ram_addr_t ram_size;
>> >> >> + ram_addr_t maxram_size;
>> >> >> + uint64_t ram_slots;
>> >> >> const char *boot_order;
>> >> >> const char *kernel_filename;
>> >> >> const char *kernel_cmdline;
>> >> >> diff --git a/qemu-options.hx b/qemu-options.hx
>> >> >> index 781af14..c6411c4 100644
>> >> >> --- a/qemu-options.hx
>> >> >> +++ b/qemu-options.hx
>> >> >> @@ -210,17 +210,20 @@ use is discouraged as it may be removed from
>> >> >> future versions.
>> >> >> ETEXI
>> >> >>
>> >> >> DEF("m", HAS_ARG, QEMU_OPTION_m,
>> >> >> - "-m [size=]megs\n"
>> >> >> + "-m[emory] [size=]megs[,slots=n,maxmem=size]\n"
>> >> >> " configure guest RAM\n"
>> >> >> " size: initial amount of guest memory (default: "
>> >> >> - stringify(DEFAULT_RAM_SIZE) "MiB)\n",
>> >> >> + stringify(DEFAULT_RAM_SIZE) "MiB)\n"
>> >> >> + " slots: number of hotplug slots (default: none)\n"
>> >> >> + " maxmem: maximum amount of guest memory (default:
>> >> >> none)\n",
>> >> >> QEMU_ARCH_ALL)
>> >> >> STEXI
>> >> >> @item -m [size=]@var{megs}
>> >> >> @findex -m
>> >> >> Set virtual RAM size to @var{megs} megabytes. Default is 128 MiB.
>> >> >> Optionally,
>> >> >> a suffix of ``M'' or ``G'' can be used to signify a value in
>> >> >> megabytes or
>> >> >> -gigabytes respectively.
>> >> >> +gigabytes respectively. Optional pair @var{slots}, @var{maxmem} could
>> >> >> be used
>> >> >> +to set amount of hotluggable memory slots and possible maximum amount
>> >> >> of memory.
>> >> >> ETEXI
>> >> >>
>> >> >> DEF("mem-path", HAS_ARG, QEMU_OPTION_mempath,
>> >> >> diff --git a/vl.c b/vl.c
>> >> >> index 8fd4ed9..9fb6fa4 100644
>> >> >> --- a/vl.c
>> >> >> +++ b/vl.c
>> >> >> @@ -520,6 +520,14 @@ static QemuOptsList qemu_mem_opts = {
>> >> >> .name = "size",
>> >> >> .type = QEMU_OPT_SIZE,
>> >> >> },
>> >> >> + {
>> >> >> + .name = "slots",
>> >> >> + .type = QEMU_OPT_NUMBER,
>> >> >> + },
>> >> >> + {
>> >> >> + .name = "maxmem",
>> >> >> + .type = QEMU_OPT_SIZE,
>> >> >> + },
>> >> >> { /* end of list */ }
>> >> >> },
>> >> >> };
>> >> >> @@ -2989,6 +2997,8 @@ int main(int argc, char **argv, char **envp)
>> >> >> const char *trace_file = NULL;
>> >> >> const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>> >> >> 1024 * 1024;
>> >> >> + ram_addr_t maxram_size = default_ram_size;
>> >> >> + uint64_t ram_slots = 0;
>> >> >>
>> >> >> atexit(qemu_run_exit_notifiers);
>> >> >> error_set_progname(argv[0]);
>> >> >> @@ -3324,6 +3334,7 @@ int main(int argc, char **argv, char **envp)
>> >> >> case QEMU_OPTION_m: {
>> >> >> uint64_t sz;
>> >> >> const char *mem_str;
>> >> >> + const char *maxmem_str, *slots_str;
>> >> >>
>> >> >> opts = qemu_opts_parse(qemu_find_opts("memory"),
>> >> >> optarg, 1);
>> >> >> @@ -3365,6 +3376,44 @@ int main(int argc, char **argv, char **envp)
>> >> >> error_report("ram size too large");
>> >> >> exit(EXIT_FAILURE);
>> >> >> }
>> >> >> +
>> >> >> + maxmem_str = qemu_opt_get(opts, "maxmem");
>> >> >> + slots_str = qemu_opt_get(opts, "slots");
>> >> >> + if (maxmem_str && slots_str) {
>> >> >> + uint64_t slots;
>> >> >> +
>> >> >> + sz = qemu_opt_get_size(opts, "maxmem", 0);
>> >> >> + if (sz < ram_size) {
>> >> >> + fprintf(stderr, "qemu: invalid -m option
>> >> >> value: maxmem "
>> >> >> + "(%" PRIu64 ") <= initial memory (%"
>> >> >> + PRIu64 ")\n", sz, ram_size);
>> >> >> + exit(EXIT_FAILURE);
>> >> >> + }
>> >> >> +
>> >> >> + slots = qemu_opt_get_number(opts, "slots", 0);
>> >> >> + if ((sz > ram_size) && !slots) {
>> >> >> + fprintf(stderr, "qemu: invalid -m option
>> >> >> value: maxmem "
>> >> >> + "(%" PRIu64 ") more than initial
>> >> >> memory (%"
>> >> >> + PRIu64 ") but no hotplug slots where "
>> >> >> + "specified\n", sz, ram_size);
>> >> >> + exit(EXIT_FAILURE);
>> >> >> + }
>> >> >> +
>> >> >> + if ((sz <= ram_size) && slots) {
>> >> >> + fprintf(stderr, "qemu: invalid -m option
>> >> >> value: %"
>> >> >> + PRIu64 " hotplug slots where
>> >> >> specified but "
>> >> >> + "maxmem (%" PRIu64 ") <= initial
>> >> >> memory (%"
>> >> >> + PRIu64 ")\n", slots, sz, ram_size);
>> >> >> + exit(EXIT_FAILURE);
>> >> >> + }
>> >> >> + maxram_size = sz;
>> >> >> + ram_slots = slots;
>> >> >> + } else if ((!maxmem_str && slots_str) ||
>> >> >> + (maxmem_str && !slots_str)) {
>> >> >> + fprintf(stderr, "qemu: invalid -m option value:
>> >> >> missing "
>> >> >> + "'%s' option\n", slots_str ? "maxmem" :
>> >> >> "slots");
>> >> >> + exit(EXIT_FAILURE);
>> >> >> + }
>> >> >> break;
>> >> >> }
>> >> >> #ifdef CONFIG_TPM
>> >> >> @@ -4422,6 +4471,8 @@ int main(int argc, char **argv, char **envp)
>> >> >> qdev_machine_init();
>> >> >>
>> >> >> current_machine->ram_size = ram_size;
>> >> >> + current_machine->maxram_size = maxram_size;
>> >> >> + current_machine->ram_slots = ram_slots;
>> >> >> current_machine->boot_order = boot_order;
>> >> >> current_machine->kernel_filename = kernel_filename;
>> >> >> current_machine->kernel_cmdline = kernel_cmdline;
>> >> >> --
>> >> >> 1.7.1
>> >>
>> >> May be I am adding very userish opinion, but ability to specify slot
>> >> state via cmdline explicitly, like in
>> >> https://github.com/vliaskov/qemu-kvm/tree/memhp-v5-wip, looks better
>> >> in sight of thoughts of future integration in libvirt and for unplug
>> >> option (where knowledge of which regions are offlined is necessary to
>> >> do the job).
>> > slots are 'not populated' by default, and if specific slot should be
>> > populated then it should have corresponding "-device dimm,slot=XXX"
>> > on QEMU CLI.
>> > There is not much point to specify on CLI not present DIMMs, that way
>> > it would be less confusing and user won't have to worry about not
>> > present DIMMs options at startup (slot/size/addr/node). That makes
>> > VM configuration flexible and allows user to decide parameters at runtime.
>> >
>>
>> Ok, so on updating memory configuration we`ll have to (un)plug the
>> device and update inactive libvirt config for appropriate part in
>> current notation by some kind of mighty mechanism from above.
> It's general (un)plug problem affecting all hotpluggable devices.
> Currently libvirt works ok with hotplugging split device model
> (backend/frontend) /i.e. dimm device uses the same model as net devices/
>
> Libvirt has notion of "--persistent/transient" for some devices
> so it could be reused for memory device as well to decide what to do
> with it.
>
Yes, I meant it, may be a bit unclear.
>> Unplugging will work well but with helper from the upper-level
>> orchestrator,
> What do you mean under helping?
Something should tell virtualized environment to offline regions which
are about to be unplugged and check if offline request completed
successfully, most probably it`ll be an extension for qga, I
referenced it as help from above in case if agent will not implement
this method. I am not aware of any implementation of ACPI notification
methods with same functionality, it`ll remove userspace feedback loop
completely.
>
>> I think that doing boot param injection in
>> non-direct-booted kernel for declaration of ZONE_MOVABLE and doing
>> appropriate calculation for the config is too hard and complex for
>> role which libvirt currently holds, so current approach is acceptable
>> there too.
> There shouldn't be need for libvirt to care about above stuff at all.
>
> It's linux thing that should be fixed there and not in libvirt.
> ACPI memory device provides all necessary info for doing it.
> i.e. if memory device is removable (has _EJ method) then OSPM
> should put it in movable zone or not attempt to eject it if
> it's not in movable zone.
>
>
Last time I tried, linux kernel was unable to do this automatically. I
had not poked yet very recent kernels, so it may work now.
>> There are two paths - simplify dimm management as much as
>> possible and therefore do not rely on logic inside libvirt, or build
>> very monstrous and complex, but self-sufficient (in libvirt scope)
>> interface for dimm operations. With those selections, I`d vote for
>> first of course.
