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

[Igor Mammedov]

On Wed, 21 May 2014 13:12:13 +0400
Andrey Korolyov <address@hidden> wrote:

> 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.
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.
 
> Unplugging will work well but with helper from the upper-level
> orchestrator,
What do you mean under helping?

> 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.


> 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.