Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[Cornelia Huck]

On Mon, 15 Aug 2016 16:03:04 +0200
Christian Borntraeger  wrote:

> On 08/15/2016 04:00 PM, David Hildenbrand wrote:
> > 
> > I'll wait a bit until sending out another series that fixes two minor style
> > problems.
> > 
> > What about > 80 chars per line. Is it okay for these definitions + function
> > prototype or a no-go?
> 
> As long as we are < 90 I think its better than wrapping.
> 

Agreed.

I think it's even ok to go over 90 if the alternative is breaking up
strings - which is extremely annoying for grepping.

Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[Christian Borntraeger]

On 08/15/2016 04:00 PM, David Hildenbrand wrote:
> 
> I'll wait a bit until sending out another series that fixes two minor style
> problems.
> 
> What about > 80 chars per line. Is it okay for these definitions + function
> prototype or a no-go?

As long as we are < 90 I think its better than wrapping.

Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[David Hildenbrand]

I'll wait a bit until sending out another series that fixes two minor style
problems.

What about > 80 chars per line. Is it okay for these definitions + function
prototype or a no-go?

David

Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[Fam Zheng]

On Mon, 08/08 14:27, Eduardo Habkost wrote:
> On Mon, Aug 08, 2016 at 09:45:04AM -0700, 
> no-re...@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> > Hi,
> > 
> > Your series seems to have some coding style problems. See output below for
> > more information:
> [...]
> 
> Does anybody know who owns this robot?

Fam does.

Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[Eduardo Habkost]

On Mon, Aug 08, 2016 at 09:45:04AM -0700, 
no-re...@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
[...]

Does anybody know who owns this robot?

-- 
Eduardo

Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[David Hildenbrand]

> On Mon, 8 Aug 2016 09:45:04 -0700 (PDT)
> no-re...@patchew.org wrote:
> 
>  something sensible, e.g. qemu-devel>
> 
> > Checking PATCH 4/29: s390x/cpumodel: introduce CPU features...
> > WARNING: line over 80 characters
> > #65: FILE: target-s390x/cpu_features.c:27:
> > +FEAT_INIT("zarch", S390_FEAT_TYPE_STFL, 1, "z/Architecture 
> > architectural mode"),  
> 
> We could conceivably break the line after one of the arguments, but
> there are some very long strings below. I'm not a fan of very long
> lines myself, but I wonder whether we should relax that limit? The
> Linux kernel has settled upon a relaxed limit and forbids splitting
> strings for greppability.

I kept these > 80 as these definitions look much cleaner this way.

> 
> 
> > Checking PATCH 5/29: s390x/cpumodel: generate CPU feature lists for CPU 
> > models...
> > ERROR: Macros with complex values should be enclosed in parenthesis
> > #113: FILE: target-s390x/gen-features.c:20:
> > +#define S390_FEAT_GROUP_PLO \
> > +S390_FEAT_PLO_CL, \
> > +S390_FEAT_PLO_CLG, \
> > +S390_FEAT_PLO_CLGR, \
> > +S390_FEAT_PLO_CLX, \
> > +S390_FEAT_PLO_CS, \
> > +S390_FEAT_PLO_CSG, \
> > +S390_FEAT_PLO_CSGR, \
> > +S390_FEAT_PLO_CSX, \
> > +S390_FEAT_PLO_DCS, \
> > +S390_FEAT_PLO_DCSG, \
> > +S390_FEAT_PLO_DCSGR, \
> > +S390_FEAT_PLO_DCSX, \
> > +S390_FEAT_PLO_CSST, \
> > +S390_FEAT_PLO_CSSTG, \
> > +S390_FEAT_PLO_CSSTGR, \
> > +S390_FEAT_PLO_CSSTX, \
> > +S390_FEAT_PLO_CSDST, \
> > +S390_FEAT_PLO_CSDSTG, \
> > +S390_FEAT_PLO_CSDSTGR, \
> > +S390_FEAT_PLO_CSDSTX, \
> > +S390_FEAT_PLO_CSTST, \
> > +S390_FEAT_PLO_CSTSTG, \
> > +S390_FEAT_PLO_CSTSTGR, \
> > +S390_FEAT_PLO_CSTSTX  
> 
> I think the check is wrong to complain here.

Yes, these complaints are wrong.

> 
> > Checking PATCH 6/29: s390x/cpumodel: generate CPU feature group lists...
> > Checking PATCH 7/29: s390x/cpumodel: introduce CPU feature group 
> > definitions...
> > WARNING: line over 80 characters
> > #71: FILE: target-s390x/cpu_features.c:363:
> > +FEAT_GROUP_INIT("gen13ptff", GEN13_PTFF, "PTFF enhancements introduced 
> > with z13"),  
> 
> Same comments as above.

Dito, the list of features just looks cleaner.

[...]
> 
> > Checking PATCH 27/29: s390x/cpumodel: implement QMP interface 
> > "query-cpu-model-expansion"...
> > WARNING: line over 80 characters
> > #152: FILE: target-s390x/cpu_models.c:408:
> > +s390_feat_bitmap_to_ascii(model->features, qdict, 
> > qdict_add_enabled_feat);

I'll fix that

> > 
> > WARNING: line over 80 characters
> > #165: FILE: target-s390x/cpu_models.c:421:
> > +CpuModelExpansionInfo 
> > *arch_query_cpu_model_expansion(CpuModelExpansionType type,
> > 
> > ERROR: space prohibited before that close parenthesis ')'
> > #181: FILE: target-s390x/cpu_models.c:437:
> > +} else if (type != CPU_MODEL_EXPANSION_TYPE_FULL ) {  
> 
> That's actually a warning I concur with :)

And this of course :)

> 
> I think we should at least come up with a check for a linux header
> update to avoid spamming the mailing list. And it would be a good idea
> to have some consensus about the 80 char limit as well.

For the definitions and the lengthy function prototypes I really don't want to
break readability just to conform to 80 chars.

Thanks.

David

Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[Cornelia Huck]

On Mon, 8 Aug 2016 09:45:04 -0700 (PDT)
no-re...@patchew.org wrote:



> Checking PATCH 4/29: s390x/cpumodel: introduce CPU features...
> WARNING: line over 80 characters
> #65: FILE: target-s390x/cpu_features.c:27:
> +FEAT_INIT("zarch", S390_FEAT_TYPE_STFL, 1, "z/Architecture architectural 
> mode"),

We could conceivably break the line after one of the arguments, but
there are some very long strings below. I'm not a fan of very long
lines myself, but I wonder whether we should relax that limit? The
Linux kernel has settled upon a relaxed limit and forbids splitting
strings for greppability.


> Checking PATCH 5/29: s390x/cpumodel: generate CPU feature lists for CPU 
> models...
> ERROR: Macros with complex values should be enclosed in parenthesis
> #113: FILE: target-s390x/gen-features.c:20:
> +#define S390_FEAT_GROUP_PLO \
> +S390_FEAT_PLO_CL, \
> +S390_FEAT_PLO_CLG, \
> +S390_FEAT_PLO_CLGR, \
> +S390_FEAT_PLO_CLX, \
> +S390_FEAT_PLO_CS, \
> +S390_FEAT_PLO_CSG, \
> +S390_FEAT_PLO_CSGR, \
> +S390_FEAT_PLO_CSX, \
> +S390_FEAT_PLO_DCS, \
> +S390_FEAT_PLO_DCSG, \
> +S390_FEAT_PLO_DCSGR, \
> +S390_FEAT_PLO_DCSX, \
> +S390_FEAT_PLO_CSST, \
> +S390_FEAT_PLO_CSSTG, \
> +S390_FEAT_PLO_CSSTGR, \
> +S390_FEAT_PLO_CSSTX, \
> +S390_FEAT_PLO_CSDST, \
> +S390_FEAT_PLO_CSDSTG, \
> +S390_FEAT_PLO_CSDSTGR, \
> +S390_FEAT_PLO_CSDSTX, \
> +S390_FEAT_PLO_CSTST, \
> +S390_FEAT_PLO_CSTSTG, \
> +S390_FEAT_PLO_CSTSTGR, \
> +S390_FEAT_PLO_CSTSTX

I think the check is wrong to complain here.

> Checking PATCH 6/29: s390x/cpumodel: generate CPU feature group lists...
> Checking PATCH 7/29: s390x/cpumodel: introduce CPU feature group 
> definitions...
> WARNING: line over 80 characters
> #71: FILE: target-s390x/cpu_features.c:363:
> +FEAT_GROUP_INIT("gen13ptff", GEN13_PTFF, "PTFF enhancements introduced 
> with z13"),

Same comments as above.


> Checking PATCH 8/29: s390x/cpumodel: register defined CPU models as 
> subclasses...
> WARNING: line over 80 characters
> #64: FILE: target-s390x/cpu_models.c:30:
> +.base_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## _BASE 
> },  \

And here. I honestly don't see a way to get this below 80 chars.


> Checking PATCH 19/29: linux-headers: update against kvm/next...
> ERROR: code indent should never use tabs
> #21: FILE: include/standard-headers/linux/input-event-codes.h:615:
> +#define KEY_RIGHT_UP^I^I^I0x266$

We should not check a linux headers update against qemu coding style.
Either ignore the respective files, or check whether this patch is a
linux headers update and nothing else. (I lack the perl skills for
that :)


> Checking PATCH 24/29: qmp: add QMP interface "query-cpu-model-expansion"...
> WARNING: line over 80 characters
> #29: FILE: include/sysemu/arch_init.h:38:
> +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,

This is a case of VeryLongVariableNames. Not sure whether we should do
anything about that.

> Checking PATCH 27/29: s390x/cpumodel: implement QMP interface 
> "query-cpu-model-expansion"...
> WARNING: line over 80 characters
> #152: FILE: target-s390x/cpu_models.c:408:
> +s390_feat_bitmap_to_ascii(model->features, qdict, 
> qdict_add_enabled_feat);
> 
> WARNING: line over 80 characters
> #165: FILE: target-s390x/cpu_models.c:421:
> +CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
> type,
> 
> ERROR: space prohibited before that close parenthesis ')'
> #181: FILE: target-s390x/cpu_models.c:437:
> +} else if (type != CPU_MODEL_EXPANSION_TYPE_FULL ) {

That's actually a warning I concur with :)

I think we should at least come up with a check for a linux header
update to avoid spamming the mailing list. And it would be a good idea
to have some consensus about the 80 char limit as well.

Re: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 1470670378-53732-1-git-send-email-d...@linux.vnet.ibm.com
Type: series
Subject: [Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] 
patchew/1470670378-53732-1-git-send-email-d...@linux.vnet.ibm.com -> 
patchew/1470670378-53732-1-git-send-email-d...@linux.vnet.ibm.com
 * [new tag] 
patchew/1470672688-6754-1-git-send-email-peter.mayd...@linaro.org -> 
patchew/1470672688-6754-1-git-send-email-peter.mayd...@linaro.org
Switched to a new branch 'test'
0f4c162 s390x/cpumodel: implement QMP interface "query-cpu-model-baseline"
82b01ce s390x/cpumodel: implement QMP interface "query-cpu-model-comparison"
53e522c s390x/cpumodel: implement QMP interface "query-cpu-model-expansion"
8f9fa25 qmp: add QMP interface "query-cpu-model-baseline"
402fc8c qmp: add QMP interface "query-cpu-model-comparison"
ff71317 qmp: add QMP interface "query-cpu-model-expansion"
89ba00e s390x/kvm: let the CPU model control CMM(A)
0d59bdc s390x/kvm: disable host model for existing compat machines
a72bde4 s390x/kvm: implement CPU model support
57ac9b5 s390x/kvm: allow runtime-instrumentation for "none" machine
4eef365 linux-headers: update against kvm/next
cc50bdd s390x/sclp: propagate hmfai
51c8a79 s390x/sclp: propagate the mha via sclp
72e2a71 s390x/sclp: propagate the ibc val(lowest and unblocked ibc)
3265bd9 s390x/sclp: indicate sclp features
f94cb6a s390x/sclp: introduce sclp feature blocks
6c5201b s390x/sclp: factor out preparation of cpu entries
c3eb0c9 s390x/cpumodel: check and apply the CPU model
2df0fc2 s390x/cpumodel: let the CPU model handle feature checks
0049535 s390x/cpumodel: expose features and feature groups as properties
38af102 s390x/cpumodel: store the CPU model in the CPU instance
29204e3 s390x/cpumodel: register defined CPU models as subclasses
aafc57b s390x/cpumodel: introduce CPU feature group definitions
108d632 s390x/cpumodel: generate CPU feature group lists
14eaffb s390x/cpumodel: generate CPU feature lists for CPU models
9cae26a s390x/cpumodel: introduce CPU features
306bed0 s390x/cpumodel: expose CPU class properties
8eca539 s390x/cpumodel: "host" and "qemu" as CPU subclasses
fe0ebf0 qmp: details about CPU definitions in query-cpu-definitions

=== OUTPUT BEGIN ===
Checking PATCH 1/29: qmp: details about CPU definitions in 
query-cpu-definitions...
Checking PATCH 2/29: s390x/cpumodel: "host" and "qemu" as CPU subclasses...
Checking PATCH 3/29: s390x/cpumodel: expose CPU class properties...
Checking PATCH 4/29: s390x/cpumodel: introduce CPU features...
WARNING: line over 80 characters
#65: FILE: target-s390x/cpu_features.c:27:
+FEAT_INIT("zarch", S390_FEAT_TYPE_STFL, 1, "z/Architecture architectural 
mode"),

WARNING: line over 80 characters
#67: FILE: target-s390x/cpu_features.c:29:
+FEAT_INIT("idtes", S390_FEAT_TYPE_STFL, 4, "IDTE selective TLB 
segment-table clearing"),

WARNING: line over 80 characters
#68: FILE: target-s390x/cpu_features.c:30:
+FEAT_INIT("idter", S390_FEAT_TYPE_STFL, 5, "IDTE selective TLB 
region-table clearing"),

WARNING: line over 80 characters
#70: FILE: target-s390x/cpu_features.c:32:
+FEAT_INIT("stfle", S390_FEAT_TYPE_STFL, 7, "Store-facility-list-extended 
facility"),

WARNING: line over 80 characters
#74: FILE: target-s390x/cpu_features.c:36:
+FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology 
facility"),

WARNING: line over 80 characters
#76: FILE: target-s390x/cpu_features.c:38:
+FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting 
facility"),

WARNING: line over 80 characters
#77: FILE: target-s390x/cpu_features.c:39:
+FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation facility 
2"),

WARNING: line over 80 characters
#78: FILE: target-s390x/cpu_features.c:40:
+FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist 
facility (excluding subfunctions)"),

WARNING: line over 80 characters
#80: FILE: target-s390x/cpu_features.c:42:
+FEAT_INIT("ldisphp", S390_FEAT_TYPE_STFL, 19, "Long-displacement facility 
h

[Qemu-devel] [Patch v2 00/29] s390x CPU models: exposing features

[David Hildenbrand]

After another very helpful discussion with Eduardo and Jiri and the lates
review, these were the major changes:
- QOM interfaces: Only documentation changes
- Minor s390x specific implementation fixups
- Renamed some s390x features, sthyi added to the default + max models
Full list of changes can be found at the end of this lengthy cover letter.

Latest version can be found on branch:
github.com/cohuck/qemu cpumodel-s390x-v2

Whenever this gets pulled, we have time to the next release to double
check all feature names and static CPU model features.

Concept

This is our second attempt to implement CPU models for s390x. We realized
that we also want to have features exposed via the CPU model. While doing
that we realized that we want to have a better interface for libvirt.

Unfortunately, CPU models on s390x are special and we have to take care of:
- A CPU like z13 looks differently in various environments (under different
  LPAR versions, under different z/VM versions, under different KVM
  versions, export regulation) - we have _a lot_ of feature variability.
- We still have certain features that are not published but might be
  implemented/introduced in the future. As they are a theoretical part
  of a CPU already, we have to find a way to model these future changes.
- We still have certain features that are already published, but not
  implemented. Implementation might be added in the future in KVM.
- We heavily rely on KVM to tell us which features it can actually
  virtualize - user space queries like "STFL(e)" give no guarantees.
- Certain "subfeatures" were introduced in one run. In practice, they are
  always around, but in theory, subfeatures could be dropped in the future.
- Just because two CPU models have the same features doesn't mean they
  are equal - some internal numbers might be different. E.g. we won't allow
  running a z13 under a zBC12 just by turning off features.
- We cannot blindly enable all possible features for a CPU generation,
  the IBC "Instruction Blocking Control" in KVM will try to block
  instructions introduced with certain features. So a CPU generation always
  has some maximum feature set that is guaranteed to work.

It all boils down to a specific released CPU to have.
a) A minimum feature set that we expect it to be have on every hypervisor.
b) A variable part that depends on the hypervisor and that could be
   extended in the future (adding not yet implemented features) that we
   always want to enable later on.
c) A variable part that we want to enable only if requested - nested
   virtualization ("vsie") and assists are one example.

As we want our users to always make use of most features, e.g. when using
the "z13" CPU model, we will have to update our CPU models between QEMU
releases, to include the latest feature additions we implemented/unlocked.
We're badically trying to be able at one point in the future to really look
like a maximum "z13" CPU in QEMU. However, that means that a "z13" CPU
looks differently on different QEMU versions. We will make sure using
compatibility machines, that migration stays safe.

However, if the feature set of a CPU model is bound to a compatibility
machine, how can it be of any help when used without a compatibility
machine? E.g. when indicating the host CPU model in "virsh capabilities",
simply saying "z13" is not really clear. Also when baselining and
comparing CPU models, we usually have no compatibility machine "context"
at hand. For this reason we introduce "static" CPU models, which will
never change, so everybody knows without a compatibility machine, what we
are talking about.

CPU definitions/models can be categorized:
1. migratable: all features _can_ be identified + properly migrated.
--> With migratable=off on x86, "host" cannot be migrated properly.  
2. migration-safe: in combination with a QEMU machine, migration will work.
--> No CPU features are lost/added during migration  
3. static: not bound to a QEMU machine - featureset will never* change.
--> Everybody knows which features are part of it, not affected by a compat  
machine. "virsh capabilities" can show this as "host" model.
*except for trivial bugfixes, especially on very old models nobody used in
production. Should be avoided.

We are currently planning to also support a "migratable=off" on s390x
for the "-cpu host" model, in order to enable all features, including not
recognized ones.

So we have on s390x:
a) A static CPU model for each released CPU that will never change and
   maps to the minimum feature set we expect to be around on all
   hypervisors. e.g. "z13-base" or "z10EC.2-base".
b) A "default" CPU model for each released CPU, that can change between
   QEMU versions and that will always include the features we expect to
   be around in our currently supported environments and will contain only
   features we expect to be stable. E.g. nested virtualization will not be