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

[Andrey Korolyov]

On Wed, May 21, 2014 at 1:52 PM, Igor Mammedov <address@hidden> wrote:
> 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.
>

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.