diff mbox

[RFC,v1,1/1] spapr: Support setting of compat CPU type for CPU cores

Message ID 1466580229-4600-1-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bharata B Rao June 22, 2016, 7:23 a.m. UTC
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>
---
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(+)

Comments

David Gibson June 23, 2016, 6:05 a.m. UTC | #1
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 <bharata@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.

> +    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.

> +            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?

>  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.

> +    }
> +
>      if (!smc->dr_cpu_enabled && dev->hotplugged) {
>          error_setg(&local_err, "CPU hotplug not supported for this machine");
>          goto out;
> @@ -256,6 +300,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 +334,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;
>  
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> @@ -300,11 +348,18 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          if (local_err) {
>              goto err;
>          }
> +        if (compat_set) {
> +            object_property_set_str(obj, compat, "compat", &local_err);
> +            if (local_err) {
> +                goto err;
> +            }
> +        }
>      }
>      object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
>      if (local_err) {
>          goto err;
>      } else {
> +        g_free(compat);
>          return;
>      }
>  
> @@ -315,9 +370,31 @@ err:
>          i--;
>      }
>      g_free(sc->threads);
> +    g_free(compat);
>      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 +465,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,
Bharata B Rao June 27, 2016, 6:12 a.m. UTC | #2
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 <bharata@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.
diff mbox

Patch

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;
+    while (featurestr) {
+        char *val;
+        if (!strncmp(featurestr, "compat=", 7)) {
+            val = strchr(featurestr, '=');
+            val++;
+            compat = g_strdup(val);
+            goto out;
+        }
+        featurestr = strtok(NULL, ",");
+    }
+
+out:
+    g_free(model_str);
+    return compat;
+}
+
 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;
+    }
+
     if (!smc->dr_cpu_enabled && dev->hotplugged) {
         error_setg(&local_err, "CPU hotplug not supported for this machine");
         goto out;
@@ -256,6 +300,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 +334,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;
 
     sc->threads = g_malloc0(size * cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
@@ -300,11 +348,18 @@  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         if (local_err) {
             goto err;
         }
+        if (compat_set) {
+            object_property_set_str(obj, compat, "compat", &local_err);
+            if (local_err) {
+                goto err;
+            }
+        }
     }
     object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
     if (local_err) {
         goto err;
     } else {
+        g_free(compat);
         return;
     }
 
@@ -315,9 +370,31 @@  err:
         i--;
     }
     g_free(sc->threads);
+    g_free(compat);
     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 +465,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,