| Message ID | 1507220690-265042-13-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | generalize parsing of cpu_model (part 3/PPC) | expand |
On Thu, 5 Oct 2017 18:24:39 +0200 Igor Mammedov <imammedo@redhat.com> 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> > --- > 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; > } Maybe add en empty line here ? > +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) { Can qdev_get_machine() return NULL really ? Shouldn't this be object_dynamic_cast(machine, TYPE_MACHINE) instead, in case we're running a user-only emulator ? > + 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;
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. It still pollutes the cpu code with spapr's past mistake though. I'm not sure if this is a good tradeoff. > --- > 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;
On Thu, 5 Oct 2017 21:05:37 +0200 Greg Kurz <groug@kaod.org> wrote: > On Thu, 5 Oct 2017 18:24:39 +0200 > Igor Mammedov <imammedo@redhat.com> 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> > > --- > > 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; > > } > > Maybe add en empty line here ? sure > > > +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) { > > Can qdev_get_machine() return NULL really ? Shouldn't this be > object_dynamic_cast(machine, TYPE_MACHINE) instead, in case > we're running a user-only emulator ? ok, I'll fix it like suggested > > > + 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; >
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. > > 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; >
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). > > 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; > > >
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; > > > > > >
On Fri, Oct 06, 2017 at 11:52:44AM +0200, Igor Mammedov wrote: > 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. Makes sense. As long as we get the behaviour right for now, we can clean up the implementation later. > btw: when pvn + power9 crushes it does so due to null vhyp > if I recall correctly Yeah, pnv is very much a work in progress, it barely works for POWER8 and is hardly tested for POWER9. It's also entirely possible we're misisng bits in the POWER9 code to handle the !vhyp case, since pseries is the only machine that's really been tested with POWER9 at all so far. > > > > 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(). Yeah, still smacks of the machine ~= target assumption that x86 people tend to make.
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;
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> --- 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(-)