Re: [Qemu-devel] [PATCH v2 12/24] ppc: move '-cpu foo, compat=xxx' parsing into ppc_cpu_parse_featurestr()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8281 bytes --]

On Mon, Oct 09, 2017 at 09:50:59PM +0200, Igor Mammedov wrote:
> there is a dedicated callback CPUClass::parse_features
> which purpose is to convert -cpu features into a set of
> global properties AND deal with compat/legacy features
> that couldn't be directly translated into CPU's properties.
> 
> Create ppc variant of it (ppc_cpu_parse_featurestr) and
> move 'compat=val' handling from spapr_cpu_core.c into it.
> That removes a dependency of board/core code on cpu_model
> parsing and would let to reuse common -cpu parsing
> introduced by 6063d4c0
> 
> Set "max-cpu-compat" property only if it exists, in practice
> it should limit 'compat' hack to spapr machine and allow
> to avoid including machine/spapr headers in target/ppc/cpu.c
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v2:
>   - set "max-cpu-compat" property only if it exists,
>     in practice it should limit 'compat' hack to spapr
>     and allow to avoid including machine/spapr headers
>     in target/ppc/cpu.c
>   - do compat= parsing only if machine."max-cpu-compat" exists
>     that would match what current code does.
>   - add newline before ppc_cpu_parse_featurestr() definition
> ---
>  include/hw/ppc/spapr.h      |  1 -
>  target/ppc/cpu-qom.h        |  1 +
>  hw/ppc/spapr.c              |  2 +-
>  hw/ppc/spapr_cpu_core.c     | 50 ---------------------------------------
>  target/ppc/translate_init.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 59 insertions(+), 52 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c1b365f..8ca4f94 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -659,7 +659,6 @@ void spapr_hotplug_req_add_by_count_indexed(sPAPRDRConnectorType drc_type,
>                                              uint32_t count, uint32_t index);
>  void spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
>                                                 uint32_t count, uint32_t index);
> -void spapr_cpu_parse_features(sPAPRMachineState *spapr);
>  int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
>  void spapr_reallocate_hpt(sPAPRMachineState *spapr, int shift,
>                            Error **errp);
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index d0cf6ca..429b47f 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -181,6 +181,7 @@ typedef struct PowerPCCPUClass {
>      DeviceRealize parent_realize;
>      DeviceUnrealize parent_unrealize;
>      void (*parent_reset)(CPUState *cpu);
> +    void (*parent_parse_features)(const char *type, char *str, Error **errp);
>  
>      uint32_t pvr;
>      bool (*pvr_match)(struct PowerPCCPUClass *pcc, uint32_t pvr);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1a2ca8a..c7a0310 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2373,7 +2373,7 @@ static void ppc_spapr_init(MachineState *machine)
>          machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
>      }
>  
> -    spapr_cpu_parse_features(spapr);
> +    cpu_parse_cpu_model(TYPE_POWERPC_CPU, machine->cpu_model);
>  
>      spapr_set_vsmt_mode(spapr, &error_fatal);
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 79a9615..b6610dd 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -21,56 +21,6 @@
>  #include "sysemu/hw_accel.h"
>  #include "qemu/error-report.h"
>  
> -void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> -{
> -    /*
> -     * Backwards compatibility hack:
> -     *
> -     *   CPUs had a "compat=" property which didn't make sense for
> -     *   anything except pseries.  It was replaced by "max-cpu-compat"
> -     *   machine option.  This supports old command lines like
> -     *       -cpu POWER8,compat=power7
> -     *   By stripping the compat option and applying it to the machine
> -     *   before passing it on to the cpu level parser.
> -     */
> -    gchar **inpieces;
> -    gchar *newprops;
> -    int i, j;
> -    gchar *compat_str = NULL;
> -
> -    inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> -
> -    /* inpieces[0] is the actual model string */
> -    i = 1;
> -    j = 1;
> -    while (inpieces[i]) {
> -        if (g_str_has_prefix(inpieces[i], "compat=")) {
> -            /* in case of multiple compat= options */
> -            g_free(compat_str);
> -            compat_str = inpieces[i];
> -        } else {
> -            j++;
> -        }
> -
> -        i++;
> -        /* Excise compat options from list */
> -        inpieces[j] = inpieces[i];
> -    }
> -
> -    if (compat_str) {
> -        char *val = compat_str + strlen("compat=");
> -
> -        object_property_set_str(OBJECT(spapr), val, "max-cpu-compat",
> -                                &error_fatal);
> -
> -    }
> -
> -    newprops = g_strjoinv(",", inpieces);
> -    cpu_parse_cpu_model(TYPE_POWERPC_CPU, newprops);
> -    g_free(newprops);
> -    g_strfreev(inpieces);
> -}
> -
>  static void spapr_cpu_reset(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 0d6379f..3d16481 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -10097,6 +10097,61 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
>      return NULL;
>  }
>  
> +static void ppc_cpu_parse_featurestr(const char *type, char *features,
> +                                     Error **errp)
> +{
> +    Object *machine = qdev_get_machine();
> +    const PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(object_class_by_name(type));
> +
> +    if (!features) {
> +        return;
> +    }
> +
> +    if (object_property_find(machine, "max-cpu-compat", NULL)) {
> +        int i;
> +        char **inpieces;
> +        char *s = features;
> +        Error *local_err = NULL;
> +        char *compat_str = NULL;
> +
> +        /*
> +         * Backwards compatibility hack:
> +         *
> +         *   CPUs had a "compat=" property which didn't make sense for
> +         *   anything except pseries.  It was replaced by "max-cpu-compat"
> +         *   machine option.  This supports old command lines like
> +         *       -cpu POWER8,compat=power7
> +         *   By stripping the compat option and applying it to the machine
> +         *   before passing it on to the cpu level parser.
> +         */
> +        inpieces = g_strsplit(features, ",", 0);
> +        *s = '\0';
> +        for (i = 0; inpieces[i]; i++) {
> +            if (g_str_has_prefix(inpieces[i], "compat=")) {
> +                compat_str = inpieces[i];
> +                continue;
> +            }
> +            if ((i != 0) && (s != features)) {
> +                s = g_stpcpy(s, ",");
> +            }
> +            s = g_stpcpy(s, inpieces[i]);
> +        }
> +
> +        if (compat_str) {
> +            char *v = compat_str + strlen("compat=");
> +            object_property_set_str(machine, v, "max-cpu-compat", &local_err);
> +        }
> +        g_strfreev(inpieces);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
> +    /* do property processing with generic handler */
> +    pcc->parent_parse_features(type, features, errp);
> +}
> +
>  const char *ppc_cpu_lookup_alias(const char *alias)
>  {
>      int ai;
> @@ -10489,6 +10544,8 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->reset = ppc_cpu_reset;
>  
>      cc->class_by_name = ppc_cpu_class_by_name;
> +    pcc->parent_parse_features = cc->parse_features;
> +    cc->parse_features = ppc_cpu_parse_featurestr;
>      cc->has_work = ppc_cpu_has_work;
>      cc->do_interrupt = ppc_cpu_do_interrupt;
>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]