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 <bhar...@linux.vnet.ibm.com>
> > ---
> > 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.
