[00/10] s390x: new guest features

Message ID	20190418113110.160664-1-borntraeger@de.ibm.com (mailing list archive)
Headers	show Return-Path: <qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org> Gateway: Authorized Use Only! Violators will be prosecuted for <qemu-devel@nongnu.org> from <borntraeger@de.ibm.com>; Thu, 18 Apr 2019 12:31:16 +0100 Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Thu, 18 Apr 2019 12:31:13 +0100 From: Christian Borntraeger <borntraeger@de.ibm.com> To: Cornelia Huck <cohuck@redhat.com> Date: Thu, 18 Apr 2019 13:31:00 +0200 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-Id: <20190418113110.160664-1-borntraeger@de.ibm.com> Subject: [Qemu-devel] [PATCH 00/10] s390x: new guest features Precedence: list Cc: "Jason J . Herne" <jjherne@linux.ibm.com>, Collin Walling <walling@linux.ibm.com>, David Hildenbrand <david@redhat.com>, qemu-devel <qemu-devel@nongnu.org>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, qemu-s390x <qemu-s390x@nongnu.org>, Richard Henderson <rth@twiddle.net> Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org>
Series	s390x: new guest features \| expand [00/10] s390x: new guest features [01/10] linux header sync [02/10] s390x/cpumodel: remove CSSKE from base model [03/10] s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3 [04/10] s390x/cpumodel: msa9 facility [05/10] s390x/cpumodel: vector enhancements [06/10] s390x/cpumodel: enhanced sort facility [07/10] s390x/cpumodel: deflate [08/10] s390x/cpumodel: add gen15 defintions [09/10] s390x/cpumodel: wire up 8561 and 8562 as gen15 machines [10/10] s390x/cpumodel: do not claim csske for expanded models in qmp

Christian Borntraeger April 18, 2019, 11:31 a.m. UTC

Adding generation 15.

Some interesting aspects:
- conditional SSKE and bpb are deprecated. This patch set addresses that
  for csske.
- no name yet for gen15, I suggest to use the assigned numbers and
  provide an alias later on. (I have split out this into a separate
  patch)

Christian Borntraeger (10):
  linux header sync
  s390x/cpumodel: remove CSSKE from base model
  s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
  s390x/cpumodel: msa9 facility
  s390x/cpumodel: vector enhancements
  s390x/cpumodel: enhanced sort facility
  s390x/cpumodel: deflate
  s390x/cpumodel: add gen15 defintions
  s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
  s390x/cpumodel: do not claim csske for expanded models in qmp

 hw/s390x/s390-virtio-ccw.c      |  6 +++
 linux-headers/asm-s390/kvm.h    |  5 +-
 target/s390x/cpu_features.c     | 54 +++++++++++++++++++
 target/s390x/cpu_features.h     |  3 ++
 target/s390x/cpu_features_def.h | 49 +++++++++++++++++
 target/s390x/cpu_models.c       | 35 ++++++++++++
 target/s390x/cpu_models.h       |  1 +
 target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
 target/s390x/kvm.c              | 18 +++++++
 9 files changed, 263 insertions(+), 2 deletions(-)

no-reply@patchew.org April 18, 2019, 12:03 p.m. UTC | #1

Patchew URL: https://patchew.org/QEMU/20190418113110.160664-1-borntraeger@de.ibm.com/



Hi,

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

Message-id: 20190418113110.160664-1-borntraeger@de.ibm.com
Type: series
Subject: [Qemu-devel] [PATCH 00/10]  s390x: new guest features

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1555587766-985-1-git-send-email-mateja.marjanovic@rt-rk.com -> patchew/1555587766-985-1-git-send-email-mateja.marjanovic@rt-rk.com
 * [new tag]         patchew/20190418113110.160664-1-borntraeger@de.ibm.com -> patchew/20190418113110.160664-1-borntraeger@de.ibm.com
Switched to a new branch 'test'
c4c9ad7 s390x/cpumodel: do not claim csske for expanded models in qmp
76c71e3 s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
544a8a6 s390x/cpumodel: add gen15 defintions
9ad6b30 s390x/cpumodel: deflate
d77001b s390x/cpumodel: enhanced sort facility
5971793 s390x/cpumodel: vector enhancements
f5fadd1 s390x/cpumodel: msa9 facility
06fb20b s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
54ab59d s390x/cpumodel: remove CSSKE from base model
60aaa57 linux header sync

=== OUTPUT BEGIN ===
1/10 Checking commit 60aaa5790659 (linux header sync)
2/10 Checking commit 54ab59dbdbd8 (s390x/cpumodel: remove CSSKE from base model)
WARNING: line over 80 characters
#55: FILE: target/s390x/cpu_models.c:113:
+        set_bit(S390_FEAT_CONDITIONAL_SSKE, (unsigned long *)&def->default_feat);

total: 0 errors, 1 warnings, 49 lines checked

Patch 2/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit 06fb20b97263 (s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3)
ERROR: line over 90 characters
#21: FILE: target/s390x/cpu_features.c:86:
+    FEAT_INIT("minste3", S390_FEAT_TYPE_STFL, 61, "Miscellaneous-Instruction-Extensions Facility 3"),

total: 1 errors, 0 warnings, 14 lines checked

Patch 3/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/10 Checking commit f5fadd1a9543 (s390x/cpumodel: msa9 facility)
ERROR: line over 90 characters
#22: FILE: target/s390x/cpu_features.c:111:
+    FEAT_INIT("msa9-base", S390_FEAT_TYPE_STFL, 155, "Message-security-assist-extension-9 facility (excluding subfunctions)"),

WARNING: line over 80 characters
#30: FILE: target/s390x/cpu_features.c:246:
+    FEAT_INIT("pckmo-ecc-p256", S390_FEAT_TYPE_PCKMO,32, "PCKMO Encrypt-ECC-P256-Key"),

ERROR: space required after that ',' (ctx:VxV)
#30: FILE: target/s390x/cpu_features.c:246:
+    FEAT_INIT("pckmo-ecc-p256", S390_FEAT_TYPE_PCKMO,32, "PCKMO Encrypt-ECC-P256-Key"),
                                                     ^

WARNING: line over 80 characters
#31: FILE: target/s390x/cpu_features.c:247:
+    FEAT_INIT("pckmo-ecc-p384", S390_FEAT_TYPE_PCKMO,33, "PCKMO Encrypt-ECC-P384-Key"),

ERROR: space required after that ',' (ctx:VxV)
#31: FILE: target/s390x/cpu_features.c:247:
+    FEAT_INIT("pckmo-ecc-p384", S390_FEAT_TYPE_PCKMO,33, "PCKMO Encrypt-ECC-P384-Key"),
                                                     ^

WARNING: line over 80 characters
#32: FILE: target/s390x/cpu_features.c:248:
+    FEAT_INIT("pckmo-ecc-p521", S390_FEAT_TYPE_PCKMO,34, "PCKMO Encrypt-ECC-P521-Key"),

ERROR: space required after that ',' (ctx:VxV)
#32: FILE: target/s390x/cpu_features.c:248:
+    FEAT_INIT("pckmo-ecc-p521", S390_FEAT_TYPE_PCKMO,34, "PCKMO Encrypt-ECC-P521-Key"),
                                                     ^

ERROR: line over 90 characters
#33: FILE: target/s390x/cpu_features.c:249:
+    FEAT_INIT("pckmo-ecc-ed25519", S390_FEAT_TYPE_PCKMO,40 , "PCKMO Encrypt-ECC-Ed25519-Key"),

ERROR: space required after that ',' (ctx:VxV)
#33: FILE: target/s390x/cpu_features.c:249:
+    FEAT_INIT("pckmo-ecc-ed25519", S390_FEAT_TYPE_PCKMO,40 , "PCKMO Encrypt-ECC-Ed25519-Key"),
                                                        ^

WARNING: line over 80 characters
#34: FILE: target/s390x/cpu_features.c:250:
+    FEAT_INIT("pckmo-ecc-ed448", S390_FEAT_TYPE_PCKMO,41 , "PCKMO Encrypt-ECC-Ed448-Key"),

ERROR: space required after that ',' (ctx:VxV)
#34: FILE: target/s390x/cpu_features.c:250:
+    FEAT_INIT("pckmo-ecc-ed448", S390_FEAT_TYPE_PCKMO,41 , "PCKMO Encrypt-ECC-Ed448-Key"),
                                                      ^

WARNING: line over 80 characters
#42: FILE: target/s390x/cpu_features.c:307:
+    FEAT_INIT("pcc-scalar-mult-p256", S390_FEAT_TYPE_PCC, 64, "PCC Scalar-Multiply-P256"),

WARNING: line over 80 characters
#43: FILE: target/s390x/cpu_features.c:308:
+    FEAT_INIT("pcc-scalar-mult-p384", S390_FEAT_TYPE_PCC, 65, "PCC Scalar-Multiply-P384"),

WARNING: line over 80 characters
#44: FILE: target/s390x/cpu_features.c:309:
+    FEAT_INIT("pcc-scalar-mult-p521", S390_FEAT_TYPE_PCC, 66, "PCC Scalar-Multiply-P521"),

ERROR: line over 90 characters
#45: FILE: target/s390x/cpu_features.c:310:
+    FEAT_INIT("pcc-scalar-mult-ed25519", S390_FEAT_TYPE_PCC, 72, "PCC Scalar-Multiply-Ed25519"),

ERROR: line over 90 characters
#46: FILE: target/s390x/cpu_features.c:311:
+    FEAT_INIT("pcc-scalar-mult-ed448", S390_FEAT_TYPE_PCC, 73, "PCC Scalar-Multiply-Ed448"),

ERROR: line over 90 characters
#47: FILE: target/s390x/cpu_features.c:312:
+    FEAT_INIT("pcc-scalar-mult-x25519", S390_FEAT_TYPE_PCC, 80, "PCC Scalar-Multiply-X25519"),

WARNING: line over 80 characters
#48: FILE: target/s390x/cpu_features.c:313:
+    FEAT_INIT("pcc-scalar-mult-x448", S390_FEAT_TYPE_PCC, 81, "PCC Scalar-Multiply-X448"),

WARNING: line over 80 characters
#57: FILE: target/s390x/cpu_features.c:326:
+    FEAT_INIT("kdsa-ecdsa-verify-p256", S390_FEAT_TYPE_KDSA, 1, "KDSA ECDSA-Verify-P256"),

WARNING: line over 80 characters
#58: FILE: target/s390x/cpu_features.c:327:
+    FEAT_INIT("kdsa-ecdsa-verify-p384", S390_FEAT_TYPE_KDSA, 2, "KDSA ECDSA-Verify-P384"),

WARNING: line over 80 characters
#59: FILE: target/s390x/cpu_features.c:328:
+    FEAT_INIT("kdsa-ecdsa-verify-p521", S390_FEAT_TYPE_KDSA, 3, "KDSA ECDSA-Verify-P521"),

WARNING: line over 80 characters
#60: FILE: target/s390x/cpu_features.c:329:
+    FEAT_INIT("kdsa-ecdsa-sign-p256", S390_FEAT_TYPE_KDSA, 9, "KDSA ECDSA-Sign-P256"),

WARNING: line over 80 characters
#61: FILE: target/s390x/cpu_features.c:330:
+    FEAT_INIT("kdsa-ecdsa-sign-p384", S390_FEAT_TYPE_KDSA, 10, "KDSA ECDSA-Sign-P384"),

WARNING: line over 80 characters
#62: FILE: target/s390x/cpu_features.c:331:
+    FEAT_INIT("kdsa-ecdsa-sign-p521", S390_FEAT_TYPE_KDSA, 11, "KDSA ECDSA-Sign-P521"),

ERROR: line over 90 characters
#63: FILE: target/s390x/cpu_features.c:332:
+    FEAT_INIT("kdsa-eecdsa-sign-p256", S390_FEAT_TYPE_KDSA, 17, "KDSA Encrypted-ECDSA-Sign-P256"),

ERROR: line over 90 characters
#64: FILE: target/s390x/cpu_features.c:333:
+    FEAT_INIT("kdsa-eecdsa-sign-p384", S390_FEAT_TYPE_KDSA, 18, "KDSA Encrypted-ECDSA-Sign-P384"),

ERROR: line over 90 characters
#65: FILE: target/s390x/cpu_features.c:334:
+    FEAT_INIT("kdsa-eecdsa-sign-p521", S390_FEAT_TYPE_KDSA, 19, "KDSA Encrypted-ECDSA-Sign-P521"),

ERROR: line over 90 characters
#66: FILE: target/s390x/cpu_features.c:335:
+    FEAT_INIT("kdsa-eddsa-verify-ed25519", S390_FEAT_TYPE_KDSA, 32, "KDSA EdDSA-Verify-Ed25519"),

ERROR: line over 90 characters
#67: FILE: target/s390x/cpu_features.c:336:
+    FEAT_INIT("kdsa-eddsa-verify-ed448", S390_FEAT_TYPE_KDSA, 36, "KDSA EdDSA-Verify-Ed448"),

ERROR: line over 90 characters
#68: FILE: target/s390x/cpu_features.c:337:
+    FEAT_INIT("kdsa-eddsa-sign-ed25519", S390_FEAT_TYPE_KDSA, 40, "KDSA EdDSA-Sign-Ed25519"),

WARNING: line over 80 characters
#69: FILE: target/s390x/cpu_features.c:338:
+    FEAT_INIT("kdsa-eddsa-sign-ed448", S390_FEAT_TYPE_KDSA, 44, "KDSA EdDSA-Sign-Ed448"),

ERROR: line over 90 characters
#70: FILE: target/s390x/cpu_features.c:339:
+    FEAT_INIT("kdsa-eeddsa-sign-ed25519", S390_FEAT_TYPE_KDSA, 48, "KDSA Encrypted-EdDSA-Sign-Ed25519"),

ERROR: line over 90 characters
#71: FILE: target/s390x/cpu_features.c:340:
+    FEAT_INIT("kdsa-eeddsa-sign-ed448", S390_FEAT_TYPE_KDSA, 52, "KDSA Encrypted-EdDSA-Sign-Ed448"),

WARNING: line over 80 characters
#87: FILE: target/s390x/cpu_features.c:499:
+    FEAT_GROUP_INIT("msa9", MSA_EXT_9, "Message-security-assist-extension 9 facility"),

ERROR: line over 90 characters
#88: FILE: target/s390x/cpu_features.c:500:
+    FEAT_GROUP_INIT("msa9_pckmo", MSA_EXT_9_PCKMO, "Message-security-assist-extension 9 PCKMO subfunctions"),

ERROR: Macros with complex values should be enclosed in parenthesis
#187: FILE: target/s390x/gen-features.c:216:
+#define S390_FEAT_GROUP_MSA_EXT_9 \
+    S390_FEAT_MSA_EXT_9,       \
+    S390_FEAT_ECDSA_VERIFY_P256, \
+    S390_FEAT_ECDSA_VERIFY_P384, \
+    S390_FEAT_ECDSA_VERIFY_P512, \
+    S390_FEAT_ECDSA_SIGN_P256, \
+    S390_FEAT_ECDSA_SIGN_P384, \
+    S390_FEAT_ECDSA_SIGN_P512, \
+    S390_FEAT_EECDSA_SIGN_P256, \
+    S390_FEAT_EECDSA_SIGN_P384, \
+    S390_FEAT_EECDSA_SIGN_P512, \
+    S390_FEAT_EDDSA_VERIFY_ED25519, \
+    S390_FEAT_EDDSA_VERIFY_ED448, \
+    S390_FEAT_EDDSA_SIGN_ED25519, \
+    S390_FEAT_EDDSA_SIGN_ED448, \
+    S390_FEAT_EEDDSA_SIGN_ED25519, \
+    S390_FEAT_EEDDSA_SIGN_ED448, \
+    S390_FEAT_PCC_SCALAR_MULT_P256, \
+    S390_FEAT_PCC_SCALAR_MULT_P384, \
+    S390_FEAT_PCC_SCALAR_MULT_P512, \
+    S390_FEAT_PCC_SCALAR_MULT_ED25519, \
+    S390_FEAT_PCC_SCALAR_MULT_ED448, \
+    S390_FEAT_PCC_SCALAR_MULT_X25519, \
+    S390_FEAT_PCC_SCALAR_MULT_X448

ERROR: code indent should never use tabs
#188: FILE: target/s390x/gen-features.c:217:
+    S390_FEAT_MSA_EXT_9,^I\$

ERROR: Macros with complex values should be enclosed in parenthesis
#212: FILE: target/s390x/gen-features.c:241:
+#define S390_FEAT_GROUP_MSA_EXT_9_PCKMO \
+    S390_FEAT_PCKMO_ECC_P256, \
+    S390_FEAT_PCKMO_ECC_P384, \
+    S390_FEAT_PCKMO_ECC_P521, \
+    S390_FEAT_PCKMO_ECC_ED25519, \
+    S390_FEAT_PCKMO_ECC_ED448

total: 22 errors, 16 warnings, 215 lines checked

Patch 4/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/10 Checking commit 59717937a65d (s390x/cpumodel: vector enhancements)
WARNING: line over 80 characters
#20: FILE: target/s390x/cpu_features.c:111:
+    FEAT_INIT("vxeh2", S390_FEAT_TYPE_STFL, 148, "Vector Enhancements facility 2"),

WARNING: line over 80 characters
#21: FILE: target/s390x/cpu_features.c:112:
+    FEAT_INIT("vxbeh", S390_FEAT_TYPE_STFL, 152, "Vector BCD enhancements facility 1"),

total: 0 errors, 2 warnings, 16 lines checked

Patch 5/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/10 Checking commit d77001b8807f (s390x/cpumodel: enhanced sort facility)
ERROR: line over 90 characters
#20: FILE: target/s390x/cpu_features.c:112:
+    FEAT_INIT("esort-base", S390_FEAT_TYPE_STFL, 150, "Enhanced-sort facility (excluding subfunctions)"),

WARNING: line over 80 characters
#33: FILE: target/s390x/cpu_features.c:349:
+    FEAT_INIT("sortl-f0", S390_FEAT_TYPE_SORTL, 192, "SORTL format 0 parameter-block"),

ERROR: Macros with complex values should be enclosed in parenthesis
#107: FILE: target/s390x/gen-features.c:248:
+#define S390_FEAT_GROUP_ENH_SORT \
+    S390_FEAT_ESORT_BASE, \
+    S390_FEAT_SORTL_SFLR, \
+    S390_FEAT_SORTL_SVLR, \
+    S390_FEAT_SORTL_32, \
+    S390_FEAT_SORTL_128, \
+    S390_FEAT_SORTL_F0

total: 2 errors, 1 warnings, 117 lines checked

Patch 6/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/10 Checking commit 9ad6b308af81 (s390x/cpumodel: deflate)
ERROR: line over 90 characters
#20: FILE: target/s390x/cpu_features.c:113:
+    FEAT_INIT("deflate-base", S390_FEAT_TYPE_STFL, 151, "Deflate-conversion facility (excluding subfunctions)"),

WARNING: line over 80 characters
#32: FILE: target/s390x/cpu_features.c:355:
+    FEAT_INIT("dfltcc-f0", S390_FEAT_TYPE_DFLTCC, 192, "DFLTCC format 0 parameter-block"),

WARNING: line over 80 characters
#56: FILE: target/s390x/cpu_features.c:522:
+    FEAT_GROUP_INIT("deflate", DEFLATE_CONVERSION, "Deflate-conversion facility"),

ERROR: Macros with complex values should be enclosed in parenthesis
#105: FILE: target/s390x/gen-features.c:257:
+#define S390_FEAT_GROUP_DEFLATE_CONVERSION \
+    S390_FEAT_DEFLATE_BASE, \
+    S390_FEAT_DEFLATE_GHDT, \
+    S390_FEAT_DEFLATE_CMPR, \
+    S390_FEAT_DEFLATE_XPND, \
+    S390_FEAT_DEFLATE_F0

total: 2 errors, 2 warnings, 113 lines checked

Patch 7/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

8/10 Checking commit 544a8a6ed245 (s390x/cpumodel: add gen15 defintions)
9/10 Checking commit 76c71e310434 (s390x/cpumodel: wire up 8561 and 8562 as gen15 machines)
10/10 Checking commit c4c9ad736e1a (s390x/cpumodel: do not claim csske for expanded models in qmp)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190418113110.160664-1-borntraeger@de.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

David Hildenbrand April 23, 2019, 12:11 p.m. UTC | #2

On 18.04.19 13:31, Christian Borntraeger wrote:
> Adding generation 15.
> 
> Some interesting aspects:
> - conditional SSKE and bpb are deprecated. This patch set addresses that
>   for csske.
> - no name yet for gen15, I suggest to use the assigned numbers and
>   provide an alias later on. (I have split out this into a separate
>   patch)
> 
> Christian Borntraeger (10):
>   linux header sync
>   s390x/cpumodel: remove CSSKE from base model
>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>   s390x/cpumodel: msa9 facility
>   s390x/cpumodel: vector enhancements
>   s390x/cpumodel: enhanced sort facility
>   s390x/cpumodel: deflate
>   s390x/cpumodel: add gen15 defintions
>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>   s390x/cpumodel: do not claim csske for expanded models in qmp
> 
>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>  linux-headers/asm-s390/kvm.h    |  5 +-
>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>  target/s390x/cpu_features.h     |  3 ++
>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>  target/s390x/cpu_models.c       | 35 ++++++++++++
>  target/s390x/cpu_models.h       |  1 +
>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>  target/s390x/kvm.c              | 18 +++++++
>  9 files changed, 263 insertions(+), 2 deletions(-)
> 

I guess to handle deprecation of CSSKE:

1. Remove it from the base + default model of the gen15, keep it in the
max model. This is completely done in target/s390x/gen-features.c.
Existing base models are not modified.

2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
will work. We can backport this to distros/stable.

CPU model expansion:

cpu_info_from_model() should already properly be based on the base
features. "gen15" vs. "gen15,csske=on" should be automatically generated
when expanding.

CPU model baseline:

s390_find_cpu_def() should make sure that CSSKE is basically ignored
when determining maximum model, however it will properly be indicated if
both models had the feature.

CPU model comparison:

Should work as expected. Availability of CSSKE will be considered when
calculating the result.

gen14,csske=on and gen15,csske=off will result in
CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.

gen14,csske=off and gen15,csske=off should result in
CPU_MODEL_COMPARE_RESULT_SUBSET

gen14,csske=on and gen15,csske=on should result in
CPU_MODEL_COMPARE_RESULT_SUBSET

Forward migration:

Now, the only issue is when csske is actually turned of in future
machines. We would e.g. have

gen15,csske=on and gen16,csske=off

While baselining will work correctly (gen15,csske=off), forward
migration is broken (comparison will properly indicate
CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
out features. Same applies to BPB.

Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
expanded models in qmp" tried to address this, however I am not really
happy with this approach. We should not play such tricks when expanding
the host model. "-cpu host" and "-cpu $expanded_host" would be
different, that is really confusing, especially think about baselining
or comparing against "host", where your patch does not apply..

Whenever users would specify "-cpu gen15", everything would be fine in
regards to forward migration. Maybe clients really have to be taught
about upcoming forward migration issues. "Don't use the host model, make
sure bpa and csske are disabled".

Christian Borntraeger April 24, 2019, 8:40 a.m. UTC | #3

On 23.04.19 14:11, David Hildenbrand wrote:
> On 18.04.19 13:31, Christian Borntraeger wrote:
>> Adding generation 15.
>>
>> Some interesting aspects:
>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>   for csske.
>> - no name yet for gen15, I suggest to use the assigned numbers and
>>   provide an alias later on. (I have split out this into a separate
>>   patch)
>>
>> Christian Borntraeger (10):
>>   linux header sync
>>   s390x/cpumodel: remove CSSKE from base model
>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>   s390x/cpumodel: msa9 facility
>>   s390x/cpumodel: vector enhancements
>>   s390x/cpumodel: enhanced sort facility
>>   s390x/cpumodel: deflate
>>   s390x/cpumodel: add gen15 defintions
>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>
>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>  target/s390x/cpu_features.h     |  3 ++
>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>  target/s390x/cpu_models.h       |  1 +
>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>  target/s390x/kvm.c              | 18 +++++++
>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>
> 
> I guess to handle deprecation of CSSKE:
> 
> 1. Remove it from the base + default model of the gen15, keep it in the
> max model. This is completely done in target/s390x/gen-features.c.
> Existing base models are not modified.
> 
> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
> will work. We can backport this to distros/stable.

Yes, I have already implemented that, still doing some testing and polishinh.
> 
> 
> CPU model expansion:
> 
> cpu_info_from_model() should already properly be based on the base
> features. "gen15" vs. "gen15,csske=on" should be automatically generated
> when expanding.
> 
> CPU model baseline:
> 
> s390_find_cpu_def() should make sure that CSSKE is basically ignored
> when determining maximum model, however it will properly be indicated if
> both models had the feature.
> 
> CPU model comparison:
> 
> Should work as expected. Availability of CSSKE will be considered when
> calculating the result.
> 
> gen14,csske=on and gen15,csske=off will result in
> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
> 
> gen14,csske=off and gen15,csske=off should result in
> CPU_MODEL_COMPARE_RESULT_SUBSET
> 
> gen14,csske=on and gen15,csske=on should result in
> CPU_MODEL_COMPARE_RESULT_SUBSET
> 
> Forward migration:
> 
> Now, the only issue is when csske is actually turned of in future
> machines. We would e.g. have
> 
> gen15,csske=on and gen16,csske=off
> 
> While baselining will work correctly (gen15,csske=off), forward
> migration is broken (comparison will properly indicate
> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
> out features. Same applies to BPB.
> 
> 
> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
> expanded models in qmp" tried to address this, however I am not really
> happy with this approach. We should not play such tricks when expanding
> the host model. "-cpu host" and "-cpu $expanded_host" would be
> different,

We discussed this some time ago and I think we agreed that for host passthrough
it is ok to be different that host-model (e.g. passing through the cpuid, passing
through all non-hypervisor managed features etc).


> that is really confusing, especially think about baselining
> or comparing against "host", where your patch does not apply..
> 
> 
> Whenever users would specify "-cpu gen15", everything would be fine in
> regards to forward migration. Maybe clients really have to be taught
> about upcoming forward migration issues. "Don't use the host model, make
> sure bpa and csske are disabled".


the thing is, I think the host-model is actually the preferred setting so I
would like to have this forward-migrateable as well. I will focus on patches
1-9 first and lets have a side discussion on patch 10.

David Hildenbrand April 24, 2019, 9:03 a.m. UTC | #4

On 24.04.19 10:40, Christian Borntraeger wrote:
> 
> 
> On 23.04.19 14:11, David Hildenbrand wrote:
>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>> Adding generation 15.
>>>
>>> Some interesting aspects:
>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>   for csske.
>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>   provide an alias later on. (I have split out this into a separate
>>>   patch)
>>>
>>> Christian Borntraeger (10):
>>>   linux header sync
>>>   s390x/cpumodel: remove CSSKE from base model
>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>   s390x/cpumodel: msa9 facility
>>>   s390x/cpumodel: vector enhancements
>>>   s390x/cpumodel: enhanced sort facility
>>>   s390x/cpumodel: deflate
>>>   s390x/cpumodel: add gen15 defintions
>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>
>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>  target/s390x/cpu_features.h     |  3 ++
>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>  target/s390x/cpu_models.h       |  1 +
>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>  target/s390x/kvm.c              | 18 +++++++
>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>
>>
>> I guess to handle deprecation of CSSKE:
>>
>> 1. Remove it from the base + default model of the gen15, keep it in the
>> max model. This is completely done in target/s390x/gen-features.c.
>> Existing base models are not modified.
>>
>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>> will work. We can backport this to distros/stable.
> 
> Yes, I have already implemented that, still doing some testing and polishinh.
>>
>>
>> CPU model expansion:
>>
>> cpu_info_from_model() should already properly be based on the base
>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>> when expanding.
>>
>> CPU model baseline:
>>
>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>> when determining maximum model, however it will properly be indicated if
>> both models had the feature.
>>
>> CPU model comparison:
>>
>> Should work as expected. Availability of CSSKE will be considered when
>> calculating the result.
>>
>> gen14,csske=on and gen15,csske=off will result in
>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>
>> gen14,csske=off and gen15,csske=off should result in
>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>
>> gen14,csske=on and gen15,csske=on should result in
>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>
>> Forward migration:
>>
>> Now, the only issue is when csske is actually turned of in future
>> machines. We would e.g. have
>>
>> gen15,csske=on and gen16,csske=off
>>
>> While baselining will work correctly (gen15,csske=off), forward
>> migration is broken (comparison will properly indicate
>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>> out features. Same applies to BPB.
>>
>>
>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>> expanded models in qmp" tried to address this, however I am not really
>> happy with this approach. We should not play such tricks when expanding
>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>> different,
> 
> We discussed this some time ago and I think we agreed that for host passthrough
> it is ok to be different that host-model (e.g. passing through the cpuid, passing
> through all non-hypervisor managed features etc).

I remember the plan was to use the "max" model to do such stuff. E.g.
-cpu max / no -cpu

Versus
-cpu host

We can have features in "host" we don't have in "max". But "-cpu host"
and it's expansion should look 100% the same.

> 
> 
>> that is really confusing, especially think about baselining
>> or comparing against "host", where your patch does not apply..
>>
>>
>> Whenever users would specify "-cpu gen15", everything would be fine in
>> regards to forward migration. Maybe clients really have to be taught
>> about upcoming forward migration issues. "Don't use the host model, make
>> sure bpa and csske are disabled".
> 
> 
> the thing is, I think the host-model is actually the preferred setting so I
> would like to have this forward-migrateable as well. I will focus on patches
> 1-9 first and lets have a side discussion on patch 10.
> 

Makes sense. This is also something that can be fixed on higher layers
(disable cpu feature for gen15 to prepare for migration).

Daniel P. Berrangé April 24, 2019, 9:30 a.m. UTC | #5

On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
> On 24.04.19 10:40, Christian Borntraeger wrote:
> > 
> > 
> > On 23.04.19 14:11, David Hildenbrand wrote:
> >> On 18.04.19 13:31, Christian Borntraeger wrote:
> >>> Adding generation 15.
> >>>
> >>> Some interesting aspects:
> >>> - conditional SSKE and bpb are deprecated. This patch set addresses that
> >>>   for csske.
> >>> - no name yet for gen15, I suggest to use the assigned numbers and
> >>>   provide an alias later on. (I have split out this into a separate
> >>>   patch)
> >>>
> >>> Christian Borntraeger (10):
> >>>   linux header sync
> >>>   s390x/cpumodel: remove CSSKE from base model
> >>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
> >>>   s390x/cpumodel: msa9 facility
> >>>   s390x/cpumodel: vector enhancements
> >>>   s390x/cpumodel: enhanced sort facility
> >>>   s390x/cpumodel: deflate
> >>>   s390x/cpumodel: add gen15 defintions
> >>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
> >>>   s390x/cpumodel: do not claim csske for expanded models in qmp
> >>>
> >>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
> >>>  linux-headers/asm-s390/kvm.h    |  5 +-
> >>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
> >>>  target/s390x/cpu_features.h     |  3 ++
> >>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
> >>>  target/s390x/cpu_models.c       | 35 ++++++++++++
> >>>  target/s390x/cpu_models.h       |  1 +
> >>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
> >>>  target/s390x/kvm.c              | 18 +++++++
> >>>  9 files changed, 263 insertions(+), 2 deletions(-)
> >>>
> >>
> >> I guess to handle deprecation of CSSKE:
> >>
> >> 1. Remove it from the base + default model of the gen15, keep it in the
> >> max model. This is completely done in target/s390x/gen-features.c.
> >> Existing base models are not modified.
> >>
> >> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
> >> will work. We can backport this to distros/stable.
> > 
> > Yes, I have already implemented that, still doing some testing and polishinh.
> >>
> >>
> >> CPU model expansion:
> >>
> >> cpu_info_from_model() should already properly be based on the base
> >> features. "gen15" vs. "gen15,csske=on" should be automatically generated
> >> when expanding.
> >>
> >> CPU model baseline:
> >>
> >> s390_find_cpu_def() should make sure that CSSKE is basically ignored
> >> when determining maximum model, however it will properly be indicated if
> >> both models had the feature.
> >>
> >> CPU model comparison:
> >>
> >> Should work as expected. Availability of CSSKE will be considered when
> >> calculating the result.
> >>
> >> gen14,csske=on and gen15,csske=off will result in
> >> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
> >>
> >> gen14,csske=off and gen15,csske=off should result in
> >> CPU_MODEL_COMPARE_RESULT_SUBSET
> >>
> >> gen14,csske=on and gen15,csske=on should result in
> >> CPU_MODEL_COMPARE_RESULT_SUBSET
> >>
> >> Forward migration:
> >>
> >> Now, the only issue is when csske is actually turned of in future
> >> machines. We would e.g. have
> >>
> >> gen15,csske=on and gen16,csske=off
> >>
> >> While baselining will work correctly (gen15,csske=off), forward
> >> migration is broken (comparison will properly indicate
> >> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
> >> out features. Same applies to BPB.
> >>
> >>
> >> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
> >> expanded models in qmp" tried to address this, however I am not really
> >> happy with this approach. We should not play such tricks when expanding
> >> the host model. "-cpu host" and "-cpu $expanded_host" would be
> >> different,
> > 
> > We discussed this some time ago and I think we agreed that for host passthrough
> > it is ok to be different that host-model (e.g. passing through the cpuid, passing
> > through all non-hypervisor managed features etc).
> 
> I remember the plan was to use the "max" model to do such stuff. E.g.
> -cpu max / no -cpu
> 
> Versus
> -cpu host
> 
> We can have features in "host" we don't have in "max". But "-cpu host"
> and it's expansion should look 100% the same.

I don't think that's the intended semantics of "max" vs "host".

The "max" CPU model is supposed to enable all features that are possible to
enable.

For KVM, thus "max" should be identical to "host".

For TCG, "max" should be everything that QEMU currently knows how to emulate.

Essentially think of "max" as a better name for "host", since "host" as
a name concept didn't make sense for TCG.

Regards,
Daniel

David Hildenbrand April 24, 2019, 9:35 a.m. UTC | #6

On 24.04.19 11:30, Daniel P. Berrangé wrote:
> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
>> On 24.04.19 10:40, Christian Borntraeger wrote:
>>>
>>>
>>> On 23.04.19 14:11, David Hildenbrand wrote:
>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>>>> Adding generation 15.
>>>>>
>>>>> Some interesting aspects:
>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>>>   for csske.
>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>>>   provide an alias later on. (I have split out this into a separate
>>>>>   patch)
>>>>>
>>>>> Christian Borntraeger (10):
>>>>>   linux header sync
>>>>>   s390x/cpumodel: remove CSSKE from base model
>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>>>   s390x/cpumodel: msa9 facility
>>>>>   s390x/cpumodel: vector enhancements
>>>>>   s390x/cpumodel: enhanced sort facility
>>>>>   s390x/cpumodel: deflate
>>>>>   s390x/cpumodel: add gen15 defintions
>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>>>
>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>>>  target/s390x/cpu_features.h     |  3 ++
>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>>>  target/s390x/cpu_models.h       |  1 +
>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>>>  target/s390x/kvm.c              | 18 +++++++
>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>>>
>>>>
>>>> I guess to handle deprecation of CSSKE:
>>>>
>>>> 1. Remove it from the base + default model of the gen15, keep it in the
>>>> max model. This is completely done in target/s390x/gen-features.c.
>>>> Existing base models are not modified.
>>>>
>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>>>> will work. We can backport this to distros/stable.
>>>
>>> Yes, I have already implemented that, still doing some testing and polishinh.
>>>>
>>>>
>>>> CPU model expansion:
>>>>
>>>> cpu_info_from_model() should already properly be based on the base
>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>>>> when expanding.
>>>>
>>>> CPU model baseline:
>>>>
>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>>>> when determining maximum model, however it will properly be indicated if
>>>> both models had the feature.
>>>>
>>>> CPU model comparison:
>>>>
>>>> Should work as expected. Availability of CSSKE will be considered when
>>>> calculating the result.
>>>>
>>>> gen14,csske=on and gen15,csske=off will result in
>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>>>
>>>> gen14,csske=off and gen15,csske=off should result in
>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>
>>>> gen14,csske=on and gen15,csske=on should result in
>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>
>>>> Forward migration:
>>>>
>>>> Now, the only issue is when csske is actually turned of in future
>>>> machines. We would e.g. have
>>>>
>>>> gen15,csske=on and gen16,csske=off
>>>>
>>>> While baselining will work correctly (gen15,csske=off), forward
>>>> migration is broken (comparison will properly indicate
>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>>>> out features. Same applies to BPB.
>>>>
>>>>
>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>>>> expanded models in qmp" tried to address this, however I am not really
>>>> happy with this approach. We should not play such tricks when expanding
>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>>>> different,
>>>
>>> We discussed this some time ago and I think we agreed that for host passthrough
>>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
>>> through all non-hypervisor managed features etc).
>>
>> I remember the plan was to use the "max" model to do such stuff. E.g.
>> -cpu max / no -cpu
>>
>> Versus
>> -cpu host
>>
>> We can have features in "host" we don't have in "max". But "-cpu host"
>> and it's expansion should look 100% the same.
> 
> I don't think that's the intended semantics of "max" vs "host".
> 
> The "max" CPU model is supposed to enable all features that are possible to
> enable.
> 
> For KVM, thus "max" should be identical to "host".

There once was a mode used by x86-64 to simply pipe through cpuid
features that were not even supported. (I remember something like
passthorugh=true), do you remember if something like that still exists?

> 
> For TCG, "max" should be everything that QEMU currently knows how to emulate.

Yes, and on s390x. "max" contains more features than "qemu".

> 
> Essentially think of "max" as a better name for "host", since "host" as
> a name concept didn't make sense for TCG.

I agree. The main question is, is it acceptable that

"-cpu host" and "-cpu $expanded_host" produce different results, after
expanding "host" via query-cpu-model-expansion?

Daniel P. Berrangé April 24, 2019, 9:41 a.m. UTC | #7

On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
> On 24.04.19 11:30, Daniel P. Berrangé wrote:
> > On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
> >> On 24.04.19 10:40, Christian Borntraeger wrote:
> >>>
> >>>
> >>> On 23.04.19 14:11, David Hildenbrand wrote:
> >>>> On 18.04.19 13:31, Christian Borntraeger wrote:
> >>>>> Adding generation 15.
> >>>>>
> >>>>> Some interesting aspects:
> >>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
> >>>>>   for csske.
> >>>>> - no name yet for gen15, I suggest to use the assigned numbers and
> >>>>>   provide an alias later on. (I have split out this into a separate
> >>>>>   patch)
> >>>>>
> >>>>> Christian Borntraeger (10):
> >>>>>   linux header sync
> >>>>>   s390x/cpumodel: remove CSSKE from base model
> >>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
> >>>>>   s390x/cpumodel: msa9 facility
> >>>>>   s390x/cpumodel: vector enhancements
> >>>>>   s390x/cpumodel: enhanced sort facility
> >>>>>   s390x/cpumodel: deflate
> >>>>>   s390x/cpumodel: add gen15 defintions
> >>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
> >>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
> >>>>>
> >>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
> >>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
> >>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
> >>>>>  target/s390x/cpu_features.h     |  3 ++
> >>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
> >>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
> >>>>>  target/s390x/cpu_models.h       |  1 +
> >>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
> >>>>>  target/s390x/kvm.c              | 18 +++++++
> >>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
> >>>>>
> >>>>
> >>>> I guess to handle deprecation of CSSKE:
> >>>>
> >>>> 1. Remove it from the base + default model of the gen15, keep it in the
> >>>> max model. This is completely done in target/s390x/gen-features.c.
> >>>> Existing base models are not modified.
> >>>>
> >>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
> >>>> will work. We can backport this to distros/stable.
> >>>
> >>> Yes, I have already implemented that, still doing some testing and polishinh.
> >>>>
> >>>>
> >>>> CPU model expansion:
> >>>>
> >>>> cpu_info_from_model() should already properly be based on the base
> >>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
> >>>> when expanding.
> >>>>
> >>>> CPU model baseline:
> >>>>
> >>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
> >>>> when determining maximum model, however it will properly be indicated if
> >>>> both models had the feature.
> >>>>
> >>>> CPU model comparison:
> >>>>
> >>>> Should work as expected. Availability of CSSKE will be considered when
> >>>> calculating the result.
> >>>>
> >>>> gen14,csske=on and gen15,csske=off will result in
> >>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
> >>>>
> >>>> gen14,csske=off and gen15,csske=off should result in
> >>>> CPU_MODEL_COMPARE_RESULT_SUBSET
> >>>>
> >>>> gen14,csske=on and gen15,csske=on should result in
> >>>> CPU_MODEL_COMPARE_RESULT_SUBSET
> >>>>
> >>>> Forward migration:
> >>>>
> >>>> Now, the only issue is when csske is actually turned of in future
> >>>> machines. We would e.g. have
> >>>>
> >>>> gen15,csske=on and gen16,csske=off
> >>>>
> >>>> While baselining will work correctly (gen15,csske=off), forward
> >>>> migration is broken (comparison will properly indicate
> >>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
> >>>> out features. Same applies to BPB.
> >>>>
> >>>>
> >>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
> >>>> expanded models in qmp" tried to address this, however I am not really
> >>>> happy with this approach. We should not play such tricks when expanding
> >>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
> >>>> different,
> >>>
> >>> We discussed this some time ago and I think we agreed that for host passthrough
> >>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
> >>> through all non-hypervisor managed features etc).
> >>
> >> I remember the plan was to use the "max" model to do such stuff. E.g.
> >> -cpu max / no -cpu
> >>
> >> Versus
> >> -cpu host
> >>
> >> We can have features in "host" we don't have in "max". But "-cpu host"
> >> and it's expansion should look 100% the same.
> > 
> > I don't think that's the intended semantics of "max" vs "host".
> > 
> > The "max" CPU model is supposed to enable all features that are possible to
> > enable.
> > 
> > For KVM, thus "max" should be identical to "host".
> 
> There once was a mode used by x86-64 to simply pipe through cpuid
> features that were not even supported. (I remember something like
> passthorugh=true), do you remember if something like that still exists?

I don't recall such a feature existing even in the past !

> > For TCG, "max" should be everything that QEMU currently knows how to emulate.
> 
> Yes, and on s390x. "max" contains more features than "qemu".
> 
> > 
> > Essentially think of "max" as a better name for "host", since "host" as
> > a name concept didn't make sense for TCG.
> 
> I agree. The main question is, is it acceptable that

Hmm, maybe I misinterpreted when you wrote

      We can have features in "host" we don't have in "max"

I read that as meaning that "-cpu host" and "-cpu max" would be
different.

> "-cpu host" and "-cpu $expanded_host" produce different results, after
> expanding "host" via query-cpu-model-expansion?

That has always been the case on x86-64, since it is not possible to set
the level, xlevel, vendor, family, model & stepping properties via -cpu,
only the feature flags.

Regards,
Daniel

David Hildenbrand April 24, 2019, 9:56 a.m. UTC | #8

On 24.04.19 11:41, Daniel P. Berrangé wrote:
> On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
>> On 24.04.19 11:30, Daniel P. Berrangé wrote:
>>> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
>>>> On 24.04.19 10:40, Christian Borntraeger wrote:
>>>>>
>>>>>
>>>>> On 23.04.19 14:11, David Hildenbrand wrote:
>>>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>>>>>> Adding generation 15.
>>>>>>>
>>>>>>> Some interesting aspects:
>>>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>>>>>   for csske.
>>>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>>>>>   provide an alias later on. (I have split out this into a separate
>>>>>>>   patch)
>>>>>>>
>>>>>>> Christian Borntraeger (10):
>>>>>>>   linux header sync
>>>>>>>   s390x/cpumodel: remove CSSKE from base model
>>>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>>>>>   s390x/cpumodel: msa9 facility
>>>>>>>   s390x/cpumodel: vector enhancements
>>>>>>>   s390x/cpumodel: enhanced sort facility
>>>>>>>   s390x/cpumodel: deflate
>>>>>>>   s390x/cpumodel: add gen15 defintions
>>>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>>>>>
>>>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>>>>>  target/s390x/cpu_features.h     |  3 ++
>>>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>>>>>  target/s390x/cpu_models.h       |  1 +
>>>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>>>>>  target/s390x/kvm.c              | 18 +++++++
>>>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>
>>>>>> I guess to handle deprecation of CSSKE:
>>>>>>
>>>>>> 1. Remove it from the base + default model of the gen15, keep it in the
>>>>>> max model. This is completely done in target/s390x/gen-features.c.
>>>>>> Existing base models are not modified.
>>>>>>
>>>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>>>>>> will work. We can backport this to distros/stable.
>>>>>
>>>>> Yes, I have already implemented that, still doing some testing and polishinh.
>>>>>>
>>>>>>
>>>>>> CPU model expansion:
>>>>>>
>>>>>> cpu_info_from_model() should already properly be based on the base
>>>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>>>>>> when expanding.
>>>>>>
>>>>>> CPU model baseline:
>>>>>>
>>>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>>>>>> when determining maximum model, however it will properly be indicated if
>>>>>> both models had the feature.
>>>>>>
>>>>>> CPU model comparison:
>>>>>>
>>>>>> Should work as expected. Availability of CSSKE will be considered when
>>>>>> calculating the result.
>>>>>>
>>>>>> gen14,csske=on and gen15,csske=off will result in
>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>>>>>
>>>>>> gen14,csske=off and gen15,csske=off should result in
>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>
>>>>>> gen14,csske=on and gen15,csske=on should result in
>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>
>>>>>> Forward migration:
>>>>>>
>>>>>> Now, the only issue is when csske is actually turned of in future
>>>>>> machines. We would e.g. have
>>>>>>
>>>>>> gen15,csske=on and gen16,csske=off
>>>>>>
>>>>>> While baselining will work correctly (gen15,csske=off), forward
>>>>>> migration is broken (comparison will properly indicate
>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>>>>>> out features. Same applies to BPB.
>>>>>>
>>>>>>
>>>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>>>>>> expanded models in qmp" tried to address this, however I am not really
>>>>>> happy with this approach. We should not play such tricks when expanding
>>>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>>>>>> different,
>>>>>
>>>>> We discussed this some time ago and I think we agreed that for host passthrough
>>>>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
>>>>> through all non-hypervisor managed features etc).
>>>>
>>>> I remember the plan was to use the "max" model to do such stuff. E.g.
>>>> -cpu max / no -cpu
>>>>
>>>> Versus
>>>> -cpu host
>>>>
>>>> We can have features in "host" we don't have in "max". But "-cpu host"
>>>> and it's expansion should look 100% the same.
>>>
>>> I don't think that's the intended semantics of "max" vs "host".
>>>
>>> The "max" CPU model is supposed to enable all features that are possible to
>>> enable.
>>>
>>> For KVM, thus "max" should be identical to "host".
>>
>> There once was a mode used by x86-64 to simply pipe through cpuid
>> features that were not even supported. (I remember something like
>> passthorugh=true), do you remember if something like that still exists?
> 
> I don't recall such a feature existing even in the past !

Maybe my mind is tricking me, or maybe that has long been removed :)

> 
>>> For TCG, "max" should be everything that QEMU currently knows how to emulate.
>>
>> Yes, and on s390x. "max" contains more features than "qemu".
>>
>>>
>>> Essentially think of "max" as a better name for "host", since "host" as
>>> a name concept didn't make sense for TCG.
>>
>> I agree. The main question is, is it acceptable that
> 
> Hmm, maybe I misinterpreted when you wrote
> 
>       We can have features in "host" we don't have in "max"
> 
> I read that as meaning that "-cpu host" and "-cpu max" would be
> different.

No you didn't misinterpret it, I agreed after you spelled it out :)

> 
>> "-cpu host" and "-cpu $expanded_host" produce different results, after
>> expanding "host" via query-cpu-model-expansion?
> 
> That has always been the case on x86-64, since it is not possible to set
> the level, xlevel, vendor, family, model & stepping properties via -cpu,
> only the feature flags.

Fair enough, but the question is if we should mess with feature flags we
can indicate on that level.

It would mean that on a specific host e.g.

"-cpu gen15,csske=on" and "-cpu gen15,csske=off"

Would work. However, "host" model expansion would give us

"-cpu gen15,csske=off"

So trying to e.g. do a query-cpu-model-comparison or
query-cpu-model-baseline against "host" and against the expanded host
model will produce different results.

Libvirt could detect "-cpu gen14,csske=on" as not runnable on the host,
because comparing "-cpu gen14,csske=on" vs. "-cpu gen15,csske=off" would
be "incompatible". But running "-cpu gen14,csske=on" on the host would
work perfectly fine.


https://wiki.qemu.org/index.php/Features/CPUModels

"-cpu host vs -cpu best" is an outdated but interesting read.
-> "-cpu host will be the "all-you-can-enable" mode"
-> "We're not going to have a mode for match-host-CPU, probably "
-> "A "best-predefined-model" mode can be implemented by libvirt. "

Sounds to me, that at least here, the issue should be sorted out either
using a new cpu model type or on the libvirt level.

"Feature deprecated and will be removed soon, remove it from the host
cpu model list if gen15 is used when setting up a new machine".

> 
> Regards,
> Daniel
>

Christian Borntraeger April 24, 2019, 10:26 a.m. UTC | #9

On 24.04.19 11:56, David Hildenbrand wrote:
> On 24.04.19 11:41, Daniel P. Berrangé wrote:
>> On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
>>> On 24.04.19 11:30, Daniel P. Berrangé wrote:
>>>> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
>>>>> On 24.04.19 10:40, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 23.04.19 14:11, David Hildenbrand wrote:
>>>>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>>>>>>> Adding generation 15.
>>>>>>>>
>>>>>>>> Some interesting aspects:
>>>>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>>>>>>   for csske.
>>>>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>>>>>>   provide an alias later on. (I have split out this into a separate
>>>>>>>>   patch)
>>>>>>>>
>>>>>>>> Christian Borntraeger (10):
>>>>>>>>   linux header sync
>>>>>>>>   s390x/cpumodel: remove CSSKE from base model
>>>>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>>>>>>   s390x/cpumodel: msa9 facility
>>>>>>>>   s390x/cpumodel: vector enhancements
>>>>>>>>   s390x/cpumodel: enhanced sort facility
>>>>>>>>   s390x/cpumodel: deflate
>>>>>>>>   s390x/cpumodel: add gen15 defintions
>>>>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>>>>>>
>>>>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>>>>>>  target/s390x/cpu_features.h     |  3 ++
>>>>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>>>>>>  target/s390x/cpu_models.h       |  1 +
>>>>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>>>>>>  target/s390x/kvm.c              | 18 +++++++
>>>>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>
>>>>>>> I guess to handle deprecation of CSSKE:
>>>>>>>
>>>>>>> 1. Remove it from the base + default model of the gen15, keep it in the
>>>>>>> max model. This is completely done in target/s390x/gen-features.c.
>>>>>>> Existing base models are not modified.
>>>>>>>
>>>>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>>>>>>> will work. We can backport this to distros/stable.
>>>>>>
>>>>>> Yes, I have already implemented that, still doing some testing and polishinh.
>>>>>>>
>>>>>>>
>>>>>>> CPU model expansion:
>>>>>>>
>>>>>>> cpu_info_from_model() should already properly be based on the base
>>>>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>>>>>>> when expanding.
>>>>>>>
>>>>>>> CPU model baseline:
>>>>>>>
>>>>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>>>>>>> when determining maximum model, however it will properly be indicated if
>>>>>>> both models had the feature.
>>>>>>>
>>>>>>> CPU model comparison:
>>>>>>>
>>>>>>> Should work as expected. Availability of CSSKE will be considered when
>>>>>>> calculating the result.
>>>>>>>
>>>>>>> gen14,csske=on and gen15,csske=off will result in
>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>>>>>>
>>>>>>> gen14,csske=off and gen15,csske=off should result in
>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>
>>>>>>> gen14,csske=on and gen15,csske=on should result in
>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>
>>>>>>> Forward migration:
>>>>>>>
>>>>>>> Now, the only issue is when csske is actually turned of in future
>>>>>>> machines. We would e.g. have
>>>>>>>
>>>>>>> gen15,csske=on and gen16,csske=off
>>>>>>>
>>>>>>> While baselining will work correctly (gen15,csske=off), forward
>>>>>>> migration is broken (comparison will properly indicate
>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>>>>>>> out features. Same applies to BPB.
>>>>>>>
>>>>>>>
>>>>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>>>>>>> expanded models in qmp" tried to address this, however I am not really
>>>>>>> happy with this approach. We should not play such tricks when expanding
>>>>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>>>>>>> different,
>>>>>>
>>>>>> We discussed this some time ago and I think we agreed that for host passthrough
>>>>>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
>>>>>> through all non-hypervisor managed features etc).
>>>>>
>>>>> I remember the plan was to use the "max" model to do such stuff. E.g.
>>>>> -cpu max / no -cpu
>>>>>
>>>>> Versus
>>>>> -cpu host
>>>>>
>>>>> We can have features in "host" we don't have in "max". But "-cpu host"
>>>>> and it's expansion should look 100% the same.
>>>>
>>>> I don't think that's the intended semantics of "max" vs "host".
>>>>
>>>> The "max" CPU model is supposed to enable all features that are possible to
>>>> enable.
>>>>
>>>> For KVM, thus "max" should be identical to "host".
>>>
>>> There once was a mode used by x86-64 to simply pipe through cpuid
>>> features that were not even supported. (I remember something like
>>> passthorugh=true), do you remember if something like that still exists?
>>
>> I don't recall such a feature existing even in the past !
> 
> Maybe my mind is tricking me, or maybe that has long been removed :)
> 
>>
>>>> For TCG, "max" should be everything that QEMU currently knows how to emulate.
>>>
>>> Yes, and on s390x. "max" contains more features than "qemu".
>>>
>>>>
>>>> Essentially think of "max" as a better name for "host", since "host" as
>>>> a name concept didn't make sense for TCG.
>>>
>>> I agree. The main question is, is it acceptable that
>>
>> Hmm, maybe I misinterpreted when you wrote
>>
>>       We can have features in "host" we don't have in "max"
>>
>> I read that as meaning that "-cpu host" and "-cpu max" would be
>> different.
> 
> No you didn't misinterpret it, I agreed after you spelled it out :)
> 
>>
>>> "-cpu host" and "-cpu $expanded_host" produce different results, after
>>> expanding "host" via query-cpu-model-expansion?
>>
>> That has always been the case on x86-64, since it is not possible to set
>> the level, xlevel, vendor, family, model & stepping properties via -cpu,
>> only the feature flags.
> 
> Fair enough, but the question is if we should mess with feature flags we
> can indicate on that level.
> 
> It would mean that on a specific host e.g.
> 
> "-cpu gen15,csske=on" and "-cpu gen15,csske=off"
> 
> Would work. However, "host" model expansion would give us
> 
> "-cpu gen15,csske=off"
> 
> So trying to e.g. do a query-cpu-model-comparison or
> query-cpu-model-baseline against "host" and against the expanded host
> model will produce different results.
> 
> Libvirt could detect "-cpu gen14,csske=on" as not runnable on the host,
> because comparing "-cpu gen14,csske=on" vs. "-cpu gen15,csske=off" would
> be "incompatible". But running "-cpu gen14,csske=on" on the host would
> work perfectly fine.

I would like to avoid special knowledge in libvirt (since we moved to have
everything in qemu). 

A more complex idea would be to extend the qmp query with a list of deprecated
features and libvirt could then disable that for expansion but allow it for
baselining.

> 
> 
> https://wiki.qemu.org/index.php/Features/CPUModels
> 
> "-cpu host vs -cpu best" is an outdated but interesting read.
> -> "-cpu host will be the "all-you-can-enable" mode"
> -> "We're not going to have a mode for match-host-CPU, probably "
> -> "A "best-predefined-model" mode can be implemented by libvirt. "
> 
> Sounds to me, that at least here, the issue should be sorted out either
> using a new cpu model type or on the libvirt level.
> 
> "Feature deprecated and will be removed soon, remove it from the host
> cpu model list if gen15 is used when setting up a new machine".
> 
>>
>> Regards,
>> Daniel
>>
> 
>

David Hildenbrand April 24, 2019, 10:27 a.m. UTC | #10

On 24.04.19 12:26, Christian Borntraeger wrote:
> 
> 
> On 24.04.19 11:56, David Hildenbrand wrote:
>> On 24.04.19 11:41, Daniel P. Berrangé wrote:
>>> On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
>>>> On 24.04.19 11:30, Daniel P. Berrangé wrote:
>>>>> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
>>>>>> On 24.04.19 10:40, Christian Borntraeger wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 23.04.19 14:11, David Hildenbrand wrote:
>>>>>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>>>>>>>> Adding generation 15.
>>>>>>>>>
>>>>>>>>> Some interesting aspects:
>>>>>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>>>>>>>   for csske.
>>>>>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>>>>>>>   provide an alias later on. (I have split out this into a separate
>>>>>>>>>   patch)
>>>>>>>>>
>>>>>>>>> Christian Borntraeger (10):
>>>>>>>>>   linux header sync
>>>>>>>>>   s390x/cpumodel: remove CSSKE from base model
>>>>>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>>>>>>>   s390x/cpumodel: msa9 facility
>>>>>>>>>   s390x/cpumodel: vector enhancements
>>>>>>>>>   s390x/cpumodel: enhanced sort facility
>>>>>>>>>   s390x/cpumodel: deflate
>>>>>>>>>   s390x/cpumodel: add gen15 defintions
>>>>>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>>>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>>>>>>>
>>>>>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>>>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>>>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>>>>>>>  target/s390x/cpu_features.h     |  3 ++
>>>>>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>>>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>>>>>>>  target/s390x/cpu_models.h       |  1 +
>>>>>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>>>>>>>  target/s390x/kvm.c              | 18 +++++++
>>>>>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>
>>>>>>>> I guess to handle deprecation of CSSKE:
>>>>>>>>
>>>>>>>> 1. Remove it from the base + default model of the gen15, keep it in the
>>>>>>>> max model. This is completely done in target/s390x/gen-features.c.
>>>>>>>> Existing base models are not modified.
>>>>>>>>
>>>>>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>>>>>>>> will work. We can backport this to distros/stable.
>>>>>>>
>>>>>>> Yes, I have already implemented that, still doing some testing and polishinh.
>>>>>>>>
>>>>>>>>
>>>>>>>> CPU model expansion:
>>>>>>>>
>>>>>>>> cpu_info_from_model() should already properly be based on the base
>>>>>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>>>>>>>> when expanding.
>>>>>>>>
>>>>>>>> CPU model baseline:
>>>>>>>>
>>>>>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>>>>>>>> when determining maximum model, however it will properly be indicated if
>>>>>>>> both models had the feature.
>>>>>>>>
>>>>>>>> CPU model comparison:
>>>>>>>>
>>>>>>>> Should work as expected. Availability of CSSKE will be considered when
>>>>>>>> calculating the result.
>>>>>>>>
>>>>>>>> gen14,csske=on and gen15,csske=off will result in
>>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>>>>>>>
>>>>>>>> gen14,csske=off and gen15,csske=off should result in
>>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>>
>>>>>>>> gen14,csske=on and gen15,csske=on should result in
>>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>>
>>>>>>>> Forward migration:
>>>>>>>>
>>>>>>>> Now, the only issue is when csske is actually turned of in future
>>>>>>>> machines. We would e.g. have
>>>>>>>>
>>>>>>>> gen15,csske=on and gen16,csske=off
>>>>>>>>
>>>>>>>> While baselining will work correctly (gen15,csske=off), forward
>>>>>>>> migration is broken (comparison will properly indicate
>>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>>>>>>>> out features. Same applies to BPB.
>>>>>>>>
>>>>>>>>
>>>>>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>>>>>>>> expanded models in qmp" tried to address this, however I am not really
>>>>>>>> happy with this approach. We should not play such tricks when expanding
>>>>>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>>>>>>>> different,
>>>>>>>
>>>>>>> We discussed this some time ago and I think we agreed that for host passthrough
>>>>>>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
>>>>>>> through all non-hypervisor managed features etc).
>>>>>>
>>>>>> I remember the plan was to use the "max" model to do such stuff. E.g.
>>>>>> -cpu max / no -cpu
>>>>>>
>>>>>> Versus
>>>>>> -cpu host
>>>>>>
>>>>>> We can have features in "host" we don't have in "max". But "-cpu host"
>>>>>> and it's expansion should look 100% the same.
>>>>>
>>>>> I don't think that's the intended semantics of "max" vs "host".
>>>>>
>>>>> The "max" CPU model is supposed to enable all features that are possible to
>>>>> enable.
>>>>>
>>>>> For KVM, thus "max" should be identical to "host".
>>>>
>>>> There once was a mode used by x86-64 to simply pipe through cpuid
>>>> features that were not even supported. (I remember something like
>>>> passthorugh=true), do you remember if something like that still exists?
>>>
>>> I don't recall such a feature existing even in the past !
>>
>> Maybe my mind is tricking me, or maybe that has long been removed :)
>>
>>>
>>>>> For TCG, "max" should be everything that QEMU currently knows how to emulate.
>>>>
>>>> Yes, and on s390x. "max" contains more features than "qemu".
>>>>
>>>>>
>>>>> Essentially think of "max" as a better name for "host", since "host" as
>>>>> a name concept didn't make sense for TCG.
>>>>
>>>> I agree. The main question is, is it acceptable that
>>>
>>> Hmm, maybe I misinterpreted when you wrote
>>>
>>>       We can have features in "host" we don't have in "max"
>>>
>>> I read that as meaning that "-cpu host" and "-cpu max" would be
>>> different.
>>
>> No you didn't misinterpret it, I agreed after you spelled it out :)
>>
>>>
>>>> "-cpu host" and "-cpu $expanded_host" produce different results, after
>>>> expanding "host" via query-cpu-model-expansion?
>>>
>>> That has always been the case on x86-64, since it is not possible to set
>>> the level, xlevel, vendor, family, model & stepping properties via -cpu,
>>> only the feature flags.
>>
>> Fair enough, but the question is if we should mess with feature flags we
>> can indicate on that level.
>>
>> It would mean that on a specific host e.g.
>>
>> "-cpu gen15,csske=on" and "-cpu gen15,csske=off"
>>
>> Would work. However, "host" model expansion would give us
>>
>> "-cpu gen15,csske=off"
>>
>> So trying to e.g. do a query-cpu-model-comparison or
>> query-cpu-model-baseline against "host" and against the expanded host
>> model will produce different results.
>>
>> Libvirt could detect "-cpu gen14,csske=on" as not runnable on the host,
>> because comparing "-cpu gen14,csske=on" vs. "-cpu gen15,csske=off" would
>> be "incompatible". But running "-cpu gen14,csske=on" on the host would
>> work perfectly fine.
> 
> I would like to avoid special knowledge in libvirt (since we moved to have
> everything in qemu). 
> 
> A more complex idea would be to extend the qmp query with a list of deprecated
> features and libvirt could then disable that for expansion but allow it for
> baselining.

That would fit in nicely into query-cpu-model-expansion. Nice idea.

Daniel P. Berrangé April 24, 2019, 10:35 a.m. UTC | #11

On Wed, Apr 24, 2019 at 12:26:26PM +0200, Christian Borntraeger wrote:
> 
> 
> On 24.04.19 11:56, David Hildenbrand wrote:
> > On 24.04.19 11:41, Daniel P. Berrangé wrote:
> >> On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
> >>> On 24.04.19 11:30, Daniel P. Berrangé wrote:
> >>>> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
> >>>>> On 24.04.19 10:40, Christian Borntraeger wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 23.04.19 14:11, David Hildenbrand wrote:
> >>>>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
> >>>>>>>> Adding generation 15.
> >>>>>>>>
> >>>>>>>> Some interesting aspects:
> >>>>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
> >>>>>>>>   for csske.
> >>>>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
> >>>>>>>>   provide an alias later on. (I have split out this into a separate
> >>>>>>>>   patch)
> >>>>>>>>
> >>>>>>>> Christian Borntraeger (10):
> >>>>>>>>   linux header sync
> >>>>>>>>   s390x/cpumodel: remove CSSKE from base model
> >>>>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
> >>>>>>>>   s390x/cpumodel: msa9 facility
> >>>>>>>>   s390x/cpumodel: vector enhancements
> >>>>>>>>   s390x/cpumodel: enhanced sort facility
> >>>>>>>>   s390x/cpumodel: deflate
> >>>>>>>>   s390x/cpumodel: add gen15 defintions
> >>>>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
> >>>>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
> >>>>>>>>
> >>>>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
> >>>>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
> >>>>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
> >>>>>>>>  target/s390x/cpu_features.h     |  3 ++
> >>>>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
> >>>>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
> >>>>>>>>  target/s390x/cpu_models.h       |  1 +
> >>>>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
> >>>>>>>>  target/s390x/kvm.c              | 18 +++++++
> >>>>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>
> >>>>>>> I guess to handle deprecation of CSSKE:
> >>>>>>>
> >>>>>>> 1. Remove it from the base + default model of the gen15, keep it in the
> >>>>>>> max model. This is completely done in target/s390x/gen-features.c.
> >>>>>>> Existing base models are not modified.
> >>>>>>>
> >>>>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
> >>>>>>> will work. We can backport this to distros/stable.
> >>>>>>
> >>>>>> Yes, I have already implemented that, still doing some testing and polishinh.
> >>>>>>>
> >>>>>>>
> >>>>>>> CPU model expansion:
> >>>>>>>
> >>>>>>> cpu_info_from_model() should already properly be based on the base
> >>>>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
> >>>>>>> when expanding.
> >>>>>>>
> >>>>>>> CPU model baseline:
> >>>>>>>
> >>>>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
> >>>>>>> when determining maximum model, however it will properly be indicated if
> >>>>>>> both models had the feature.
> >>>>>>>
> >>>>>>> CPU model comparison:
> >>>>>>>
> >>>>>>> Should work as expected. Availability of CSSKE will be considered when
> >>>>>>> calculating the result.
> >>>>>>>
> >>>>>>> gen14,csske=on and gen15,csske=off will result in
> >>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
> >>>>>>>
> >>>>>>> gen14,csske=off and gen15,csske=off should result in
> >>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
> >>>>>>>
> >>>>>>> gen14,csske=on and gen15,csske=on should result in
> >>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
> >>>>>>>
> >>>>>>> Forward migration:
> >>>>>>>
> >>>>>>> Now, the only issue is when csske is actually turned of in future
> >>>>>>> machines. We would e.g. have
> >>>>>>>
> >>>>>>> gen15,csske=on and gen16,csske=off
> >>>>>>>
> >>>>>>> While baselining will work correctly (gen15,csske=off), forward
> >>>>>>> migration is broken (comparison will properly indicate
> >>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
> >>>>>>> out features. Same applies to BPB.
> >>>>>>>
> >>>>>>>
> >>>>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
> >>>>>>> expanded models in qmp" tried to address this, however I am not really
> >>>>>>> happy with this approach. We should not play such tricks when expanding
> >>>>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
> >>>>>>> different,
> >>>>>>
> >>>>>> We discussed this some time ago and I think we agreed that for host passthrough
> >>>>>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
> >>>>>> through all non-hypervisor managed features etc).
> >>>>>
> >>>>> I remember the plan was to use the "max" model to do such stuff. E.g.
> >>>>> -cpu max / no -cpu
> >>>>>
> >>>>> Versus
> >>>>> -cpu host
> >>>>>
> >>>>> We can have features in "host" we don't have in "max". But "-cpu host"
> >>>>> and it's expansion should look 100% the same.
> >>>>
> >>>> I don't think that's the intended semantics of "max" vs "host".
> >>>>
> >>>> The "max" CPU model is supposed to enable all features that are possible to
> >>>> enable.
> >>>>
> >>>> For KVM, thus "max" should be identical to "host".
> >>>
> >>> There once was a mode used by x86-64 to simply pipe through cpuid
> >>> features that were not even supported. (I remember something like
> >>> passthorugh=true), do you remember if something like that still exists?
> >>
> >> I don't recall such a feature existing even in the past !
> > 
> > Maybe my mind is tricking me, or maybe that has long been removed :)
> > 
> >>
> >>>> For TCG, "max" should be everything that QEMU currently knows how to emulate.
> >>>
> >>> Yes, and on s390x. "max" contains more features than "qemu".
> >>>
> >>>>
> >>>> Essentially think of "max" as a better name for "host", since "host" as
> >>>> a name concept didn't make sense for TCG.
> >>>
> >>> I agree. The main question is, is it acceptable that
> >>
> >> Hmm, maybe I misinterpreted when you wrote
> >>
> >>       We can have features in "host" we don't have in "max"
> >>
> >> I read that as meaning that "-cpu host" and "-cpu max" would be
> >> different.
> > 
> > No you didn't misinterpret it, I agreed after you spelled it out :)
> > 
> >>
> >>> "-cpu host" and "-cpu $expanded_host" produce different results, after
> >>> expanding "host" via query-cpu-model-expansion?
> >>
> >> That has always been the case on x86-64, since it is not possible to set
> >> the level, xlevel, vendor, family, model & stepping properties via -cpu,
> >> only the feature flags.
> > 
> > Fair enough, but the question is if we should mess with feature flags we
> > can indicate on that level.
> > 
> > It would mean that on a specific host e.g.
> > 
> > "-cpu gen15,csske=on" and "-cpu gen15,csske=off"
> > 
> > Would work. However, "host" model expansion would give us
> > 
> > "-cpu gen15,csske=off"
> > 
> > So trying to e.g. do a query-cpu-model-comparison or
> > query-cpu-model-baseline against "host" and against the expanded host
> > model will produce different results.
> > 
> > Libvirt could detect "-cpu gen14,csske=on" as not runnable on the host,
> > because comparing "-cpu gen14,csske=on" vs. "-cpu gen15,csske=off" would
> > be "incompatible". But running "-cpu gen14,csske=on" on the host would
> > work perfectly fine.
> 
> I would like to avoid special knowledge in libvirt (since we moved to have
> everything in qemu). 
> 
> A more complex idea would be to extend the qmp query with a list of deprecated
> features and libvirt could then disable that for expansion but allow it for
> baselining.

Funny that you mention deprecation with CPU features....

  https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03969.html

So that makes 4 things we want to report deprecation for. Devices,
machine types, cpu models and now cpu features.

Regards,
Daniel

Christian Borntraeger April 24, 2019, 10:38 a.m. UTC | #12

On 24.04.19 12:27, David Hildenbrand wrote:
> On 24.04.19 12:26, Christian Borntraeger wrote:
>>
>>
>> On 24.04.19 11:56, David Hildenbrand wrote:
>>> On 24.04.19 11:41, Daniel P. Berrangé wrote:
>>>> On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
>>>>> On 24.04.19 11:30, Daniel P. Berrangé wrote:
>>>>>> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
>>>>>>> On 24.04.19 10:40, Christian Borntraeger wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 23.04.19 14:11, David Hildenbrand wrote:
>>>>>>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>>>>>>>>> Adding generation 15.
>>>>>>>>>>
>>>>>>>>>> Some interesting aspects:
>>>>>>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>>>>>>>>   for csske.
>>>>>>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>>>>>>>>   provide an alias later on. (I have split out this into a separate
>>>>>>>>>>   patch)
>>>>>>>>>>
>>>>>>>>>> Christian Borntraeger (10):
>>>>>>>>>>   linux header sync
>>>>>>>>>>   s390x/cpumodel: remove CSSKE from base model
>>>>>>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>>>>>>>>   s390x/cpumodel: msa9 facility
>>>>>>>>>>   s390x/cpumodel: vector enhancements
>>>>>>>>>>   s390x/cpumodel: enhanced sort facility
>>>>>>>>>>   s390x/cpumodel: deflate
>>>>>>>>>>   s390x/cpumodel: add gen15 defintions
>>>>>>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>>>>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>>>>>>>>
>>>>>>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>>>>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>>>>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>>>>>>>>  target/s390x/cpu_features.h     |  3 ++
>>>>>>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>>>>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>>>>>>>>  target/s390x/cpu_models.h       |  1 +
>>>>>>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>>>>>>>>  target/s390x/kvm.c              | 18 +++++++
>>>>>>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I guess to handle deprecation of CSSKE:
>>>>>>>>>
>>>>>>>>> 1. Remove it from the base + default model of the gen15, keep it in the
>>>>>>>>> max model. This is completely done in target/s390x/gen-features.c.
>>>>>>>>> Existing base models are not modified.
>>>>>>>>>
>>>>>>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>>>>>>>>> will work. We can backport this to distros/stable.
>>>>>>>>
>>>>>>>> Yes, I have already implemented that, still doing some testing and polishinh.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> CPU model expansion:
>>>>>>>>>
>>>>>>>>> cpu_info_from_model() should already properly be based on the base
>>>>>>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>>>>>>>>> when expanding.
>>>>>>>>>
>>>>>>>>> CPU model baseline:
>>>>>>>>>
>>>>>>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>>>>>>>>> when determining maximum model, however it will properly be indicated if
>>>>>>>>> both models had the feature.
>>>>>>>>>
>>>>>>>>> CPU model comparison:
>>>>>>>>>
>>>>>>>>> Should work as expected. Availability of CSSKE will be considered when
>>>>>>>>> calculating the result.
>>>>>>>>>
>>>>>>>>> gen14,csske=on and gen15,csske=off will result in
>>>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>>>>>>>>
>>>>>>>>> gen14,csske=off and gen15,csske=off should result in
>>>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>>>
>>>>>>>>> gen14,csske=on and gen15,csske=on should result in
>>>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>>>
>>>>>>>>> Forward migration:
>>>>>>>>>
>>>>>>>>> Now, the only issue is when csske is actually turned of in future
>>>>>>>>> machines. We would e.g. have
>>>>>>>>>
>>>>>>>>> gen15,csske=on and gen16,csske=off
>>>>>>>>>
>>>>>>>>> While baselining will work correctly (gen15,csske=off), forward
>>>>>>>>> migration is broken (comparison will properly indicate
>>>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>>>>>>>>> out features. Same applies to BPB.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>>>>>>>>> expanded models in qmp" tried to address this, however I am not really
>>>>>>>>> happy with this approach. We should not play such tricks when expanding
>>>>>>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>>>>>>>>> different,
>>>>>>>>
>>>>>>>> We discussed this some time ago and I think we agreed that for host passthrough
>>>>>>>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
>>>>>>>> through all non-hypervisor managed features etc).
>>>>>>>
>>>>>>> I remember the plan was to use the "max" model to do such stuff. E.g.
>>>>>>> -cpu max / no -cpu
>>>>>>>
>>>>>>> Versus
>>>>>>> -cpu host
>>>>>>>
>>>>>>> We can have features in "host" we don't have in "max". But "-cpu host"
>>>>>>> and it's expansion should look 100% the same.
>>>>>>
>>>>>> I don't think that's the intended semantics of "max" vs "host".
>>>>>>
>>>>>> The "max" CPU model is supposed to enable all features that are possible to
>>>>>> enable.
>>>>>>
>>>>>> For KVM, thus "max" should be identical to "host".
>>>>>
>>>>> There once was a mode used by x86-64 to simply pipe through cpuid
>>>>> features that were not even supported. (I remember something like
>>>>> passthorugh=true), do you remember if something like that still exists?
>>>>
>>>> I don't recall such a feature existing even in the past !
>>>
>>> Maybe my mind is tricking me, or maybe that has long been removed :)
>>>
>>>>
>>>>>> For TCG, "max" should be everything that QEMU currently knows how to emulate.
>>>>>
>>>>> Yes, and on s390x. "max" contains more features than "qemu".
>>>>>
>>>>>>
>>>>>> Essentially think of "max" as a better name for "host", since "host" as
>>>>>> a name concept didn't make sense for TCG.
>>>>>
>>>>> I agree. The main question is, is it acceptable that
>>>>
>>>> Hmm, maybe I misinterpreted when you wrote
>>>>
>>>>       We can have features in "host" we don't have in "max"
>>>>
>>>> I read that as meaning that "-cpu host" and "-cpu max" would be
>>>> different.
>>>
>>> No you didn't misinterpret it, I agreed after you spelled it out :)
>>>
>>>>
>>>>> "-cpu host" and "-cpu $expanded_host" produce different results, after
>>>>> expanding "host" via query-cpu-model-expansion?
>>>>
>>>> That has always been the case on x86-64, since it is not possible to set
>>>> the level, xlevel, vendor, family, model & stepping properties via -cpu,
>>>> only the feature flags.
>>>
>>> Fair enough, but the question is if we should mess with feature flags we
>>> can indicate on that level.
>>>
>>> It would mean that on a specific host e.g.
>>>
>>> "-cpu gen15,csske=on" and "-cpu gen15,csske=off"
>>>
>>> Would work. However, "host" model expansion would give us
>>>
>>> "-cpu gen15,csske=off"
>>>
>>> So trying to e.g. do a query-cpu-model-comparison or
>>> query-cpu-model-baseline against "host" and against the expanded host
>>> model will produce different results.
>>>
>>> Libvirt could detect "-cpu gen14,csske=on" as not runnable on the host,
>>> because comparing "-cpu gen14,csske=on" vs. "-cpu gen15,csske=off" would
>>> be "incompatible". But running "-cpu gen14,csske=on" on the host would
>>> work perfectly fine.
>>
>> I would like to avoid special knowledge in libvirt (since we moved to have
>> everything in qemu). 
>>
>> A more complex idea would be to extend the qmp query with a list of deprecated
>> features and libvirt could then disable that for expansion but allow it for
>> baselining.
> 
> That would fit in nicely into query-cpu-model-expansion. Nice idea.

Hmm, I think that should also be possible as an add-on patch series later on
(e.g. in 6 month or so). We would have instances of gen15 machines with host-model
that start with csske but sooner or later they will be restarted and then they no 
longer have csske.
As we do not change anything for baselining this would allow us to separate patch10
from the other patches and deal with it later with a proper series on its on?
Need to think about that.

David Hildenbrand April 24, 2019, 11:49 a.m. UTC | #13

On 24.04.19 12:38, Christian Borntraeger wrote:
> 
> 
> On 24.04.19 12:27, David Hildenbrand wrote:
>> On 24.04.19 12:26, Christian Borntraeger wrote:
>>>
>>>
>>> On 24.04.19 11:56, David Hildenbrand wrote:
>>>> On 24.04.19 11:41, Daniel P. Berrangé wrote:
>>>>> On Wed, Apr 24, 2019 at 11:35:40AM +0200, David Hildenbrand wrote:
>>>>>> On 24.04.19 11:30, Daniel P. Berrangé wrote:
>>>>>>> On Wed, Apr 24, 2019 at 11:03:03AM +0200, David Hildenbrand wrote:
>>>>>>>> On 24.04.19 10:40, Christian Borntraeger wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 23.04.19 14:11, David Hildenbrand wrote:
>>>>>>>>>> On 18.04.19 13:31, Christian Borntraeger wrote:
>>>>>>>>>>> Adding generation 15.
>>>>>>>>>>>
>>>>>>>>>>> Some interesting aspects:
>>>>>>>>>>> - conditional SSKE and bpb are deprecated. This patch set addresses that
>>>>>>>>>>>   for csske.
>>>>>>>>>>> - no name yet for gen15, I suggest to use the assigned numbers and
>>>>>>>>>>>   provide an alias later on. (I have split out this into a separate
>>>>>>>>>>>   patch)
>>>>>>>>>>>
>>>>>>>>>>> Christian Borntraeger (10):
>>>>>>>>>>>   linux header sync
>>>>>>>>>>>   s390x/cpumodel: remove CSSKE from base model
>>>>>>>>>>>   s390x/cpumodel: Miscellaneous-Instruction-Extensions Facility 3
>>>>>>>>>>>   s390x/cpumodel: msa9 facility
>>>>>>>>>>>   s390x/cpumodel: vector enhancements
>>>>>>>>>>>   s390x/cpumodel: enhanced sort facility
>>>>>>>>>>>   s390x/cpumodel: deflate
>>>>>>>>>>>   s390x/cpumodel: add gen15 defintions
>>>>>>>>>>>   s390x/cpumodel: wire up 8561 and 8562 as gen15 machines
>>>>>>>>>>>   s390x/cpumodel: do not claim csske for expanded models in qmp
>>>>>>>>>>>
>>>>>>>>>>>  hw/s390x/s390-virtio-ccw.c      |  6 +++
>>>>>>>>>>>  linux-headers/asm-s390/kvm.h    |  5 +-
>>>>>>>>>>>  target/s390x/cpu_features.c     | 54 +++++++++++++++++++
>>>>>>>>>>>  target/s390x/cpu_features.h     |  3 ++
>>>>>>>>>>>  target/s390x/cpu_features_def.h | 49 +++++++++++++++++
>>>>>>>>>>>  target/s390x/cpu_models.c       | 35 ++++++++++++
>>>>>>>>>>>  target/s390x/cpu_models.h       |  1 +
>>>>>>>>>>>  target/s390x/gen-features.c     | 94 ++++++++++++++++++++++++++++++++-
>>>>>>>>>>>  target/s390x/kvm.c              | 18 +++++++
>>>>>>>>>>>  9 files changed, 263 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I guess to handle deprecation of CSSKE:
>>>>>>>>>>
>>>>>>>>>> 1. Remove it from the base + default model of the gen15, keep it in the
>>>>>>>>>> max model. This is completely done in target/s390x/gen-features.c.
>>>>>>>>>> Existing base models are not modified.
>>>>>>>>>>
>>>>>>>>>> 2. Add CSSKE to "ignored_base_feat", so fallback of gen15 to e.g. z14
>>>>>>>>>> will work. We can backport this to distros/stable.
>>>>>>>>>
>>>>>>>>> Yes, I have already implemented that, still doing some testing and polishinh.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> CPU model expansion:
>>>>>>>>>>
>>>>>>>>>> cpu_info_from_model() should already properly be based on the base
>>>>>>>>>> features. "gen15" vs. "gen15,csske=on" should be automatically generated
>>>>>>>>>> when expanding.
>>>>>>>>>>
>>>>>>>>>> CPU model baseline:
>>>>>>>>>>
>>>>>>>>>> s390_find_cpu_def() should make sure that CSSKE is basically ignored
>>>>>>>>>> when determining maximum model, however it will properly be indicated if
>>>>>>>>>> both models had the feature.
>>>>>>>>>>
>>>>>>>>>> CPU model comparison:
>>>>>>>>>>
>>>>>>>>>> Should work as expected. Availability of CSSKE will be considered when
>>>>>>>>>> calculating the result.
>>>>>>>>>>
>>>>>>>>>> gen14,csske=on and gen15,csske=off will result in
>>>>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE.
>>>>>>>>>>
>>>>>>>>>> gen14,csske=off and gen15,csske=off should result in
>>>>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>>>>
>>>>>>>>>> gen14,csske=on and gen15,csske=on should result in
>>>>>>>>>> CPU_MODEL_COMPARE_RESULT_SUBSET
>>>>>>>>>>
>>>>>>>>>> Forward migration:
>>>>>>>>>>
>>>>>>>>>> Now, the only issue is when csske is actually turned of in future
>>>>>>>>>> machines. We would e.g. have
>>>>>>>>>>
>>>>>>>>>> gen15,csske=on and gen16,csske=off
>>>>>>>>>>
>>>>>>>>>> While baselining will work correctly (gen15,csske=off), forward
>>>>>>>>>> migration is broken (comparison will properly indicate
>>>>>>>>>> CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE), which is expected when ripping
>>>>>>>>>> out features. Same applies to BPB.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Your patch "[PATCH 10/10] s390x/cpumodel: do not claim csske for
>>>>>>>>>> expanded models in qmp" tried to address this, however I am not really
>>>>>>>>>> happy with this approach. We should not play such tricks when expanding
>>>>>>>>>> the host model. "-cpu host" and "-cpu $expanded_host" would be
>>>>>>>>>> different,
>>>>>>>>>
>>>>>>>>> We discussed this some time ago and I think we agreed that for host passthrough
>>>>>>>>> it is ok to be different that host-model (e.g. passing through the cpuid, passing
>>>>>>>>> through all non-hypervisor managed features etc).
>>>>>>>>
>>>>>>>> I remember the plan was to use the "max" model to do such stuff. E.g.
>>>>>>>> -cpu max / no -cpu
>>>>>>>>
>>>>>>>> Versus
>>>>>>>> -cpu host
>>>>>>>>
>>>>>>>> We can have features in "host" we don't have in "max". But "-cpu host"
>>>>>>>> and it's expansion should look 100% the same.
>>>>>>>
>>>>>>> I don't think that's the intended semantics of "max" vs "host".
>>>>>>>
>>>>>>> The "max" CPU model is supposed to enable all features that are possible to
>>>>>>> enable.
>>>>>>>
>>>>>>> For KVM, thus "max" should be identical to "host".
>>>>>>
>>>>>> There once was a mode used by x86-64 to simply pipe through cpuid
>>>>>> features that were not even supported. (I remember something like
>>>>>> passthorugh=true), do you remember if something like that still exists?
>>>>>
>>>>> I don't recall such a feature existing even in the past !
>>>>
>>>> Maybe my mind is tricking me, or maybe that has long been removed :)
>>>>
>>>>>
>>>>>>> For TCG, "max" should be everything that QEMU currently knows how to emulate.
>>>>>>
>>>>>> Yes, and on s390x. "max" contains more features than "qemu".
>>>>>>
>>>>>>>
>>>>>>> Essentially think of "max" as a better name for "host", since "host" as
>>>>>>> a name concept didn't make sense for TCG.
>>>>>>
>>>>>> I agree. The main question is, is it acceptable that
>>>>>
>>>>> Hmm, maybe I misinterpreted when you wrote
>>>>>
>>>>>       We can have features in "host" we don't have in "max"
>>>>>
>>>>> I read that as meaning that "-cpu host" and "-cpu max" would be
>>>>> different.
>>>>
>>>> No you didn't misinterpret it, I agreed after you spelled it out :)
>>>>
>>>>>
>>>>>> "-cpu host" and "-cpu $expanded_host" produce different results, after
>>>>>> expanding "host" via query-cpu-model-expansion?
>>>>>
>>>>> That has always been the case on x86-64, since it is not possible to set
>>>>> the level, xlevel, vendor, family, model & stepping properties via -cpu,
>>>>> only the feature flags.
>>>>
>>>> Fair enough, but the question is if we should mess with feature flags we
>>>> can indicate on that level.
>>>>
>>>> It would mean that on a specific host e.g.
>>>>
>>>> "-cpu gen15,csske=on" and "-cpu gen15,csske=off"
>>>>
>>>> Would work. However, "host" model expansion would give us
>>>>
>>>> "-cpu gen15,csske=off"
>>>>
>>>> So trying to e.g. do a query-cpu-model-comparison or
>>>> query-cpu-model-baseline against "host" and against the expanded host
>>>> model will produce different results.
>>>>
>>>> Libvirt could detect "-cpu gen14,csske=on" as not runnable on the host,
>>>> because comparing "-cpu gen14,csske=on" vs. "-cpu gen15,csske=off" would
>>>> be "incompatible". But running "-cpu gen14,csske=on" on the host would
>>>> work perfectly fine.
>>>
>>> I would like to avoid special knowledge in libvirt (since we moved to have
>>> everything in qemu). 
>>>
>>> A more complex idea would be to extend the qmp query with a list of deprecated
>>> features and libvirt could then disable that for expansion but allow it for
>>> baselining.
>>
>> That would fit in nicely into query-cpu-model-expansion. Nice idea.
> 
> Hmm, I think that should also be possible as an add-on patch series later on
> (e.g. in 6 month or so). We would have instances of gen15 machines with host-model
> that start with csske but sooner or later they will be restarted and then they no 
> longer have csske.
> As we do not change anything for baselining this would allow us to separate patch10
> from the other patches and deal with it later with a proper series on its on?
> Need to think about that.

Makes sense, we already have forward migration issues to > gen15 with
guests started in the past when dropping features. No need to rush with
that.

[00/10] s390x: new guest features

Message

Comments