On Fri, Oct 06, 2017 at 11:03:52AM +0200, Igor Mammedov wrote: > On Fri, 6 Oct 2017 14:54:47 +1100 > David Gibson <da...@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 <imamm...@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).
> > 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
