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

[Igor Mammedov]

On Fri, 31 May 2019 09:59:14 -0500
Richard Henderson <address@hidden> wrote:

> On 5/31/19 9:23 AM, Igor Mammedov wrote:
> > On Fri, 31 May 2019 08:43:05 -0500
> > Richard Henderson <address@hidden> 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~