[v2] softmmu/vl.c: fix too slow TCG regression

Commit a1b18df9a4 moved -m option parsing after configure_accelerators()
that broke TCG accelerator initialization which accesses global ram_size
from size_code_gen_buffer() which is equal to 0 at that moment.

Partially revert a1b18df9a4, by returning set_memory_options() to its
original location and only keep 32-bit host VA check and 'memory-backend'
size check introduced by fe64d06afc at current place.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * check if user used -m SZ before comparing ram_size with backend's size.
    Fixes case where qemu exits with error when backend's size doesn't match
    dafault ram_size (i.e. user haven't used -m option  at all)
    (Peter Maydell <peter.maydell@linaro.org>)
  * keep order addr space size check for 32-bit host after
    backend size check. So it would be affected by backend's size as well.
---
 softmmu/vl.c | 54 ++++++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

Can we improve the patch subject? "vl: Fix slower TCG on softmmu" or better?

On 2/27/20 5:14 PM, Igor Mammedov wrote:
> Commit a1b18df9a4 moved -m option parsing after configure_accelerators()
> that broke TCG accelerator initialization which accesses global ram_size
> from size_code_gen_buffer() which is equal to 0 at that moment.
> 
> Partially revert a1b18df9a4, by returning set_memory_options() to its
> original location and only keep 32-bit host VA check and 'memory-backend'
> size check introduced by fe64d06afc at current place.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
> v2:
>    * check if user used -m SZ before comparing ram_size with backend's size.
>      Fixes case where qemu exits with error when backend's size doesn't match
>      dafault ram_size (i.e. user haven't used -m option  at all)
>      (Peter Maydell <peter.maydell@linaro.org>)
>    * keep order addr space size check for 32-bit host after
>      backend size check. So it would be affected by backend's size as well.
> ---
>   softmmu/vl.c | 54 ++++++++++++++++++++++++++++------------------------
>   1 file changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index a9cce78f45..16ff5a16a3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2558,7 +2558,7 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
>   }
>   
>   
> -static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
> +static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>                                  MachineClass *mc)
>   {
>       uint64_t sz;
> @@ -2634,30 +2634,8 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>           exit(EXIT_FAILURE);
>       }
>   
> -    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);
> -        backend_size = object_property_get_uint(backend, "size",  &error_abort);
> -        if (mem_str && backend_size != ram_size) {
> -                error_report("Size specified by -m option must match size of "
> -                             "explicitly specified 'memory-backend' property");
> -                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);
> -        }
> -    }
> -
>       loc_pop(&loc);
> +    return !!mem_str;
>   }
>   
>   static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
> @@ -2861,6 +2839,7 @@ void qemu_init(int argc, char **argv, char **envp)
>       bool list_data_dirs = false;
>       char *dir, **dirs;
>       const char *mem_path = NULL;
> +    bool have_custom_ram_size;
>       BlockdevOptionsQueue bdo_queue = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
>       QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list);
>       int mem_prealloc = 0; /* force preallocation of physical target memory */
> @@ -3821,6 +3800,9 @@ void qemu_init(int argc, char **argv, char **envp)
>       machine_class = select_machine();
>       object_set_machine_compat_props(machine_class->compat_props);
>   
> +    have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
> +                                              machine_class);
> +
>       os_daemonize();
>   
>       /*
> @@ -4296,7 +4278,29 @@ void qemu_init(int argc, char **argv, char **envp)
>           current_machine->cpu_type = parse_cpu_option(cpu_option);
>       }
>   
> -    set_memory_options(&ram_slots, &maxram_size, machine_class);
> +    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);
> +        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);
> +        }
> +        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);
> +        }
> +    }
> +
>       current_machine->ram_size = ram_size;
>       current_machine->maxram_size = maxram_size;
>       current_machine->ram_slots = ram_slots;
>

On 2/27/20 5:14 PM, Igor Mammedov wrote:
> Commit a1b18df9a4 moved -m option parsing after configure_accelerators()
> that broke TCG accelerator initialization which accesses global ram_size
> from size_code_gen_buffer() which is equal to 0 at that moment.
> 
> Partially revert a1b18df9a4, by returning set_memory_options() to its
> original location and only keep 32-bit host VA check and 'memory-backend'
> size check introduced by fe64d06afc at current place.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>    * check if user used -m SZ before comparing ram_size with backend's size.
>      Fixes case where qemu exits with error when backend's size doesn't match
>      dafault ram_size (i.e. user haven't used -m option  at all)
>      (Peter Maydell <peter.maydell@linaro.org>)
>    * keep order addr space size check for 32-bit host after
>      backend size check. So it would be affected by backend's size as well.
> ---
>   softmmu/vl.c | 54 ++++++++++++++++++++++++++++------------------------
>   1 file changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index a9cce78f45..16ff5a16a3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2558,7 +2558,7 @@ static bool object_create_delayed(const char *type, QemuOpts *opts)
>   }
>   
>   
> -static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
> +static bool set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>                                  MachineClass *mc)
>   {
>       uint64_t sz;
> @@ -2634,30 +2634,8 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>           exit(EXIT_FAILURE);
>       }
>   
> -    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);
> -        backend_size = object_property_get_uint(backend, "size",  &error_abort);
> -        if (mem_str && backend_size != ram_size) {
> -                error_report("Size specified by -m option must match size of "
> -                             "explicitly specified 'memory-backend' property");
> -                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);
> -        }
> -    }
> -
>       loc_pop(&loc);
> +    return !!mem_str;
>   }
>   
>   static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
> @@ -2861,6 +2839,7 @@ void qemu_init(int argc, char **argv, char **envp)
>       bool list_data_dirs = false;
>       char *dir, **dirs;
>       const char *mem_path = NULL;
> +    bool have_custom_ram_size;
>       BlockdevOptionsQueue bdo_queue = QSIMPLEQ_HEAD_INITIALIZER(bdo_queue);
>       QemuPluginList plugin_list = QTAILQ_HEAD_INITIALIZER(plugin_list);
>       int mem_prealloc = 0; /* force preallocation of physical target memory */
> @@ -3821,6 +3800,9 @@ void qemu_init(int argc, char **argv, char **envp)
>       machine_class = select_machine();
>       object_set_machine_compat_props(machine_class->compat_props);
>   
> +    have_custom_ram_size = set_memory_options(&ram_slots, &maxram_size,
> +                                              machine_class);
> +
>       os_daemonize();
>   
>       /*
> @@ -4296,7 +4278,29 @@ void qemu_init(int argc, char **argv, char **envp)
>           current_machine->cpu_type = parse_cpu_option(cpu_option);
>       }
>   
> -    set_memory_options(&ram_slots, &maxram_size, machine_class);
> +    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);
> +        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);
> +        }
> +        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);
> +        }
> +    }
> +
>       current_machine->ram_size = ram_size;
>       current_machine->maxram_size = maxram_size;
>       current_machine->ram_slots = ram_slots;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

On Thu, 27 Feb 2020 at 16:15, Igor Mammedov <imammedo@redhat.com> wrote:
>
> Commit a1b18df9a4 moved -m option parsing after configure_accelerators()
> that broke TCG accelerator initialization which accesses global ram_size
> from size_code_gen_buffer() which is equal to 0 at that moment.
>
> Partially revert a1b18df9a4, by returning set_memory_options() to its
> original location and only keep 32-bit host VA check and 'memory-backend'
> size check introduced by fe64d06afc at current place.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   * check if user used -m SZ before comparing ram_size with backend's size.
>     Fixes case where qemu exits with error when backend's size doesn't match
>     dafault ram_size (i.e. user haven't used -m option  at all)
>     (Peter Maydell <peter.maydell@linaro.org>)
>   * keep order addr space size check for 32-bit host after
>     backend size check. So it would be affected by backend's size as well.

Applied to master as a fix for the tcg perf regression, thanks.

-- PMM