Re: [Qemu-devel] [PATCH v4 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Feb 05, 2018 at 06:08:29PM +0100, Igor Mammedov wrote:
> With all targets defining CPU_RESOLVING_TYPE, refactor
> cpu_parse_cpu_model(type, cpu_model) to parse_cpu_model(cpu_model)
> so that callers won't have to know internal resolving cpu
> type. Place it in exec.c so it could be called from both
> target independed vl.c and *-user/main.c.
> 
> That allows us to stop abusing cpu type from
>   MachineClass::default_cpu_type
> as resolver class in vl.c which were confusing part of
> cpu_parse_cpu_model().
> 
> Also with new parse_cpu_model(), the last users of cpu_init()
> in null-machine.c and bsd/linux-user targets could be switched
> to cpu_create() API and cpu_init() API will be removed by
> follow up patch.
> 
> With no longer users left remove MachineState::cpu_model field,
> new code should use MachineState::cpu_type instead and
> leave cpu_model parsing to generic code in vl.c.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>   - actually remove no longer used MachineState::cpu_model field
>     that I've lost somewhere during respins
> 
>   - squash in [PATCH v3 25/25] cpu: get rid of cpu_generic_init()
>     as after rework/rebase cpu_generic_init() is being removed by
>     this patch and only check removal was left in 25/25, which
>     should be removed together with cpu_generic_init() in this patch
> 
> CC: Richard Henderson <richard.henderson@linaro.org>
> CC: "Emilio G. Cota" <cota@braap.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: "Alex Bennée" <alex.bennee@linaro.org>
> CC: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
> 
> fixup: Use cpu_create(type) instead of  cpu_init(cpu_model)
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/boards.h    |  1 -
>  include/qom/cpu.h      | 16 ++--------------
>  bsd-user/main.c        |  4 +++-
>  exec.c                 | 23 +++++++++++++++++++++++
>  hw/core/null-machine.c |  6 +++---
>  linux-user/main.c      |  8 ++++++--
>  qom/cpu.c              | 48 ++----------------------------------------------
>  vl.c                   | 10 +++-------
>  8 files changed, 42 insertions(+), 74 deletions(-)

Less 32 lines.  Nice.  :)


[...]
> @@ -335,22 +304,9 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
>  static void cpu_common_parse_features(const char *typename, char *features,
>                                        Error **errp)
>  {
> -    char *featurestr; /* Single "key=value" string being parsed */
>      char *val;
> -    static bool cpu_globals_initialized;
> -
> -    /* TODO: all callers of ->parse_features() need to be changed to
> -     * call it only once, so we can remove this check (or change it
> -     * to assert(!cpu_globals_initialized).
> -     * Current callers of ->parse_features() are:
> -     * - cpu_generic_init()
> -     */
> -    if (cpu_globals_initialized) {
> -        return;

Suggestion: replace this with assert(!cpu_globals_initialized)
just to make sure there are no bugs that make us register CPU
globals twice.

This shouldn't block the patch, though:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> -    }
> -    cpu_globals_initialized = true;
> -
> -    featurestr = features ? strtok(features, ",") : NULL;
> +    /* Single "key=value" string being parsed */
> +    char *featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
>          val = strchr(featurestr, '=');
[...]

-- 
Eduardo