diff mbox series

[v16,13/23] target/rx: Fix cpu types and names

Message ID 20190531134315.4109-14-richard.henderson@linaro.org
State New
Headers show
Series Add RX architecture | expand

Commit Message

Richard Henderson May 31, 2019, 1:43 p.m. UTC 
There was confusion here about abstract classes and naming cpus.
We had registered a concrete class named "-rxcpu".  This was put
into the default cpu fields, and matched, so basic tests worked.
However, no value for -cpu could ever match in rx_cpu_class_by_name.

Rename the base class to "rx-cpu" and make it abstract.  This
matches what we do for most other targets.  Create a new concrete
cpu with the name "rx62n-rx-cpu".

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/rx/cpu.h | 12 ++++++------
 hw/rx/rx-virt.c |  2 +-
 hw/rx/rx62n.c   |  2 +-
 target/rx/cpu.c | 43 ++++++++++++++++++++++++++-----------------
 4 files changed, 34 insertions(+), 25 deletions(-)

-- 
2.17.1

Comments

Igor Mammedov May 31, 2019, 2:23 p.m. UTC | #1 
On Fri, 31 May 2019 08:43:05 -0500
Richard Henderson <richard.henderson@linaro.org> wrote:

> There was confusion here about abstract classes and naming cpus.

> We had registered a concrete class named "-rxcpu".  This was put

> into the default cpu fields, and matched, so basic tests worked.

> However, no value for -cpu could ever match in rx_cpu_class_by_name.

> 

> Rename the base class to "rx-cpu" and make it abstract.  This

> matches what we do for most other targets.  Create a new concrete

> cpu with the name "rx62n-rx-cpu".


since it hasn't been merged yet, it would be better to squash this
fixup into 3/23

[...]

> diff --git a/target/rx/cpu.c b/target/rx/cpu.c

> index 3268077d08..41fe1de4bb 100644

> --- a/target/rx/cpu.c

> +++ b/target/rx/cpu.c

> @@ -74,13 +74,14 @@ static void rx_cpu_list_entry(gpointer data, gpointer user_data)

>      const char *typename = object_class_get_name(OBJECT_CLASS(data));

>      int len = strlen(typename) - strlen(RX_CPU_TYPE_SUFFIX);

>  

> -    qemu_printf("%.*s\n", len, typename);

> +    qemu_printf("  %.*s\n", len, typename);

>  }

>  

>  void rx_cpu_list(void)

>  {

> -    GSList *list;

> -    list = object_class_get_list_sorted(TYPE_RXCPU, false);

> +    GSList *list = object_class_get_list_sorted(TYPE_RX_CPU, false);

> +

> +    qemu_printf("Available CPUs:\n");

>      g_slist_foreach(list, rx_cpu_list_entry, NULL);

>      g_slist_free(list);

>  }

> @@ -88,15 +89,17 @@ void rx_cpu_list(void)

>  static ObjectClass *rx_cpu_class_by_name(const char cpu_model)

>  {

>      ObjectClass *oc;

> -    char *typename = NULL;

> +    char *typename;

>  

> -    typename = g_strdup_printf(RX_CPU_TYPE_NAME(""));

> +    typename = g_strdup_printf(RX_CPU_TYPE_NAME("%s"), cpu_model);

>      oc = object_class_by_name(typename);


in case of new cpu, I'd allow only typename as cpu_model

s/typename/cpu_model/
  
which is compatible with '-device' naming and QMP/monitor interfaces
that we support.

and I would not add other naming schemes /like adding suffix to cpu_model or .../
that  are existing in QEMU for legacy reasons.


> -    if (oc != NULL && object_class_is_abstract(oc)) {

> -        oc = NULL;

> -    }

> -

>      g_free(typename);

> +

> +    if (oc == NULL ||

> +        object_class_is_abstract(oc) ||

> +        !object_class_dynamic_cast(oc, TYPE_RX_CPU)) {

> +        return NULL;

> +    }

>      return oc;

>  }

>  

[...]
Richard Henderson May 31, 2019, 2:59 p.m. UTC | #2 
On 5/31/19 9:23 AM, Igor Mammedov wrote:
> On Fri, 31 May 2019 08:43:05 -0500

> Richard Henderson <richard.henderson@linaro.org> wrote:

> 

>> There was confusion here about abstract classes and naming cpus.

>> We had registered a concrete class named "-rxcpu".  This was put

>> into the default cpu fields, and matched, so basic tests worked.

>> However, no value for -cpu could ever match in rx_cpu_class_by_name.

>>

>> Rename the base class to "rx-cpu" and make it abstract.  This

>> matches what we do for most other targets.  Create a new concrete

>> cpu with the name "rx62n-rx-cpu".

> 

> since it hasn't been merged yet, it would be better to squash this

> fixup into 3/23


Except that it's not just 3/23 but also 8/23.  Which is why it was so much
easier to leave it separate for review.

I suppose this could be split and squashed, it you insist.  I don't see any
particular value in that though.

>> -    typename = g_strdup_printf(RX_CPU_TYPE_NAME(""));

>> +    typename = g_strdup_printf(RX_CPU_TYPE_NAME("%s"), cpu_model);

>>      oc = object_class_by_name(typename);

> 

> in case of new cpu, I'd allow only typename as cpu_model

> 

> s/typename/cpu_model/

>   

> which is compatible with '-device' naming and QMP/monitor interfaces

> that we support.

> 

> and I would not add other naming schemes /like adding suffix to cpu_model or .../

> that  are existing in QEMU for legacy reasons.


I don't understand what you're looking for.

Do you want a type called "rx62n" for the concrete cpu instance?
That seems to be contrary to every other device in our system.

I hope you're not suggesting that the command-line be "-cpu rx62n-rx-cpu".
That seems pointlessly verbose.

If we're going to change the way we do things, we should do that everywhere,
and not make things different for only one target.


r~
Igor Mammedov May 31, 2019, 3:15 p.m. UTC | #3 
On Fri, 31 May 2019 09:59:14 -0500
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 5/31/19 9:23 AM, Igor Mammedov wrote:

> > On Fri, 31 May 2019 08:43:05 -0500

> > Richard Henderson <richard.henderson@linaro.org> wrote:

> >   

> >> There was confusion here about abstract classes and naming cpus.

> >> We had registered a concrete class named "-rxcpu".  This was put

> >> into the default cpu fields, and matched, so basic tests worked.

> >> However, no value for -cpu could ever match in rx_cpu_class_by_name.

> >>

> >> Rename the base class to "rx-cpu" and make it abstract.  This

> >> matches what we do for most other targets.  Create a new concrete

> >> cpu with the name "rx62n-rx-cpu".  

> > 

> > since it hasn't been merged yet, it would be better to squash this

> > fixup into 3/23  

> 

> Except that it's not just 3/23 but also 8/23.  Which is why it was so much

> easier to leave it separate for review.

> 

> I suppose this could be split and squashed, it you insist.  I don't see any

> particular value in that though.


well,
one has to wonder why previous patches looked strange before they stumble
on this one. So it's awkward fro reviewer and I'd guess for whomever would
dig through the history later.

> 

> >> -    typename = g_strdup_printf(RX_CPU_TYPE_NAME(""));

> >> +    typename = g_strdup_printf(RX_CPU_TYPE_NAME("%s"), cpu_model);

> >>      oc = object_class_by_name(typename);  

> > 

> > in case of new cpu, I'd allow only typename as cpu_model

> > 

> > s/typename/cpu_model/

> >   

> > which is compatible with '-device' naming and QMP/monitor interfaces

> > that we support.

> > 

> > and I would not add other naming schemes /like adding suffix to cpu_model or .../

> > that  are existing in QEMU for legacy reasons.  

> 

> I don't understand what you're looking for.

> 

> Do you want a type called "rx62n" for the concrete cpu instance?

> That seems to be contrary to every other device in our system.

> 

> I hope you're not suggesting that the command-line be "-cpu rx62n-rx-cpu".

> That seems pointlessly verbose.

the other interfaces (qmp/monitor/-device) are using verbose form only,
so it would be better to just be consistent with them.

> 

> If we're going to change the way we do things, we should do that everywhere,

> and not make things different for only one target.

We can't do the same for already existing cpus since it might break existing
configurations out-there. But at least the new code won't get in the way
when we get to deprecating multiple ways one could name cpu with -cpu.

> 

> 

> r~
diff mbox series

Patch

diff --git a/target/rx/cpu.h b/target/rx/cpu.h
index 8c1a4e448d..a0b6975963 100644
--- a/target/rx/cpu.h
+++ b/target/rx/cpu.h
@@ -24,14 +24,14 @@ 
 #include "hw/registerfields.h"
 #include "qom/cpu.h"
 
-#define TYPE_RXCPU "rxcpu"
+#define TYPE_RX_CPU "rx-cpu"
 
 #define RXCPU_CLASS(klass)                                     \
-    OBJECT_CLASS_CHECK(RXCPUClass, (klass), TYPE_RXCPU)
+    OBJECT_CLASS_CHECK(RXCPUClass, (klass), TYPE_RX_CPU)
 #define RXCPU(obj) \
-    OBJECT_CHECK(RXCPU, (obj), TYPE_RXCPU)
+    OBJECT_CHECK(RXCPU, (obj), TYPE_RX_CPU)
 #define RXCPU_GET_CLASS(obj) \
-    OBJECT_GET_CLASS(RXCPUClass, (obj), TYPE_RXCPU)
+    OBJECT_GET_CLASS(RXCPUClass, (obj), TYPE_RX_CPU)
 
 /*
  * RXCPUClass:
@@ -164,9 +164,9 @@  static inline RXCPU *rx_env_get_cpu(CPURXState *env)
 
 #define ENV_OFFSET offsetof(RXCPU, env)
 
-#define RX_CPU_TYPE_SUFFIX "-" TYPE_RXCPU
+#define RX_CPU_TYPE_SUFFIX "-" TYPE_RX_CPU
 #define RX_CPU_TYPE_NAME(model) model RX_CPU_TYPE_SUFFIX
-#define CPU_RESOLVING_TYPE TYPE_RXCPU
+#define CPU_RESOLVING_TYPE TYPE_RX_CPU
 
 extern const char rx_crname[][6];
 
diff --git a/hw/rx/rx-virt.c b/hw/rx/rx-virt.c
index 3deb7cb335..72a2989fcf 100644
--- a/hw/rx/rx-virt.c
+++ b/hw/rx/rx-virt.c
@@ -88,7 +88,7 @@  static void rxvirt_class_init(ObjectClass *oc, void *data)
     mc->desc = "RX QEMU Virtual Target";
     mc->init = rxvirt_init;
     mc->is_default = 1;
-    mc->default_cpu_type = TYPE_RXCPU;
+    mc->default_cpu_type = RX_CPU_TYPE_NAME("rx62n");
 }
 
 static const TypeInfo rxvirt_type = {
diff --git a/hw/rx/rx62n.c b/hw/rx/rx62n.c
index c6660b75b4..3a8fe7b0bf 100644
--- a/hw/rx/rx62n.c
+++ b/hw/rx/rx62n.c
@@ -195,7 +195,7 @@  static void rx62n_realize(DeviceState *dev, Error **errp)
     }
 
     object_initialize_child(OBJECT(s), "cpu", &s->cpu,
-                            sizeof(RXCPU), TYPE_RXCPU,
+                            sizeof(RXCPU), RX_CPU_TYPE_NAME("rx62n"),
                             errp, NULL);
     object_property_set_bool(OBJECT(&s->cpu), true, "realized", errp);
 
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 3268077d08..41fe1de4bb 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -74,13 +74,14 @@  static void rx_cpu_list_entry(gpointer data, gpointer user_data)
     const char *typename = object_class_get_name(OBJECT_CLASS(data));
     int len = strlen(typename) - strlen(RX_CPU_TYPE_SUFFIX);
 
-    qemu_printf("%.*s\n", len, typename);
+    qemu_printf("  %.*s\n", len, typename);
 }
 
 void rx_cpu_list(void)
 {
-    GSList *list;
-    list = object_class_get_list_sorted(TYPE_RXCPU, false);
+    GSList *list = object_class_get_list_sorted(TYPE_RX_CPU, false);
+
+    qemu_printf("Available CPUs:\n");
     g_slist_foreach(list, rx_cpu_list_entry, NULL);
     g_slist_free(list);
 }
@@ -88,15 +89,17 @@  void rx_cpu_list(void)
 static ObjectClass *rx_cpu_class_by_name(const char *cpu_model)
 {
     ObjectClass *oc;
-    char *typename = NULL;
+    char *typename;
 
-    typename = g_strdup_printf(RX_CPU_TYPE_NAME(""));
+    typename = g_strdup_printf(RX_CPU_TYPE_NAME("%s"), cpu_model);
     oc = object_class_by_name(typename);
-    if (oc != NULL && object_class_is_abstract(oc)) {
-        oc = NULL;
-    }
-
     g_free(typename);
+
+    if (oc == NULL ||
+        object_class_is_abstract(oc) ||
+        !object_class_dynamic_cast(oc, TYPE_RX_CPU)) {
+        return NULL;
+    }
     return oc;
 }
 
@@ -166,7 +169,7 @@  static void rx_cpu_init(Object *obj)
     qdev_init_gpio_in(DEVICE(cpu), rx_cpu_set_irq, 2);
 }
 
-static void rxcpu_class_init(ObjectClass *klass, void *data)
+static void rx_cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     CPUClass *cc = CPU_CLASS(klass);
@@ -195,22 +198,28 @@  static void rxcpu_class_init(ObjectClass *klass, void *data)
     cc->gdb_num_core_regs = 26;
 }
 
-static const TypeInfo rxcpu_info = {
-    .name = TYPE_RXCPU,
+static const TypeInfo rx_cpu_info = {
+    .name = TYPE_RX_CPU,
     .parent = TYPE_CPU,
     .instance_size = sizeof(RXCPU),
     .instance_init = rx_cpu_init,
-    .abstract = false,
+    .abstract = true,
     .class_size = sizeof(RXCPUClass),
-    .class_init = rxcpu_class_init,
+    .class_init = rx_cpu_class_init,
 };
 
-static void rxcpu_register_types(void)
+static const TypeInfo rx62n_rx_cpu_info = {
+    .name = RX_CPU_TYPE_NAME("rx62n"),
+    .parent = TYPE_RX_CPU,
+};
+
+static void rx_cpu_register_types(void)
 {
-    type_register_static(&rxcpu_info);
+    type_register_static(&rx_cpu_info);
+    type_register_static(&rx62n_rx_cpu_info);
 }
 
-type_init(rxcpu_register_types)
+type_init(rx_cpu_register_types)
 
 static uint32_t extable[32];