QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
@ 2019-11-08 11:07 David Hildenbrand
  2019-11-08 11:07 ` [PATCH v1 1/2] s390x/cpumodels: Factor out CPU feature dependencies David Hildenbrand
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-11-08 11:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, David Hildenbrand, Cornelia Huck,
	Richard Henderson, Markus Armbruster, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Michael Mueller,
	Jiri Denemark, Eduardo Habkost

There was recently a discussion regarding CPU model versions. That concept
does not fit s390x where we have a lot of feature variability. I
proposed an alternative approach in [1], which might work for x86 as well
(but I am not sure if x86 still can or wants to switch to that), and
requires no real changes in upper layers.

[1] and patch #2 contains more information on the motivation for this.

E.g., specifying/expanding "z14-best" will result in the "best feature
set possible on this accelerator, hw and, firmware". While a "z13" does
not work under TCG and some z/VM versions, "z13-best" will work.

As I had a couple of spare minutes this week, I hacked a quick prototype.
I did not heavily test this (quick sanity tests under TCG only), but it
should be decent enough to get the idea and play with it. I'll not be
working next week, which is why I sent this out now for discussion.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07222.html

Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Michael Mueller <mimu@linux.ibm.com>

David Hildenbrand (2):
  s390x/cpumodels: Factor out CPU feature dependencies
  s390x/cpumodel: Introduce "best" model variants

 target/s390x/cpu-qom.h    |   1 +
 target/s390x/cpu_models.c | 211 +++++++++++++++++++++++++++-----------
 2 files changed, 153 insertions(+), 59 deletions(-)

-- 
2.21.0



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v1 1/2] s390x/cpumodels: Factor out CPU feature dependencies
  2019-11-08 11:07 [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
@ 2019-11-08 11:07 ` David Hildenbrand
  2019-11-08 11:07 ` [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-11-08 11:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, David Hildenbrand, Cornelia Huck,
	Richard Henderson, Markus Armbruster, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Michael Mueller,
	Jiri Denemark, Eduardo Habkost

Currently we check "if bit X is set, bit Y is required". We want
to perform "if bit Y is not set, bit X must also not be set" when
creating CPU models that will pass all consistency checks.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 115 +++++++++++++++++++-------------------
 1 file changed, 58 insertions(+), 57 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 009afc38b9..57c06e5ea1 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -88,6 +88,59 @@ static S390CPUDef s390_cpu_defs[] = {
     CPUDEF_INIT(0x8562, 15, 1, 47, 0x08000000U, "gen15b", "IBM 8562 GA1"),
 };
 
+static const int cpu_feature_dependencies[][2] = {
+    { S390_FEAT_IPTE_RANGE, S390_FEAT_DAT_ENH },
+    { S390_FEAT_IDTE_SEGMENT, S390_FEAT_DAT_ENH },
+    { S390_FEAT_IDTE_REGION, S390_FEAT_DAT_ENH },
+    { S390_FEAT_IDTE_REGION, S390_FEAT_IDTE_SEGMENT },
+    { S390_FEAT_LOCAL_TLB_CLEARING, S390_FEAT_DAT_ENH},
+    { S390_FEAT_LONG_DISPLACEMENT_FAST, S390_FEAT_LONG_DISPLACEMENT },
+    { S390_FEAT_DFP_FAST, S390_FEAT_DFP },
+    { S390_FEAT_TRANSACTIONAL_EXE, S390_FEAT_STFLE_49 },
+    { S390_FEAT_EDAT_2, S390_FEAT_EDAT},
+    { S390_FEAT_MSA_EXT_5, S390_FEAT_KIMD_SHA_512 },
+    { S390_FEAT_MSA_EXT_5, S390_FEAT_KLMD_SHA_512 },
+    { S390_FEAT_MSA_EXT_4, S390_FEAT_MSA_EXT_3 },
+    { S390_FEAT_SIE_CMMA, S390_FEAT_CMM },
+    { S390_FEAT_SIE_CMMA, S390_FEAT_SIE_GSLS },
+    { S390_FEAT_SIE_PFMFI, S390_FEAT_EDAT },
+    { S390_FEAT_MSA_EXT_8, S390_FEAT_MSA_EXT_3 },
+    { S390_FEAT_MSA_EXT_9, S390_FEAT_MSA_EXT_3 },
+    { S390_FEAT_MSA_EXT_9, S390_FEAT_MSA_EXT_4 },
+    { S390_FEAT_MULTIPLE_EPOCH, S390_FEAT_TOD_CLOCK_STEERING },
+    { S390_FEAT_VECTOR_PACKED_DECIMAL, S390_FEAT_VECTOR },
+    { S390_FEAT_VECTOR_ENH, S390_FEAT_VECTOR },
+    { S390_FEAT_INSTRUCTION_EXEC_PROT, S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2 },
+    { S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2, S390_FEAT_ESOP },
+    { S390_FEAT_CMM_NT, S390_FEAT_CMM },
+    { S390_FEAT_GUARDED_STORAGE, S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2 },
+    { S390_FEAT_MULTIPLE_EPOCH, S390_FEAT_STORE_CLOCK_FAST },
+    { S390_FEAT_MULTIPLE_EPOCH, S390_FEAT_TOD_CLOCK_STEERING },
+    { S390_FEAT_SEMAPHORE_ASSIST, S390_FEAT_STFLE_49 },
+    { S390_FEAT_KIMD_SHA3_224, S390_FEAT_MSA },
+    { S390_FEAT_KIMD_SHA3_256, S390_FEAT_MSA },
+    { S390_FEAT_KIMD_SHA3_384, S390_FEAT_MSA },
+    { S390_FEAT_KIMD_SHA3_512, S390_FEAT_MSA },
+    { S390_FEAT_KIMD_SHAKE_128, S390_FEAT_MSA },
+    { S390_FEAT_KIMD_SHAKE_256, S390_FEAT_MSA },
+    { S390_FEAT_KLMD_SHA3_224, S390_FEAT_MSA },
+    { S390_FEAT_KLMD_SHA3_256, S390_FEAT_MSA },
+    { S390_FEAT_KLMD_SHA3_384, S390_FEAT_MSA },
+    { S390_FEAT_KLMD_SHA3_512, S390_FEAT_MSA },
+    { S390_FEAT_KLMD_SHAKE_128, S390_FEAT_MSA },
+    { S390_FEAT_KLMD_SHAKE_256, S390_FEAT_MSA },
+    { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
+    { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
+    { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
+    { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
+    { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
+    { S390_FEAT_PTFF_QSIE, S390_FEAT_MULTIPLE_EPOCH },
+    { S390_FEAT_PTFF_QTOUE, S390_FEAT_MULTIPLE_EPOCH },
+    { S390_FEAT_PTFF_STOE, S390_FEAT_MULTIPLE_EPOCH },
+    { S390_FEAT_PTFF_STOUE, S390_FEAT_MULTIPLE_EPOCH },
+    { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP },
+};
+
 #define QEMU_MAX_CPU_TYPE 0x2964
 #define QEMU_MAX_CPU_GEN 13
 #define QEMU_MAX_CPU_EC_GA 2
@@ -768,66 +821,14 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
 
 static void check_consistency(const S390CPUModel *model)
 {
-    static int dep[][2] = {
-        { S390_FEAT_IPTE_RANGE, S390_FEAT_DAT_ENH },
-        { S390_FEAT_IDTE_SEGMENT, S390_FEAT_DAT_ENH },
-        { S390_FEAT_IDTE_REGION, S390_FEAT_DAT_ENH },
-        { S390_FEAT_IDTE_REGION, S390_FEAT_IDTE_SEGMENT },
-        { S390_FEAT_LOCAL_TLB_CLEARING, S390_FEAT_DAT_ENH},
-        { S390_FEAT_LONG_DISPLACEMENT_FAST, S390_FEAT_LONG_DISPLACEMENT },
-        { S390_FEAT_DFP_FAST, S390_FEAT_DFP },
-        { S390_FEAT_TRANSACTIONAL_EXE, S390_FEAT_STFLE_49 },
-        { S390_FEAT_EDAT_2, S390_FEAT_EDAT},
-        { S390_FEAT_MSA_EXT_5, S390_FEAT_KIMD_SHA_512 },
-        { S390_FEAT_MSA_EXT_5, S390_FEAT_KLMD_SHA_512 },
-        { S390_FEAT_MSA_EXT_4, S390_FEAT_MSA_EXT_3 },
-        { S390_FEAT_SIE_CMMA, S390_FEAT_CMM },
-        { S390_FEAT_SIE_CMMA, S390_FEAT_SIE_GSLS },
-        { S390_FEAT_SIE_PFMFI, S390_FEAT_EDAT },
-        { S390_FEAT_MSA_EXT_8, S390_FEAT_MSA_EXT_3 },
-        { S390_FEAT_MSA_EXT_9, S390_FEAT_MSA_EXT_3 },
-        { S390_FEAT_MSA_EXT_9, S390_FEAT_MSA_EXT_4 },
-        { S390_FEAT_MULTIPLE_EPOCH, S390_FEAT_TOD_CLOCK_STEERING },
-        { S390_FEAT_VECTOR_PACKED_DECIMAL, S390_FEAT_VECTOR },
-        { S390_FEAT_VECTOR_ENH, S390_FEAT_VECTOR },
-        { S390_FEAT_INSTRUCTION_EXEC_PROT, S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2 },
-        { S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2, S390_FEAT_ESOP },
-        { S390_FEAT_CMM_NT, S390_FEAT_CMM },
-        { S390_FEAT_GUARDED_STORAGE, S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2 },
-        { S390_FEAT_MULTIPLE_EPOCH, S390_FEAT_STORE_CLOCK_FAST },
-        { S390_FEAT_MULTIPLE_EPOCH, S390_FEAT_TOD_CLOCK_STEERING },
-        { S390_FEAT_SEMAPHORE_ASSIST, S390_FEAT_STFLE_49 },
-        { S390_FEAT_KIMD_SHA3_224, S390_FEAT_MSA },
-        { S390_FEAT_KIMD_SHA3_256, S390_FEAT_MSA },
-        { S390_FEAT_KIMD_SHA3_384, S390_FEAT_MSA },
-        { S390_FEAT_KIMD_SHA3_512, S390_FEAT_MSA },
-        { S390_FEAT_KIMD_SHAKE_128, S390_FEAT_MSA },
-        { S390_FEAT_KIMD_SHAKE_256, S390_FEAT_MSA },
-        { S390_FEAT_KLMD_SHA3_224, S390_FEAT_MSA },
-        { S390_FEAT_KLMD_SHA3_256, S390_FEAT_MSA },
-        { S390_FEAT_KLMD_SHA3_384, S390_FEAT_MSA },
-        { S390_FEAT_KLMD_SHA3_512, S390_FEAT_MSA },
-        { S390_FEAT_KLMD_SHAKE_128, S390_FEAT_MSA },
-        { S390_FEAT_KLMD_SHAKE_256, S390_FEAT_MSA },
-        { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
-        { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
-        { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
-        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
-        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
-        { S390_FEAT_PTFF_QSIE, S390_FEAT_MULTIPLE_EPOCH },
-        { S390_FEAT_PTFF_QTOUE, S390_FEAT_MULTIPLE_EPOCH },
-        { S390_FEAT_PTFF_STOE, S390_FEAT_MULTIPLE_EPOCH },
-        { S390_FEAT_PTFF_STOUE, S390_FEAT_MULTIPLE_EPOCH },
-        { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP },
-    };
     int i;
 
-    for (i = 0; i < ARRAY_SIZE(dep); i++) {
-        if (test_bit(dep[i][0], model->features) &&
-            !test_bit(dep[i][1], model->features)) {
+    for (i = 0; i < ARRAY_SIZE(cpu_feature_dependencies); i++) {
+        if (test_bit(cpu_feature_dependencies[i][0], model->features) &&
+            !test_bit(cpu_feature_dependencies[i][1], model->features)) {
             warn_report("\'%s\' requires \'%s\'.",
-                        s390_feat_def(dep[i][0])->name,
-                        s390_feat_def(dep[i][1])->name);
+                        s390_feat_def(cpu_feature_dependencies[i][0])->name,
+                        s390_feat_def(cpu_feature_dependencies[i][1])->name);
         }
     }
 }
-- 
2.21.0



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 11:07 [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
  2019-11-08 11:07 ` [PATCH v1 1/2] s390x/cpumodels: Factor out CPU feature dependencies David Hildenbrand
@ 2019-11-08 11:07 ` David Hildenbrand
  2019-11-08 19:51   ` Eduardo Habkost
  2019-11-08 11:10 ` [PATCH v1 0/2] " Peter Maydell
  2019-11-08 16:59 ` no-reply
  3 siblings, 1 reply; 12+ messages in thread
From: David Hildenbrand @ 2019-11-08 11:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, David Hildenbrand, Cornelia Huck,
	Richard Henderson, Markus Armbruster, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Michael Mueller,
	Jiri Denemark, Eduardo Habkost

For a specific CPU model, we have a lot of feature variability depending on
- The microcode version of the HW
- The hypervisor we're running on (LPAR vs. KVM vs. z/VM)
- The hypervisor version we're running on
- The KVM version
- KVM module parameters (especially, "nested=1")
- The accelerator

Our default models are migration safe, however can only be changed
between QEMU releases (glued to QEMU machine). This somewhat collides
with the feature variability we have. E.g., the z13 model will not run
under TCG. There is the demand from higher levels in the stack to "have the
best CPU model possible on a given accelerator, firmware and HW", which
should especially include all features that fix security issues.
Especially, if we have a new feature due to a security flaw, we want to
have a way to backport this feature to older QEMU versions and a way to
automatically enable it when asked.

This is where "best" CPU models come into play. If upper layers specify
"z14-best" on a z14, they will get the best possible feature set in that
configuration. "best" usually means "maximum features", besides deprecated
features. This will then, for example, include nested virtualization
("SIE" feature) when KVM+HW support is enabled, or fixes via
microcode updates (e.g., spectre)

"best" models are not migration safe. Upper layers can expand these
models to migration-safe and static variants, allowing them to be
migrated.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-qom.h    |  1 +
 target/s390x/cpu_models.c | 96 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index b809ec8418..73901d1410 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -52,6 +52,7 @@ typedef struct S390CPUClass {
     bool kvm_required;
     bool is_static;
     bool is_migration_safe;
+    bool is_best;
     const char *desc;
 
     DeviceRealize parent_realize;
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 57c06e5ea1..a379b4c15d 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -557,11 +557,16 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
     obj = object_new(object_class_get_name(oc));
     cpu = S390_CPU(obj);
 
-    if (!cpu->model) {
+    if (!cpu->model && !S390_CPU_CLASS(oc)->is_best) {
         error_setg(errp, "Details about the host CPU model are not available, "
                          "it cannot be used.");
         object_unref(obj);
         return;
+    } else if (!cpu->model) {
+        error_setg(errp, "There is not best CPU model that is runnable,"
+                         "therefore, it cannot be used.");
+        object_unref(obj);
+        return;
     }
 
     if (qdict) {
@@ -932,7 +937,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
         return;
     }
 
-    if (!cpu->model) {
+    if (!cpu->model && !xcc->is_best) {
         /* no host model support -> perform compatibility stuff */
         apply_cpu_model(NULL, errp);
         return;
@@ -944,6 +949,11 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
         return;
     }
 
+    if (xcc->is_best && !cpu->model) {
+        error_setg(errp, "Selected CPU model is too new.");
+        return;
+    }
+
     /* copy over properties that can vary */
     cpu->model->lowest_ibc = max_model->lowest_ibc;
     cpu->model->cpu_id = max_model->cpu_id;
@@ -1156,6 +1166,58 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
     memcpy(cpu->model, &s390_qemu_cpu_model, sizeof(*cpu->model));
 }
 
+static void s390_best_cpu_model_initfn(Object *obj)
+{
+    const S390CPUModel *max_model;
+    S390CPU *cpu = S390_CPU(obj);
+    S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu);
+    Error *local_err = NULL;
+    int i;
+
+    if (kvm_enabled() && !kvm_s390_cpu_models_supported()) {
+        return;
+    }
+
+    max_model = get_max_cpu_model(&local_err);
+    if (local_err) {
+        /* we expect errors only under KVM, when actually querying the kernel */
+        g_assert(kvm_enabled());
+        error_report_err(local_err);
+        return;
+    }
+
+    /*
+     * Similar to baselining against the "max" model. However, features
+     * are handled differently and are not used for the search for a definition.
+     */
+    if (xcc->cpu_def->gen == max_model->def->gen) {
+        if (xcc->cpu_def->ec_ga > max_model->def->ec_ga) {
+            return;
+        }
+    } else if (xcc->cpu_def->gen > max_model->def->gen) {
+        return;
+    }
+
+    /* The model is theoretically runnable, construct the features. */
+    cpu->model = g_new(S390CPUModel, 1);
+    cpu->model->def = xcc->cpu_def;
+    bitmap_copy(cpu->model->features, xcc->cpu_def->full_feat, S390_FEAT_MAX);
+
+    /* Mask of features that are not available in the "max" model */
+    bitmap_and(cpu->model->features, cpu->model->features, max_model->features,
+               S390_FEAT_MAX);
+
+    /* Mask off deprecated features */
+    clear_bit(S390_FEAT_CONDITIONAL_SSKE, cpu->model->features);
+
+    /* Make sure every model passes consistency checks */
+    for (i = 0; i < ARRAY_SIZE(cpu_feature_dependencies); i++) {
+        if (!test_bit(cpu_feature_dependencies[i][1], cpu->model->features)) {
+            clear_bit(cpu_feature_dependencies[i][0], cpu->model->features);
+        }
+    }
+}
+
 static void s390_max_cpu_model_initfn(Object *obj)
 {
     const S390CPUModel *max_model;
@@ -1235,6 +1297,20 @@ static void s390_base_cpu_model_class_init(ObjectClass *oc, void *data)
     xcc->desc = xcc->cpu_def->desc;
 }
 
+static void s390_best_cpu_model_class_init(ObjectClass *oc, void *data)
+{
+    S390CPUClass *xcc = S390_CPU_CLASS(oc);
+
+    /*
+     * The "best" models are neither static nor migration safe, similar to
+     * the "max" model.
+     */
+    xcc->is_best = true;
+    xcc->cpu_def = (const S390CPUDef *) data;
+    xcc->desc = g_strdup_printf("%s with best features supported by the accelerator in the current host",
+                                xcc->cpu_def->desc);
+}
+
 static void s390_cpu_model_class_init(ObjectClass *oc, void *data)
 {
     S390CPUClass *xcc = S390_CPU_CLASS(oc);
@@ -1280,6 +1356,12 @@ static char *s390_base_cpu_type_name(const char *model_name)
     return g_strdup_printf(S390_CPU_TYPE_NAME("%s-base"), model_name);
 }
 
+/* Generate type name for a best cpu model. Caller has to free the string. */
+static char *s390_best_cpu_type_name(const char *model_name)
+{
+    return g_strdup_printf(S390_CPU_TYPE_NAME("%s-best"), model_name);
+}
+
 ObjectClass *s390_cpu_class_by_name(const char *name)
 {
     char *typename = s390_cpu_type_name(name);
@@ -1380,9 +1462,19 @@ static void register_types(void)
             .class_init = s390_cpu_model_class_init,
             .class_data = (void *) &s390_cpu_defs[i],
         };
+        char *best_name = s390_best_cpu_type_name(s390_cpu_defs[i].name);
+        TypeInfo ti_best = {
+            .name = best_name,
+            .parent = TYPE_S390_CPU,
+            .instance_init = s390_best_cpu_model_initfn,
+            .instance_finalize = s390_cpu_model_finalize,
+            .class_init = s390_best_cpu_model_class_init,
+            .class_data = (void *) &s390_cpu_defs[i],
+        };
 
         type_register_static(&ti_base);
         type_register_static(&ti);
+        type_register_static(&ti_best);
         g_free(base_name);
         g_free(name);
     }
-- 
2.21.0



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 11:07 [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
  2019-11-08 11:07 ` [PATCH v1 1/2] s390x/cpumodels: Factor out CPU feature dependencies David Hildenbrand
  2019-11-08 11:07 ` [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
@ 2019-11-08 11:10 ` " Peter Maydell
  2019-11-08 12:46   ` David Hildenbrand
  2019-11-08 16:59 ` no-reply
  3 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2019-11-08 11:10 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, Cornelia Huck, Richard Henderson, QEMU Developers,
	Markus Armbruster, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Michael Mueller, Jiri Denemark, Eduardo Habkost

On Fri, 8 Nov 2019 at 11:08, David Hildenbrand <david@redhat.com> wrote:
>
> There was recently a discussion regarding CPU model versions. That concept
> does not fit s390x where we have a lot of feature variability. I
> proposed an alternative approach in [1], which might work for x86 as well
> (but I am not sure if x86 still can or wants to switch to that), and
> requires no real changes in upper layers.
>
> [1] and patch #2 contains more information on the motivation for this.
>
> E.g., specifying/expanding "z14-best" will result in the "best feature
> set possible on this accelerator, hw and, firmware". While a "z13" does
> not work under TCG and some z/VM versions, "z13-best" will work.

I think other architectures call this concept "max", not "best".
If we can manage some cross-architecture consistency that would
be helpful, but is s390x using 'max' already for something else?

thanks
-- PMM


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 11:10 ` [PATCH v1 0/2] " Peter Maydell
@ 2019-11-08 12:46   ` David Hildenbrand
  2019-11-08 13:02     ` Peter Maydell
  0 siblings, 1 reply; 12+ messages in thread
From: David Hildenbrand @ 2019-11-08 12:46 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, Cornelia Huck, Richard Henderson, QEMU Developers,
	Markus Armbruster, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Michael Mueller, Jiri Denemark, Eduardo Habkost

On 08.11.19 12:10, Peter Maydell wrote:
> On Fri, 8 Nov 2019 at 11:08, David Hildenbrand <david@redhat.com> wrote:
>>
>> There was recently a discussion regarding CPU model versions. That concept
>> does not fit s390x where we have a lot of feature variability. I
>> proposed an alternative approach in [1], which might work for x86 as well
>> (but I am not sure if x86 still can or wants to switch to that), and
>> requires no real changes in upper layers.
>>
>> [1] and patch #2 contains more information on the motivation for this.
>>
>> E.g., specifying/expanding "z14-best" will result in the "best feature
>> set possible on this accelerator, hw and, firmware". While a "z13" does
>> not work under TCG and some z/VM versions, "z13-best" will work.
> 
> I think other architectures call this concept "max", not "best".
> If we can manage some cross-architecture consistency that would
> be helpful, but is s390x using 'max' already for something else?

We have the "max" model just like other architectures

s390 max             Enables all features supported by the accelerator 
in the current host

It is basically the "host" model under KVM, and the "qemu" model under 
TCG (with minor differences for the latter).


This series introduces e.g.,

s390 z900-best       IBM zSeries 900 GA1 with best features supported by 
the accelerator in the current host

s390 z14-best        IBM z14 GA1 with best features supported by the 
accelerator in the current host

s390 z14ZR1-best     IBM z14 Model ZR1 GA1 with best features supported 
by the accelerator in the current host

s390 gen15a-best     IBM z15 GA1 with best features supported by the 
accelerator in the current host

s390 gen15b-best     IBM 8562 GA1 with best features supported by the 
accelerator in the current host


There is a small but important difference between "max"/"host" and 
"best". Max really means "all features", including deprecated ones. 
"best", however, can disable experimental or deprecated features. Or any 
other features we don't want to be enabled when somebody selects a model 
manually.

On s390x, the feature "csske" is deprecated. New HW still has it, but we 
want new guests to run without this facility. Dropping it from "max" 
would affect existing setups. We already changed the default model 
(e.g., -cpu z13) to disable it with never QEMU machines.

E.g., nested virtualization features on some architectures could be a 
feature set you want to disable, although contained in the "max" model. 
(e.g., no migration support yet).


I am not completely against calling these "max" models instead of "best" 
models, but I think this makes it clearer that there is indeed a difference.

Maybe, we even want a "-cpu best" that would not map to "-cpu 
host"/"-cpu max", but to a cleaned up "-cpu host"/"-cpu max" (e.g., 
disable deprecated features). Long term, we might even want to change 
the default when no "-cpu" is specified to "-cpu best" - which should 
now be possible with the latest QEMU changes to query the default model 
for a specific QEMU machine.

-- 

Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 12:46   ` David Hildenbrand
@ 2019-11-08 13:02     ` Peter Maydell
  2019-11-08 19:10       ` Eduardo Habkost
  0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2019-11-08 13:02 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, Cornelia Huck, Richard Henderson, QEMU Developers,
	Markus Armbruster, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Michael Mueller, Jiri Denemark, Eduardo Habkost

On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote:
> There is a small but important difference between "max"/"host" and
> "best". Max really means "all features", including deprecated ones.
> "best", however, can disable experimental or deprecated features. Or any
> other features we don't want to be enabled when somebody selects a model
> manually.
>
> On s390x, the feature "csske" is deprecated. New HW still has it, but we
> want new guests to run without this facility. Dropping it from "max"
> would affect existing setups. We already changed the default model
> (e.g., -cpu z13) to disable it with never QEMU machines.
>
> E.g., nested virtualization features on some architectures could be a
> feature set you want to disable, although contained in the "max" model.
> (e.g., no migration support yet).
>
>
> I am not completely against calling these "max" models instead of "best"
> models, but I think this makes it clearer that there is indeed a difference.

Hmm. I see the distinction, but is it one that's sufficiently
worth making that we want to expose it to our users, possibly
try to add it to the other architectures, etc ? How bad is it
if the CPU provides some legacy deprecated feature that the
guest just doesn't use ?

'max' already shouldn't include experimental features, at least
for Arm -- those should be off by default, because they're
experimental and you only want users to get them if they
explicitly opt in via '-cpu something,+x-my-feature'.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 11:07 [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
                   ` (2 preceding siblings ...)
  2019-11-08 11:10 ` [PATCH v1 0/2] " Peter Maydell
@ 2019-11-08 16:59 ` no-reply
  3 siblings, 0 replies; 12+ messages in thread
From: no-reply @ 2019-11-08 16:59 UTC (permalink / raw)
  To: david
  Cc: thuth, berrange, frankja, david, cohuck, richard.henderson,
	qemu-devel, armbru, pasic, borntraeger, qemu-s390x, mimu,
	jdenemar, ehabkost

Patchew URL: https://patchew.org/QEMU/20191108110714.7475-1-david@redhat.com/



Hi,

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

Subject: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
Type: series
Message-id: 20191108110714.7475-1-david@redhat.com

=== 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
 - [tag update]      patchew/20191108124219.31348-1-edgar.iglesias@gmail.com -> patchew/20191108124219.31348-1-edgar.iglesias@gmail.com
 - [tag update]      patchew/20191108150123.12213-1-marcandre.lureau@redhat.com -> patchew/20191108150123.12213-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
414a63f s390x/cpumodel: Introduce "best" model variants
40c0719 s390x/cpumodels: Factor out CPU feature dependencies

=== OUTPUT BEGIN ===
1/2 Checking commit 40c07194c720 (s390x/cpumodels: Factor out CPU feature dependencies)
2/2 Checking commit 414a63fe401d (s390x/cpumodel: Introduce "best" model variants)
ERROR: line over 90 characters
#167: FILE: target/s390x/cpu_models.c:1311:
+    xcc->desc = g_strdup_printf("%s with best features supported by the accelerator in the current host",

total: 1 errors, 0 warnings, 152 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 13:02     ` Peter Maydell
@ 2019-11-08 19:10       ` Eduardo Habkost
  2019-11-08 22:58         ` David Hildenbrand
  2019-11-09 16:07         ` Peter Maydell
  0 siblings, 2 replies; 12+ messages in thread
From: Eduardo Habkost @ 2019-11-08 19:10 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, David Hildenbrand, Cornelia Huck,
	Richard Henderson, QEMU Developers, Markus Armbruster,
	Halil Pasic, Christian Borntraeger, qemu-s390x, Michael Mueller,
	Jiri Denemark

On Fri, Nov 08, 2019 at 01:02:28PM +0000, Peter Maydell wrote:
> On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote:
> > There is a small but important difference between "max"/"host" and
> > "best". Max really means "all features", including deprecated ones.
> > "best", however, can disable experimental or deprecated features. Or any
> > other features we don't want to be enabled when somebody selects a model
> > manually.

On x86, this is implemented by "host".  "max" gives you the full
set of features that can be enabled by the user.  "host" gives
you a reasonable set of features you will want to see enabled by
default when the user says "use the host CPU".

> >
> > On s390x, the feature "csske" is deprecated. New HW still has it, but we
> > want new guests to run without this facility. Dropping it from "max"
> > would affect existing setups. We already changed the default model
> > (e.g., -cpu z13) to disable it with never QEMU machines.
> >
> > E.g., nested virtualization features on some architectures could be a
> > feature set you want to disable, although contained in the "max" model.
> > (e.g., no migration support yet).
> >
> >
> > I am not completely against calling these "max" models instead of "best"
> > models, but I think this makes it clearer that there is indeed a difference.
> 
> Hmm. I see the distinction, but is it one that's sufficiently
> worth making that we want to expose it to our users, possibly
> try to add it to the other architectures, etc ? How bad is it
> if the CPU provides some legacy deprecated feature that the
> guest just doesn't use ?
> 

"max" isn't something we want to expose to end users.  It is
something we need to expose to other software components.

> 'max' already shouldn't include experimental features, at least
> for Arm -- those should be off by default, because they're
> experimental and you only want users to get them if they
> explicitly opt in via '-cpu something,+x-my-feature'.

The whole point of "max" is to tell management software which
features are valid to be enabled in a host.  If "+x-my-feature"
works, "x-my-feature" must be included in "max".

-- 
Eduardo



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 11:07 ` [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
@ 2019-11-08 19:51   ` Eduardo Habkost
  2019-11-08 21:18     ` David Hildenbrand
  0 siblings, 1 reply; 12+ messages in thread
From: Eduardo Habkost @ 2019-11-08 19:51 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, Cornelia Huck, Richard Henderson, qemu-devel,
	Markus Armbruster, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Michael Mueller, Jiri Denemark

On Fri, Nov 08, 2019 at 12:07:14PM +0100, David Hildenbrand wrote:
> For a specific CPU model, we have a lot of feature variability depending on
> - The microcode version of the HW
> - The hypervisor we're running on (LPAR vs. KVM vs. z/VM)
> - The hypervisor version we're running on
> - The KVM version
> - KVM module parameters (especially, "nested=1")
> - The accelerator
> 
> Our default models are migration safe, however can only be changed
> between QEMU releases (glued to QEMU machine). This somewhat collides
> with the feature variability we have. E.g., the z13 model will not run
> under TCG. There is the demand from higher levels in the stack to "have the
> best CPU model possible on a given accelerator, firmware and HW", which
> should especially include all features that fix security issues.
> Especially, if we have a new feature due to a security flaw, we want to
> have a way to backport this feature to older QEMU versions and a way to
> automatically enable it when asked.
> 
> This is where "best" CPU models come into play. If upper layers specify
> "z14-best" on a z14, they will get the best possible feature set in that
> configuration. "best" usually means "maximum features", besides deprecated
> features. This will then, for example, include nested virtualization
> ("SIE" feature) when KVM+HW support is enabled, or fixes via
> microcode updates (e.g., spectre)
> 
> "best" models are not migration safe. Upper layers can expand these
> models to migration-safe and static variants, allowing them to be
> migrated.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Makes sense to me, and the code looks good.  I just have one
question below:

> ---
[...]
> +static void s390_best_cpu_model_initfn(Object *obj)
> +{
> +    const S390CPUModel *max_model;
> +    S390CPU *cpu = S390_CPU(obj);
> +    S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu);
> +    Error *local_err = NULL;
> +    int i;
> +
> +    if (kvm_enabled() && !kvm_s390_cpu_models_supported()) {
> +        return;
> +    }
> +
> +    max_model = get_max_cpu_model(&local_err);
> +    if (local_err) {
> +        /* we expect errors only under KVM, when actually querying the kernel */
> +        g_assert(kvm_enabled());
> +        error_report_err(local_err);
> +        return;
> +    }
> +
> +    /*
> +     * Similar to baselining against the "max" model. However, features
> +     * are handled differently and are not used for the search for a definition.
> +     */
> +    if (xcc->cpu_def->gen == max_model->def->gen) {
> +        if (xcc->cpu_def->ec_ga > max_model->def->ec_ga) {
> +            return;
> +        }
> +    } else if (xcc->cpu_def->gen > max_model->def->gen) {
> +        return;
> +    }

What exactly is expected to happen if we return from the function
here?

(In x86, we worked around the inability to report errors inside
instance_init by adding another step to CPU object initialization
called "CPU expansion", implemented by
x86_cpu_expand_features().)

> +
> +    /* The model is theoretically runnable, construct the features. */
> +    cpu->model = g_new(S390CPUModel, 1);
> +    cpu->model->def = xcc->cpu_def;
> +    bitmap_copy(cpu->model->features, xcc->cpu_def->full_feat, S390_FEAT_MAX);
> +
> +    /* Mask of features that are not available in the "max" model */
> +    bitmap_and(cpu->model->features, cpu->model->features, max_model->features,
> +               S390_FEAT_MAX);
> +
> +    /* Mask off deprecated features */
> +    clear_bit(S390_FEAT_CONDITIONAL_SSKE, cpu->model->features);
> +
> +    /* Make sure every model passes consistency checks */
> +    for (i = 0; i < ARRAY_SIZE(cpu_feature_dependencies); i++) {
> +        if (!test_bit(cpu_feature_dependencies[i][1], cpu->model->features)) {
> +            clear_bit(cpu_feature_dependencies[i][0], cpu->model->features);
> +        }
> +    }
> +}
[...]

-- 
Eduardo



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 19:51   ` Eduardo Habkost
@ 2019-11-08 21:18     ` David Hildenbrand
  0 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-11-08 21:18 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Thomas Huth, Daniel P. Berrangé,
	Janosch Frank, Cornelia Huck, Richard Henderson, qemu-devel,
	Markus Armbruster, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Michael Mueller, Jiri Denemark

On 08.11.19 20:51, Eduardo Habkost wrote:
> On Fri, Nov 08, 2019 at 12:07:14PM +0100, David Hildenbrand wrote:
>> For a specific CPU model, we have a lot of feature variability depending on
>> - The microcode version of the HW
>> - The hypervisor we're running on (LPAR vs. KVM vs. z/VM)
>> - The hypervisor version we're running on
>> - The KVM version
>> - KVM module parameters (especially, "nested=1")
>> - The accelerator
>>
>> Our default models are migration safe, however can only be changed
>> between QEMU releases (glued to QEMU machine). This somewhat collides
>> with the feature variability we have. E.g., the z13 model will not run
>> under TCG. There is the demand from higher levels in the stack to "have the
>> best CPU model possible on a given accelerator, firmware and HW", which
>> should especially include all features that fix security issues.
>> Especially, if we have a new feature due to a security flaw, we want to
>> have a way to backport this feature to older QEMU versions and a way to
>> automatically enable it when asked.
>>
>> This is where "best" CPU models come into play. If upper layers specify
>> "z14-best" on a z14, they will get the best possible feature set in that
>> configuration. "best" usually means "maximum features", besides deprecated
>> features. This will then, for example, include nested virtualization
>> ("SIE" feature) when KVM+HW support is enabled, or fixes via
>> microcode updates (e.g., spectre)
>>
>> "best" models are not migration safe. Upper layers can expand these
>> models to migration-safe and static variants, allowing them to be
>> migrated.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Makes sense to me, and the code looks good.  I just have one
> question below:
> 
>> ---
> [...]
>> +static void s390_best_cpu_model_initfn(Object *obj)
>> +{
>> +    const S390CPUModel *max_model;
>> +    S390CPU *cpu = S390_CPU(obj);
>> +    S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu);
>> +    Error *local_err = NULL;
>> +    int i;
>> +
>> +    if (kvm_enabled() && !kvm_s390_cpu_models_supported()) {
>> +        return;
>> +    }
>> +
>> +    max_model = get_max_cpu_model(&local_err);
>> +    if (local_err) {
>> +        /* we expect errors only under KVM, when actually querying the kernel */
>> +        g_assert(kvm_enabled());
>> +        error_report_err(local_err);
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Similar to baselining against the "max" model. However, features
>> +     * are handled differently and are not used for the search for a definition.
>> +     */
>> +    if (xcc->cpu_def->gen == max_model->def->gen) {
>> +        if (xcc->cpu_def->ec_ga > max_model->def->ec_ga) {
>> +            return;
>> +        }
>> +    } else if (xcc->cpu_def->gen > max_model->def->gen) {
>> +        return;
>> +    }
> 
> What exactly is expected to happen if we return from the function
> here?

cpu->model is NULL. That fact (and xcc->is_best) is checked when the 
model is to be used (e.g., via qmp or when creating VCPUs), and a rather 
generic error is reported. This is suboptimal and ...

> 
> (In x86, we worked around the inability to report errors inside
> instance_init by adding another step to CPU object initialization
> called "CPU expansion", implemented by
> x86_cpu_expand_features().)

... doing something like that makes a lot of sense. We also have to 
rework this for the "host" and "max" model.

I'll look into that when I'm back from holidays in one week.

Thanks!

-- 

Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 19:10       ` Eduardo Habkost
@ 2019-11-08 22:58         ` David Hildenbrand
  2019-11-09 16:07         ` Peter Maydell
  1 sibling, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-11-08 22:58 UTC (permalink / raw)
  To: Eduardo Habkost, Peter Maydell
  Cc: Thomas Huth, Daniel P. Berrangé,
	Janosch Frank, Cornelia Huck, Richard Henderson, QEMU Developers,
	Markus Armbruster, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Michael Mueller, Jiri Denemark

On 08.11.19 20:10, Eduardo Habkost wrote:
> On Fri, Nov 08, 2019 at 01:02:28PM +0000, Peter Maydell wrote:
>> On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote:
>>> There is a small but important difference between "max"/"host" and
>>> "best". Max really means "all features", including deprecated ones.
>>> "best", however, can disable experimental or deprecated features. Or any
>>> other features we don't want to be enabled when somebody selects a model
>>> manually.
> 
> On x86, this is implemented by "host".  "max" gives you the full
> set of features that can be enabled by the user.  "host" gives
> you a reasonable set of features you will want to see enabled by
> default when the user says "use the host CPU".

Interesting. Maybe for s390x we might want to split host/max at one 
time, too. E.g., exclude deprecated features from "host". Thanks for 
that info.

-- 

Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants
  2019-11-08 19:10       ` Eduardo Habkost
  2019-11-08 22:58         ` David Hildenbrand
@ 2019-11-09 16:07         ` Peter Maydell
  1 sibling, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2019-11-09 16:07 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Thomas Huth, Daniel P . Berrangé,
	Janosch Frank, David Hildenbrand, Cornelia Huck,
	Richard Henderson, QEMU Developers, Markus Armbruster,
	Halil Pasic, Christian Borntraeger, qemu-s390x, Michael Mueller,
	Jiri Denemark

On Fri, 8 Nov 2019 at 19:11, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Fri, Nov 08, 2019 at 01:02:28PM +0000, Peter Maydell wrote:
> > On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote:
> > > There is a small but important difference between "max"/"host" and
> > > "best". Max really means "all features", including deprecated ones.
> > > "best", however, can disable experimental or deprecated features. Or any
> > > other features we don't want to be enabled when somebody selects a model
> > > manually.
>
> On x86, this is implemented by "host".  "max" gives you the full
> set of features that can be enabled by the user.  "host" gives
> you a reasonable set of features you will want to see enabled by
> default when the user says "use the host CPU".

How does "host" work for TCG on x86?

> > Hmm. I see the distinction, but is it one that's sufficiently
> > worth making that we want to expose it to our users, possibly
> > try to add it to the other architectures, etc ? How bad is it
> > if the CPU provides some legacy deprecated feature that the
> > guest just doesn't use ?
> >
>
> "max" isn't something we want to expose to end users.  It is
> something we need to expose to other software components.

We seem to have a disagreement here about what 'max' is intended
for and what its semantics for. That seems unfortunate...

For Arm, "max" is absolutely something we want to expose to
end users. It's the easiest way for a user to say "give me
something that's going to work". "host" doesn't work on TCG,
only with KVM.

> > 'max' already shouldn't include experimental features, at least
> > for Arm -- those should be off by default, because they're
> > experimental and you only want users to get them if they
> > explicitly opt in via '-cpu something,+x-my-feature'.
>
> The whole point of "max" is to tell management software which
> features are valid to be enabled in a host.  If "+x-my-feature"
> works, "x-my-feature" must be included in "max".

No, definitely not. Experimental features should never be
enabled by default -- the user must explicitly opt into them
so they are aware that they're using something that might
change behaviour or go away in a future QEMU version.

Also, from my point of view "max" is not for the benefit
of management software in particular. It's a convenience
for users primarily (and also for management software if
it doesn't want to care whether it's running KVM or TCG).

If management software wants a way to ask "what features
might be valid" we should provide them with one which
doesn't require those features to be enabled-by-default
in the 'max' CPU.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 11:07 [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
2019-11-08 11:07 ` [PATCH v1 1/2] s390x/cpumodels: Factor out CPU feature dependencies David Hildenbrand
2019-11-08 11:07 ` [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants David Hildenbrand
2019-11-08 19:51   ` Eduardo Habkost
2019-11-08 21:18     ` David Hildenbrand
2019-11-08 11:10 ` [PATCH v1 0/2] " Peter Maydell
2019-11-08 12:46   ` David Hildenbrand
2019-11-08 13:02     ` Peter Maydell
2019-11-08 19:10       ` Eduardo Habkost
2019-11-08 22:58         ` David Hildenbrand
2019-11-09 16:07         ` Peter Maydell
2019-11-08 16:59 ` no-reply

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git