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

[David Hildenbrand]

> 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