Currently we only present the plain z900 feature bits to the guest,
but QEMU already emulates some additional features (but not all of
the next CPU generation, so we can not use the next CPU level as
default yet). Since newer Linux kernels are checking the feature bits
and refuse to work if a required feature is missing, we should present
as much of the supported features as possible when we are running
with the default "qemu" CPU.
This patch now adds the "stfle", "extended immediate" and "stckf"
facility bits to the "qemu" CPU, which are already supported facilities.
It is unfortunately still not enough to run e.g. recent Fedora kernels,
but at least it's a first step into the right direction.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/s390x/cpu_models.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 8d27363..18789ab 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -658,6 +658,24 @@ static void check_compatibility(const S390CPUModel *max_model,
                   "available in the configuration: ");
 }
 
+/**
+ * The base TCG CPU model "qemu" is based on the z900. However, we already
+ * can also emulate some additional features of later CPU generations, so
+ * we add these additional feature bits here.
+ */
+static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
+{
+    int i, feats[] = {
+        S390_FEAT_STFLE,
+        S390_FEAT_EXTENDED_IMMEDIATE,
+        S390_FEAT_STORE_CLOCK_FAST
+    };
+
+    for (i = 0; i < ARRAY_SIZE(feats); i++) {
+        set_bit(feats[i], fbm);
+    }
+}
+
 static S390CPUModel *get_max_cpu_model(Error **errp)
 {
     static S390CPUModel max_model;
@@ -670,10 +688,11 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
     if (kvm_enabled()) {
         kvm_s390_get_host_cpu_model(&max_model, errp);
     } else {
-        /* TCG emulates a z900 */
+        /* TCG emulates a z900 (with some additional features) */
         max_model.def = &s390_cpu_defs[0];
         bitmap_copy(max_model.features, max_model.def->default_feat,
                     S390_FEAT_MAX);
+        add_qemu_cpu_model_features(max_model.features);
     }
     if (!*errp) {
         cached = true;
@@ -925,11 +944,15 @@ static void s390_host_cpu_model_initfn(Object *obj)
 
 static void s390_qemu_cpu_model_initfn(Object *obj)
 {
+    static S390CPUDef s390_qemu_cpu_defs;
     S390CPU *cpu = S390_CPU(obj);
 
     cpu->model = g_malloc0(sizeof(*cpu->model));
-    /* TCG emulates a z900 */
-    cpu->model->def = &s390_cpu_defs[0];
+    /* TCG emulates a z900 (with some additional features) */
+    memcpy(&s390_qemu_cpu_defs, &s390_cpu_defs[0], sizeof(s390_qemu_cpu_defs));
+    add_qemu_cpu_model_features(s390_qemu_cpu_defs.default_feat);
+    add_qemu_cpu_model_features(s390_qemu_cpu_defs.full_feat);
+    cpu->model->def = &s390_qemu_cpu_defs;
     bitmap_copy(cpu->model->features, cpu->model->def->default_feat,
                 S390_FEAT_MAX);
 }
-- 
1.8.3.1

On 2017-05-17 17:35, Thomas Huth wrote:
> Currently we only present the plain z900 feature bits to the guest,
> but QEMU already emulates some additional features (but not all of
> the next CPU generation, so we can not use the next CPU level as
> default yet). Since newer Linux kernels are checking the feature bits
> and refuse to work if a required feature is missing, we should present
> as much of the supported features as possible when we are running
> with the default "qemu" CPU.
> This patch now adds the "stfle", "extended immediate" and "stckf"
> facility bits to the "qemu" CPU, which are already supported facilities.
> It is unfortunately still not enough to run e.g. recent Fedora kernels,
> but at least it's a first step into the right direction.

This indeed looks like a step in a good direction. At least it makes the
recently added STFLE emulation useful :-).

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/cpu_models.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 8d27363..18789ab 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -658,6 +658,24 @@ static void check_compatibility(const S390CPUModel *max_model,
>                    "available in the configuration: ");
>  }
>  
> +/**
> + * The base TCG CPU model "qemu" is based on the z900. However, we already
> + * can also emulate some additional features of later CPU generations, so
> + * we add these additional feature bits here.
> + */
> +static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
> +{
> +    int i, feats[] = {
> +        S390_FEAT_STFLE,
> +        S390_FEAT_EXTENDED_IMMEDIATE,
> +        S390_FEAT_STORE_CLOCK_FAST

According to my list, QEMU also fully implements the following
facilities:

S390_FEAT_LONG_DISPLACEMENT
S390_FEAT_GENERAL_INSTRUCTIONS_EXT
S390_FEAT_EXECUTE_EXT
S390_FEAT_STFLE_45

> +    };
> +
> +    for (i = 0; i < ARRAY_SIZE(feats); i++) {
> +        set_bit(feats[i], fbm);
> +    }
> +}
> +
>  static S390CPUModel *get_max_cpu_model(Error **errp)
>  {
>      static S390CPUModel max_model;
> @@ -670,10 +688,11 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>      if (kvm_enabled()) {
>          kvm_s390_get_host_cpu_model(&max_model, errp);
>      } else {
> -        /* TCG emulates a z900 */
> +        /* TCG emulates a z900 (with some additional features) */
>          max_model.def = &s390_cpu_defs[0];
>          bitmap_copy(max_model.features, max_model.def->default_feat,
>                      S390_FEAT_MAX);
> +        add_qemu_cpu_model_features(max_model.features);
>      }
>      if (!*errp) {
>          cached = true;
> @@ -925,11 +944,15 @@ static void s390_host_cpu_model_initfn(Object *obj)
>  
>  static void s390_qemu_cpu_model_initfn(Object *obj)
>  {
> +    static S390CPUDef s390_qemu_cpu_defs;
>      S390CPU *cpu = S390_CPU(obj);
>  
>      cpu->model = g_malloc0(sizeof(*cpu->model));
> -    /* TCG emulates a z900 */
> -    cpu->model->def = &s390_cpu_defs[0];
> +    /* TCG emulates a z900 (with some additional features) */
> +    memcpy(&s390_qemu_cpu_defs, &s390_cpu_defs[0], sizeof(s390_qemu_cpu_defs));
> +    add_qemu_cpu_model_features(s390_qemu_cpu_defs.default_feat);
> +    add_qemu_cpu_model_features(s390_qemu_cpu_defs.full_feat);
> +    cpu->model->def = &s390_qemu_cpu_defs;
>      bitmap_copy(cpu->model->features, cpu->model->def->default_feat,
>                  S390_FEAT_MAX);
>  }

Otherwise it looks fine to me.

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

On 17.05.2017 18:26, Aurelien Jarno wrote:
> On 2017-05-17 17:35, Thomas Huth wrote:
>> Currently we only present the plain z900 feature bits to the guest,
>> but QEMU already emulates some additional features (but not all of
>> the next CPU generation, so we can not use the next CPU level as
>> default yet). Since newer Linux kernels are checking the feature bits
>> and refuse to work if a required feature is missing, we should present
>> as much of the supported features as possible when we are running
>> with the default "qemu" CPU.
>> This patch now adds the "stfle", "extended immediate" and "stckf"
>> facility bits to the "qemu" CPU, which are already supported facilities.
>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>> but at least it's a first step into the right direction.
> 
> This indeed looks like a step in a good direction. At least it makes the
> recently added STFLE emulation useful :-).
> 
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  target/s390x/cpu_models.c | 29 ++++++++++++++++++++++++++---
>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 8d27363..18789ab 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -658,6 +658,24 @@ static void check_compatibility(const S390CPUModel *max_model,
>>                    "available in the configuration: ");
>>  }
>>  
>> +/**
>> + * The base TCG CPU model "qemu" is based on the z900. However, we already
>> + * can also emulate some additional features of later CPU generations, so
>> + * we add these additional feature bits here.
>> + */
>> +static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
>> +{
>> +    int i, feats[] = {
>> +        S390_FEAT_STFLE,
>> +        S390_FEAT_EXTENDED_IMMEDIATE,
>> +        S390_FEAT_STORE_CLOCK_FAST
> 
> According to my list, QEMU also fully implements the following
> facilities:
> 
> S390_FEAT_LONG_DISPLACEMENT
> S390_FEAT_GENERAL_INSTRUCTIONS_EXT
> S390_FEAT_EXECUTE_EXT
> S390_FEAT_STFLE_45

Ok, thanks, I'll respin and add them to the list!

>> +    };
>> +
>> +    for (i = 0; i < ARRAY_SIZE(feats); i++) {
>> +        set_bit(feats[i], fbm);
>> +    }
>> +}
>> +
>>  static S390CPUModel *get_max_cpu_model(Error **errp)
>>  {
>>      static S390CPUModel max_model;
>> @@ -670,10 +688,11 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>>      if (kvm_enabled()) {
>>          kvm_s390_get_host_cpu_model(&max_model, errp);
>>      } else {
>> -        /* TCG emulates a z900 */
>> +        /* TCG emulates a z900 (with some additional features) */
>>          max_model.def = &s390_cpu_defs[0];
>>          bitmap_copy(max_model.features, max_model.def->default_feat,
>>                      S390_FEAT_MAX);
>> +        add_qemu_cpu_model_features(max_model.features);
>>      }
>>      if (!*errp) {
>>          cached = true;
>> @@ -925,11 +944,15 @@ static void s390_host_cpu_model_initfn(Object *obj)
>>  
>>  static void s390_qemu_cpu_model_initfn(Object *obj)
>>  {
>> +    static S390CPUDef s390_qemu_cpu_defs;
>>      S390CPU *cpu = S390_CPU(obj);
>>  
>>      cpu->model = g_malloc0(sizeof(*cpu->model));
>> -    /* TCG emulates a z900 */
>> -    cpu->model->def = &s390_cpu_defs[0];
>> +    /* TCG emulates a z900 (with some additional features) */
>> +    memcpy(&s390_qemu_cpu_defs, &s390_cpu_defs[0], sizeof(s390_qemu_cpu_defs));
>> +    add_qemu_cpu_model_features(s390_qemu_cpu_defs.default_feat);
>> +    add_qemu_cpu_model_features(s390_qemu_cpu_defs.full_feat);
>> +    cpu->model->def = &s390_qemu_cpu_defs;
>>      bitmap_copy(cpu->model->features, cpu->model->def->default_feat,
>>                  S390_FEAT_MAX);
>>  }
> 
> Otherwise it looks fine to me.
> 
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

Thanks!

 Thomas

On 17.05.2017 18:26, Aurelien Jarno wrote:
> On 2017-05-17 17:35, Thomas Huth wrote:
>> Currently we only present the plain z900 feature bits to the guest,
>> but QEMU already emulates some additional features (but not all of
>> the next CPU generation, so we can not use the next CPU level as
>> default yet). Since newer Linux kernels are checking the feature bits
>> and refuse to work if a required feature is missing, we should present
>> as much of the supported features as possible when we are running
>> with the default "qemu" CPU.
>> This patch now adds the "stfle", "extended immediate" and "stckf"
>> facility bits to the "qemu" CPU, which are already supported facilities.
>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>> but at least it's a first step into the right direction.
> 
> This indeed looks like a step in a good direction. At least it makes the
> recently added STFLE emulation useful :-).
> 
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  target/s390x/cpu_models.c | 29 ++++++++++++++++++++++++++---
>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 8d27363..18789ab 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -658,6 +658,24 @@ static void check_compatibility(const S390CPUModel *max_model,
>>                    "available in the configuration: ");
>>  }
>>  
>> +/**
>> + * The base TCG CPU model "qemu" is based on the z900. However, we already
>> + * can also emulate some additional features of later CPU generations, so
>> + * we add these additional feature bits here.
>> + */
>> +static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
>> +{
>> +    int i, feats[] = {
>> +        S390_FEAT_STFLE,
>> +        S390_FEAT_EXTENDED_IMMEDIATE,
>> +        S390_FEAT_STORE_CLOCK_FAST
> 
> According to my list, QEMU also fully implements the following
> facilities:
> 
> S390_FEAT_LONG_DISPLACEMENT
> S390_FEAT_GENERAL_INSTRUCTIONS_EXT
> S390_FEAT_EXECUTE_EXT
> S390_FEAT_STFLE_45
> 

If S390_FEAT_LONG_DISPLACEMENT is supported, so is
S390_FEAT_LONG_DISPLACEMENT_FAST (because TCG obvious has high
performance ;) )

-- 

Thanks,

David

On 17.05.2017 17:35, Thomas Huth wrote:
> Currently we only present the plain z900 feature bits to the guest,
> but QEMU already emulates some additional features (but not all of
> the next CPU generation, so we can not use the next CPU level as
> default yet). Since newer Linux kernels are checking the feature bits
> and refuse to work if a required feature is missing, we should present
> as much of the supported features as possible when we are running
> with the default "qemu" CPU.
> This patch now adds the "stfle", "extended immediate" and "stckf"
> facility bits to the "qemu" CPU, which are already supported facilities.
> It is unfortunately still not enough to run e.g. recent Fedora kernels,
> but at least it's a first step into the right direction.
> 

Three things:

1. Should we care about backwards compatibility? I think so. This should
be fixed up using compat machine properties. (qemu model is a
migration-safe model and could e.g. be used in KVM setups, too).

2. I would recommend to not enable STFLE for now. Why?

It is/was an indication that the system is running on a z9 (and
implicitly has the basic features). This was not only done because
people were lazy, but because this bit was implicitly connected to other
machine properties.

One popular example is the "DAT-enhancement facility 2". It introduced
the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
introduced. SO there is no way to check if the instruction is available
and actually working.

I heard rumors that if the STFLE is available, this is also an
indication for DAT-enhancement facility 2. target/s390x/kvm.c uses the
same heuristic in kvm_s390_get_host_cpu_model().

Please note that we added a feature representation for this facility,
because this would allow us later on to at least model removal of such a
facility (if HW actually would drop it) on a CPU model level.

3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
explicitly tests for such inconsistencies.

So your QEMU CPU model would have a feature, but you would not be able
to run that model using QEMU when manually specifying it on the command
line. Especially, expanding the "qemu" model and feeding it back to QEMU
will fail.

So I am not sure if we should introduce such inconsistencies at that
point. Rather fix up the basics and then move the CPU model to a
consistent model.


> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/cpu_models.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 8d27363..18789ab 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -658,6 +658,24 @@ static void check_compatibility(const S390CPUModel *max_model,
>                    "available in the configuration: ");
>  }
>  
> +/**
> + * The base TCG CPU model "qemu" is based on the z900. However, we already
> + * can also emulate some additional features of later CPU generations, so
> + * we add these additional feature bits here.
> + */
> +static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
> +{
> +    int i, feats[] = {
> +        S390_FEAT_STFLE,
> +        S390_FEAT_EXTENDED_IMMEDIATE,
> +        S390_FEAT_STORE_CLOCK_FAST
> +    };
> +
> +    for (i = 0; i < ARRAY_SIZE(feats); i++) {
> +        set_bit(feats[i], fbm);
> +    }
> +}
> +
>  static S390CPUModel *get_max_cpu_model(Error **errp)
>  {
>      static S390CPUModel max_model;
> @@ -670,10 +688,11 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>      if (kvm_enabled()) {
>          kvm_s390_get_host_cpu_model(&max_model, errp);
>      } else {
> -        /* TCG emulates a z900 */
> +        /* TCG emulates a z900 (with some additional features) */
>          max_model.def = &s390_cpu_defs[0];
>          bitmap_copy(max_model.features, max_model.def->default_feat,
>                      S390_FEAT_MAX);
> +        add_qemu_cpu_model_features(max_model.features);
>      }
>      if (!*errp) {
>          cached = true;
> @@ -925,11 +944,15 @@ static void s390_host_cpu_model_initfn(Object *obj)
>  
>  static void s390_qemu_cpu_model_initfn(Object *obj)
>  {
> +    static S390CPUDef s390_qemu_cpu_defs;
>      S390CPU *cpu = S390_CPU(obj);
>  
>      cpu->model = g_malloc0(sizeof(*cpu->model));
> -    /* TCG emulates a z900 */
> -    cpu->model->def = &s390_cpu_defs[0];
> +    /* TCG emulates a z900 (with some additional features) */
> +    memcpy(&s390_qemu_cpu_defs, &s390_cpu_defs[0], sizeof(s390_qemu_cpu_defs));
> +    add_qemu_cpu_model_features(s390_qemu_cpu_defs.default_feat);
> +    add_qemu_cpu_model_features(s390_qemu_cpu_defs.full_feat);
> +    cpu->model->def = &s390_qemu_cpu_defs;
>      bitmap_copy(cpu->model->features, cpu->model->def->default_feat,
>                  S390_FEAT_MAX);
>  }
> 


-- 

Thanks,

David

On 17.05.2017 18:49, David Hildenbrand wrote:
> On 17.05.2017 17:35, Thomas Huth wrote:
>> Currently we only present the plain z900 feature bits to the guest,
>> but QEMU already emulates some additional features (but not all of
>> the next CPU generation, so we can not use the next CPU level as
>> default yet). Since newer Linux kernels are checking the feature bits
>> and refuse to work if a required feature is missing, we should present
>> as much of the supported features as possible when we are running
>> with the default "qemu" CPU.
>> This patch now adds the "stfle", "extended immediate" and "stckf"
>> facility bits to the "qemu" CPU, which are already supported facilities.
>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>> but at least it's a first step into the right direction.
>>
> 
> Three things:
> 
> 1. Should we care about backwards compatibility? I think so. This should
> be fixed up using compat machine properties. (qemu model is a
> migration-safe model and could e.g. be used in KVM setups, too).

Theoretically, I agree, but do we really care about backwards
compatibility at this point in time? All major distro kernels (except
Debian, I think) currently do not work in QEMU, so there is currently
not that much that can be migrated...
And currently, the "qemu" CPU is the very same as the "z900" CPU, so you
might also get along with simply using "-cpu z900" on the destination
instead, I guess.

> 2. I would recommend to not enable STFLE for now. Why?
> 
> It is/was an indication that the system is running on a z9 (and
> implicitly has the basic features). This was not only done because
> people were lazy, but because this bit was implicitly connected to other
> machine properties.

Uh, that's ugly!

> One popular example is the "DAT-enhancement facility 2". It introduced
> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
> introduced. SO there is no way to check if the instruction is available
> and actually working.

Does the Linux kernel use this instruction at all? I just grep'ed
through the kernel sources and did not find it. If the Linux kernel does
not use it, I think we should ignore this interdependency and just
provide the STFLE feature bit to the guest - since recent Linux kernels
depend on it.

> Please note that we added a feature representation for this facility,
> because this would allow us later on to at least model removal of such a
> facility (if HW actually would drop it) on a CPU model level.

What about STFLE bit 78, according to my version of the POP, it says:

"The enhanced-DAT facility 2 is installed in the
 z/Architecture architectural mode."

?

> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
> explicitly tests for such inconsistencies.
> 
> So your QEMU CPU model would have a feature, but you would not be able
> to run that model using QEMU when manually specifying it on the command
> line. Especially, expanding the "qemu" model and feeding it back to QEMU
> will fail.

I've checked that I can also successfully disable the features again at
the command line, using "-cpu qemu,eimm=false" for example, so not sure
what exactly you're talking about here. Could you please elaborate?

> So I am not sure if we should introduce such inconsistencies at that
> point. Rather fix up the basics and then move the CPU model to a
> consistent model.

I think we're very far away from being able to use the next official CPU
model generation in QEMU TCG, so having at least something that let's us
run other recent distro kernels apart from the Debian ones would be very
helpful. I also understand the "qemu" CPU this way: "Simply give me the
best CPU features that TCG currently can provide". If you want to have a
"consistent" CPU state, you can simply use an official model like "z900"
instead.

 Thomas

On 2017-05-18 03:55, Thomas Huth wrote:
> On 17.05.2017 18:49, David Hildenbrand wrote:
> > On 17.05.2017 17:35, Thomas Huth wrote:
> > 2. I would recommend to not enable STFLE for now. Why?
> > 
> > It is/was an indication that the system is running on a z9 (and
> > implicitly has the basic features). This was not only done because
> > people were lazy, but because this bit was implicitly connected to other
> > machine properties.
> 
> Uh, that's ugly!
> 
> > One popular example is the "DAT-enhancement facility 2". It introduced
> > the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
> > introduced. SO there is no way to check if the instruction is available
> > and actually working.
> 
> Does the Linux kernel use this instruction at all? I just grep'ed
> through the kernel sources and did not find it. If the Linux kernel does
> not use it, I think we should ignore this interdependency and just
> provide the STFLE feature bit to the guest - since recent Linux kernels
> depend on it.
> 
> > Please note that we added a feature representation for this facility,
> > because this would allow us later on to at least model removal of such a
> > facility (if HW actually would drop it) on a CPU model level.
> 
> What about STFLE bit 78, according to my version of the POP, it says:
> 
> "The enhanced-DAT facility 2 is installed in the
>  z/Architecture architectural mode."

No that is different. IBM has chosen very confusing names for the DAT
related facilities:
- DAT-Enhancement Facility 1 => bits 3, 4 & 5
- DAT-Enhancement Facility 2 
- Enhanced-DAT Facility 1    => bit 8
- Enhanced-DAT Facility 2    => bit 78

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

On 18.05.2017 03:55, Thomas Huth wrote:
> On 17.05.2017 18:49, David Hildenbrand wrote:
>> On 17.05.2017 17:35, Thomas Huth wrote:
>>> Currently we only present the plain z900 feature bits to the guest,
>>> but QEMU already emulates some additional features (but not all of
>>> the next CPU generation, so we can not use the next CPU level as
>>> default yet). Since newer Linux kernels are checking the feature bits
>>> and refuse to work if a required feature is missing, we should present
>>> as much of the supported features as possible when we are running
>>> with the default "qemu" CPU.
>>> This patch now adds the "stfle", "extended immediate" and "stckf"
>>> facility bits to the "qemu" CPU, which are already supported facilities.
>>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>>> but at least it's a first step into the right direction.
>>>
>>
>> Three things:
>>
>> 1. Should we care about backwards compatibility? I think so. This should
>> be fixed up using compat machine properties. (qemu model is a
>> migration-safe model and could e.g. be used in KVM setups, too).
> 
> Theoretically, I agree, but do we really care about backwards
> compatibility at this point in time? All major distro kernels (except
> Debian, I think) currently do not work in QEMU, so there is currently
> not that much that can be migrated...
> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you
> might also get along with simply using "-cpu z900" on the destination
> instead, I guess.

If possible, I would like to avoid changing migration safe CPU model.
And I guess it shouldn't be too hard for now (unless we really change
the base model to e.g. a z9, then some more work might have to be done)

I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat
machines should do the trick.

> 
>> 2. I would recommend to not enable STFLE for now. Why?
>>
>> It is/was an indication that the system is running on a z9 (and
>> implicitly has the basic features). This was not only done because
>> people were lazy, but because this bit was implicitly connected to other
>> machine properties.
> 
> Uh, that's ugly!
> 
>> One popular example is the "DAT-enhancement facility 2". It introduced
>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
>> introduced. SO there is no way to check if the instruction is available
>> and actually working.
> 
> Does the Linux kernel use this instruction at all? I just grep'ed
> through the kernel sources and did not find it. If the Linux kernel does
> not use it, I think we should ignore this interdependency and just
> provide the STFLE feature bit to the guest - since recent Linux kernels
> depend on it.

Yes, current linux doesn't use it, I don't remember if previous versions
did. Most likely not. The question is if they relied on the stfle==z9
assumption. The STFLE facility really is special in that sense.

> 
>> Please note that we added a feature representation for this facility,
>> because this would allow us later on to at least model removal of such a
>> facility (if HW actually would drop it) on a CPU model level.
> 
> What about STFLE bit 78, according to my version of the POP, it says:
> 
> "The enhanced-DAT facility 2 is installed in the
>  z/Architecture architectural mode."
> 
> ?

As Aurelien already mentioned, there seemed to be different ways to
enhance DAT :) enhanced-DAT facility 2 is 2GB page support.

> 
>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
>> explicitly tests for such inconsistencies.
>>
>> So your QEMU CPU model would have a feature, but you would not be able
>> to run that model using QEMU when manually specifying it on the command
>> line. Especially, expanding the "qemu" model and feeding it back to QEMU
>> will fail.
> 
> I've checked that I can also successfully disable the features again at
> the command line, using "-cpu qemu,eimm=false" for example, so not sure
> what exactly you're talking about here. Could you please elaborate?

Assume libvirt/the user expands the CPU model name "qemu" via
"qmp-expand-cpu-model "qemu", you will get something like

"z900-base,.....,stfle=on"

If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will
detect the inconsistency when setting the property and abort. However,
"-cpu qemu" will succeed. Please note that these checks actually make
sense for KVM:

If you're on a z13 and configure a zEC12, you might be tempted to add
e.g. "vx=on". However, IBC (Instruction Blocking Control) in the machine
will block any attempt to execute a vx instruction. So these checks make
sure that only facilities really supported for a machine generation can
be enabled.

If we really want that, we might decide to drop such checks for models <
e.g. z9, because nobody will most likely care.

> 
>> So I am not sure if we should introduce such inconsistencies at that
>> point. Rather fix up the basics and then move the CPU model to a
>> consistent model.
> 
> I think we're very far away from being able to use the next official CPU
> model generation in QEMU TCG, so having at least something that let's us
> run other recent distro kernels apart from the Debian ones would be very
> helpful. I also understand the "qemu" CPU this way: "Simply give me the
> best CPU features that TCG currently can provide". If you want to have a
> "consistent" CPU state, you can simply use an official model like "z900"
> instead.

"qemu" is just like what "host" is for kvm. A consistent model, because
it is the default.

However, KVM folks also have the requirement to allow
"unfiltered"/"inconsistent" models, e.g. for development purposes.

There was the idea to introduce a CPU model "-cpu off", that would act
as before, without any CPU model support: Blindly enable anything we can.

This model would of course not be static, not migration-safe, and one
would not be able to modify features. This would map to an "unfiltered
qemu" model under TCG. We could blindly enable anything there.

> 
>  Thomas
> 


-- 

Thanks,

David

On 05/18/2017 10:48 AM, David Hildenbrand wrote:
> On 18.05.2017 03:55, Thomas Huth wrote:
>> On 17.05.2017 18:49, David Hildenbrand wrote:
>>> On 17.05.2017 17:35, Thomas Huth wrote:
>>>> Currently we only present the plain z900 feature bits to the guest,
>>>> but QEMU already emulates some additional features (but not all of
>>>> the next CPU generation, so we can not use the next CPU level as
>>>> default yet). Since newer Linux kernels are checking the feature bits
>>>> and refuse to work if a required feature is missing, we should present
>>>> as much of the supported features as possible when we are running
>>>> with the default "qemu" CPU.
>>>> This patch now adds the "stfle", "extended immediate" and "stckf"
>>>> facility bits to the "qemu" CPU, which are already supported facilities.
>>>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>>>> but at least it's a first step into the right direction.
>>>>
>>>
>>> Three things:
>>>
>>> 1. Should we care about backwards compatibility? I think so. This should
>>> be fixed up using compat machine properties. (qemu model is a
>>> migration-safe model and could e.g. be used in KVM setups, too).
>>
>> Theoretically, I agree, but do we really care about backwards
>> compatibility at this point in time? All major distro kernels (except
>> Debian, I think) currently do not work in QEMU, so there is currently
>> not that much that can be migrated...
>> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you
>> might also get along with simply using "-cpu z900" on the destination
>> instead, I guess.
> 
> If possible, I would like to avoid changing migration safe CPU model.
> And I guess it shouldn't be too hard for now (unless we really change
> the base model to e.g. a z9, then some more work might have to be done)
> 
> I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat
> machines should do the trick.
> 
>>
>>> 2. I would recommend to not enable STFLE for now. Why?
>>>
>>> It is/was an indication that the system is running on a z9 (and
>>> implicitly has the basic features). This was not only done because
>>> people were lazy, but because this bit was implicitly connected to other
>>> machine properties.
>>
>> Uh, that's ugly!
>>
>>> One popular example is the "DAT-enhancement facility 2". It introduced
>>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
>>> introduced. SO there is no way to check if the instruction is available
>>> and actually working.
>>
>> Does the Linux kernel use this instruction at all? I just grep'ed
>> through the kernel sources and did not find it. If the Linux kernel does
>> not use it, I think we should ignore this interdependency and just
>> provide the STFLE feature bit to the guest - since recent Linux kernels
>> depend on it.
> 
> Yes, current linux doesn't use it, I don't remember if previous versions
> did. Most likely not. The question is if they relied on the stfle==z9
> assumption. The STFLE facility really is special in that sense.
> 
>>
>>> Please note that we added a feature representation for this facility,
>>> because this would allow us later on to at least model removal of such a
>>> facility (if HW actually would drop it) on a CPU model level.
>>
>> What about STFLE bit 78, according to my version of the POP, it says:
>>
>> "The enhanced-DAT facility 2 is installed in the
>>  z/Architecture architectural mode."
>>
>> ?
> 
> As Aurelien already mentioned, there seemed to be different ways to
> enhance DAT :) enhanced-DAT facility 2 is 2GB page support.
> 
>>
>>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
>>> explicitly tests for such inconsistencies.
>>>
>>> So your QEMU CPU model would have a feature, but you would not be able
>>> to run that model using QEMU when manually specifying it on the command
>>> line. Especially, expanding the "qemu" model and feeding it back to QEMU
>>> will fail.
>>
>> I've checked that I can also successfully disable the features again at
>> the command line, using "-cpu qemu,eimm=false" for example, so not sure
>> what exactly you're talking about here. Could you please elaborate?
> 
> Assume libvirt/the user expands the CPU model name "qemu" via
> "qmp-expand-cpu-model "qemu", you will get something like
> 
> "z900-base,.....,stfle=on"
> 
> If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will
> detect the inconsistency when setting the property and abort. However,
> "-cpu qemu" will succeed. Please note that these checks actually make
> sense for KVM:
> 

Jason (now on cc) has a patch prepared for other reasons that disabled features
for given machines. I kept the ESOP example in that patch.
That would allow us to disable STFLE for old machines but enable it for 2.10

copy/pasted and hand edited to get rid of the unrelated changes:


diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 3c12735..26b0ac9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -30,6 +30,7 @@
 #include "ipl.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "hw/s390x/css-bridge.h"
+#include "cpu_models.h"

 static const char *const reset_dev_types[] = {
     TYPE_VIRTUAL_CSS_BRIDGE,
@@ -481,6 +482,7 @@ DEFINE_CCW_MACHINE(2_10, "2.10", true);
 static void ccw_machine_2_9_instance_options(MachineState *machine)
 {
     ccw_machine_2_10_instance_options(machine);
+    s390_cpudef_featoff_greater(12, 1, S390_FEAT_ESOP);
 }

 static void ccw_machine_2_9_class_options(MachineClass *mc)
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 71ddb6c..5f295a5 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -78,6 +78,32 @@ static S390CPUDef s390_cpu_defs[] = {
     CPUDEF_INIT(0x3906, 14, 1, 47, 0x08000000U, "z14", "IBM z14 GA1"),
 };

+void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat)
+{
+    const S390CPUDef *def;
+
+    def = s390_find_cpu_def(0, gen, ec_ga, NULL);
+    clear_bit(feat, (unsigned long *)&def->default_feat);
+}
+
+void s390_cpudef_featoff_greater(uint8_t gen, uint8_t ec_ga, S390Feat feat)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) {
+        const S390CPUDef *def = &s390_cpu_defs[i];
+
+        if (def->gen < gen) {
+            continue;
+        }
+        if (def->gen == gen && def->ec_ga < ec_ga) {
+            continue;
+        }
+
+        clear_bit(feat, (unsigned long *)&def->default_feat);
+    }
+}
+
 uint32_t s390_get_hmfai(void)
 {
     static S390CPU *cpu;
diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h
index 136a602..881eb65 100644
--- a/target/s390x/cpu_models.h
+++ b/target/s390x/cpu_models.h
@@ -69,6 +69,8 @@ typedef struct S390CPUModel {
 #define ibc_gen(x)        (x == 0 ? 0 : ((x >> 4) + S390_GEN_Z10))
 #define ibc_ec_ga(x)      (x & 0xf)

+void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat);
+void s390_cpudef_featoff_greater(uint8_t gen, uint8_t ec_ga, S390Feat feat);
 uint32_t s390_get_hmfai(void);
 uint8_t s390_get_mha_pow(void);
 uint32_t s390_get_ibc_val(void);
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 3efd5dd..105e5f5 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -515,6 +515,7 @@ static uint16_t default_GEN12_GA1[] = {
     S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
     S390_FEAT_ADAPTER_INT_SUPPRESSION,
     S390_FEAT_EDAT_2,
+    S390_FEAT_ESOP,
 };


Maybe we should split that out and merge such a patch sooner than the
(yet in development) other changes?



> If you're on a z13 and configure a zEC12, you might be tempted to add
> e.g. "vx=on". However, IBC (Instruction Blocking Control) in the machine
> will block any attempt to execute a vx instruction. So these checks make
> sure that only facilities really supported for a machine generation can
> be enabled.
> 
> If we really want that, we might decide to drop such checks for models <
> e.g. z9, because nobody will most likely care.
> 
>>
>>> So I am not sure if we should introduce such inconsistencies at that
>>> point. Rather fix up the basics and then move the CPU model to a
>>> consistent model.
>>
>> I think we're very far away from being able to use the next official CPU
>> model generation in QEMU TCG, so having at least something that let's us
>> run other recent distro kernels apart from the Debian ones would be very
>> helpful. I also understand the "qemu" CPU this way: "Simply give me the
>> best CPU features that TCG currently can provide". If you want to have a
>> "consistent" CPU state, you can simply use an official model like "z900"
>> instead.
> 
> "qemu" is just like what "host" is for kvm. A consistent model, because
> it is the default.
> 
> However, KVM folks also have the requirement to allow
> "unfiltered"/"inconsistent" models, e.g. for development purposes.
> 
> There was the idea to introduce a CPU model "-cpu off", that would act
> as before, without any CPU model support: Blindly enable anything we can.
> 
> This model would of course not be static, not migration-safe, and one
> would not be able to modify features. This would map to an "unfiltered
> qemu" model under TCG. We could blindly enable anything there.
> 
>>
>>  Thomas
>>
> 
> 

On 18.05.2017 11:00, Christian Borntraeger wrote:
> On 05/18/2017 10:48 AM, David Hildenbrand wrote:
>> On 18.05.2017 03:55, Thomas Huth wrote:
>>> On 17.05.2017 18:49, David Hildenbrand wrote:
>>>> On 17.05.2017 17:35, Thomas Huth wrote:
>>>>> Currently we only present the plain z900 feature bits to the guest,
>>>>> but QEMU already emulates some additional features (but not all of
>>>>> the next CPU generation, so we can not use the next CPU level as
>>>>> default yet). Since newer Linux kernels are checking the feature bits
>>>>> and refuse to work if a required feature is missing, we should present
>>>>> as much of the supported features as possible when we are running
>>>>> with the default "qemu" CPU.
>>>>> This patch now adds the "stfle", "extended immediate" and "stckf"
>>>>> facility bits to the "qemu" CPU, which are already supported facilities.
>>>>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>>>>> but at least it's a first step into the right direction.
>>>>>
>>>>
>>>> Three things:
>>>>
>>>> 1. Should we care about backwards compatibility? I think so. This should
>>>> be fixed up using compat machine properties. (qemu model is a
>>>> migration-safe model and could e.g. be used in KVM setups, too).
>>>
>>> Theoretically, I agree, but do we really care about backwards
>>> compatibility at this point in time? All major distro kernels (except
>>> Debian, I think) currently do not work in QEMU, so there is currently
>>> not that much that can be migrated...
>>> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you
>>> might also get along with simply using "-cpu z900" on the destination
>>> instead, I guess.
>>
>> If possible, I would like to avoid changing migration safe CPU model.
>> And I guess it shouldn't be too hard for now (unless we really change
>> the base model to e.g. a z9, then some more work might have to be done)
>>
>> I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat
>> machines should do the trick.
>>
>>>
>>>> 2. I would recommend to not enable STFLE for now. Why?
>>>>
>>>> It is/was an indication that the system is running on a z9 (and
>>>> implicitly has the basic features). This was not only done because
>>>> people were lazy, but because this bit was implicitly connected to other
>>>> machine properties.
>>>
>>> Uh, that's ugly!
>>>
>>>> One popular example is the "DAT-enhancement facility 2". It introduced
>>>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
>>>> introduced. SO there is no way to check if the instruction is available
>>>> and actually working.
>>>
>>> Does the Linux kernel use this instruction at all? I just grep'ed
>>> through the kernel sources and did not find it. If the Linux kernel does
>>> not use it, I think we should ignore this interdependency and just
>>> provide the STFLE feature bit to the guest - since recent Linux kernels
>>> depend on it.
>>
>> Yes, current linux doesn't use it, I don't remember if previous versions
>> did. Most likely not. The question is if they relied on the stfle==z9
>> assumption. The STFLE facility really is special in that sense.
>>
>>>
>>>> Please note that we added a feature representation for this facility,
>>>> because this would allow us later on to at least model removal of such a
>>>> facility (if HW actually would drop it) on a CPU model level.
>>>
>>> What about STFLE bit 78, according to my version of the POP, it says:
>>>
>>> "The enhanced-DAT facility 2 is installed in the
>>>  z/Architecture architectural mode."
>>>
>>> ?
>>
>> As Aurelien already mentioned, there seemed to be different ways to
>> enhance DAT :) enhanced-DAT facility 2 is 2GB page support.
>>
>>>
>>>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
>>>> explicitly tests for such inconsistencies.
>>>>
>>>> So your QEMU CPU model would have a feature, but you would not be able
>>>> to run that model using QEMU when manually specifying it on the command
>>>> line. Especially, expanding the "qemu" model and feeding it back to QEMU
>>>> will fail.
>>>
>>> I've checked that I can also successfully disable the features again at
>>> the command line, using "-cpu qemu,eimm=false" for example, so not sure
>>> what exactly you're talking about here. Could you please elaborate?
>>
>> Assume libvirt/the user expands the CPU model name "qemu" via
>> "qmp-expand-cpu-model "qemu", you will get something like
>>
>> "z900-base,.....,stfle=on"
>>
>> If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will
>> detect the inconsistency when setting the property and abort. However,
>> "-cpu qemu" will succeed. Please note that these checks actually make
>> sense for KVM:
>>
> 
> Jason (now on cc) has a patch prepared for other reasons that disabled features
> for given machines. I kept the ESOP example in that patch.
> That would allow us to disable STFLE for old machines but enable it for 2.10
[...]
> Maybe we should split that out and merge such a patch sooner than the
> (yet in development) other changes?

Yes, that sounds like a good idea, I think we could use the same
mechanism here, too, so please split it out and submit it earlier!

 Thanks a lot,
  Thomas

On 18.05.2017 11:05, Thomas Huth wrote:
> On 18.05.2017 11:00, Christian Borntraeger wrote:
>> On 05/18/2017 10:48 AM, David Hildenbrand wrote:
>>> On 18.05.2017 03:55, Thomas Huth wrote:
>>>> On 17.05.2017 18:49, David Hildenbrand wrote:
>>>>> On 17.05.2017 17:35, Thomas Huth wrote:
>>>>>> Currently we only present the plain z900 feature bits to the guest,
>>>>>> but QEMU already emulates some additional features (but not all of
>>>>>> the next CPU generation, so we can not use the next CPU level as
>>>>>> default yet). Since newer Linux kernels are checking the feature bits
>>>>>> and refuse to work if a required feature is missing, we should present
>>>>>> as much of the supported features as possible when we are running
>>>>>> with the default "qemu" CPU.
>>>>>> This patch now adds the "stfle", "extended immediate" and "stckf"
>>>>>> facility bits to the "qemu" CPU, which are already supported facilities.
>>>>>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>>>>>> but at least it's a first step into the right direction.
>>>>>>
>>>>>
>>>>> Three things:
>>>>>
>>>>> 1. Should we care about backwards compatibility? I think so. This should
>>>>> be fixed up using compat machine properties. (qemu model is a
>>>>> migration-safe model and could e.g. be used in KVM setups, too).
>>>>
>>>> Theoretically, I agree, but do we really care about backwards
>>>> compatibility at this point in time? All major distro kernels (except
>>>> Debian, I think) currently do not work in QEMU, so there is currently
>>>> not that much that can be migrated...
>>>> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you
>>>> might also get along with simply using "-cpu z900" on the destination
>>>> instead, I guess.
>>>
>>> If possible, I would like to avoid changing migration safe CPU model.
>>> And I guess it shouldn't be too hard for now (unless we really change
>>> the base model to e.g. a z9, then some more work might have to be done)
>>>
>>> I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat
>>> machines should do the trick.
>>>
>>>>
>>>>> 2. I would recommend to not enable STFLE for now. Why?
>>>>>
>>>>> It is/was an indication that the system is running on a z9 (and
>>>>> implicitly has the basic features). This was not only done because
>>>>> people were lazy, but because this bit was implicitly connected to other
>>>>> machine properties.
>>>>
>>>> Uh, that's ugly!
>>>>
>>>>> One popular example is the "DAT-enhancement facility 2". It introduced
>>>>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
>>>>> introduced. SO there is no way to check if the instruction is available
>>>>> and actually working.
>>>>
>>>> Does the Linux kernel use this instruction at all? I just grep'ed
>>>> through the kernel sources and did not find it. If the Linux kernel does
>>>> not use it, I think we should ignore this interdependency and just
>>>> provide the STFLE feature bit to the guest - since recent Linux kernels
>>>> depend on it.
>>>
>>> Yes, current linux doesn't use it, I don't remember if previous versions
>>> did. Most likely not. The question is if they relied on the stfle==z9
>>> assumption. The STFLE facility really is special in that sense.
>>>
>>>>
>>>>> Please note that we added a feature representation for this facility,
>>>>> because this would allow us later on to at least model removal of such a
>>>>> facility (if HW actually would drop it) on a CPU model level.
>>>>
>>>> What about STFLE bit 78, according to my version of the POP, it says:
>>>>
>>>> "The enhanced-DAT facility 2 is installed in the
>>>>  z/Architecture architectural mode."
>>>>
>>>> ?
>>>
>>> As Aurelien already mentioned, there seemed to be different ways to
>>> enhance DAT :) enhanced-DAT facility 2 is 2GB page support.
>>>
>>>>
>>>>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
>>>>> explicitly tests for such inconsistencies.
>>>>>
>>>>> So your QEMU CPU model would have a feature, but you would not be able
>>>>> to run that model using QEMU when manually specifying it on the command
>>>>> line. Especially, expanding the "qemu" model and feeding it back to QEMU
>>>>> will fail.
>>>>
>>>> I've checked that I can also successfully disable the features again at
>>>> the command line, using "-cpu qemu,eimm=false" for example, so not sure
>>>> what exactly you're talking about here. Could you please elaborate?
>>>
>>> Assume libvirt/the user expands the CPU model name "qemu" via
>>> "qmp-expand-cpu-model "qemu", you will get something like
>>>
>>> "z900-base,.....,stfle=on"
>>>
>>> If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will
>>> detect the inconsistency when setting the property and abort. However,
>>> "-cpu qemu" will succeed. Please note that these checks actually make
>>> sense for KVM:
>>>
>>
>> Jason (now on cc) has a patch prepared for other reasons that disabled features
>> for given machines. I kept the ESOP example in that patch.
>> That would allow us to disable STFLE for old machines but enable it for 2.10
> [...]
>> Maybe we should split that out and merge such a patch sooner than the
>> (yet in development) other changes?
> 
> Yes, that sounds like a good idea, I think we could use the same
> mechanism here, too, so please split it out and submit it earlier!
> 
>  Thanks a lot,
>   Thomas
> 

I think this is useful but a different use case:

What Christian/Jason have here is a way to fixup default models (e.g.
z900, ZEC12...). This is necessary when introducing new features /
movinf features from FULL into DEFAULT.

We don't want to fixup default models but the s390x-cpu-qemu.

-- 

Thanks,

David

On 18.05.2017 11:14, David Hildenbrand wrote:
> On 18.05.2017 11:05, Thomas Huth wrote:
>> On 18.05.2017 11:00, Christian Borntraeger wrote:
>>> On 05/18/2017 10:48 AM, David Hildenbrand wrote:
>>>> On 18.05.2017 03:55, Thomas Huth wrote:
>>>>> On 17.05.2017 18:49, David Hildenbrand wrote:
>>>>>> On 17.05.2017 17:35, Thomas Huth wrote:
>>>>>>> Currently we only present the plain z900 feature bits to the guest,
>>>>>>> but QEMU already emulates some additional features (but not all of
>>>>>>> the next CPU generation, so we can not use the next CPU level as
>>>>>>> default yet). Since newer Linux kernels are checking the feature bits
>>>>>>> and refuse to work if a required feature is missing, we should present
>>>>>>> as much of the supported features as possible when we are running
>>>>>>> with the default "qemu" CPU.
>>>>>>> This patch now adds the "stfle", "extended immediate" and "stckf"
>>>>>>> facility bits to the "qemu" CPU, which are already supported facilities.
>>>>>>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>>>>>>> but at least it's a first step into the right direction.
>>>>>>>
>>>>>>
>>>>>> Three things:
>>>>>>
>>>>>> 1. Should we care about backwards compatibility? I think so. This should
>>>>>> be fixed up using compat machine properties. (qemu model is a
>>>>>> migration-safe model and could e.g. be used in KVM setups, too).
>>>>>
>>>>> Theoretically, I agree, but do we really care about backwards
>>>>> compatibility at this point in time? All major distro kernels (except
>>>>> Debian, I think) currently do not work in QEMU, so there is currently
>>>>> not that much that can be migrated...
>>>>> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you
>>>>> might also get along with simply using "-cpu z900" on the destination
>>>>> instead, I guess.
>>>>
>>>> If possible, I would like to avoid changing migration safe CPU model.
>>>> And I guess it shouldn't be too hard for now (unless we really change
>>>> the base model to e.g. a z9, then some more work might have to be done)
>>>>
>>>> I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat
>>>> machines should do the trick.
>>>>
>>>>>
>>>>>> 2. I would recommend to not enable STFLE for now. Why?
>>>>>>
>>>>>> It is/was an indication that the system is running on a z9 (and
>>>>>> implicitly has the basic features). This was not only done because
>>>>>> people were lazy, but because this bit was implicitly connected to other
>>>>>> machine properties.
>>>>>
>>>>> Uh, that's ugly!
>>>>>
>>>>>> One popular example is the "DAT-enhancement facility 2". It introduced
>>>>>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
>>>>>> introduced. SO there is no way to check if the instruction is available
>>>>>> and actually working.
>>>>>
>>>>> Does the Linux kernel use this instruction at all? I just grep'ed
>>>>> through the kernel sources and did not find it. If the Linux kernel does
>>>>> not use it, I think we should ignore this interdependency and just
>>>>> provide the STFLE feature bit to the guest - since recent Linux kernels
>>>>> depend on it.
>>>>
>>>> Yes, current linux doesn't use it, I don't remember if previous versions
>>>> did. Most likely not. The question is if they relied on the stfle==z9
>>>> assumption. The STFLE facility really is special in that sense.
>>>>
>>>>>
>>>>>> Please note that we added a feature representation for this facility,
>>>>>> because this would allow us later on to at least model removal of such a
>>>>>> facility (if HW actually would drop it) on a CPU model level.
>>>>>
>>>>> What about STFLE bit 78, according to my version of the POP, it says:
>>>>>
>>>>> "The enhanced-DAT facility 2 is installed in the
>>>>>  z/Architecture architectural mode."
>>>>>
>>>>> ?
>>>>
>>>> As Aurelien already mentioned, there seemed to be different ways to
>>>> enhance DAT :) enhanced-DAT facility 2 is 2GB page support.
>>>>
>>>>>
>>>>>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
>>>>>> explicitly tests for such inconsistencies.
>>>>>>
>>>>>> So your QEMU CPU model would have a feature, but you would not be able
>>>>>> to run that model using QEMU when manually specifying it on the command
>>>>>> line. Especially, expanding the "qemu" model and feeding it back to QEMU
>>>>>> will fail.
>>>>>
>>>>> I've checked that I can also successfully disable the features again at
>>>>> the command line, using "-cpu qemu,eimm=false" for example, so not sure
>>>>> what exactly you're talking about here. Could you please elaborate?
>>>>
>>>> Assume libvirt/the user expands the CPU model name "qemu" via
>>>> "qmp-expand-cpu-model "qemu", you will get something like
>>>>
>>>> "z900-base,.....,stfle=on"
>>>>
>>>> If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will
>>>> detect the inconsistency when setting the property and abort. However,
>>>> "-cpu qemu" will succeed. Please note that these checks actually make
>>>> sense for KVM:
>>>>
>>>
>>> Jason (now on cc) has a patch prepared for other reasons that disabled features
>>> for given machines. I kept the ESOP example in that patch.
>>> That would allow us to disable STFLE for old machines but enable it for 2.10
>> [...]
>>> Maybe we should split that out and merge such a patch sooner than the
>>> (yet in development) other changes?
>>
>> Yes, that sounds like a good idea, I think we could use the same
>> mechanism here, too, so please split it out and submit it earlier!
>>
> I think this is useful but a different use case:
> 
> What Christian/Jason have here is a way to fixup default models (e.g.
> z900, ZEC12...). This is necessary when introducing new features /
> movinf features from FULL into DEFAULT.
> 
> We don't want to fixup default models but the s390x-cpu-qemu.

Looking at the functions again, I think you're right, David... it's
similar at a first glance, but I'd need slightly different functions
here. Ok, so never mind, I guess I have to do it in a different way here...

 Thomas