Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Wed, May 08, 2019 at 10:34:44AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <address@hidden> writes:
>> 
>> > On Mon, May 06, 2019 at 01:53:28PM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <address@hidden> writes:
>> >> 
>> >> > This series adds a new CPUClass::class_name_format field, which
>> >> > allows us to delete 16 of the 21 *_cpu_class_by_name() functions
>> >> > that exist today.
>> >> 
>> >> Which five remain, and why?
>> >
>> > alpha_cpu_class_by_name:
>> > * Translates aliases based on alpha_cpu_aliases;
>> > * Falls back to "ev67" unconditionally
>> >   (there's a "TODO: remove match everything nonsense" comment).
>> >
>> > cris_cpu_class_by_name:
>> > * Translates "any" alias to "crisv32" if CONFIG_USER_ONLY.
>> >
>> > ppc_cpu_class_by_name:
>> > * Supports lookup by PVR if CPU model is a 8 digit hex number;
>> > * Converts CPU model to lowercase.
>> >
>> > superh_cpu_class_by_name:
>> > * Translates "any" alias to TYPE_SH7750R_CPU.
>> >
>> > sparc_cpu_class_by_name:
>> > * Replaces whitespaces with '-' on CPU model name.
>> 
>> I'm of course asking because I wonder whether we can dumb down this CPU
>> naming business to something simpler and more regular.
>
> We can, but that's not on my list of priorities.  Any volunteers?

Fair enough.  Except for...

>> 
> [...]
>> Observations:
>> 
>> * The CPU type name format is generally "%s-T-cpu", where T is either
>>   <TARGET> or <TARGET>64.
>> 
>>   Exceptions:
>> 
>>   - openrisc, sh4 uses or1k, superh instead.  Looks pointless to me.
>> 
>>   - i386 uses x86_64 instead of i38664.  Makes sense.
>> 
>>   - hppa, microblaze, nios2 and tilegx use CPU type name format "T-cpu",
>>     ignoring the user's cpu model.  These exceptions looks pointless to
>>     me.
>> 
>> * The user's CPU model is generally the "%s" part of the format.
>> 
>>   Exceptions:
>> 
>>   - alpha additionaly recognizes full type names.  If that's useful for
>>     alpha (I'm not sure it is), why isn't it useful for all other
>>     targets?
>> 
>>   - cris and sh4 additionaly recognize an "any" alias, cris only #ifdef
>>     CONFIG_USER_ONLY.
>> 
>>     Until PATCH 4, arm also recognizes an "any" alias #ifdef
>>     CONFIG_USER_ONLY.  PATCH 4 drops that, because it's redundant with
>>     the "any" CPU, which is a copy instead of an alias.  Sure we want to
>>     do have different targets do "any" in different ways?
>> 
>>     See aliases below.
>> 
>>   - ppc additionaly recognizes PVR aliases and additional (case
>>     insensitive) aliases.  Feels overengineered to me.  See aliases
>>     below.
>> 
>>   - sparc additionally recognizes aliases with ' ' instead of '-'.
>>     Feels pointless to me.  See aliases below.

... this, perhaps:

>> * What about deprecating pointless exceptions?

Deprecating unwanted stuff now is likely to make a later cleanup so much
easier.

>> * Aliases
>> 
>>   We have several targets roll their own CPU name aliases code.
>>   Assuming aliases are here to stay (i.e. we're not deprecating all of
>>   them): what about letting each CPU type specify a set of aliases, so
>>   we can recognize them in generic code?
>
> Yes.  I considered adding alias support to generic code, but
> decided to do this one step at a time.

Okay.  Consider adding suitable TODO comments.