Re: [PATCH 21/29] vl: separate qemu_resolve_machine_memdev

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 27 Oct 2020 14:21:36 -0400
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This is a bit nasty: the machine is storing a string and later
> resolving it.  We probably want to remove the memdev property
> and instead make this a memory-set command.  "-M memdev" can be
> handled as a legacy option that is special cased by
> machine_set_property.
I'd treat this description as topic starter and drop it from this patch in v2.
with that,
  Reviewed-by: Igor Mammedov <imammedo@redhat.com>

how  memory-set would help or be any better than memdev?

memdev should be a link property, but due to RAM allocation
being dependent on used accel we can't create actual backend
until accelerator is initialized (i.e. after machine options parsed,
which forced me to make memdev a string that refers to a backend
created later).

If we can make RAM allocation independent from used accelerator
(if I recall right it has TCG dependency) and if we split -m CLI
handling and default ram_memdev_id (which is implicit CLI),
then we can make -M memdev a link and move RAM backends to
qemu_create_early_backends() time.

Which in context of creating machine via QMP would imply that
link should be set explicitly via QMP after backend is created.

Flow could look like this:
CLI part:
  -m / defaults - preps and 'if not NUMA'
     executes QMP
       object_add memory-backend-foo,size=X,id=(ram_memdev_id)
     in case -M memory-backend is not set explicitly, or fetch
     id of explicitly provided backend (which would be created by
     qemu_create_early_backends)
QMP part:
   object_add machine_foo,id=my_machine
   set (link) my_machine.memory-backend=(ram_memdev_id)

that way we do no need to create a separate memory-set command,
we can  handle it as a normal property, where all compat stuff
is kept in CLI part.

For CLI part handling there are 2 caveats:

 * Xen doesn't use memdev at all, it allocates memory region directly.
   Not sure what we should do in this case
   (may be we can create a separate xen-ram backend for it and remove
    'mr == &ram_memory' in xen_ram_alloc() hack)

 * legacy S390 machine types (<5.0) fixup ram_size before creating backend, if user
   provided value is not correct 5c30ef937f5 (i.e. not aligned properly).
   I guess we are stuck with it, given it's version-ed machine type. The rest of the code was
   fixed to error-out in the case board doesn't like -m value.
   Or we can treat it as user error (which should be corrected by user) and deprecate/remove fixup. 

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  softmmu/vl.c | 70 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 37 insertions(+), 33 deletions(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 9a3c92387e..1485aba8be 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2834,6 +2834,42 @@ static bool have_custom_ram_size(void)
>      return !!qemu_opt_get(opts, "size");
>  }
>  
> +static void qemu_resolve_machine_memdev(void)
> +{
> +    if (current_machine->ram_memdev_id) {
> +        Object *backend;
> +        ram_addr_t backend_size;
> +
> +        backend = object_resolve_path_type(current_machine->ram_memdev_id,
> +                                           TYPE_MEMORY_BACKEND, NULL);
> +        if (!backend) {
> +            error_report("Memory backend '%s' not found",
> +                         current_machine->ram_memdev_id);
> +            exit(EXIT_FAILURE);
> +        }
> +        backend_size = object_property_get_uint(backend, "size",  &error_abort);
> +        if (have_custom_ram_size() && backend_size != ram_size) {
> +                error_report("Size specified by -m option must match size of "
> +                             "explicitly specified 'memory-backend' property");
> +                exit(EXIT_FAILURE);
> +        }
> +        if (mem_path) {
> +            error_report("'-mem-path' can't be used together with"
> +                         "'-machine memory-backend'");
> +            exit(EXIT_FAILURE);
> +        }
> +        ram_size = backend_size;
> +    }
> +
> +    if (!xen_enabled()) {
> +        /* On 32-bit hosts, QEMU is limited by virtual address space */
> +        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> +            error_report("at most 2047 MB RAM can be simulated");
> +            exit(1);
> +        }
> +    }
> +}
> +
>  static void set_memory_options(MachineClass *mc)
>  {
>      uint64_t sz;
> @@ -4476,39 +4512,7 @@ void qemu_init(int argc, char **argv, char **envp)
>          current_machine->cpu_type = parse_cpu_option(cpu_option);
>      }
>  
> -    if (current_machine->ram_memdev_id) {
> -        Object *backend;
> -        ram_addr_t backend_size;
> -
> -        backend = object_resolve_path_type(current_machine->ram_memdev_id,
> -                                           TYPE_MEMORY_BACKEND, NULL);
> -        if (!backend) {
> -            error_report("Memory backend '%s' not found",
> -                         current_machine->ram_memdev_id);
> -            exit(EXIT_FAILURE);
> -        }
> -        backend_size = object_property_get_uint(backend, "size",  &error_abort);
> -        if (have_custom_ram_size() && backend_size != ram_size) {
> -                error_report("Size specified by -m option must match size of "
> -                             "explicitly specified 'memory-backend' property");
> -                exit(EXIT_FAILURE);
> -        }
> -        if (mem_path) {
> -            error_report("'-mem-path' can't be used together with"
> -                         "'-machine memory-backend'");
> -            exit(EXIT_FAILURE);
> -        }
> -        ram_size = backend_size;
> -    }
> -
> -    if (!xen_enabled()) {
> -        /* On 32-bit hosts, QEMU is limited by virtual address space */
> -        if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> -            error_report("at most 2047 MB RAM can be simulated");
> -            exit(1);
> -        }
> -    }
> -
> +    qemu_resolve_machine_memdev();
>      parse_numa_opts(current_machine);
>  
>      /* do monitor/qmp handling at preconfig state if requested */