qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 05/31] vl.c: extend -m option to support opti


From: Andrey Korolyov
Subject: Re: [Qemu-devel] [PATCH v2 05/31] vl.c: extend -m option to support options for memory hotplug
Date: Wed, 21 May 2014 13:12:13 +0400

On Wed, May 21, 2014 at 12:55 PM, Igor Mammedov <address@hidden> wrote:
> On Wed, 21 May 2014 12:27:05 +0400
> Andrey Korolyov <address@hidden> wrote:
>
>> On Wed, May 21, 2014 at 12:10 PM, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
>> >
>> > 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 address@hidden
>> >>  @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.
Unplugging will work well but with helper from the upper-level
orchestrator, 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 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]