Re: [Qemu-devel] [Patch v2 04/29] s390x/cpumodel: introduce CPU features

[David Hildenbrand <dahi@linux.vnet.ibm.com>]

> On 08/08/2016 05:32 PM, David Hildenbrand wrote:
> 
> In general this this very good. Mostly bike shedding and naming.

Thanks!

> 
> > +/* indexed by feature number for easy lookup */
> > +static const S390FeatDef s390_features[] = {
> > +    FEAT_INIT("n3", S390_FEAT_TYPE_STFL, 0, "Instructions marked as n3"),  
> 
> /proc/cpuinfo calls this "esan3", so use the same?

Agreed.

> 
> > +    FEAT_INIT("zarch", S390_FEAT_TYPE_STFL, 1, "z/Architecture architectural mode"),
> > +    FEAT_INIT("dateh", S390_FEAT_TYPE_STFL, 3, "DAT-enhancement facility"),
> > +    FEAT_INIT("idtes", S390_FEAT_TYPE_STFL, 4, "IDTE selective TLB segment-table clearing"),
> > +    FEAT_INIT("idter", S390_FEAT_TYPE_STFL, 5, "IDTE selective TLB region-table clearing"),
> > +    FEAT_INIT("asnlxr", S390_FEAT_TYPE_STFL, 6, "ASN-and-LX reuse facility"),
> > +    FEAT_INIT("stfle", S390_FEAT_TYPE_STFL, 7, "Store-facility-list-extended facility"),
> > +    FEAT_INIT("edat", S390_FEAT_TYPE_STFL, 8, "Enhanced-DAT facility"),
> > +    FEAT_INIT("srs", S390_FEAT_TYPE_STFL, 9, "Sense-running-status facility"),
> > +    FEAT_INIT("csske", S390_FEAT_TYPE_STFL, 10, "Conditional-SSKE facility"),
> > +    FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology facility"),
> > +    FEAT_INIT("ipter", S390_FEAT_TYPE_STFL, 13, "IPTE-range facility"),
> > +    FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting facility"),
> > +    FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation facility 2"),
> > +    FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist facility (excluding subfunctions)"),  
> 
> Hmm, same for all other msa variants below. Why not just "msa" (as /proc/cpuinfo)? Does "base"
> indicate that we have the facility bit/instruction,but no subfunction like SHA?

Yes, as the description says "excluding subfunctions".

msa is a feature group, containing all introduced subfunctions.

msa-base is just the STFL bit. As discussed a while ago, we need this, because
we could have some variance at that point int the future.

> 
> > +    FEAT_INIT("ldisp", S390_FEAT_TYPE_STFL, 18, "Long-displacement facility"),
> > +    FEAT_INIT("ldisphp", S390_FEAT_TYPE_STFL, 19, "Long-displacement facility has high performance"),
> > +    FEAT_INIT("hfpm", S390_FEAT_TYPE_STFL, 20, "HFP-multiply-add/subtract facility"),
> > +    FEAT_INIT("eimm", S390_FEAT_TYPE_STFL, 21, "Extended-immediate facility"),
> > +    FEAT_INIT("etf3", S390_FEAT_TYPE_STFL, 22, "Extended-translation facility 3"),
> > +    FEAT_INIT("hfpue", S390_FEAT_TYPE_STFL, 23, "HFP-unnormalized-extension facility"),
> > +    FEAT_INIT("etf2eh", S390_FEAT_TYPE_STFL, 24, "ETF2-enhancement facility"),
> > +    FEAT_INIT("stckf", S390_FEAT_TYPE_STFL, 25, "Store-clock-fast facility"),
> > +    FEAT_INIT("parseh", S390_FEAT_TYPE_STFL, 26, "Parsing-enhancement facility"),
> > +    FEAT_INIT("mvcos", S390_FEAT_TYPE_STFL, 27, "Move-with-optional-specification facility"),
> > +    FEAT_INIT("tods-base", S390_FEAT_TYPE_STFL, 28, "TOD-clock-steering facility (excluding subfunctions)"),
> > +    FEAT_INIT("etf3eh", S390_FEAT_TYPE_STFL, 30, "ETF3-enhancement facility"),
> > +    FEAT_INIT("ectg", S390_FEAT_TYPE_STFL, 31, "Extract-CPU-time facility"),
> > +    FEAT_INIT("csst", S390_FEAT_TYPE_STFL, 32, "Compare-and-swap-and-store facility"),
> > +    FEAT_INIT("csst2", S390_FEAT_TYPE_STFL, 33, "Compare-and-swap-and-store facility 2"),
> > +    FEAT_INIT("ginste", S390_FEAT_TYPE_STFL, 34, "General-instructions-extension facility"),
> > +    FEAT_INIT("exrl", S390_FEAT_TYPE_STFL, 35, "Execute-extensions facility"),
> > +    FEAT_INIT("emon", S390_FEAT_TYPE_STFL, 36, "Enhanced-monitor facility"),
> > +    FEAT_INIT("fpe", S390_FEAT_TYPE_STFL, 37, "Floating-point extension facility"),
> > +    FEAT_INIT("sprogp", S390_FEAT_TYPE_STFL, 40, "Set-program-parameters facility"),
> > +    FEAT_INIT("fpseh", S390_FEAT_TYPE_STFL, 41, "Floating-point-support-enhancement facilities"),
> > +    FEAT_INIT("dfp", S390_FEAT_TYPE_STFL, 42, "DFP (decimal-floating-point) facility"),
> > +    FEAT_INIT("dfphp", S390_FEAT_TYPE_STFL, 43, "DFP (decimal-floating-point) facility has high performance"),
> > +    FEAT_INIT("pfpo", S390_FEAT_TYPE_STFL, 44, "PFPO instruction"),
> > +    FEAT_INIT("gen11e", S390_FEAT_TYPE_STFL, 45, "Various facilities introduced with z196"),  
> 
> Hmm, not sure if gen11e is the best name.
> 
> It is for
> "The distinct-operands, fast-BCR-serialization, high-
> word, and population-count facilities, the
> interlocked-access facility 1, and the load/store-on-
> condition facility 1 are installed in the z/Architecture
> architectural mode."
> 
> which is certainly too long,even for the comment. So what about 
> "stfle45" instead of "gen11e" ? 

well, it at least tells you that it is a an enhancement part of hw generation
11. But I don't have a strong opinion on that. These should usually never be
presented to the user either way (because contained in the base models).

David