[PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

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

[PATCH v1 1/2] s390x/cpumodels: Factor out CPU feature dependencies

[David Hildenbrand]

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

[PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[no-reply]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Eduardo Habkost]

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

Re: [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants

[Eduardo Habkost]

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

Re: [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

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

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 09.11.19 17:07, Peter Maydell wrote:
> 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?

I think just like on s390x, host is limited to KVM only.

> 
>>> 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.

t460s: ~/git/qemu master $ s390x-softmmu/qemu-system-s390x -cpu help | 
grep max
s390 max             Enables all features supported by the accelerator 
in the current host

t460s: ~/git/qemu master $ x86_64-softmmu/qemu-system-x86_64 -cpu help | 
grep max
x86 max                   Enables all features supported by the 
accelerator in the current host

x86 and s390x interpret the "all features supported" as "possible in 
this configuration", not "supported" like in a support statement.

When not passing a "-cpu", you will automatically get the default model 
assigned (e.g., host vs. qemu model on s390x). "max" does no mimic that!

> 
>>> '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
> 

My personal opinion: "max" really means "all features". If we want an 
automatic way to specify something you requested ("give me something 
that's going to work") we either have to change the definition of the 
max model for alla rchitectures or introduce something that really 
matches the "no -cpu specified" - e.g., "best".

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote:
> My personal opinion: "max" really means "all features". If we want an
> automatic way to specify something you requested ("give me something
> that's going to work") we either have to change the definition of the
> max model for alla rchitectures or introduce something that really
> matches the "no -cpu specified" - e.g., "best".

I don't strongly object to 'max' including deprecated features,
but I do definitely object to 'max' including by default any
experimental (x- prefix) features. Those should never be
enabled (whatever the '-cpu foo' name) unless the user has
specifically opted into them: that's the point of the x- prefix.
We need to be able to tell from the command line whether it's
got any non-standard weirdness enabled.

thanks
-- PMM

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 18.11.19 11:53, Peter Maydell wrote:
> On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote:
>> My personal opinion: "max" really means "all features". If we want an
>> automatic way to specify something you requested ("give me something
>> that's going to work") we either have to change the definition of the
>> max model for alla rchitectures or introduce something that really
>> matches the "no -cpu specified" - e.g., "best".
> 
> I don't strongly object to 'max' including deprecated features,
> but I do definitely object to 'max' including by default any
> experimental (x- prefix) features. Those should never be
> enabled (whatever the '-cpu foo' name) unless the user has
> specifically opted into them: that's the point of the x- prefix.
> We need to be able to tell from the command line whether it's
> got any non-standard weirdness enabled.

I'll let Eduardo respond to that, as we don't really have experimental 
features on s390x, especially under KVM ("host" corresponds to "max").

> 
> thanks
> -- PMM
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

On Mon, 18 Nov 2019 at 10:56, David Hildenbrand <david@redhat.com> wrote:
>
> On 18.11.19 11:53, Peter Maydell wrote:
> > On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote:
> >> My personal opinion: "max" really means "all features". If we want an
> >> automatic way to specify something you requested ("give me something
> >> that's going to work") we either have to change the definition of the
> >> max model for alla rchitectures or introduce something that really
> >> matches the "no -cpu specified" - e.g., "best".
> >
> > I don't strongly object to 'max' including deprecated features,
> > but I do definitely object to 'max' including by default any
> > experimental (x- prefix) features. Those should never be
> > enabled (whatever the '-cpu foo' name) unless the user has
> > specifically opted into them: that's the point of the x- prefix.
> > We need to be able to tell from the command line whether it's
> > got any non-standard weirdness enabled.
>
> I'll let Eduardo respond to that, as we don't really have experimental
> features on s390x, especially under KVM ("host" corresponds to "max").

Yeah, I would expect that if the kernel has fixed the KVM
interface to a feature then it wouldn't be experimental.
Experimental mostly will apply to TCG, where we might
have implementations based on a draft version of an
architecture specification (like the riscv hypervisor spec)
that could incompatibly change in future.

thanks
-- PMM

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Eduardo Habkost]

On Mon, Nov 18, 2019 at 11:56:43AM +0100, David Hildenbrand wrote:
> On 18.11.19 11:53, Peter Maydell wrote:
> > On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote:
> > > My personal opinion: "max" really means "all features". If we want an
> > > automatic way to specify something you requested ("give me something
> > > that's going to work") we either have to change the definition of the
> > > max model for alla rchitectures or introduce something that really
> > > matches the "no -cpu specified" - e.g., "best".
> > 
> > I don't strongly object to 'max' including deprecated features,
> > but I do definitely object to 'max' including by default any
> > experimental (x- prefix) features. Those should never be
> > enabled (whatever the '-cpu foo' name) unless the user has
> > specifically opted into them: that's the point of the x- prefix.
> > We need to be able to tell from the command line whether it's
> > got any non-standard weirdness enabled.
> 
> I'll let Eduardo respond to that, as we don't really have experimental
> features on s390x, especially under KVM ("host" corresponds to "max").

Be them experimental or deprecated, we need all features included
on "max" if we want to make them usable through libvirt.  The
fact Peter cares about defaults in "max" when used by humans
indicates we have incompatible definitions of "max", and I don't
think we can conciliate them.

We could rename the CPU model that is intended for humans on arm,
or we can document clearly that the semantics of "max" in
x86/s390x are completely different from arm.  Peter, what do you
prefer?

-- 
Eduardo

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

On Mon, 18 Nov 2019 at 18:49, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Be them experimental or deprecated, we need all features included
> on "max" if we want to make them usable through libvirt.  The
> fact Peter cares about defaults in "max" when used by humans
> indicates we have incompatible definitions of "max", and I don't
> think we can conciliate them.
>
> We could rename the CPU model that is intended for humans on arm,
> or we can document clearly that the semantics of "max" in
> x86/s390x are completely different from arm.  Peter, what do you
> prefer?

I don't want us to have different definitions of max on different
architectures if we can avoid it. More importantly, I don't think that
x- features should ever be enabled by default for *any* CPU type
on *any* architecture (unless the CPU type itself was experimental,
I suppose), whatever that architecture's semantics for 'max',
'best', etc are.

I think the solution to this is to not use 'max' as some odd way of
letting libvirt do feature detection. I'm not sure how trying to
use 'max' as a proxy for "all features on" would work for features
which can't exist on 'max' but do exist on other CPU types
(for instance for Arm some features make no sense on the
A-profile 'max' CPU and aren't provided there, but do exist for
M-profile CPUs like cortex-m4), so I don't see how libvirt
can usefully use it anyway.

Why should it matter whether a feature is enabled
or disabled by default in the CPU type? It ought to be probeable
by QMP either way (ie there is a difference between
"this CPU has this feature switch and it is on by default",
"this CPU has this feature switch and it is off by default"
and "this CPU does not have this feature switch at all",
and presumably libvirt needs to distinguish them).

thanks
-- PMM

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Eduardo Habkost]

On Mon, Nov 18, 2019 at 09:19:55PM +0000, Peter Maydell wrote:
> On Mon, 18 Nov 2019 at 18:49, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > Be them experimental or deprecated, we need all features included
> > on "max" if we want to make them usable through libvirt.  The
> > fact Peter cares about defaults in "max" when used by humans
> > indicates we have incompatible definitions of "max", and I don't
> > think we can conciliate them.
> >
> > We could rename the CPU model that is intended for humans on arm,
> > or we can document clearly that the semantics of "max" in
> > x86/s390x are completely different from arm.  Peter, what do you
> > prefer?
> 
> I don't want us to have different definitions of max on different
> architectures if we can avoid it. More importantly, I don't think that
> x- features should ever be enabled by default for *any* CPU type
> on *any* architecture (unless the CPU type itself was experimental,
> I suppose), whatever that architecture's semantics for 'max',
> 'best', etc are.
> 
> I think the solution to this is to not use 'max' as some odd way of
> letting libvirt do feature detection. I'm not sure how trying to
> use 'max' as a proxy for "all features on" would work for features
> which can't exist on 'max' but do exist on other CPU types
> (for instance for Arm some features make no sense on the
> A-profile 'max' CPU and aren't provided there, but do exist for
> M-profile CPUs like cortex-m4), so I don't see how libvirt
> can usefully use it anyway.

libvirt uses it through query-cpu-model-expansion.

> 
> Why should it matter whether a feature is enabled
> or disabled by default in the CPU type? It ought to be probeable
> by QMP either way (ie there is a difference between
> "this CPU has this feature switch and it is on by default",
> "this CPU has this feature switch and it is off by default"
> and "this CPU does not have this feature switch at all",
> and presumably libvirt needs to distinguish them).

Its use case is neither "this CPU has this feature switch" or for
"it is on|off by default".  We use it to probe for "this feature
can be enabled in this host hardware+kernel+QEMU combination".

In other words, in x86 and s390x "max" is just a reserved name
for the query-cpu-model-expansion command arguments in s390x and
x86.  The fact that it is visible to users can be considered a
bug, and we can fix that.

If you still don't like how query-cpu-model-expansion works, we
can also discuss that.  But I'm not sure it would be a good use
of our (and libvirt developers') time.

-- 
Eduardo

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

On Mon, 18 Nov 2019 at 22:04, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Mon, Nov 18, 2019 at 09:19:55PM +0000, Peter Maydell wrote:
> > Why should it matter whether a feature is enabled
> > or disabled by default in the CPU type? It ought to be probeable
> > by QMP either way (ie there is a difference between
> > "this CPU has this feature switch and it is on by default",
> > "this CPU has this feature switch and it is off by default"
> > and "this CPU does not have this feature switch at all",
> > and presumably libvirt needs to distinguish them).
>
> Its use case is neither "this CPU has this feature switch" or for
> "it is on|off by default".  We use it to probe for "this feature
> can be enabled in this host hardware+kernel+QEMU combination".

OK. Well, the answer to that depends on the name of the CPU,
in general. So you can't use a fake CPU name to try to answer
the question.

> In other words, in x86 and s390x "max" is just a reserved name
> for the query-cpu-model-expansion command arguments in s390x and
> x86.  The fact that it is visible to users can be considered a
> bug, and we can fix that.

I think 'max' is useful to users, and we've provided it to users,
so removing it again would be a compatibility break. I'm not
entirely sure where we go from here...

> If you still don't like how query-cpu-model-expansion works, we
> can also discuss that.  But I'm not sure it would be a good use
> of our (and libvirt developers') time.

I don't hugely care about query-cpu-model-expansion. I
just don't want it to have bad effects on the semantics
of user-facing stuff like x- properties.

thanks
-- PMM

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 19.11.19 10:22, Peter Maydell wrote:
> On Mon, 18 Nov 2019 at 22:04, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>> On Mon, Nov 18, 2019 at 09:19:55PM +0000, Peter Maydell wrote:
>>> Why should it matter whether a feature is enabled
>>> or disabled by default in the CPU type? It ought to be probeable
>>> by QMP either way (ie there is a difference between
>>> "this CPU has this feature switch and it is on by default",
>>> "this CPU has this feature switch and it is off by default"
>>> and "this CPU does not have this feature switch at all",
>>> and presumably libvirt needs to distinguish them).
>>
>> Its use case is neither "this CPU has this feature switch" or for
>> "it is on|off by default".  We use it to probe for "this feature
>> can be enabled in this host hardware+kernel+QEMU combination".
> 
> OK. Well, the answer to that depends on the name of the CPU,
> in general. So you can't use a fake CPU name to try to answer
> the question.
> 
>> In other words, in x86 and s390x "max" is just a reserved name
>> for the query-cpu-model-expansion command arguments in s390x and
>> x86.  The fact that it is visible to users can be considered a
>> bug, and we can fix that.
> 
> I think 'max' is useful to users, and we've provided it to users,
> so removing it again would be a compatibility break. I'm not
> entirely sure where we go from here...

I agree, right now on s390x "-cpu max" behaves almost like "-cpu 
qemu"/"-cpu host", but you don't have to care about the accelerator. 
Removing it might we evil. Removing it is not really an option.

> 
>> If you still don't like how query-cpu-model-expansion works, we
>> can also discuss that.  But I'm not sure it would be a good use
>> of our (and libvirt developers') time.
> 
> I don't hugely care about query-cpu-model-expansion. I
> just don't want it to have bad effects on the semantics
> of user-facing stuff like x- properties.

IMHO, max should really include all features (yes, also the bad 
x-features on arm :) ) and we should have a way to give users the 
opportunity to specify "just give me the best model independent of the 
accelerator" - something like a "best" model, but I don't care about the 
name.

E.g., we could introduce a new model for that purpose and start warning 
if "-cpu max" is used (and recommend them to use the other model) to run 
a VM and not simply to query model properties via the qmp interfaces. 
The users are aware of the model restrictions and can switch.

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Peter Maydell]

On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote:
>
> On 19.11.19 10:22, Peter Maydell wrote:
> > I don't hugely care about query-cpu-model-expansion. I
> > just don't want it to have bad effects on the semantics
> > of user-facing stuff like x- properties.
>
> IMHO, max should really include all features (yes, also the bad
> x-features on arm :) ) and we should have a way to give users the
> opportunity to specify "just give me the best model independent of the
> accelerator" - something like a "best" model, but I don't care about the
> name.

How would "max includes all features" work if we have two
x- features (or even two normal features!) which are incompatible
with each other? How does it work for features which are
valid for some other CPU type but not for 'max'? The design
seems to assume a rather simplified system where every
feature is independent and can always be applied to every
CPU, which I don't think is guaranteed to be the case.

thanks
-- PMM

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 19.11.19 11:36, Peter Maydell wrote:
> On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 19.11.19 10:22, Peter Maydell wrote:
>>> I don't hugely care about query-cpu-model-expansion. I
>>> just don't want it to have bad effects on the semantics
>>> of user-facing stuff like x- properties.
>>
>> IMHO, max should really include all features (yes, also the bad
>> x-features on arm :) ) and we should have a way to give users the
>> opportunity to specify "just give me the best model independent of the
>> accelerator" - something like a "best" model, but I don't care about the
>> name.
> 
> How would "max includes all features" work if we have two
> x- features (or even two normal features!) which are incompatible
> with each other? How does it work for features which are

I assume the "max" model should at least be consistent, see below.

> valid for some other CPU type but not for 'max'? The design
> seems to assume a rather simplified system where every
> feature is independent and can always be applied to every
> CPU, which I don't think is guaranteed to be the case.

I do agree that the use case of "max" for detecting which features can 
be enabled for any CPU model is quite simplified and would also not work 
like this on s390x. It really means "give me the maximum/latest/greatest 
you can". Some examples on s390x:

On s390x, we don't allow to enable new features for older CPU 
generation. "vx" was introduced with a z13. If "max" is a z13, it can 
include "vx", if available. However, if you select a z12 (zEC12), you 
cannot enable "vx":

"Feature '%s' is not available for CPU model '%s', it was introduced 
with later models."

Also, we have dependency checks 
(target/s390x/cpu_models.c:check_consistency()), that at least warn on 
inconsistencies between features (feature X requires feature Y).

We would need more fine-grained "max" models. E.g., z13-max vs. z13-best 
vs. z13-base.

- z13-max: Maximum features that can be enabled on the current
            accel/host for a z13.
- z13-best: Best features that can be enabled on the current
            accel/host for a z13.
- z13-base: base feature set, independent of current
            accel/host for a z13. (we do have this already on s390x)

> 
> thanks
> -- PMM
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Eduardo Habkost]

On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote:
> On 19.11.19 11:36, Peter Maydell wrote:
> > On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote:
> > > 
> > > On 19.11.19 10:22, Peter Maydell wrote:
> > > > I don't hugely care about query-cpu-model-expansion. I
> > > > just don't want it to have bad effects on the semantics
> > > > of user-facing stuff like x- properties.
> > > 
> > > IMHO, max should really include all features (yes, also the bad
> > > x-features on arm :) ) and we should have a way to give users the
> > > opportunity to specify "just give me the best model independent of the
> > > accelerator" - something like a "best" model, but I don't care about the
> > > name.

I'm in agreement with Peter, here: if "max" is user-visible, it's
better to make it provide more usable defaults.


> > 
> > How would "max includes all features" work if we have two
> > x- features (or even two normal features!) which are incompatible
> > with each other? How does it work for features which are
> 
> I assume the "max" model should at least be consistent, see below.
> 
> > valid for some other CPU type but not for 'max'? The design
> > seems to assume a rather simplified system where every
> > feature is independent and can always be applied to every
> > CPU, which I don't think is guaranteed to be the case.
> 
> I do agree that the use case of "max" for detecting which features can be
> enabled for any CPU model is quite simplified and would also not work like
> this on s390x. It really means "give me the maximum/latest/greatest you
> can". Some examples on s390x:
> 
> On s390x, we don't allow to enable new features for older CPU generation.
> "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if
> available. However, if you select a z12 (zEC12), you cannot enable "vx":
> 
> "Feature '%s' is not available for CPU model '%s', it was introduced with
> later models."
> 
> Also, we have dependency checks
> (target/s390x/cpu_models.c:check_consistency()), that at least warn on
> inconsistencies between features (feature X requires feature Y).
> 
> We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs.
> z13-base.
> 
> - z13-max: Maximum features that can be enabled on the current
>            accel/host for a z13.
> - z13-best: Best features that can be enabled on the current
>            accel/host for a z13.
> - z13-base: base feature set, independent of current
>            accel/host for a z13. (we do have this already on s390x)

We don't need to create new CPU types for that, do we?  These
different modes could be configured by a CPU option (e.g.
"z13,features=max"[1], "z13,features=best").

If we do add new CPU options to tune feature defaults, libvirt
can start using these options on query-cpu-model-expansion, and
everybody will be happy.  No need to change defaults in the "max"
CPU model in a way that makes it less usable just to make
query-cpu-model-expansion work.

[1] Probably we need to call it something else instead of
    "features=max", just to avoid confusion with the "max" CPU
    model.  Maybe "experimental-features=on",
    "recommended-features=on"?

-- 
Eduardo

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 19.11.19 20:42, Eduardo Habkost wrote:
> On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote:
>> On 19.11.19 11:36, Peter Maydell wrote:
>>> On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 19.11.19 10:22, Peter Maydell wrote:
>>>>> I don't hugely care about query-cpu-model-expansion. I
>>>>> just don't want it to have bad effects on the semantics
>>>>> of user-facing stuff like x- properties.
>>>>
>>>> IMHO, max should really include all features (yes, also the bad
>>>> x-features on arm :) ) and we should have a way to give users the
>>>> opportunity to specify "just give me the best model independent of the
>>>> accelerator" - something like a "best" model, but I don't care about the
>>>> name.
> 
> I'm in agreement with Peter, here: if "max" is user-visible, it's
> better to make it provide more usable defaults.

Agreed, unless we warn the user about the model.

>>>
>>> How would "max includes all features" work if we have two
>>> x- features (or even two normal features!) which are incompatible
>>> with each other? How does it work for features which are
>>
>> I assume the "max" model should at least be consistent, see below.
>>
>>> valid for some other CPU type but not for 'max'? The design
>>> seems to assume a rather simplified system where every
>>> feature is independent and can always be applied to every
>>> CPU, which I don't think is guaranteed to be the case.
>>
>> I do agree that the use case of "max" for detecting which features can be
>> enabled for any CPU model is quite simplified and would also not work like
>> this on s390x. It really means "give me the maximum/latest/greatest you
>> can". Some examples on s390x:
>>
>> On s390x, we don't allow to enable new features for older CPU generation.
>> "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if
>> available. However, if you select a z12 (zEC12), you cannot enable "vx":
>>
>> "Feature '%s' is not available for CPU model '%s', it was introduced with
>> later models."
>>
>> Also, we have dependency checks
>> (target/s390x/cpu_models.c:check_consistency()), that at least warn on
>> inconsistencies between features (feature X requires feature Y).
>>
>> We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs.
>> z13-base.
>>
>> - z13-max: Maximum features that can be enabled on the current
>>             accel/host for a z13.
>> - z13-best: Best features that can be enabled on the current
>>             accel/host for a z13.
>> - z13-base: base feature set, independent of current
>>             accel/host for a z13. (we do have this already on s390x)
> 
> We don't need to create new CPU types for that, do we?  These
> different modes could be configured by a CPU option (e.g.
> "z13,features=max"[1], "z13,features=best").

I somewhat don't like such options mixed into ordinary feature 
configuration if we can avoid it. It allows for things like

z13,features=max,features=best

The z13 model is migration-safe. So would be "z13,vx=off". 
"z13,features=best" would no longer be migration safe.

IOW, reusing an existing model along with that feels wrong, read below. 
Especially, I dislike that one config option (features=best) disables 
and enables features at the same time. Read below.

> 
> If we do add new CPU options to tune feature defaults, libvirt
> can start using these options on query-cpu-model-expansion, and
> everybody will be happy.  No need to change defaults in the "max"
> CPU model in a way that makes it less usable just to make
> query-cpu-model-expansion work.
> 
> [1] Probably we need to call it something else instead of
>      "features=max", just to avoid confusion with the "max" CPU
>      model.  Maybe "experimental-features=on",
>      "recommended-features=on"?
We already do have feature groups on s390x. So these could behave like 
"host/accelerator dependent" feature groups. I would prefer that over 
the suggestion above.

The only downside is that z13,recommended-features=on could actually 
"disable" features that are already in z13 (e.g., "vx" is part of z13, 
but if it's not available in the host we would have to disable it). I 
don't like these semantic. Maybe introducing new types of models that 
don't have any features as default could come into play.

E.g.,

z13-best => z13-raw,recommended-features=on
z13-max => z13-raw,recommended-features=on,experimental-features=on

Maybe we can find a better name for "recommended-features" and 
"experimental-features" to highlight that these are only "features 
available via the accelerator in the current host"

We could also expose:

z13-full => z13-raw,all-features=on

Which would include all features theoretically valid for a model (but 
even if not available).

-- 

Thanks,

David / dhildenb

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[Eduardo Habkost]

On Wed, Nov 20, 2019 at 11:28:29AM +0100, David Hildenbrand wrote:
> On 19.11.19 20:42, Eduardo Habkost wrote:
> > On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote:
> > > On 19.11.19 11:36, Peter Maydell wrote:
> > > > On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote:
> > > > > 
> > > > > On 19.11.19 10:22, Peter Maydell wrote:
> > > > > > I don't hugely care about query-cpu-model-expansion. I
> > > > > > just don't want it to have bad effects on the semantics
> > > > > > of user-facing stuff like x- properties.
> > > > > 
> > > > > IMHO, max should really include all features (yes, also the bad
> > > > > x-features on arm :) ) and we should have a way to give users the
> > > > > opportunity to specify "just give me the best model independent of the
> > > > > accelerator" - something like a "best" model, but I don't care about the
> > > > > name.
> > 
> > I'm in agreement with Peter, here: if "max" is user-visible, it's
> > better to make it provide more usable defaults.
> 
> Agreed, unless we warn the user about the model.
> 
> > > > 
> > > > How would "max includes all features" work if we have two
> > > > x- features (or even two normal features!) which are incompatible
> > > > with each other? How does it work for features which are
> > > 
> > > I assume the "max" model should at least be consistent, see below.
> > > 
> > > > valid for some other CPU type but not for 'max'? The design
> > > > seems to assume a rather simplified system where every
> > > > feature is independent and can always be applied to every
> > > > CPU, which I don't think is guaranteed to be the case.
> > > 
> > > I do agree that the use case of "max" for detecting which features can be
> > > enabled for any CPU model is quite simplified and would also not work like
> > > this on s390x. It really means "give me the maximum/latest/greatest you
> > > can". Some examples on s390x:
> > > 
> > > On s390x, we don't allow to enable new features for older CPU generation.
> > > "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if
> > > available. However, if you select a z12 (zEC12), you cannot enable "vx":
> > > 
> > > "Feature '%s' is not available for CPU model '%s', it was introduced with
> > > later models."
> > > 
> > > Also, we have dependency checks
> > > (target/s390x/cpu_models.c:check_consistency()), that at least warn on
> > > inconsistencies between features (feature X requires feature Y).
> > > 
> > > We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs.
> > > z13-base.
> > > 
> > > - z13-max: Maximum features that can be enabled on the current
> > >             accel/host for a z13.
> > > - z13-best: Best features that can be enabled on the current
> > >             accel/host for a z13.
> > > - z13-base: base feature set, independent of current
> > >             accel/host for a z13. (we do have this already on s390x)
> > 
> > We don't need to create new CPU types for that, do we?  These
> > different modes could be configured by a CPU option (e.g.
> > "z13,features=max"[1], "z13,features=best").
> 
> I somewhat don't like such options mixed into ordinary feature configuration
> if we can avoid it. It allows for things like
> 
> z13,features=max,features=best
> 
> The z13 model is migration-safe. So would be "z13,vx=off".
> "z13,features=best" would no longer be migration safe.
> 
> IOW, reusing an existing model along with that feels wrong, read below.
> Especially, I dislike that one config option (features=best) disables and
> enables features at the same time. Read below.
> 
> > 
> > If we do add new CPU options to tune feature defaults, libvirt
> > can start using these options on query-cpu-model-expansion, and
> > everybody will be happy.  No need to change defaults in the "max"
> > CPU model in a way that makes it less usable just to make
> > query-cpu-model-expansion work.
> > 
> > [1] Probably we need to call it something else instead of
> >      "features=max", just to avoid confusion with the "max" CPU
> >      model.  Maybe "experimental-features=on",
> >      "recommended-features=on"?
> We already do have feature groups on s390x. So these could behave like
> "host/accelerator dependent" feature groups. I would prefer that over the
> suggestion above.
> 
> The only downside is that z13,recommended-features=on could actually
> "disable" features that are already in z13 (e.g., "vx" is part of z13, but
> if it's not available in the host we would have to disable it). I don't like
> these semantic. Maybe introducing new types of models that don't have any
> features as default could come into play.
> 
> E.g.,
> 
> z13-best => z13-raw,recommended-features=on
> z13-max => z13-raw,recommended-features=on,experimental-features=on
> 
> Maybe we can find a better name for "recommended-features" and
> "experimental-features" to highlight that these are only "features available
> via the accelerator in the current host"
> 
> We could also expose:
> 
> z13-full => z13-raw,all-features=on
> 
> Which would include all features theoretically valid for a model (but even
> if not available).

I don't have a strong opinion on the specifics.  If you believe
that's the best solution, I trust your judgement.

My only point was that we don't always need to add new CPU types
for every conceivable use case, and some (but not all) problems
can be solved by simple new properties.

(To be honest, I'm now having trouble mapping these ideas to real
world problems or use cases.  What are the specific problems
we're trying to solve right now?)

-- 
Eduardo

Re: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 20.11.19 15:04, Eduardo Habkost wrote:
> On Wed, Nov 20, 2019 at 11:28:29AM +0100, David Hildenbrand wrote:
>> On 19.11.19 20:42, Eduardo Habkost wrote:
>>> On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote:
>>>> On 19.11.19 11:36, Peter Maydell wrote:
>>>>> On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote:
>>>>>>
>>>>>> On 19.11.19 10:22, Peter Maydell wrote:
>>>>>>> I don't hugely care about query-cpu-model-expansion. I
>>>>>>> just don't want it to have bad effects on the semantics
>>>>>>> of user-facing stuff like x- properties.
>>>>>>
>>>>>> IMHO, max should really include all features (yes, also the bad
>>>>>> x-features on arm :) ) and we should have a way to give users the
>>>>>> opportunity to specify "just give me the best model independent of the
>>>>>> accelerator" - something like a "best" model, but I don't care about the
>>>>>> name.
>>>
>>> I'm in agreement with Peter, here: if "max" is user-visible, it's
>>> better to make it provide more usable defaults.
>>
>> Agreed, unless we warn the user about the model.
>>
>>>>>
>>>>> How would "max includes all features" work if we have two
>>>>> x- features (or even two normal features!) which are incompatible
>>>>> with each other? How does it work for features which are
>>>>
>>>> I assume the "max" model should at least be consistent, see below.
>>>>
>>>>> valid for some other CPU type but not for 'max'? The design
>>>>> seems to assume a rather simplified system where every
>>>>> feature is independent and can always be applied to every
>>>>> CPU, which I don't think is guaranteed to be the case.
>>>>
>>>> I do agree that the use case of "max" for detecting which features can be
>>>> enabled for any CPU model is quite simplified and would also not work like
>>>> this on s390x. It really means "give me the maximum/latest/greatest you
>>>> can". Some examples on s390x:
>>>>
>>>> On s390x, we don't allow to enable new features for older CPU generation.
>>>> "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if
>>>> available. However, if you select a z12 (zEC12), you cannot enable "vx":
>>>>
>>>> "Feature '%s' is not available for CPU model '%s', it was introduced with
>>>> later models."
>>>>
>>>> Also, we have dependency checks
>>>> (target/s390x/cpu_models.c:check_consistency()), that at least warn on
>>>> inconsistencies between features (feature X requires feature Y).
>>>>
>>>> We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs.
>>>> z13-base.
>>>>
>>>> - z13-max: Maximum features that can be enabled on the current
>>>>              accel/host for a z13.
>>>> - z13-best: Best features that can be enabled on the current
>>>>              accel/host for a z13.
>>>> - z13-base: base feature set, independent of current
>>>>              accel/host for a z13. (we do have this already on s390x)
>>>
>>> We don't need to create new CPU types for that, do we?  These
>>> different modes could be configured by a CPU option (e.g.
>>> "z13,features=max"[1], "z13,features=best").
>>
>> I somewhat don't like such options mixed into ordinary feature configuration
>> if we can avoid it. It allows for things like
>>
>> z13,features=max,features=best
>>
>> The z13 model is migration-safe. So would be "z13,vx=off".
>> "z13,features=best" would no longer be migration safe.
>>
>> IOW, reusing an existing model along with that feels wrong, read below.
>> Especially, I dislike that one config option (features=best) disables and
>> enables features at the same time. Read below.
>>
>>>
>>> If we do add new CPU options to tune feature defaults, libvirt
>>> can start using these options on query-cpu-model-expansion, and
>>> everybody will be happy.  No need to change defaults in the "max"
>>> CPU model in a way that makes it less usable just to make
>>> query-cpu-model-expansion work.
>>>
>>> [1] Probably we need to call it something else instead of
>>>       "features=max", just to avoid confusion with the "max" CPU
>>>       model.  Maybe "experimental-features=on",
>>>       "recommended-features=on"?
>> We already do have feature groups on s390x. So these could behave like
>> "host/accelerator dependent" feature groups. I would prefer that over the
>> suggestion above.
>>
>> The only downside is that z13,recommended-features=on could actually
>> "disable" features that are already in z13 (e.g., "vx" is part of z13, but
>> if it's not available in the host we would have to disable it). I don't like
>> these semantic. Maybe introducing new types of models that don't have any
>> features as default could come into play.
>>
>> E.g.,
>>
>> z13-best => z13-raw,recommended-features=on
>> z13-max => z13-raw,recommended-features=on,experimental-features=on
>>
>> Maybe we can find a better name for "recommended-features" and
>> "experimental-features" to highlight that these are only "features available
>> via the accelerator in the current host"
>>
>> We could also expose:
>>
>> z13-full => z13-raw,all-features=on
>>
>> Which would include all features theoretically valid for a model (but even
>> if not available).
> 
> I don't have a strong opinion on the specifics.  If you believe
> that's the best solution, I trust your judgement.
> 
> My only point was that we don't always need to add new CPU types
> for every conceivable use case, and some (but not all) problems
> can be solved by simple new properties.

Makes sense.

> 
> (To be honest, I'm now having trouble mapping these ideas to real
> world problems or use cases.  What are the specific problems
> we're trying to solve right now?)

I mostly only care about z13-best or "z13-raw,recommended-features=on" 
for now. What's described in patch #2. Allow upper layers (or user on 
the command line) get a CPU model that has the best features supported 
by the accelerator on the current host. Esp., include new features to 
mitigate security issues.


-- 

Thanks,

David / dhildenb