* [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
@ 2016-06-18 8:34 Bharata B Rao
2016-06-21 5:10 ` David Gibson
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Bharata B Rao @ 2016-06-18 8:34 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, david, imammedo, thuth, Bharata B Rao
Compat CPU type is typically specified on -cpu cmdline option like:
-cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
With the introduction of sPAPR CPU core devices, we need to support
the same for core devices too.
Support the specification of CPU compat type on device_add command for
sPAPRCPUCore devices like:
(qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
Applies on ppc-for-2.7 branch of David Gibson's tree.
hw/ppc/spapr.c | 8 +++++
hw/ppc/spapr_cpu_core.c | 73 +++++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr_cpu_core.h | 2 ++
3 files changed, 83 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 778fa25..2049d7d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
if (i < spapr_cores) {
char *type = spapr_get_cpu_core_type(machine->cpu_model);
Object *core;
+ char *compat;
if (!object_class_by_name(type)) {
error_report("Unable to find sPAPR CPU Core definition");
@@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine)
&error_fatal);
object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
&error_fatal);
+ compat = spapr_get_cpu_compat_type(machine->cpu_model);
+ if (compat) {
+ object_property_set_str(core, compat, "compat",
+ &error_fatal);
+ g_free(compat);
+ }
+
object_property_set_bool(core, true, "realized", &error_fatal);
}
}
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3a5da09..9eb63cc 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
return core_type;
}
+/*
+ * Returns the CPU compat type specified in -cpu @model.
+ */
+char *spapr_get_cpu_compat_type(const char *model)
+{
+ char *compat_type = NULL;
+ gchar **model_pieces = g_strsplit(model, ",", 2);
+
+ if (model_pieces[1]) {
+ gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
+
+ compat_type = g_strdup_printf("%s", compat_pieces[1]);
+ }
+
+ g_strfreev(model_pieces);
+ return compat_type;
+}
+
static void spapr_core_release(DeviceState *dev, void *opaque)
{
sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
@@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
CPUCore *cc = CPU_CORE(dev);
char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
const char *type = object_get_typename(OBJECT(dev));
+ char *base_compat_type = NULL;
+ char *compat = NULL;
+ bool compat_set;
if (strcmp(base_core_type, type)) {
error_setg(&local_err, "CPU core type should be %s", base_core_type);
goto out;
}
+ base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
+ compat = object_property_get_str(OBJECT(dev), "compat", NULL);
+ compat_set = (compat && *compat) ? true : false;
+
+ if (base_compat_type) {
+ if ((compat_set && strcmp(base_compat_type, compat)) ||
+ !compat_set) {
+ error_setg(&local_err, "CPU compat type should be %s",
+ base_compat_type);
+ goto out;
+ }
+ } else if (compat_set) {
+ error_setg(&local_err, "CPU compat type shouldn't be set");
+ goto out;
+ }
+
if (!smc->dr_cpu_enabled && dev->hotplugged) {
error_setg(&local_err, "CPU hotplug not supported for this machine");
goto out;
@@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
}
out:
+ g_free(compat);
+ g_free(base_compat_type);
g_free(base_core_type);
error_propagate(errp, local_err);
}
@@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
Error *local_err = NULL;
Object *obj;
int i;
+ char *compat = object_property_get_str(OBJECT(sc), "compat", NULL);
+ bool compat_set = (compat && *compat) ? true : false;
sc->threads = g_malloc0(size * cc->nr_threads);
for (i = 0; i < cc->nr_threads; i++) {
@@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
snprintf(id, sizeof(id), "thread[%d]", i);
object_property_add_child(OBJECT(sc), id, obj, &local_err);
if (local_err) {
+ g_free(compat);
goto err;
}
+ if (compat_set) {
+ CPUClass *cc = CPU_GET_CLASS(CPU(obj));
+ char *featurestr = g_strdup_printf("compat=%s", compat);
+
+ cc->parse_features(CPU(obj), featurestr, &local_err);
+ g_free(featurestr);
+ }
}
+
+ g_free(compat);
object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
if (local_err) {
goto err;
@@ -318,6 +369,27 @@ err:
error_propagate(errp, local_err);
}
+static char *spapr_cpu_core_prop_get_compat(Object *obj, Error **errp)
+{
+ sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
+
+ return g_strdup(core->cpu_compat);
+}
+
+static void spapr_cpu_core_prop_set_compat(Object *obj, const char *val,
+ Error **errp)
+{
+ sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
+
+ core->cpu_compat = g_strdup(val);
+}
+
+static void spapr_cpu_core_instance_init(Object *obj)
+{
+ object_property_add_str(obj, "compat", spapr_cpu_core_prop_get_compat,
+ spapr_cpu_core_prop_set_compat, NULL);
+}
+
static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
@@ -388,6 +460,7 @@ static const TypeInfo spapr_cpu_core_type_info = {
.parent = TYPE_CPU_CORE,
.abstract = true,
.instance_size = sizeof(sPAPRCPUCore),
+ .instance_init = spapr_cpu_core_instance_init,
.class_init = spapr_cpu_core_class_init,
};
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1c9b319..e697dab 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -24,11 +24,13 @@ typedef struct sPAPRCPUCore {
/*< public >*/
void *threads;
ObjectClass *cpu_class;
+ char *cpu_compat;
} sPAPRCPUCore;
void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
Error **errp);
char *spapr_get_cpu_core_type(const char *model);
+char *spapr_get_cpu_compat_type(const char *model);
void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
Error **errp);
void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-18 8:34 [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores Bharata B Rao
@ 2016-06-21 5:10 ` David Gibson
2016-06-22 2:36 ` Bharata B Rao
2016-06-21 6:04 ` Thomas Huth
2016-06-21 7:09 ` Igor Mammedov
2 siblings, 1 reply; 14+ messages in thread
From: David Gibson @ 2016-06-21 5:10 UTC (permalink / raw)
To: Bharata B Rao; +Cc: qemu-devel, qemu-ppc, imammedo, thuth
[-- Attachment #1: Type: text/plain, Size: 8470 bytes --]
On Sat, Jun 18, 2016 at 02:04:06PM +0530, Bharata B Rao wrote:
> Compat CPU type is typically specified on -cpu cmdline option like:
> -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> With the introduction of sPAPR CPU core devices, we need to support
> the same for core devices too.
>
> Support the specification of CPU compat type on device_add command for
> sPAPRCPUCore devices like:
> (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> Applies on ppc-for-2.7 branch of David Gibson's tree.
The implementation looks ok apart from a few nits noted below.
There's a larger problem here, though, in that this doesn't advertise
the necessary compat= property via query-hotpluggable-cpus qmp and hmp
interfaces. Which means that management has no good way of knowing
it's necessary.
>
> hw/ppc/spapr.c | 8 +++++
> hw/ppc/spapr_cpu_core.c | 73 +++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_cpu_core.h | 2 ++
> 3 files changed, 83 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 778fa25..2049d7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> if (i < spapr_cores) {
> char *type = spapr_get_cpu_core_type(machine->cpu_model);
> Object *core;
> + char *compat;
>
> if (!object_class_by_name(type)) {
> error_report("Unable to find sPAPR CPU Core definition");
> @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine)
> &error_fatal);
> object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> &error_fatal);
> + compat = spapr_get_cpu_compat_type(machine->cpu_model);
> + if (compat) {
> + object_property_set_str(core, compat, "compat",
> + &error_fatal);
> + g_free(compat);
> + }
> +
> object_property_set_bool(core, true, "realized", &error_fatal);
> }
> }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3a5da09..9eb63cc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> return core_type;
> }
>
> +/*
> + * Returns the CPU compat type specified in -cpu @model.
> + */
> +char *spapr_get_cpu_compat_type(const char *model)
> +{
> + char *compat_type = NULL;
> + gchar **model_pieces = g_strsplit(model, ",", 2);
> +
> + if (model_pieces[1]) {
> + gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> +
> + compat_type = g_strdup_printf("%s", compat_pieces[1]);
> + }
> +
> + g_strfreev(model_pieces);
> + return compat_type;
> +}
> +
> static void spapr_core_release(DeviceState *dev, void *opaque)
> {
> sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> CPUCore *cc = CPU_CORE(dev);
> char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> const char *type = object_get_typename(OBJECT(dev));
> + char *base_compat_type = NULL;
> + char *compat = NULL;
> + bool compat_set;
>
> if (strcmp(base_core_type, type)) {
> error_setg(&local_err, "CPU core type should be %s", base_core_type);
> goto out;
> }
>
> + base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
This can go in the initializer to match the base_core_type.
> + compat = object_property_get_str(OBJECT(dev), "compat", NULL);
> + compat_set = (compat && *compat) ? true : false;
You don't need the ?:, the condition is already a boolean.
> +
> + if (base_compat_type) {
> + if ((compat_set && strcmp(base_compat_type, compat)) ||
> + !compat_set) {
> + error_setg(&local_err, "CPU compat type should be %s",
> + base_compat_type);
> + goto out;
> + }
> + } else if (compat_set) {
> + error_setg(&local_err, "CPU compat type shouldn't be set");
> + goto out;
> + }
> +
> if (!smc->dr_cpu_enabled && dev->hotplugged) {
> error_setg(&local_err, "CPU hotplug not supported for this machine");
> goto out;
> @@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> }
>
> out:
> + g_free(compat);
> + g_free(base_compat_type);
> g_free(base_core_type);
> error_propagate(errp, local_err);
> }
> @@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> Error *local_err = NULL;
> Object *obj;
> int i;
> + char *compat = object_property_get_str(OBJECT(sc), "compat", NULL);
> + bool compat_set = (compat && *compat) ? true : false;
Again, don't need ?: here.
>
> sc->threads = g_malloc0(size * cc->nr_threads);
> for (i = 0; i < cc->nr_threads; i++) {
> @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> snprintf(id, sizeof(id), "thread[%d]", i);
> object_property_add_child(OBJECT(sc), id, obj, &local_err);
> if (local_err) {
> + g_free(compat);
> goto err;
> }
> + if (compat_set) {
> + CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> + char *featurestr = g_strdup_printf("compat=%s", compat);
> +
> + cc->parse_features(CPU(obj), featurestr, &local_err);
Hmm.. would it make more sense to just do an object_property_set()
rather than calling into parse_features?
> + g_free(featurestr);
> + }
> }
> +
> + g_free(compat);
> object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> if (local_err) {
> goto err;
> @@ -318,6 +369,27 @@ err:
> error_propagate(errp, local_err);
> }
>
> +static char *spapr_cpu_core_prop_get_compat(Object *obj, Error **errp)
> +{
> + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> + return g_strdup(core->cpu_compat);
> +}
> +
> +static void spapr_cpu_core_prop_set_compat(Object *obj, const char *val,
> + Error **errp)
> +{
> + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> + core->cpu_compat = g_strdup(val);
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> + object_property_add_str(obj, "compat", spapr_cpu_core_prop_get_compat,
> + spapr_cpu_core_prop_set_compat, NULL);
> +}
> +
> static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -388,6 +460,7 @@ static const TypeInfo spapr_cpu_core_type_info = {
> .parent = TYPE_CPU_CORE,
> .abstract = true,
> .instance_size = sizeof(sPAPRCPUCore),
> + .instance_init = spapr_cpu_core_instance_init,
> .class_init = spapr_cpu_core_class_init,
> };
>
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1c9b319..e697dab 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -24,11 +24,13 @@ typedef struct sPAPRCPUCore {
> /*< public >*/
> void *threads;
> ObjectClass *cpu_class;
> + char *cpu_compat;
> } sPAPRCPUCore;
>
> void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp);
> char *spapr_get_cpu_core_type(const char *model);
> +char *spapr_get_cpu_compat_type(const char *model);
> void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp);
> void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-18 8:34 [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores Bharata B Rao
2016-06-21 5:10 ` David Gibson
@ 2016-06-21 6:04 ` Thomas Huth
2016-06-22 2:31 ` Bharata B Rao
2016-06-21 7:09 ` Igor Mammedov
2 siblings, 1 reply; 14+ messages in thread
From: Thomas Huth @ 2016-06-21 6:04 UTC (permalink / raw)
To: Bharata B Rao, qemu-devel; +Cc: qemu-ppc, david, imammedo
On 18.06.2016 10:34, Bharata B Rao wrote:
> Compat CPU type is typically specified on -cpu cmdline option like:
> -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> With the introduction of sPAPR CPU core devices, we need to support
> the same for core devices too.
>
> Support the specification of CPU compat type on device_add command for
> sPAPRCPUCore devices like:
> (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> Applies on ppc-for-2.7 branch of David Gibson's tree.
>
> hw/ppc/spapr.c | 8 +++++
> hw/ppc/spapr_cpu_core.c | 73 +++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_cpu_core.h | 2 ++
> 3 files changed, 83 insertions(+)
[...]
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3a5da09..9eb63cc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> return core_type;
> }
>
> +/*
> + * Returns the CPU compat type specified in -cpu @model.
> + */
> +char *spapr_get_cpu_compat_type(const char *model)
> +{
> + char *compat_type = NULL;
> + gchar **model_pieces = g_strsplit(model, ",", 2);
> +
> + if (model_pieces[1]) {
> + gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> +
> + compat_type = g_strdup_printf("%s", compat_pieces[1]);
> + }
> +
> + g_strfreev(model_pieces);
> + return compat_type;
> +}
Shouldn't you check for "compat=" somewhere in this function (and
properly iterate over all parts of the comma separated string)? In case
we support more properties than "compat=" some day, this will go wrong
otherwise...
Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-18 8:34 [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores Bharata B Rao
2016-06-21 5:10 ` David Gibson
2016-06-21 6:04 ` Thomas Huth
@ 2016-06-21 7:09 ` Igor Mammedov
2016-06-21 7:42 ` Igor Mammedov
2016-06-22 2:30 ` Bharata B Rao
2 siblings, 2 replies; 14+ messages in thread
From: Igor Mammedov @ 2016-06-21 7:09 UTC (permalink / raw)
To: Bharata B Rao; +Cc: qemu-devel, qemu-ppc, david, thuth, Eduardo Habkost
On Sat, 18 Jun 2016 14:04:06 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> Compat CPU type is typically specified on -cpu cmdline option like:
> -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> With the introduction of sPAPR CPU core devices, we need to support
> the same for core devices too.
>
> Support the specification of CPU compat type on device_add command for
> sPAPRCPUCore devices like:
> (qemu) device_add
> POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> Applies on ppc-for-2.7 branch of David Gibson's tree.
>
> hw/ppc/spapr.c | 8 +++++
> hw/ppc/spapr_cpu_core.c | 73
> +++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 83
> insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 778fa25..2049d7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> *machine) if (i < spapr_cores) {
> char *type =
> spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> + char *compat;
>
> if (!object_class_by_name(type)) {
> error_report("Unable to find sPAPR CPU Core
> definition"); @@ -1818,6 +1819,13 @@ static void
> ppc_spapr_init(MachineState *machine) &error_fatal);
> object_property_set_int(core, core_dt_id,
> CPU_CORE_PROP_CORE_ID, &error_fatal);
> + compat =
> spapr_get_cpu_compat_type(machine->cpu_model);
> + if (compat) {
> + object_property_set_str(core, compat, "compat",
> + &error_fatal);
> + g_free(compat);
> + }
> +
> object_property_set_bool(core, true, "realized",
> &error_fatal); }
> }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3a5da09..9eb63cc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> return core_type;
> }
>
> +/*
> + * Returns the CPU compat type specified in -cpu @model.
> + */
> +char *spapr_get_cpu_compat_type(const char *model)
CPUClass already has such parser and hook to override it in case of
target does legacy parsing, see CPUClass->parse_features.
Maybe extending generic core in similar way and reusing generic parser
will do the job.
However we are in progress [1] of converting legacy
-cpu cpuname,feat1=x,feat2=y,...
into a set of global properties
-global cputype.feat1=x ...
it would be better if you wouldn't start using -cpu for features
but use directly -global mechanism,
for that you need only have a corresponding property in spapr_core type.
> +{
> + char *compat_type = NULL;
> + gchar **model_pieces = g_strsplit(model, ",", 2);
> +
> + if (model_pieces[1]) {
> + gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> +
> + compat_type = g_strdup_printf("%s", compat_pieces[1]);
> + }
> +
> + g_strfreev(model_pieces);
> + return compat_type;
> +}
> +
> static void spapr_core_release(DeviceState *dev, void *opaque)
> {
> sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev, CPUCore *cc = CPU_CORE(dev);
> char *base_core_type =
> spapr_get_cpu_core_type(machine->cpu_model); const char *type =
> object_get_typename(OBJECT(dev));
> + char *base_compat_type = NULL;
> + char *compat = NULL;
> + bool compat_set;
>
> if (strcmp(base_core_type, type)) {
> error_setg(&local_err, "CPU core type should be %s",
> base_core_type); goto out;
> }
>
> + base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
> + compat = object_property_get_str(OBJECT(dev), "compat", NULL);
> + compat_set = (compat && *compat) ? true : false;
> +
> + if (base_compat_type) {
> + if ((compat_set && strcmp(base_compat_type, compat)) ||
> + !compat_set) {
> + error_setg(&local_err, "CPU compat type should be %s",
> + base_compat_type);
> + goto out;
> + }
> + } else if (compat_set) {
> + error_setg(&local_err, "CPU compat type shouldn't be set");
> + goto out;
> + }
> +
> if (!smc->dr_cpu_enabled && dev->hotplugged) {
> error_setg(&local_err, "CPU hotplug not supported for this
> machine"); goto out;
> @@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev, }
>
> out:
> + g_free(compat);
> + g_free(base_compat_type);
> g_free(base_core_type);
> error_propagate(errp, local_err);
> }
> @@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState
> *dev, Error **errp) Error *local_err = NULL;
> Object *obj;
> int i;
> + char *compat = object_property_get_str(OBJECT(sc), "compat",
> NULL);
> + bool compat_set = (compat && *compat) ? true : false;
>
> sc->threads = g_malloc0(size * cc->nr_threads);
> for (i = 0; i < cc->nr_threads; i++) {
> @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState
> *dev, Error **errp) snprintf(id, sizeof(id), "thread[%d]", i);
> object_property_add_child(OBJECT(sc), id, obj, &local_err);
> if (local_err) {
> + g_free(compat);
> goto err;
> }
> + if (compat_set) {
> + CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> + char *featurestr = g_strdup_printf("compat=%s", compat);
> +
> + cc->parse_features(CPU(obj), featurestr, &local_err);
> + g_free(featurestr);
> + }
> }
> +
> + g_free(compat);
> object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child,
> &local_err); if (local_err) {
> goto err;
> @@ -318,6 +369,27 @@ err:
> error_propagate(errp, local_err);
> }
>
> +static char *spapr_cpu_core_prop_get_compat(Object *obj, Error
> **errp) +{
> + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> + return g_strdup(core->cpu_compat);
> +}
> +
> +static void spapr_cpu_core_prop_set_compat(Object *obj, const char
> *val,
> + Error **errp)
> +{
> + sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> + core->cpu_compat = g_strdup(val);
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> + object_property_add_str(obj, "compat",
> spapr_cpu_core_prop_get_compat,
> + spapr_cpu_core_prop_set_compat, NULL);
> +}
> +
> static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -388,6 +460,7 @@ static const TypeInfo spapr_cpu_core_type_info = {
> .parent = TYPE_CPU_CORE,
> .abstract = true,
> .instance_size = sizeof(sPAPRCPUCore),
> + .instance_init = spapr_cpu_core_instance_init,
> .class_init = spapr_cpu_core_class_init,
> };
>
> diff --git a/include/hw/ppc/spapr_cpu_core.h
> b/include/hw/ppc/spapr_cpu_core.h index 1c9b319..e697dab 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -24,11 +24,13 @@ typedef struct sPAPRCPUCore {
> /*< public >*/
> void *threads;
> ObjectClass *cpu_class;
> + char *cpu_compat;
> } sPAPRCPUCore;
>
> void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState
> *dev, Error **errp);
> char *spapr_get_cpu_core_type(const char *model);
> +char *spapr_get_cpu_compat_type(const char *model);
> void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp);
> void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-21 7:09 ` Igor Mammedov
@ 2016-06-21 7:42 ` Igor Mammedov
2016-06-22 2:30 ` Bharata B Rao
1 sibling, 0 replies; 14+ messages in thread
From: Igor Mammedov @ 2016-06-21 7:42 UTC (permalink / raw)
To: Bharata B Rao; +Cc: thuth, qemu-ppc, qemu-devel, Eduardo Habkost, david
On Tue, 21 Jun 2016 09:09:57 +0200
Igor Mammedov <imammedo@redhat.com> wrote:
[...]
>
> However we are in progress [1] of converting legacy
1) https://www.mail-archive.com/qemu-devel@nongnu.org/msg378123.html
> -cpu cpuname,feat1=x,feat2=y,...
> into a set of global properties
> -global cputype.feat1=x ...
>
> it would be better if you wouldn't start using -cpu for features
> but use directly -global mechanism,
> for that you need only have a corresponding property in spapr_core
> type.
[...]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-21 7:09 ` Igor Mammedov
2016-06-21 7:42 ` Igor Mammedov
@ 2016-06-22 2:30 ` Bharata B Rao
2016-06-22 7:09 ` Igor Mammedov
1 sibling, 1 reply; 14+ messages in thread
From: Bharata B Rao @ 2016-06-22 2:30 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, qemu-ppc, david, thuth, Eduardo Habkost
On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> On Sat, 18 Jun 2016 14:04:06 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
>
> > Compat CPU type is typically specified on -cpu cmdline option like:
> > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > With the introduction of sPAPR CPU core devices, we need to support
> > the same for core devices too.
> >
> > Support the specification of CPU compat type on device_add command for
> > sPAPRCPUCore devices like:
> > (qemu) device_add
> > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > Applies on ppc-for-2.7 branch of David Gibson's tree.
> >
> > hw/ppc/spapr.c | 8 +++++
> > hw/ppc/spapr_cpu_core.c | 73
> > +++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 83
> > insertions(+)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 778fa25..2049d7d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > *machine) if (i < spapr_cores) {
> > char *type =
> > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > + char *compat;
> >
> > if (!object_class_by_name(type)) {
> > error_report("Unable to find sPAPR CPU Core
> > definition"); @@ -1818,6 +1819,13 @@ static void
> > ppc_spapr_init(MachineState *machine) &error_fatal);
> > object_property_set_int(core, core_dt_id,
> > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > + compat =
> > spapr_get_cpu_compat_type(machine->cpu_model);
> > + if (compat) {
> > + object_property_set_str(core, compat, "compat",
> > + &error_fatal);
> > + g_free(compat);
> > + }
> > +
> > object_property_set_bool(core, true, "realized",
> > &error_fatal); }
> > }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a5da09..9eb63cc 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> > return core_type;
> > }
> >
> > +/*
> > + * Returns the CPU compat type specified in -cpu @model.
> > + */
> > +char *spapr_get_cpu_compat_type(const char *model)
> CPUClass already has such parser and hook to override it in case of
> target does legacy parsing, see CPUClass->parse_features.
> Maybe extending generic core in similar way and reusing generic parser
> will do the job.
In fact, I am already using CPUClass->parse_features() to set the
feature properties (which right now is only compat= for us) for core device.
What I need in the above routine spapr_get_cpu_compat_type() is to
extract "compat=", so that I can verify that the compat type specified
with -device matches with what was specified with -cpu.
>
> However we are in progress [1] of converting legacy
> -cpu cpuname,feat1=x,feat2=y,...
> into a set of global properties
> -global cputype.feat1=x ...
>
> it would be better if you wouldn't start using -cpu for features
> but use directly -global mechanism,
> for that you need only have a corresponding property in spapr_core type.
Will this work be part of 2.7 ? If not, we will have to use -cpu to extract
the compat type since powerpc core hotplug is already upstream.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-21 6:04 ` Thomas Huth
@ 2016-06-22 2:31 ` Bharata B Rao
0 siblings, 0 replies; 14+ messages in thread
From: Bharata B Rao @ 2016-06-22 2:31 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, qemu-ppc, david, imammedo
On Tue, Jun 21, 2016 at 08:04:12AM +0200, Thomas Huth wrote:
> On 18.06.2016 10:34, Bharata B Rao wrote:
> > Compat CPU type is typically specified on -cpu cmdline option like:
> > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > With the introduction of sPAPR CPU core devices, we need to support
> > the same for core devices too.
> >
> > Support the specification of CPU compat type on device_add command for
> > sPAPRCPUCore devices like:
> > (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > Applies on ppc-for-2.7 branch of David Gibson's tree.
> >
> > hw/ppc/spapr.c | 8 +++++
> > hw/ppc/spapr_cpu_core.c | 73 +++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_cpu_core.h | 2 ++
> > 3 files changed, 83 insertions(+)
> [...]
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a5da09..9eb63cc 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> > return core_type;
> > }
> >
> > +/*
> > + * Returns the CPU compat type specified in -cpu @model.
> > + */
> > +char *spapr_get_cpu_compat_type(const char *model)
> > +{
> > + char *compat_type = NULL;
> > + gchar **model_pieces = g_strsplit(model, ",", 2);
> > +
> > + if (model_pieces[1]) {
> > + gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> > +
> > + compat_type = g_strdup_printf("%s", compat_pieces[1]);
> > + }
> > +
> > + g_strfreev(model_pieces);
> > + return compat_type;
> > +}
>
> Shouldn't you check for "compat=" somewhere in this function (and
> properly iterate over all parts of the comma separated string)? In case
> we support more properties than "compat=" some day, this will go wrong
> otherwise...
Currently since it is only "compat=", anything else would fail gracefully
when the property gets set. But obviously that is not future-safe as you
note. Let me rework on this.
Regards,
Bharata
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-21 5:10 ` David Gibson
@ 2016-06-22 2:36 ` Bharata B Rao
2016-06-22 3:03 ` David Gibson
0 siblings, 1 reply; 14+ messages in thread
From: Bharata B Rao @ 2016-06-22 2:36 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-devel, qemu-ppc, imammedo, thuth
On Tue, Jun 21, 2016 at 03:10:00PM +1000, David Gibson wrote:
> On Sat, Jun 18, 2016 at 02:04:06PM +0530, Bharata B Rao wrote:
> > Compat CPU type is typically specified on -cpu cmdline option like:
> > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > With the introduction of sPAPR CPU core devices, we need to support
> > the same for core devices too.
> >
> > Support the specification of CPU compat type on device_add command for
> > sPAPRCPUCore devices like:
> > (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > Applies on ppc-for-2.7 branch of David Gibson's tree.
>
> The implementation looks ok apart from a few nits noted below.
>
> There's a larger problem here, though, in that this doesn't advertise
> the necessary compat= property via query-hotpluggable-cpus qmp and hmp
> interfaces. Which means that management has no good way of knowing
> it's necessary.
>
> >
> > hw/ppc/spapr.c | 8 +++++
> > hw/ppc/spapr_cpu_core.c | 73 +++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_cpu_core.h | 2 ++
> > 3 files changed, 83 insertions(+)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 778fa25..2049d7d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > if (i < spapr_cores) {
> > char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > Object *core;
> > + char *compat;
> >
> > if (!object_class_by_name(type)) {
> > error_report("Unable to find sPAPR CPU Core definition");
> > @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine)
> > &error_fatal);
> > object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> > &error_fatal);
> > + compat = spapr_get_cpu_compat_type(machine->cpu_model);
> > + if (compat) {
> > + object_property_set_str(core, compat, "compat",
> > + &error_fatal);
> > + g_free(compat);
> > + }
> > +
> > object_property_set_bool(core, true, "realized", &error_fatal);
> > }
> > }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a5da09..9eb63cc 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> > return core_type;
> > }
> >
> > +/*
> > + * Returns the CPU compat type specified in -cpu @model.
> > + */
> > +char *spapr_get_cpu_compat_type(const char *model)
> > +{
> > + char *compat_type = NULL;
> > + gchar **model_pieces = g_strsplit(model, ",", 2);
> > +
> > + if (model_pieces[1]) {
> > + gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> > +
> > + compat_type = g_strdup_printf("%s", compat_pieces[1]);
> > + }
> > +
> > + g_strfreev(model_pieces);
> > + return compat_type;
> > +}
> > +
> > static void spapr_core_release(DeviceState *dev, void *opaque)
> > {
> > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > CPUCore *cc = CPU_CORE(dev);
> > char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> > const char *type = object_get_typename(OBJECT(dev));
> > + char *base_compat_type = NULL;
> > + char *compat = NULL;
> > + bool compat_set;
> >
> > if (strcmp(base_core_type, type)) {
> > error_setg(&local_err, "CPU core type should be %s", base_core_type);
> > goto out;
> > }
> >
> > + base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
>
> This can go in the initializer to match the base_core_type.
Had it that way, but since there was an error exit possibility when
base_core_type is not matching, I thought better to initialize base_compat_type
after that check.
>
> > + compat = object_property_get_str(OBJECT(dev), "compat", NULL);
> > + compat_set = (compat && *compat) ? true : false;
>
> You don't need the ?:, the condition is already a boolean.
Yeah.
>
> > +
> > + if (base_compat_type) {
> > + if ((compat_set && strcmp(base_compat_type, compat)) ||
> > + !compat_set) {
> > + error_setg(&local_err, "CPU compat type should be %s",
> > + base_compat_type);
> > + goto out;
> > + }
> > + } else if (compat_set) {
> > + error_setg(&local_err, "CPU compat type shouldn't be set");
> > + goto out;
> > + }
> > +
> > if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > error_setg(&local_err, "CPU hotplug not supported for this machine");
> > goto out;
> > @@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > }
> >
> > out:
> > + g_free(compat);
> > + g_free(base_compat_type);
> > g_free(base_core_type);
> > error_propagate(errp, local_err);
> > }
> > @@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > Error *local_err = NULL;
> > Object *obj;
> > int i;
> > + char *compat = object_property_get_str(OBJECT(sc), "compat", NULL);
> > + bool compat_set = (compat && *compat) ? true : false;
>
> Again, don't need ?: here.
>
> >
> > sc->threads = g_malloc0(size * cc->nr_threads);
> > for (i = 0; i < cc->nr_threads; i++) {
> > @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > snprintf(id, sizeof(id), "thread[%d]", i);
> > object_property_add_child(OBJECT(sc), id, obj, &local_err);
> > if (local_err) {
> > + g_free(compat);
> > goto err;
> > }
> > + if (compat_set) {
> > + CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> > + char *featurestr = g_strdup_printf("compat=%s", compat);
> > +
> > + cc->parse_features(CPU(obj), featurestr, &local_err);
>
> Hmm.. would it make more sense to just do an object_property_set()
> rather than calling into parse_features?
It would work, but I guess better to use ->parse_features() to ensure
future additional properties would work seamlessly.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-22 2:36 ` Bharata B Rao
@ 2016-06-22 3:03 ` David Gibson
0 siblings, 0 replies; 14+ messages in thread
From: David Gibson @ 2016-06-22 3:03 UTC (permalink / raw)
To: Bharata B Rao; +Cc: qemu-devel, qemu-ppc, imammedo, thuth
[-- Attachment #1: Type: text/plain, Size: 6014 bytes --]
On Wed, Jun 22, 2016 at 08:06:50AM +0530, Bharata B Rao wrote:
> On Tue, Jun 21, 2016 at 03:10:00PM +1000, David Gibson wrote:
> > On Sat, Jun 18, 2016 at 02:04:06PM +0530, Bharata B Rao wrote:
> > > Compat CPU type is typically specified on -cpu cmdline option like:
> > > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > With the introduction of sPAPR CPU core devices, we need to support
> > > the same for core devices too.
> > >
> > > Support the specification of CPU compat type on device_add command for
> > > sPAPRCPUCore devices like:
> > > (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > >
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> >
> > The implementation looks ok apart from a few nits noted below.
> >
> > There's a larger problem here, though, in that this doesn't advertise
> > the necessary compat= property via query-hotpluggable-cpus qmp and hmp
> > interfaces. Which means that management has no good way of knowing
> > it's necessary.
> >
> > >
> > > hw/ppc/spapr.c | 8 +++++
> > > hw/ppc/spapr_cpu_core.c | 73 +++++++++++++++++++++++++++++++++++++++++
> > > include/hw/ppc/spapr_cpu_core.h | 2 ++
> > > 3 files changed, 83 insertions(+)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 778fa25..2049d7d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > > if (i < spapr_cores) {
> > > char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > > Object *core;
> > > + char *compat;
> > >
> > > if (!object_class_by_name(type)) {
> > > error_report("Unable to find sPAPR CPU Core definition");
> > > @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine)
> > > &error_fatal);
> > > object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> > > &error_fatal);
> > > + compat = spapr_get_cpu_compat_type(machine->cpu_model);
> > > + if (compat) {
> > > + object_property_set_str(core, compat, "compat",
> > > + &error_fatal);
> > > + g_free(compat);
> > > + }
> > > +
> > > object_property_set_bool(core, true, "realized", &error_fatal);
> > > }
> > > }
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 3a5da09..9eb63cc 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> > > return core_type;
> > > }
> > >
> > > +/*
> > > + * Returns the CPU compat type specified in -cpu @model.
> > > + */
> > > +char *spapr_get_cpu_compat_type(const char *model)
> > > +{
> > > + char *compat_type = NULL;
> > > + gchar **model_pieces = g_strsplit(model, ",", 2);
> > > +
> > > + if (model_pieces[1]) {
> > > + gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> > > +
> > > + compat_type = g_strdup_printf("%s", compat_pieces[1]);
> > > + }
> > > +
> > > + g_strfreev(model_pieces);
> > > + return compat_type;
> > > +}
> > > +
> > > static void spapr_core_release(DeviceState *dev, void *opaque)
> > > {
> > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > > @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > CPUCore *cc = CPU_CORE(dev);
> > > char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> > > const char *type = object_get_typename(OBJECT(dev));
> > > + char *base_compat_type = NULL;
> > > + char *compat = NULL;
> > > + bool compat_set;
> > >
> > > if (strcmp(base_core_type, type)) {
> > > error_setg(&local_err, "CPU core type should be %s", base_core_type);
> > > goto out;
> > > }
> > >
> > > + base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
> >
> > This can go in the initializer to match the base_core_type.
>
> Had it that way, but since there was an error exit possibility when
> base_core_type is not matching, I thought better to initialize base_compat_type
> after that check.
Ah, good point. I forgot that get_cpu_compat_type() had an implicit
allocation that needs cleanup.
> > > @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > snprintf(id, sizeof(id), "thread[%d]", i);
> > > object_property_add_child(OBJECT(sc), id, obj, &local_err);
> > > if (local_err) {
> > > + g_free(compat);
> > > goto err;
> > > }
> > > + if (compat_set) {
> > > + CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> > > + char *featurestr = g_strdup_printf("compat=%s", compat);
> > > +
> > > + cc->parse_features(CPU(obj), featurestr, &local_err);
> >
> > Hmm.. would it make more sense to just do an object_property_set()
> > rather than calling into parse_features?
>
> It would work, but I guess better to use ->parse_features() to ensure
> future additional properties would work seamlessly.
Hmm.. except that the string you're forming to pass to parse_features
is always just "compat=%s". So you'd have to change that bit anyway,
so I don't see that using parse_features really buys you anything.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-22 2:30 ` Bharata B Rao
@ 2016-06-22 7:09 ` Igor Mammedov
2016-06-23 15:55 ` Eduardo Habkost
0 siblings, 1 reply; 14+ messages in thread
From: Igor Mammedov @ 2016-06-22 7:09 UTC (permalink / raw)
To: Bharata B Rao; +Cc: thuth, qemu-ppc, qemu-devel, Eduardo Habkost, david
On Wed, 22 Jun 2016 08:00:28 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > On Sat, 18 Jun 2016 14:04:06 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >
> > > Compat CPU type is typically specified on -cpu cmdline option
> > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > With the introduction of sPAPR CPU core devices, we need to
> > > support the same for core devices too.
> > >
> > > Support the specification of CPU compat type on device_add
> > > command for sPAPRCPUCore devices like:
> > > (qemu) device_add
> > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > >
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > >
> > > hw/ppc/spapr.c | 8 +++++
> > > hw/ppc/spapr_cpu_core.c | 73
> > > +++++++++++++++++++++++++++++++++++++++++
> > > include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 83
> > > insertions(+)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 778fa25..2049d7d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > *machine) if (i < spapr_cores) {
> > > char *type =
> > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > + char *compat;
> > >
> > > if (!object_class_by_name(type)) {
> > > error_report("Unable to find sPAPR CPU Core
> > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > object_property_set_int(core, core_dt_id,
> > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > + compat =
> > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > + if (compat) {
> > > + object_property_set_str(core, compat,
> > > "compat",
> > > + &error_fatal);
> > > + g_free(compat);
> > > + }
> > > +
> > > object_property_set_bool(core, true, "realized",
> > > &error_fatal); }
> > > }
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 3a5da09..9eb63cc 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > *model) return core_type;
> > > }
> > >
> > > +/*
> > > + * Returns the CPU compat type specified in -cpu @model.
> > > + */
> > > +char *spapr_get_cpu_compat_type(const char *model)
> > CPUClass already has such parser and hook to override it in case of
> > target does legacy parsing, see CPUClass->parse_features.
> > Maybe extending generic core in similar way and reusing generic
> > parser will do the job.
>
> In fact, I am already using CPUClass->parse_features() to set the
> feature properties (which right now is only compat= for us) for core
> device.
>
> What I need in the above routine spapr_get_cpu_compat_type() is to
> extract "compat=", so that I can verify that the compat type specified
> with -device matches with what was specified with -cpu.
>
> >
> > However we are in progress [1] of converting legacy
> > -cpu cpuname,feat1=x,feat2=y,...
> > into a set of global properties
> > -global cputype.feat1=x ...
> >
> > it would be better if you wouldn't start using -cpu for features
> > but use directly -global mechanism,
> > for that you need only have a corresponding property in spapr_core
> > type.
>
> Will this work be part of 2.7 ? If not, we will have to use -cpu to
> extract the compat type since powerpc core hotplug is already
> upstream.
You can use -global without that work but so far it looks like it will
make into 2.7.
CCed Eduardo so he could say it for sure as it goes via his tree.
>
> Regards,
> Bharata.
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-22 7:09 ` Igor Mammedov
@ 2016-06-23 15:55 ` Eduardo Habkost
2016-06-24 1:40 ` Bharata B Rao
0 siblings, 1 reply; 14+ messages in thread
From: Eduardo Habkost @ 2016-06-23 15:55 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Bharata B Rao, thuth, qemu-ppc, qemu-devel, david
On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> On Wed, 22 Jun 2016 08:00:28 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
>
> > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >
> > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > With the introduction of sPAPR CPU core devices, we need to
> > > > support the same for core devices too.
> > > >
> > > > Support the specification of CPU compat type on device_add
> > > > command for sPAPRCPUCore devices like:
> > > > (qemu) device_add
> > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > >
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > >
> > > > hw/ppc/spapr.c | 8 +++++
> > > > hw/ppc/spapr_cpu_core.c | 73
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > > include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 83
> > > > insertions(+)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 778fa25..2049d7d 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > *machine) if (i < spapr_cores) {
> > > > char *type =
> > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > + char *compat;
> > > >
> > > > if (!object_class_by_name(type)) {
> > > > error_report("Unable to find sPAPR CPU Core
> > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > object_property_set_int(core, core_dt_id,
> > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > + compat =
> > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > + if (compat) {
> > > > + object_property_set_str(core, compat,
> > > > "compat",
> > > > + &error_fatal);
> > > > + g_free(compat);
> > > > + }
> > > > +
> > > > object_property_set_bool(core, true, "realized",
> > > > &error_fatal); }
> > > > }
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index 3a5da09..9eb63cc 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > *model) return core_type;
> > > > }
> > > >
> > > > +/*
> > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > + */
> > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > CPUClass already has such parser and hook to override it in case of
> > > target does legacy parsing, see CPUClass->parse_features.
> > > Maybe extending generic core in similar way and reusing generic
> > > parser will do the job.
> >
> > In fact, I am already using CPUClass->parse_features() to set the
> > feature properties (which right now is only compat= for us) for core
> > device.
> >
> > What I need in the above routine spapr_get_cpu_compat_type() is to
> > extract "compat=", so that I can verify that the compat type specified
> > with -device matches with what was specified with -cpu.
> >
> > >
> > > However we are in progress [1] of converting legacy
> > > -cpu cpuname,feat1=x,feat2=y,...
> > > into a set of global properties
> > > -global cputype.feat1=x ...
> > >
> > > it would be better if you wouldn't start using -cpu for features
> > > but use directly -global mechanism,
> > > for that you need only have a corresponding property in spapr_core
> > > type.
> >
> > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > extract the compat type since powerpc core hotplug is already
> > upstream.
> You can use -global without that work but so far it looks like it will
> make into 2.7.
I expect the -cpu => globals series to be included soon, unless
there are objections by others.
But as Igor said, globals are already supported. If you want to
set a property in the CPU core objects, you just need to use:
-global spapr-cpu-core.compat=XXX
The conflict here seems to be that you want the "-cpu" option to
affect the cpu-core objects, not just the VCPU/thread objects.
But we have been assuming that "-cpu" would only affect the
VCPU/thread objects.
What if you just call the cpu_common_parse_features() function
from Igor's series using "spapr-cpu-core" as the typename? This
way all the "-cpu" options would be simply translated to
spapr-cpu-core global properties.
Is "core" the only property in TYPE_POWERPC_CPU, currently?
--
Eduardo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-23 15:55 ` Eduardo Habkost
@ 2016-06-24 1:40 ` Bharata B Rao
2016-06-24 2:37 ` David Gibson
2016-06-24 13:36 ` Eduardo Habkost
0 siblings, 2 replies; 14+ messages in thread
From: Bharata B Rao @ 2016-06-24 1:40 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Igor Mammedov, thuth, qemu-ppc, qemu-devel, david
On Thu, Jun 23, 2016 at 12:55:36PM -0300, Eduardo Habkost wrote:
> On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> > On Wed, 22 Jun 2016 08:00:28 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >
> > > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > >
> > > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > > With the introduction of sPAPR CPU core devices, we need to
> > > > > support the same for core devices too.
> > > > >
> > > > > Support the specification of CPU compat type on device_add
> > > > > command for sPAPRCPUCore devices like:
> > > > > (qemu) device_add
> > > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > >
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > >
> > > > > hw/ppc/spapr.c | 8 +++++
> > > > > hw/ppc/spapr_cpu_core.c | 73
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 83
> > > > > insertions(+)
> > > > >
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 778fa25..2049d7d 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > > *machine) if (i < spapr_cores) {
> > > > > char *type =
> > > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > > + char *compat;
> > > > >
> > > > > if (!object_class_by_name(type)) {
> > > > > error_report("Unable to find sPAPR CPU Core
> > > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > > object_property_set_int(core, core_dt_id,
> > > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > > + compat =
> > > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > > + if (compat) {
> > > > > + object_property_set_str(core, compat,
> > > > > "compat",
> > > > > + &error_fatal);
> > > > > + g_free(compat);
> > > > > + }
> > > > > +
> > > > > object_property_set_bool(core, true, "realized",
> > > > > &error_fatal); }
> > > > > }
> > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > index 3a5da09..9eb63cc 100644
> > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > > *model) return core_type;
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > > + */
> > > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > > CPUClass already has such parser and hook to override it in case of
> > > > target does legacy parsing, see CPUClass->parse_features.
> > > > Maybe extending generic core in similar way and reusing generic
> > > > parser will do the job.
> > >
> > > In fact, I am already using CPUClass->parse_features() to set the
> > > feature properties (which right now is only compat= for us) for core
> > > device.
> > >
> > > What I need in the above routine spapr_get_cpu_compat_type() is to
> > > extract "compat=", so that I can verify that the compat type specified
> > > with -device matches with what was specified with -cpu.
> > >
> > > >
> > > > However we are in progress [1] of converting legacy
> > > > -cpu cpuname,feat1=x,feat2=y,...
> > > > into a set of global properties
> > > > -global cputype.feat1=x ...
> > > >
> > > > it would be better if you wouldn't start using -cpu for features
> > > > but use directly -global mechanism,
> > > > for that you need only have a corresponding property in spapr_core
> > > > type.
> > >
> > > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > > extract the compat type since powerpc core hotplug is already
> > > upstream.
> > You can use -global without that work but so far it looks like it will
> > make into 2.7.
>
> I expect the -cpu => globals series to be included soon, unless
> there are objections by others.
>
> But as Igor said, globals are already supported. If you want to
> set a property in the CPU core objects, you just need to use:
>
> -global spapr-cpu-core.compat=XXX
>
> The conflict here seems to be that you want the "-cpu" option to
> affect the cpu-core objects, not just the VCPU/thread objects.
> But we have been assuming that "-cpu" would only affect the
> VCPU/thread objects.
-cpu property affects the CPU thread objects in our case too. Since those
CPU thread objects are aggregated under a core object, I was attempting
to add compat= with core device and validate that with -cpu type,compat=
from cmdline.
What we really need is this:
If -global cputype.compat= or -cpu cpuname,compat= is set, we need to
apply the same to all the CPU thread objects of cpu core object. Hence
I am thinking we don't need a separate property for this in the core object.
Regards,
Bharata.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-24 1:40 ` Bharata B Rao
@ 2016-06-24 2:37 ` David Gibson
2016-06-24 13:36 ` Eduardo Habkost
1 sibling, 0 replies; 14+ messages in thread
From: David Gibson @ 2016-06-24 2:37 UTC (permalink / raw)
To: Bharata B Rao; +Cc: Eduardo Habkost, Igor Mammedov, thuth, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6467 bytes --]
On Fri, Jun 24, 2016 at 07:10:06AM +0530, Bharata B Rao wrote:
> On Thu, Jun 23, 2016 at 12:55:36PM -0300, Eduardo Habkost wrote:
> > On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> > > On Wed, 22 Jun 2016 08:00:28 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >
> > > > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >
> > > > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > > > With the introduction of sPAPR CPU core devices, we need to
> > > > > > support the same for core devices too.
> > > > > >
> > > > > > Support the specification of CPU compat type on device_add
> > > > > > command for sPAPRCPUCore devices like:
> > > > > > (qemu) device_add
> > > > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > > >
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > ---
> > > > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > > >
> > > > > > hw/ppc/spapr.c | 8 +++++
> > > > > > hw/ppc/spapr_cpu_core.c | 73
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 83
> > > > > > insertions(+)
> > > > > >
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 778fa25..2049d7d 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > > > *machine) if (i < spapr_cores) {
> > > > > > char *type =
> > > > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > > > + char *compat;
> > > > > >
> > > > > > if (!object_class_by_name(type)) {
> > > > > > error_report("Unable to find sPAPR CPU Core
> > > > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > > > object_property_set_int(core, core_dt_id,
> > > > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > > > + compat =
> > > > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > > > + if (compat) {
> > > > > > + object_property_set_str(core, compat,
> > > > > > "compat",
> > > > > > + &error_fatal);
> > > > > > + g_free(compat);
> > > > > > + }
> > > > > > +
> > > > > > object_property_set_bool(core, true, "realized",
> > > > > > &error_fatal); }
> > > > > > }
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index 3a5da09..9eb63cc 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > > > *model) return core_type;
> > > > > > }
> > > > > >
> > > > > > +/*
> > > > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > > > + */
> > > > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > > > CPUClass already has such parser and hook to override it in case of
> > > > > target does legacy parsing, see CPUClass->parse_features.
> > > > > Maybe extending generic core in similar way and reusing generic
> > > > > parser will do the job.
> > > >
> > > > In fact, I am already using CPUClass->parse_features() to set the
> > > > feature properties (which right now is only compat= for us) for core
> > > > device.
> > > >
> > > > What I need in the above routine spapr_get_cpu_compat_type() is to
> > > > extract "compat=", so that I can verify that the compat type specified
> > > > with -device matches with what was specified with -cpu.
> > > >
> > > > >
> > > > > However we are in progress [1] of converting legacy
> > > > > -cpu cpuname,feat1=x,feat2=y,...
> > > > > into a set of global properties
> > > > > -global cputype.feat1=x ...
> > > > >
> > > > > it would be better if you wouldn't start using -cpu for features
> > > > > but use directly -global mechanism,
> > > > > for that you need only have a corresponding property in spapr_core
> > > > > type.
> > > >
> > > > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > > > extract the compat type since powerpc core hotplug is already
> > > > upstream.
> > > You can use -global without that work but so far it looks like it will
> > > make into 2.7.
> >
> > I expect the -cpu => globals series to be included soon, unless
> > there are objections by others.
> >
> > But as Igor said, globals are already supported. If you want to
> > set a property in the CPU core objects, you just need to use:
> >
> > -global spapr-cpu-core.compat=XXX
> >
> > The conflict here seems to be that you want the "-cpu" option to
> > affect the cpu-core objects, not just the VCPU/thread objects.
> > But we have been assuming that "-cpu" would only affect the
> > VCPU/thread objects.
>
> -cpu property affects the CPU thread objects in our case too. Since those
> CPU thread objects are aggregated under a core object, I was attempting
> to add compat= with core device and validate that with -cpu type,compat=
> from cmdline.
>
> What we really need is this:
>
> If -global cputype.compat= or -cpu cpuname,compat= is set, we need to
> apply the same to all the CPU thread objects of cpu core object. Hence
> I am thinking we don't need a separate property for this in the core object.
Right, I was just coming to the same conclusion. Since we always need
to set the compat property uniformly, I don't think we actually need
to bounce this information via the core object. Instead, the user can
either use -global or -cpu to set it directly on all thread objects.
In the unlikely event we ever have a setup where we need different
cores in different compat modes we can add a property to the core
object then.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores
2016-06-24 1:40 ` Bharata B Rao
2016-06-24 2:37 ` David Gibson
@ 2016-06-24 13:36 ` Eduardo Habkost
1 sibling, 0 replies; 14+ messages in thread
From: Eduardo Habkost @ 2016-06-24 13:36 UTC (permalink / raw)
To: Bharata B Rao; +Cc: Igor Mammedov, thuth, qemu-ppc, qemu-devel, david
On Fri, Jun 24, 2016 at 07:10:06AM +0530, Bharata B Rao wrote:
> On Thu, Jun 23, 2016 at 12:55:36PM -0300, Eduardo Habkost wrote:
> > On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> > > On Wed, 22 Jun 2016 08:00:28 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >
> > > > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > >
> > > > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > > > With the introduction of sPAPR CPU core devices, we need to
> > > > > > support the same for core devices too.
> > > > > >
> > > > > > Support the specification of CPU compat type on device_add
> > > > > > command for sPAPRCPUCore devices like:
> > > > > > (qemu) device_add
> > > > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > > >
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > ---
> > > > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > > >
> > > > > > hw/ppc/spapr.c | 8 +++++
> > > > > > hw/ppc/spapr_cpu_core.c | 73
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 83
> > > > > > insertions(+)
> > > > > >
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 778fa25..2049d7d 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > > > *machine) if (i < spapr_cores) {
> > > > > > char *type =
> > > > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > > > + char *compat;
> > > > > >
> > > > > > if (!object_class_by_name(type)) {
> > > > > > error_report("Unable to find sPAPR CPU Core
> > > > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > > > object_property_set_int(core, core_dt_id,
> > > > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > > > + compat =
> > > > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > > > + if (compat) {
> > > > > > + object_property_set_str(core, compat,
> > > > > > "compat",
> > > > > > + &error_fatal);
> > > > > > + g_free(compat);
> > > > > > + }
> > > > > > +
> > > > > > object_property_set_bool(core, true, "realized",
> > > > > > &error_fatal); }
> > > > > > }
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index 3a5da09..9eb63cc 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > > > *model) return core_type;
> > > > > > }
> > > > > >
> > > > > > +/*
> > > > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > > > + */
> > > > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > > > CPUClass already has such parser and hook to override it in case of
> > > > > target does legacy parsing, see CPUClass->parse_features.
> > > > > Maybe extending generic core in similar way and reusing generic
> > > > > parser will do the job.
> > > >
> > > > In fact, I am already using CPUClass->parse_features() to set the
> > > > feature properties (which right now is only compat= for us) for core
> > > > device.
> > > >
> > > > What I need in the above routine spapr_get_cpu_compat_type() is to
> > > > extract "compat=", so that I can verify that the compat type specified
> > > > with -device matches with what was specified with -cpu.
> > > >
> > > > >
> > > > > However we are in progress [1] of converting legacy
> > > > > -cpu cpuname,feat1=x,feat2=y,...
> > > > > into a set of global properties
> > > > > -global cputype.feat1=x ...
> > > > >
> > > > > it would be better if you wouldn't start using -cpu for features
> > > > > but use directly -global mechanism,
> > > > > for that you need only have a corresponding property in spapr_core
> > > > > type.
> > > >
> > > > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > > > extract the compat type since powerpc core hotplug is already
> > > > upstream.
> > > You can use -global without that work but so far it looks like it will
> > > make into 2.7.
> >
> > I expect the -cpu => globals series to be included soon, unless
> > there are objections by others.
> >
> > But as Igor said, globals are already supported. If you want to
> > set a property in the CPU core objects, you just need to use:
> >
> > -global spapr-cpu-core.compat=XXX
> >
> > The conflict here seems to be that you want the "-cpu" option to
> > affect the cpu-core objects, not just the VCPU/thread objects.
> > But we have been assuming that "-cpu" would only affect the
> > VCPU/thread objects.
>
> -cpu property affects the CPU thread objects in our case too. Since those
> CPU thread objects are aggregated under a core object, I was attempting
> to add compat= with core device and validate that with -cpu type,compat=
> from cmdline.
>
> What we really need is this:
>
> If -global cputype.compat= or -cpu cpuname,compat= is set, we need to
> apply the same to all the CPU thread objects of cpu core object. Hence
> I am thinking we don't need a separate property for this in the core object.
I assumed you really needed a spapr-cpu-core property for some
other reason. If all you need is to affect the VCPU thread
objects, then you only need cpu_common_parse_features() (the
default implementation of CPUClass::parse_features()).
Note that Igor's series is changing the
CPUClass::parse_features() interface. The current version needs
to be called once for each CPU, the version by Igor will call it
only once, before CPU creation. I suggest using his series as
base, so you don't need to redo your work later.
If everything goes well, I plan to submit a pull request
including his series on Monday.
--
Eduardo
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-06-24 13:36 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-18 8:34 [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores Bharata B Rao
2016-06-21 5:10 ` David Gibson
2016-06-22 2:36 ` Bharata B Rao
2016-06-22 3:03 ` David Gibson
2016-06-21 6:04 ` Thomas Huth
2016-06-22 2:31 ` Bharata B Rao
2016-06-21 7:09 ` Igor Mammedov
2016-06-21 7:42 ` Igor Mammedov
2016-06-22 2:30 ` Bharata B Rao
2016-06-22 7:09 ` Igor Mammedov
2016-06-23 15:55 ` Eduardo Habkost
2016-06-24 1:40 ` Bharata B Rao
2016-06-24 2:37 ` David Gibson
2016-06-24 13:36 ` Eduardo Habkost
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.