Re: [Qemu-devel] [PATCH v17 13/24] target/rx: Fix cpu types and names

[Yoshinori Sato]

On Fri, 07 Jun 2019 23:19:49 +0900,
Igor Mammedov wrote:
> 
> On Fri,  7 Jun 2019 18:11:05 +0900
> Yoshinori Sato <address@hidden> wrote:
> 
> > From: Richard Henderson <address@hidden>
> > 
> > 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".
> My comments on v16 weren't addressed at all.
> 
> I don't see any valid reason for this patch to exist.
> It should be split and merged with patches that introduce 'confusion'
> so that broken code won't exist in the first place.
> 
> more below ...
> 
> > Signed-off-by: Richard Henderson <address@hidden>
> > Signed-off-by: Yoshinori Sato <address@hidden>
> > ---
> >  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(-)
> > 
> > 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);
> 
> again repeating v16: see alpha_cpu_class_by_name() as preferred
> way to use this method.

OK.
I misunderstood. I'll fix like alpha cpu.

> 
> > -    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 const TypeInfo rx62n_rx_cpu_info = {
> > +    .name = RX_CPU_TYPE_NAME("rx62n"),
> > +    .parent = TYPE_RX_CPU,
> >  };
> >  
> > -static void rxcpu_register_types(void)
> > +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];
> >  
> 
> 

-- 
Yosinori Sato