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

[David Gibson]

On Fri, Oct 06, 2017 at 11:03:52AM +0200, Igor Mammedov wrote:
> 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.

Yeah, I think we should do that.  Or possibly we could do something
with the vhyp interface.  If there was ever a vhyp machine other than
spapr (unlikely) then compat modes would also make sense for it (vhyp
being non-NULL basically means that our vcpu is never allowed to enter
hypervisor mode, so hypervisor privileged registers like LPCR need to
be handled by the machine instead).

> > 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.

Hm, ok.  Still seems a bit odd to me, but I'll take your word for it.

> 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;  
> > 
> 

-- 
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

signature.asc
Description: PGP signature