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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 6 Oct 2017 20:17:41 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Oct 06, 2017 at 11:03:52AM +0200, Igor Mammedov wrote:
> > On Fri, 6 Oct 2017 14:54:47 +1100
> > David Gibson <david@gibson.dropbear.id.au> 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 <imammedo@redhat.com>    
> > > 
> > > 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).
I'll use cast to spapr machine here for now, and leave it up to you
to change it in future to something else if it make sense.

btw: when pvn + power9 crushes it does so due to null vhyp
if I recall correctly

> > > 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.
all targets that have special handling for -cpu options like
parse '+-foo' format or 'foo' or fixup value of property or rename property
to new name do provide target specific CPUClass::parse_features callback
to deal with it, the rest of the targets use generic cpu_common_parse_features().


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