Re: [Qemu-devel] [PATCH for-2.11 2/6] ppc: make cpu_model translation to type consistent

[Igor Mammedov]

On Fri, 25 Aug 2017 19:45:38 +1000
David Gibson <address@hidden> wrote:

> On Fri, Aug 25, 2017 at 09:27:40AM +0200, Igor Mammedov wrote:
> > On Fri, 25 Aug 2017 11:38:19 +1000
> > David Gibson <address@hidden> wrote:
> >   
> > > On Thu, Aug 24, 2017 at 10:21:47AM +0200, Igor Mammedov wrote:  
> > > > PPC handles -cpu FOO rather incosistently,
> > > > i.e. it does case-insensitive matching of FOO to
> > > > a CPU type (see: ppc_cpu_compare_class_name) but
> > > > handles alias names as case-sensitive, as result:
> > > > 
> > > >  # qemu-system-ppc64 -M mac99 -cpu g3
> > > >  qemu-system-ppc64: unable to find CPU model ' kN�U'
> > > > 
> > > >  # qemu-system-ppc64 -cpu 970MP_V1.1
> > > >  qemu-system-ppc64: Unable to find sPAPR CPU Core definition
> > > > 
> > > > while
> > > > 
> > > >  # qemu-system-ppc64 -M mac99 -cpu G3
> > > >  # qemu-system-ppc64 -cpu 970MP_v1.1
> > > > 
> > > > start up just fine.
> > > > 
> > > > Considering we can't take case-insensitive matching away,
> > > > make it case-insensitive for  all alias/type/core_type
> > > > lookups.
> > > > 
> > > > As side effect it allows to remove duplicate core types
> > > > which are the same but use lower-case letters in name.
> > > > 
> > > > Signed-off-by: Igor Mammedov <address@hidden>    
> > > 
> > > Hmm.  So, I'm certainly good with making the matching more consistent,
> > > and removing extra entries differing only in case.
> > > 
> > > However, I don't like capitalizing everything in the model table.  The
> > > "canonical" capitalization - as in how it appears in manuals and
> > > marketing info - is quite frequently mixed case.  So I think making
> > > everything caps in the table will make it harder to read in the
> > > context of looking at the corresponding documentation.  
> > only cpu models => typenames are made caps, the rest  
> 
> Yes, I know.  A number of the CPU model names themselves have mixed
> case.  Really.
> 
> > incl. description is kept as is in mixed case.
> > It looks like _desc is the thing one would use for
> > 1:1 lookup in spec, while _name isn't direct match consistently
> > (i.e. sometimes it skips spaces and sometimes spaces are replaced by 
> > underscore).
> > So keeping _name in mixed case unnecessarily complicates name->type lookup.
> > 
> > These mixed case + case-insensitive is what made lookup
> > code over-engineered and overly complex.  
> 
> I'm fine with making all the lookups case insensitive.  I just don't
> want to drop the case on the names in the actual code.
I've tried to find a way but considering that names are statically
'converted' into typenames and lack of ability to magically
uppercase them with preprocessor, I've gave up on this approach.

Providing that there is _desc with model name that should be
greppable in spec and complexity of lookup code that mixed-cased
typenames incur, I've opted in favor of simplifying lookup
code at expense uniform typenames (which is inter QEMU thingy).
It is easier to maintain and less chances to make mistake.

Other targets use exact match for cpu_model so whether
they use mixed casing or not doesn't matter.

> > As result it evolved (regressed) to inconsistent name handling
> > since it's rather hard to understand what's going on there.  
> 
> Yeah, I know :/.
> 
> > > Can we instead retain mixed case in the table, and capitalize both
> > > sides at the comparison time?  
> > it has to be the same-case since PPC has case-insensitive cpu_model
> > matching (sometimes :/). Normalized /same case/ typenames
> > are necessary for object_class_by_name() to work as it does exact match.
> > So it would be possible to replace custom fetching of CPU types
> > and iterating over it with custom comparator with object_class_by_name().
> > As it's done in 4/6.
> > This patch and 3/6 are only preparatory patches for 4/6.  
> 
> Ah.  Hrm.  That's awkward.
It's fine to squash all 3 in 1 patch, but then it becomes huge and
unreview-able, as logic changes are buried within names normalization.