[4/4] hw/arm/virt: Use new cpu_generic_new()

Message ID 1486996099-15820-5-git-send-email-peter.maydell@linaro.org
State Rejected
Headers show
Series
  • cpu: Implement cpu_generic_new()
Related show

Commit Message

Peter Maydell Feb. 13, 2017, 2:28 p.m.
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(-)

-- 
2.7.4

Comments

Igor Mammedov Feb. 20, 2017, 7:10 p.m. | #1
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
Eduardo Habkost Feb. 21, 2017, 4:09 p.m. | #2
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

Patch hide | download patch | download mbox

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