[Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Peter Maydell]

This patchset adds a new function cpu_generic_new()
which is similar to cpu_generic_init() except that it
does not realize the created CPU object. This means that
board code can do a "new cpu; set QOM properties; realize"
sequence without having to do all the work of splitting
the CPU model string and calling parse_features by hand.

Patch 2 clarifies a TODO comment, hopefully correctly,
based on an email conversation I had with Eduardo a
little while back.

Patches 3 and 4 change the ARM boards which currently
call parse_features by hand to use the new function.


If there's consensus that this is the right general
direction to go in, then I think that some other
architectures could also make cleanups to use this:
 * cpu_s390x_create() is almost exactly this function,
   give or take some fine detail of error handling
 * ppc_cpu_parse_features is almost the same thing,
   except that it doesn't actually create the CPU object,
   it only calls parse_features
 * hw/i386/pc.c does a manual parse_features

I'm not strongly attached to this particular approach
(though it seems like a reasonable one, especially given
the proliferation of different arch-specific helpers
listed above and the bugs in boards which don't call
parse_features when they should), but I would like us to
figure out and document what the right way for a board
to create and configure its CPU objects is...


Michael Davidsaver (1):
  cpu: add cpu_generic_new()

Peter Maydell (3):
  cpu: Clarify TODO comment in cpu_generic_new()
  hw/arm/integrator: Use new cpu_generic_new()
  hw/arm/virt: Use new cpu_generic_new()

 include/qom/cpu.h     | 17 +++++++++++++++++
 hw/arm/integratorcp.c | 22 ++--------------------
 hw/arm/virt.c         | 24 ++----------------------
 qom/cpu.c             | 37 ++++++++++++++++++++++++++-----------
 4 files changed, 47 insertions(+), 53 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] cpu: add cpu_generic_new()

[Peter Maydell]

From: Michael Davidsaver <mdavidsaver@gmail.com>

Add a new API cpu_generic_new() which creates a QOM CPU object
(including calling the CPU class parse_features method) but
does not realize it, and reimplement cpu_generic_init() to
simply call cpu_generic_new() and then immediately realize.
the CPU.

The motivation for this is that there is currently no
way for board code to take advantage of the cpu_generic_init()
convenience function if it needs to set QOM properties
on the created CPU. Instead it has to do it all manually, which
is prone to bugs like that fixed in commit 00909b585861 (where
a board code forgot to call parse_features which meant that
command line +feature,-feature flags were ignored).

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
[PMM: renamed new function to cpu_generic_new(), rewrote
 commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qom/cpu.h | 17 +++++++++++++++++
 qom/cpu.c         | 27 ++++++++++++++++++---------
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 45bcf21..e900586 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -599,11 +599,28 @@ void cpu_reset(CPUState *cpu);
 ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model);
 
 /**
+ * cpu_generic_new:
+ * @typename: The CPU base type.
+ * @cpu_model: The model string including optional parameters.
+ *
+ * Instantiates a CPU, processes optional parameters but does not realize it.
+ * This is the recommended way to create a CPU object which needs to be
+ * configured by then setting QOM properties on it. The configured CPU can
+ * then be realized in the usual way by calling
+ *    object_property_set_bool(cpuobj, true, "realized", &err);
+ *
+ * Returns: A #CPUState or %NULL if an error occurred.
+ */
+CPUState *cpu_generic_new(const char *typename, const char *cpu_model);
+
+/**
  * cpu_generic_init:
  * @typename: The CPU base type.
  * @cpu_model: The model string including optional parameters.
  *
  * Instantiates a CPU, processes optional parameters and realizes the CPU.
+ * This is equivalent to calling cpu_generic_new() and then immediately
+ * realizing the CPU object.
  *
  * Returns: A #CPUState or %NULL if an error occurred.
  */
diff --git a/qom/cpu.c b/qom/cpu.c
index 0e19b1a..a783aec 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -47,6 +47,22 @@ bool cpu_exists(int64_t id)
 
 CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
 {
+    CPUState *cpu = cpu_generic_new(typename, cpu_model);
+    if (cpu) {
+        Error *err = NULL;
+        object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+
+        if (err != NULL) {
+            error_report_err(err);
+            object_unref(OBJECT(cpu));
+            return NULL;
+        }
+    }
+    return cpu;
+}
+
+CPUState *cpu_generic_new(const char *typename, const char *cpu_model)
+{
     char *str, *name, *featurestr;
     CPUState *cpu = NULL;
     ObjectClass *oc;
@@ -70,19 +86,12 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
     cc->parse_features(object_class_get_name(oc), featurestr, &err);
     g_free(str);
     if (err != NULL) {
-        goto out;
-    }
-
-    cpu = CPU(object_new(object_class_get_name(oc)));
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
-
-out:
-    if (err != NULL) {
         error_report_err(err);
-        object_unref(OBJECT(cpu));
         return NULL;
     }
 
+    cpu = CPU(object_new(object_class_get_name(oc)));
+
     return cpu;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/4] cpu: add cpu_generic_new()

[Eduardo Habkost]

On Mon, Feb 13, 2017 at 02:28:16PM +0000, Peter Maydell wrote:
> From: Michael Davidsaver <mdavidsaver@gmail.com>
> 
> Add a new API cpu_generic_new() which creates a QOM CPU object
> (including calling the CPU class parse_features method) but
> does not realize it, and reimplement cpu_generic_init() to
> simply call cpu_generic_new() and then immediately realize.
> the CPU.
> 
> The motivation for this is that there is currently no
> way for board code to take advantage of the cpu_generic_init()
> convenience function if it needs to set QOM properties
> on the created CPU. Instead it has to do it all manually, which
> is prone to bugs like that fixed in commit 00909b585861 (where
> a board code forgot to call parse_features which meant that
> command line +feature,-feature flags were ignored).
> 
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> [PMM: renamed new function to cpu_generic_new(), rewrote
>  commit message]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/qom/cpu.h | 17 +++++++++++++++++
>  qom/cpu.c         | 27 ++++++++++++++++++---------
>  2 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 45bcf21..e900586 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -599,11 +599,28 @@ void cpu_reset(CPUState *cpu);
>  ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model);
>  
>  /**
> + * cpu_generic_new:
> + * @typename: The CPU base type.
> + * @cpu_model: The model string including optional parameters.
> + *
> + * Instantiates a CPU, processes optional parameters but does not realize it.
> + * This is the recommended way to create a CPU object which needs to be
> + * configured by then setting QOM properties on it. The configured CPU can
> + * then be realized in the usual way by calling
> + *    object_property_set_bool(cpuobj, true, "realized", &err);
> + *
> + * Returns: A #CPUState or %NULL if an error occurred.
> + */
> +CPUState *cpu_generic_new(const char *typename, const char *cpu_model);

I am not sure we will stlil add this, but in case we are going to
add this helper temporarily until we have Igor's generic
cpu_model parsing, it would be useful to make the function
signature be:
  CPUState *cpu_generic_new(const char *typename, MachineState *machine)
and parse machine->cpu_model inside the function implementation.

This way we could replace the helper with
  object_new(machine->cpu_typename)
more easily once we add Igor's cleanup.

-- 
Eduardo

[Qemu-devel] [PATCH 2/4] cpu: Clarify TODO comment in cpu_generic_new()

[Peter Maydell]

The TODO comment in cpu_generic_new() suggests that we want to
move to having all callers do the parse_features work by hand.
In fact the intention is that we would prefer to have this
happen automatically via machine core work or similar common
code changes. In the meantime boards should use the
cpu_generic_new() wrapper rather than calling parse_features
themselves, because this means we only need to change one
place in future if we change how parse_features gets called.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Change based on email conversation with Eduardo...hopefully
I have understood the intention here correctly.
---
 qom/cpu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/qom/cpu.c b/qom/cpu.c
index a783aec..eacce5e 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -80,8 +80,14 @@ CPUState *cpu_generic_new(const char *typename, const char *cpu_model)
 
     cc = CPU_CLASS(oc);
     featurestr = strtok(NULL, ",");
-    /* TODO: all callers of cpu_generic_init() need to be converted to
-     * call parse_features() only once, before calling cpu_generic_init().
+    /* TODO: we should really arrange to have parse_features() called
+     * only once, since it needs only to be called once per CPU class
+     * rather than once per instance of that class. Perhaps this would
+     * be done by changes to how machine core code works or by doing
+     * something in main(). In the meantime, board code should prefer
+     * to use this function rather than calling parse_features()
+     * manually, so that refactoring how we handle this is easier in
+     * future.
      */
     cc->parse_features(object_class_get_name(oc), featurestr, &err);
     g_free(str);
-- 
2.7.4

[Qemu-devel] [PATCH 3/4] hw/arm/integrator: Use new cpu_generic_new()

[Peter Maydell]

Use the new cpu_generic_new() function rather than doing the
work of creating it and calling parse_features by hand.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/integratorcp.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 5610ffc..beffaf4 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -577,42 +577,24 @@ static void integratorcp_init(MachineState *machine)
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
-    char **cpustr;
-    ObjectClass *cpu_oc;
-    CPUClass *cc;
     Object *cpuobj;
     ARMCPU *cpu;
-    const char *typename;
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
     qemu_irq pic[32];
     DeviceState *dev, *sic, *icp;
     int i;
-    Error *err = NULL;
 
     if (!cpu_model) {
         cpu_model = "arm926";
     }
 
-    cpustr = g_strsplit(cpu_model, ",", 2);
-
-    cpu_oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
-    if (!cpu_oc) {
+    cpuobj = OBJECT(cpu_generic_new(TYPE_ARM_CPU, cpu_model));
+    if (!cpuobj) {
         fprintf(stderr, "Unable to find CPU definition\n");
         exit(1);
     }
-    typename = object_class_get_name(cpu_oc);
-
-    cc = CPU_CLASS(cpu_oc);
-    cc->parse_features(typename, cpustr[1], &err);
-    g_strfreev(cpustr);
-    if (err) {
-        error_report_err(err);
-        exit(1);
-    }
-
-    cpuobj = object_new(typename);
 
     /* By default ARM1176 CPUs have EL3 enabled.  This board does not
      * currently support EL3 so the CPU EL3 property is disabled before
-- 
2.7.4

[Qemu-devel] [PATCH 4/4] hw/arm/virt: Use new cpu_generic_new()

[Peter Maydell]

Use the new cpu_generic_new() rather than calling
parse_features by hand.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f3440f2..bcb9a6d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -172,7 +172,6 @@ static const char *valid_cpus[] = {
 static bool cpuname_valid(const char *cpu)
 {
     int i;
-
     for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
         if (strcmp(cpu, valid_cpus[i]) == 0) {
             return true;
@@ -1206,10 +1205,6 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     const char *cpu_model = machine->cpu_model;
     char **cpustr;
-    ObjectClass *oc;
-    const char *typename;
-    CPUClass *cc;
-    Error *err = NULL;
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
     uint8_t clustersz;
 
@@ -1240,6 +1235,7 @@ static void machvirt_init(MachineState *machine)
         error_report("mach-virt: CPU %s not supported", cpustr[0]);
         exit(1);
     }
+    g_strfreev(cpustr);
 
     /* If we have an EL3 boot ROM then the assumption is that it will
      * implement PSCI itself, so disable QEMU's internal implementation
@@ -1309,24 +1305,8 @@ static void machvirt_init(MachineState *machine)
 
     create_fdt(vms);
 
-    oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
-    if (!oc) {
-        error_report("Unable to find CPU definition");
-        exit(1);
-    }
-    typename = object_class_get_name(oc);
-
-    /* convert -smp CPU options specified by the user into global props */
-    cc = CPU_CLASS(oc);
-    cc->parse_features(typename, cpustr[1], &err);
-    g_strfreev(cpustr);
-    if (err) {
-        error_report_err(err);
-        exit(1);
-    }
-
     for (n = 0; n < smp_cpus; n++) {
-        Object *cpuobj = object_new(typename);
+        Object *cpuobj = OBJECT(cpu_generic_new(TYPE_ARM_CPU, cpu_model));
         if (!vmc->disallow_affinity_adjustment) {
             /* Adjust MPIDR like 64-bit KVM hosts, which incorporate the
              * GIC's target-list limitations. 32-bit KVM hosts currently
-- 
2.7.4

Re: [Qemu-devel] [PATCH 4/4] hw/arm/virt: Use new cpu_generic_new()

[Igor Mammedov]

On Mon, 13 Feb 2017 14:28:19 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> Use the new cpu_generic_new() rather than calling
> parse_features by hand.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/virt.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f3440f2..bcb9a6d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -172,7 +172,6 @@ static const char *valid_cpus[] = {
>  static bool cpuname_valid(const char *cpu)
>  {
>      int i;
> -
>      for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
>          if (strcmp(cpu, valid_cpus[i]) == 0) {
>              return true;
> @@ -1206,10 +1205,6 @@ static void machvirt_init(MachineState *machine)
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      const char *cpu_model = machine->cpu_model;
>      char **cpustr;
> -    ObjectClass *oc;
> -    const char *typename;
> -    CPUClass *cc;
> -    Error *err = NULL;
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>      uint8_t clustersz;
>  
> @@ -1240,6 +1235,7 @@ static void machvirt_init(MachineState *machine)
>          error_report("mach-virt: CPU %s not supported", cpustr[0]);
>          exit(1);
>      }
> +    g_strfreev(cpustr);
>  
>      /* If we have an EL3 boot ROM then the assumption is that it will
>       * implement PSCI itself, so disable QEMU's internal implementation
> @@ -1309,24 +1305,8 @@ static void machvirt_init(MachineState *machine)
>  
>      create_fdt(vms);
>  
> -    oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
> -    if (!oc) {
> -        error_report("Unable to find CPU definition");
> -        exit(1);
> -    }
> -    typename = object_class_get_name(oc);
> -
> -    /* convert -smp CPU options specified by the user into global props */
> -    cc = CPU_CLASS(oc);
> -    cc->parse_features(typename, cpustr[1], &err);
> -    g_strfreev(cpustr);
> -    if (err) {
> -        error_report_err(err);
> -        exit(1);
> -    }
> -
>      for (n = 0; n < smp_cpus; n++) {
> -        Object *cpuobj = object_new(typename);
> +        Object *cpuobj = OBJECT(cpu_generic_new(TYPE_ARM_CPU, cpu_model));
It unnecessarily pushes cpu_model parsing farther down the call stack
when all we need for CPU creation is type name.

Since I'm looking at adding '-device cpu' support to virt-arm,
I would prefer to leave object_new() here so that creation logic
would be similar to device_add and not mix '-cpu' parsing to typename
+ global properties that could be moved to generic machine code
with actual CPU creation.


>          if (!vmc->disallow_affinity_adjustment) {
>              /* Adjust MPIDR like 64-bit KVM hosts, which incorporate the
>               * GIC's target-list limitations. 32-bit KVM hosts currently

Re: [Qemu-devel] [PATCH 4/4] hw/arm/virt: Use new cpu_generic_new()

[Eduardo Habkost]

On Mon, Feb 20, 2017 at 08:10:29PM +0100, Igor Mammedov wrote:
> On Mon, 13 Feb 2017 14:28:19 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > Use the new cpu_generic_new() rather than calling
> > parse_features by hand.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/virt.c | 24 ++----------------------
> >  1 file changed, 2 insertions(+), 22 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index f3440f2..bcb9a6d 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -172,7 +172,6 @@ static const char *valid_cpus[] = {
> >  static bool cpuname_valid(const char *cpu)
> >  {
> >      int i;
> > -
> >      for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
> >          if (strcmp(cpu, valid_cpus[i]) == 0) {
> >              return true;
> > @@ -1206,10 +1205,6 @@ static void machvirt_init(MachineState *machine)
> >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> >      const char *cpu_model = machine->cpu_model;
> >      char **cpustr;
> > -    ObjectClass *oc;
> > -    const char *typename;
> > -    CPUClass *cc;
> > -    Error *err = NULL;
> >      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> >      uint8_t clustersz;
> >  
> > @@ -1240,6 +1235,7 @@ static void machvirt_init(MachineState *machine)
> >          error_report("mach-virt: CPU %s not supported", cpustr[0]);
> >          exit(1);
> >      }
> > +    g_strfreev(cpustr);
> >  
> >      /* If we have an EL3 boot ROM then the assumption is that it will
> >       * implement PSCI itself, so disable QEMU's internal implementation
> > @@ -1309,24 +1305,8 @@ static void machvirt_init(MachineState *machine)
> >  
> >      create_fdt(vms);
> >  
> > -    oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
> > -    if (!oc) {
> > -        error_report("Unable to find CPU definition");
> > -        exit(1);
> > -    }
> > -    typename = object_class_get_name(oc);
> > -
> > -    /* convert -smp CPU options specified by the user into global props */
> > -    cc = CPU_CLASS(oc);
> > -    cc->parse_features(typename, cpustr[1], &err);
> > -    g_strfreev(cpustr);
> > -    if (err) {
> > -        error_report_err(err);
> > -        exit(1);
> > -    }
> > -
> >      for (n = 0; n < smp_cpus; n++) {
> > -        Object *cpuobj = object_new(typename);
> > +        Object *cpuobj = OBJECT(cpu_generic_new(TYPE_ARM_CPU, cpu_model));
> It unnecessarily pushes cpu_model parsing farther down the call stack
> when all we need for CPU creation is type name.

I don't see a problem in pushing it down the call stack.  It is
just replacing logic that is duplicated on lots of boards to a
wrapper. When we refactor the parsing logic, it will be easier to
do it if parsing is inside a single wrapper insted of being
duplicated on multiple boards.

> 
> Since I'm looking at adding '-device cpu' support to virt-arm,
> I would prefer to leave object_new() here so that creation logic
> would be similar to device_add and not mix '-cpu' parsing to typename
> + global properties that could be moved to generic machine code
> with actual CPU creation.

We can still move parsing to generic machine code, with the new
wrapper. While we don't do that, we can have a helper that makes
the current API easier to use. Once we move parsing to generic
machine code, we can easily replace cpu_generic_new() with
object_new().

> 
> 
> >          if (!vmc->disallow_affinity_adjustment) {
> >              /* Adjust MPIDR like 64-bit KVM hosts, which incorporate the
> >               * GIC's target-list limitations. 32-bit KVM hosts currently
> 

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Peter Maydell]

On 13 February 2017 at 14:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset adds a new function cpu_generic_new()
> which is similar to cpu_generic_init() except that it
> does not realize the created CPU object. This means that
> board code can do a "new cpu; set QOM properties; realize"
> sequence without having to do all the work of splitting
> the CPU model string and calling parse_features by hand.
>
> Patch 2 clarifies a TODO comment, hopefully correctly,
> based on an email conversation I had with Eduardo a
> little while back.
>
> Patches 3 and 4 change the ARM boards which currently
> call parse_features by hand to use the new function.
>
>
> If there's consensus that this is the right general
> direction to go in, then I think that some other
> architectures could also make cleanups to use this:
>  * cpu_s390x_create() is almost exactly this function,
>    give or take some fine detail of error handling
>  * ppc_cpu_parse_features is almost the same thing,
>    except that it doesn't actually create the CPU object,
>    it only calls parse_features
>  * hw/i386/pc.c does a manual parse_features
>
> I'm not strongly attached to this particular approach
> (though it seems like a reasonable one, especially given
> the proliferation of different arch-specific helpers
> listed above and the bugs in boards which don't call
> parse_features when they should), but I would like us to
> figure out and document what the right way for a board
> to create and configure its CPU objects is...

I know it's only been a few days, but I just wanted to
say that I'd appreciate it if we could manage relatively
quick review on this one, since I have a set of patches
pending that depend on this and which it would be nice
to get into 2.9 (softfreeze approaching rapidly).

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Eduardo Habkost]

On Thu, Feb 16, 2017 at 04:40:43PM +0000, Peter Maydell wrote:
> On 13 February 2017 at 14:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> > This patchset adds a new function cpu_generic_new()
> > which is similar to cpu_generic_init() except that it
> > does not realize the created CPU object. This means that
> > board code can do a "new cpu; set QOM properties; realize"
> > sequence without having to do all the work of splitting
> > the CPU model string and calling parse_features by hand.
> >
> > Patch 2 clarifies a TODO comment, hopefully correctly,
> > based on an email conversation I had with Eduardo a
> > little while back.
> >
> > Patches 3 and 4 change the ARM boards which currently
> > call parse_features by hand to use the new function.
> >
> >
> > If there's consensus that this is the right general
> > direction to go in, then I think that some other
> > architectures could also make cleanups to use this:
> >  * cpu_s390x_create() is almost exactly this function,
> >    give or take some fine detail of error handling
> >  * ppc_cpu_parse_features is almost the same thing,
> >    except that it doesn't actually create the CPU object,
> >    it only calls parse_features
> >  * hw/i386/pc.c does a manual parse_features
> >
> > I'm not strongly attached to this particular approach
> > (though it seems like a reasonable one, especially given
> > the proliferation of different arch-specific helpers
> > listed above and the bugs in boards which don't call
> > parse_features when they should), but I would like us to
> > figure out and document what the right way for a board
> > to create and configure its CPU objects is...
> 
> I know it's only been a few days, but I just wanted to
> say that I'd appreciate it if we could manage relatively
> quick review on this one, since I have a set of patches
> pending that depend on this and which it would be nice
> to get into 2.9 (softfreeze approaching rapidly).

I was away from work for the past 3 weeks and I am catching up on
e-mail, right now. Is this proposal still up, or is this series
obsoleted by something else?

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Peter Maydell]

On 21 February 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Feb 16, 2017 at 04:40:43PM +0000, Peter Maydell wrote:
>> I know it's only been a few days, but I just wanted to
>> say that I'd appreciate it if we could manage relatively
>> quick review on this one, since I have a set of patches
>> pending that depend on this and which it would be nice
>> to get into 2.9 (softfreeze approaching rapidly).
>
> I was away from work for the past 3 weeks and I am catching up on
> e-mail, right now. Is this proposal still up, or is this series
> obsoleted by something else?

Given Igor's view that this isn't the way we want to go, I have
decoupled the other patches I mention above from this patchset
(making them do an in-line call to parse_features etc like
the virt.c code etc etc do at the moment). So there's no rush
and we can take the time to decide what the right approach is
(which we can then implement for QEMU 2.10, I expect).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Igor Mammedov]

On Mon, 13 Feb 2017 14:28:15 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> This patchset adds a new function cpu_generic_new()
> which is similar to cpu_generic_init() except that it
> does not realize the created CPU object. This means that
> board code can do a "new cpu; set QOM properties; realize"
> sequence without having to do all the work of splitting
> the CPU model string and calling parse_features by hand.
> 
> Patch 2 clarifies a TODO comment, hopefully correctly,
> based on an email conversation I had with Eduardo a
> little while back.
> 
> Patches 3 and 4 change the ARM boards which currently
> call parse_features by hand to use the new function.
> 
> 
> If there's consensus that this is the right general
> direction to go in, then I think that some other
> architectures could also make cleanups to use this:
>  * cpu_s390x_create() is almost exactly this function,
>    give or take some fine detail of error handling
>  * ppc_cpu_parse_features is almost the same thing,
>    except that it doesn't actually create the CPU object,
>    it only calls parse_features
>  * hw/i386/pc.c does a manual parse_features
> 
> I'm not strongly attached to this particular approach
> (though it seems like a reasonable one, especially given
> the proliferation of different arch-specific helpers
> listed above and the bugs in boards which don't call
> parse_features when they should), but I would like us to
> figure out and document what the right way for a board
> to create and configure its CPU objects is...

series looks like a step back adding yet another way
to create CPU and makes code even more inconsistent,
instead of removing TODO item by doing proper generalization.
So I'm sort of object to it.

I'll just posted RFC which show idea how generalization of
cpu_model/features parsing should be implemented.

However I don't have cycles to complete it, only
virt-arm/spapr/pc are converted as example.
One who would pick the task up should complete it for
all boards to make code consistent.

> 
> 
> Michael Davidsaver (1):
>   cpu: add cpu_generic_new()
> 
> Peter Maydell (3):
>   cpu: Clarify TODO comment in cpu_generic_new()
>   hw/arm/integrator: Use new cpu_generic_new()
>   hw/arm/virt: Use new cpu_generic_new()
> 
>  include/qom/cpu.h     | 17 +++++++++++++++++
>  hw/arm/integratorcp.c | 22 ++--------------------
>  hw/arm/virt.c         | 24 ++----------------------
>  qom/cpu.c             | 37 ++++++++++++++++++++++++++-----------
>  4 files changed, 47 insertions(+), 53 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Eduardo Habkost]

On Fri, Feb 17, 2017 at 08:05:22PM +0100, Igor Mammedov wrote:
> On Mon, 13 Feb 2017 14:28:15 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > This patchset adds a new function cpu_generic_new()
> > which is similar to cpu_generic_init() except that it
> > does not realize the created CPU object. This means that
> > board code can do a "new cpu; set QOM properties; realize"
> > sequence without having to do all the work of splitting
> > the CPU model string and calling parse_features by hand.
> > 
> > Patch 2 clarifies a TODO comment, hopefully correctly,
> > based on an email conversation I had with Eduardo a
> > little while back.
> > 
> > Patches 3 and 4 change the ARM boards which currently
> > call parse_features by hand to use the new function.
> > 
> > 
> > If there's consensus that this is the right general
> > direction to go in, then I think that some other
> > architectures could also make cleanups to use this:
> >  * cpu_s390x_create() is almost exactly this function,
> >    give or take some fine detail of error handling
> >  * ppc_cpu_parse_features is almost the same thing,
> >    except that it doesn't actually create the CPU object,
> >    it only calls parse_features
> >  * hw/i386/pc.c does a manual parse_features
> > 
> > I'm not strongly attached to this particular approach
> > (though it seems like a reasonable one, especially given
> > the proliferation of different arch-specific helpers
> > listed above and the bugs in boards which don't call
> > parse_features when they should), but I would like us to
> > figure out and document what the right way for a board
> > to create and configure its CPU objects is...
> 
> series looks like a step back adding yet another way
> to create CPU and makes code even more inconsistent,
> instead of removing TODO item by doing proper generalization.
> So I'm sort of object to it.
> 
> I'll just posted RFC which show idea how generalization of
> cpu_model/features parsing should be implemented.
> 
> However I don't have cycles to complete it, only
> virt-arm/spapr/pc are converted as example.
> One who would pick the task up should complete it for
> all boards to make code consistent.

If conversion of the code will take a while and this wrapper
removes code duplication of the current boards, I believe this
series is stlil a step in the right direction, even if the plan
is to replace the wrapper one day. But I will take a look at your
RFC first, before giving my opinion on which approach to take.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> This patchset adds a new function cpu_generic_new()
> which is similar to cpu_generic_init() except that it
> does not realize the created CPU object. This means that
> board code can do a "new cpu; set QOM properties; realize"
> sequence without having to do all the work of splitting
> the CPU model string and calling parse_features by hand.


Just going through my review queue and I see this needs re-basing. Is
there going to be another rev or was a different approach suggested?

>
> Patch 2 clarifies a TODO comment, hopefully correctly,
> based on an email conversation I had with Eduardo a
> little while back.
>
> Patches 3 and 4 change the ARM boards which currently
> call parse_features by hand to use the new function.
>
>
> If there's consensus that this is the right general
> direction to go in, then I think that some other
> architectures could also make cleanups to use this:
>  * cpu_s390x_create() is almost exactly this function,
>    give or take some fine detail of error handling
>  * ppc_cpu_parse_features is almost the same thing,
>    except that it doesn't actually create the CPU object,
>    it only calls parse_features
>  * hw/i386/pc.c does a manual parse_features
>
> I'm not strongly attached to this particular approach
> (though it seems like a reasonable one, especially given
> the proliferation of different arch-specific helpers
> listed above and the bugs in boards which don't call
> parse_features when they should), but I would like us to
> figure out and document what the right way for a board
> to create and configure its CPU objects is...
>
>
> Michael Davidsaver (1):
>   cpu: add cpu_generic_new()
>
> Peter Maydell (3):
>   cpu: Clarify TODO comment in cpu_generic_new()
>   hw/arm/integrator: Use new cpu_generic_new()
>   hw/arm/virt: Use new cpu_generic_new()
>
>  include/qom/cpu.h     | 17 +++++++++++++++++
>  hw/arm/integratorcp.c | 22 ++--------------------
>  hw/arm/virt.c         | 24 ++----------------------
>  qom/cpu.c             | 37 ++++++++++++++++++++++++++-----------
>  4 files changed, 47 insertions(+), 53 deletions(-)


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Eduardo Habkost]

On Mon, Jun 26, 2017 at 02:28:13PM +0100, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > This patchset adds a new function cpu_generic_new()
> > which is similar to cpu_generic_init() except that it
> > does not realize the created CPU object. This means that
> > board code can do a "new cpu; set QOM properties; realize"
> > sequence without having to do all the work of splitting
> > the CPU model string and calling parse_features by hand.
> 
> 
> Just going through my review queue and I see this needs re-basing. Is
> there going to be another rev or was a different approach suggested?

The right way to go is not clear.  We know we want to remove duplication
of CPU creation code, but probably we should first refactor the -cpu
parsing code, so parsing happens: 1) only once; 2) earlier in main(),
preferably before machine->init() runs; 3) inside generic code instead
of arch-specific code; 4) preferably using the QemuOpts parser instead
of the current strtok()-based custom parsers.

After the parsing code mess is sorted out, writing a generic CPU
creation wrapper will probably be easier (and safer).

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()

[Igor Mammedov]

On Mon, 26 Jun 2017 23:33:48 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 26, 2017 at 02:28:13PM +0100, Alex Bennée wrote:
> > 
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >   
> > > This patchset adds a new function cpu_generic_new()
> > > which is similar to cpu_generic_init() except that it
> > > does not realize the created CPU object. This means that
> > > board code can do a "new cpu; set QOM properties; realize"
> > > sequence without having to do all the work of splitting
> > > the CPU model string and calling parse_features by hand.  
> > 
> > 
> > Just going through my review queue and I see this needs re-basing. Is
> > there going to be another rev or was a different approach suggested?  
> 
> The right way to go is not clear.  We know we want to remove duplication
> of CPU creation code, but probably we should first refactor the -cpu
> parsing code, so parsing happens: 1) only once; 2) earlier in main(),
> preferably before machine->init() runs; 3) inside generic code instead
> of arch-specific code; 4) preferably using the QemuOpts parser instead
> of the current strtok()-based custom parsers.
> 
> After the parsing code mess is sorted out, writing a generic CPU
> creation wrapper will probably be easier (and safer).

Also there is legacy cpu features parsing/handling in sparc target,
so we might need to clean it up and convert to property based features
(as have been done for i386) before making generic cpu creation.