From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41275) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHPnJ-0007eD-PE for qemu-devel@nongnu.org; Mon, 27 Jun 2016 02:13:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHPnF-0006Cr-E5 for qemu-devel@nongnu.org; Mon, 27 Jun 2016 02:13:16 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:57300) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHPnF-0006BN-5e for qemu-devel@nongnu.org; Mon, 27 Jun 2016 02:13:13 -0400 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u5R5wpbE049379 for ; Mon, 27 Jun 2016 02:13:11 -0400 Received: from e28smtp09.in.ibm.com (e28smtp09.in.ibm.com [125.16.236.9]) by mx0a-001b2d01.pphosted.com with ESMTP id 23sjufum80-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 27 Jun 2016 02:13:11 -0400 Received: from localhost by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 27 Jun 2016 11:43:08 +0530 Date: Mon, 27 Jun 2016 11:42:57 +0530 From: Bharata B Rao Reply-To: bharata@linux.vnet.ibm.com References: <1466580229-4600-1-git-send-email-bharata@linux.vnet.ibm.com> <20160623060514.GD17152@voom.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160623060514.GD17152@voom.fritz.box> Message-Id: <20160627061257.GD27296@in.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v1 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 Thu, Jun 23, 2016 at 04:05:14PM +1000, David Gibson wrote: > On Wed, Jun 22, 2016 at 12:53:49PM +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 > > --- > > Changes in v1: > > - In the routine that extracts "compat=" from -cpu cmdline, made the parsing > > generic as suggested by Thomas Huth so that it works in the presence of > > any other additional features. > > - Addressed review comments by David with major one being setting of > > compat property directly instead of going via ->parse_features(). > > > > TODO: > > - Reconcile with Igor's work that make cpu features as global properties. > > - Find a way to export the compat infomation via query-hotpluggable-cpus. > > > > v0: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05279.html > > > > hw/ppc/spapr.c | 8 +++++ > > hw/ppc/spapr_cpu_core.c | 78 +++++++++++++++++++++++++++++++++++++++++ > > include/hw/ppc/spapr_cpu_core.h | 2 ++ > > 3 files changed, 88 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..d500cd6 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -96,6 +96,31 @@ 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 *model_str = g_strdup(model); > > + char *featurestr, *compat = NULL; > > + > > + featurestr = model_str ? strtok(model_str, ",") : NULL; > > AFAICT strtok() is not thread-safe. Followed cpu_common_parse_features(). > > > + while (featurestr) { > > + char *val; > > + if (!strncmp(featurestr, "compat=", 7)) { > > + val = strchr(featurestr, '='); > > You don't technically need the strchr(), since from the strncmp above, > you know the answer will be featurestr + 6. Ok fixed. > > > + val++; > > + compat = g_strdup(val); > > + goto out; > > + } > > + featurestr = strtok(NULL, ","); > > + } > > + > > +out: > > + g_free(model_str); > > + return compat; > > +} > > Couldn't we use one of the existing opts parsing functions in qemu for > the above, anyway? Using qemu_get_opt() is one option but that would need some work of setting the opts infrastructure for parsing -cpu option, but I assume with Igor's global CPU features work, that would be unwelcome. > > > static void spapr_core_release(DeviceState *dev, void *opaque) > > { > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > @@ -223,12 +248,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; > > + > > + 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; > > I don't think we want this else clause, because it will forbid the use > of -global core_type,compat=whatever, which Igor says is the preferred > approach for the future (IIUC, using -global is equivalent to setting > the property explicitly on every instance). > > The if clause should be ok, since it implements a fallback to using > -cpu, which we do want. In my new version, I am not touching ->pre_plug() at all since there is nothing to validate. All we do is to apply the compat property to all CPU threads if it is specified. Regards, Bharata.