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

[Richard Henderson]

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.

>> -    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~