qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 12/23] ppc: move '-cpu foo, compat=xxx' parsing


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH 12/23] ppc: move '-cpu foo, compat=xxx' parsing into ppc_cpu_parse_featurestr()
Date: Fri, 6 Oct 2017 11:03:52 +0200

On Fri, 6 Oct 2017 14:54:47 +1100
David Gibson <address@hidden> wrote:

> On Thu, Oct 05, 2017 at 06:24:39PM +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
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>  
> 
> Hrm.  I'm a bit unsure about this.  The fact that the board code is
> involved in the parsing here is deliberate.  Basically 'compat=' never
> made sense as a cpu property, it always should have been a machine
> property.
> 
> With this patch we'll still (correctly) fail on a non spapr machine at
> the point where we do:
> 
> +            object_property_set_str(machine, v, "max-cpu-compat", 
> &local_err);
> 
> Though probably with a rather cryptic error.
I's possible to guard it and apply to only SPAPR machine
or trying to find property first before setting it.

> 
> It still pollutes the cpu code with spapr's past mistake though.  I'm
> not sure if this is a good tradeoff.
CPUClass::parse_features callback was introduced to deal with 
legacy and compatibility nuances of "-cpu" CLI option, so I'm moving
compat mistake back where it belongs so it will pollute code dealing
with hacks.

It also cleans the road for removal of global cpu_model by moving
dependency from board code to a callback designed to parse cpu_model.


> > ---
> >  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 | 58 
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 60 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 ff87f15..01b3012 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2366,7 +2366,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 3dea5ff..427d47f 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 c6399a3..5ee91e8 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -10313,6 +10313,62 @@ static ObjectClass *ppc_cpu_class_by_name(const 
> > char *name)
> >  
> >      return NULL;
> >  }
> > +static void ppc_cpu_parse_featurestr(const char *typename, char *features,
> > +                                     Error **errp)
> > +{
> > +    const PowerPCCPUClass *pcc;
> > +    char *compat_str = NULL;
> > +    char *s = features;
> > +    char **inpieces;
> > +    Error *local_err = NULL;
> > +    int i;
> > +
> > +    if (!features) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * 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) {
> > +        Object *machine = qdev_get_machine();
> > +        if (machine) {
> > +            char *v = compat_str + strlen("compat=");
> > +            object_property_set_str(machine, v, "max-cpu-compat", 
> > &local_err);
> > +        } else {
> > +            error_setg(&local_err, "Not supported property: %s", 
> > compat_str);
> > +        }
> > +    }
> > +    g_strfreev(inpieces);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +    /* do property processing with generic handler */
> > +    pcc = POWERPC_CPU_CLASS(object_class_by_name(typename));
> > +    pcc->parent_parse_features(typename, features, errp);
> > +}
> >  
> >  const char *ppc_cpu_lookup_alias(const char *alias)
> >  {
> > @@ -10706,6 +10762,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;  
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]