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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5017 bytes --]

On Fri, Aug 25, 2017 at 01:40:07PM +0200, Igor Mammedov wrote:
> On Fri, 25 Aug 2017 19:45:38 +1000
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <imammedo@redhat.com>    
> > > > 
> > > > 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.

Ok.

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

Yeah, I guess.  It just looks weird to have non-standard capsing in
the table field.

Any chance we could standardize on lowercase rather than upper - for
some reason that would seem less odd to me (I guess because C-ish
names and identifiers are usually lowercased).

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

Sorry, I meant it's awkward that that object_class_by_name() uses exact
and so we'd need a custom iterator to do a case insensitive search.
The patch structure is fine.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]