From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43555) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFY2N-0003WR-Lk for qemu-devel@nongnu.org; Tue, 21 Jun 2016 22:37:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFY2J-0002X0-9h for qemu-devel@nongnu.org; Tue, 21 Jun 2016 22:37:06 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:38198 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFY2J-0002Wj-48 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 22:37:03 -0400 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u5M2YErv005397 for ; Tue, 21 Jun 2016 22:37:02 -0400 Received: from e23smtp08.au.ibm.com (e23smtp08.au.ibm.com [202.81.31.141]) by mx0b-001b2d01.pphosted.com with ESMTP id 23q6wcmjj7-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 21 Jun 2016 22:37:02 -0400 Received: from localhost by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 22 Jun 2016 12:36:59 +1000 Date: Wed, 22 Jun 2016 08:06:50 +0530 From: Bharata B Rao Reply-To: bharata@linux.vnet.ibm.com References: <1466238846-21365-1-git-send-email-bharata@linux.vnet.ibm.com> <20160621051000.GD14861@voom.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160621051000.GD14861@voom.fritz.box> Message-Id: <20160622023650.GD5613@in.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, imammedo@redhat.com, thuth@redhat.com 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 > > --- > > 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.