Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[David Hildenbrand]

On 02.06.2017 16:34, Thomas Huth wrote:
> On 02.06.2017 16:04, Aurelien Jarno wrote:
> [...]
>> I understand your point. That said I doubt we will support the SIE
>> instruction soon (it looks quite complicated and I can't find any doc).
> 
> There is unfortunately no public documentation for the SIE instruction.
> The only spec that is available is a very old one from the S/370 age:
> 
> http://bitsavers.trailing-edge.com/pdf/ibm/370/princOps/SA22-7095-0_370-XA_Interpretive_Execution_Jan84.pdf
> 

And of course the kvm (esp. vsie) implementation is very helpful
understanding how it works and what some bits mean :)

>  Thomas
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[Thomas Huth]

On 02.06.2017 16:04, Aurelien Jarno wrote:
[...]
> I understand your point. That said I doubt we will support the SIE
> instruction soon (it looks quite complicated and I can't find any doc).

There is unfortunately no public documentation for the SIE instruction.
The only spec that is available is a very old one from the S/370 age:

http://bitsavers.trailing-edge.com/pdf/ibm/370/princOps/SA22-7095-0_370-XA_Interpretive_Execution_Jan84.pdf

 Thomas

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[David Hildenbrand]

On 02.06.2017 16:04, Aurelien Jarno wrote:
> On 2017-06-02 13:30, David Hildenbrand wrote:
>> On 02.06.2017 10:09, Thomas Huth wrote:
>>> On 01.06.2017 21:17, Aurelien Jarno wrote:
 On 2017-06-01 11:04, David Hildenbrand wrote:
> On 01.06.2017 10:38, David Hildenbrand wrote:
>> On 01.06.2017 00:01, Aurelien Jarno wrote:
>>> At the same time fix the TCG version of get_max_cpu_model to return the
>>> maximum model like on KVM. Remove the ETF2 and long-displacement
>>
>> I don't understand the part
>> "fix the TCG version of get_max_cpu_model to return the maximum model
>> like on KVM".
>>
>> Can you elaborate?
>>
>>> facilities from the additional features as it is included in the z800.
>>>
>>> Signed-off-by: Aurelien Jarno 
>>> ---
>>>  target/s390x/cpu_models.c | 13 ++---
>>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index fc3cb25cc3..c13bbd852c 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -668,8 +668,6 @@ static void 
>>> add_qemu_cpu_model_features(S390FeatBitmap fbm)
>>>  static const int feats[] = {
>>>  S390_FEAT_STFLE,
>>>  S390_FEAT_EXTENDED_IMMEDIATE,
>>> -S390_FEAT_EXTENDED_TRANSLATION_2,
>>> -S390_FEAT_LONG_DISPLACEMENT,
>>>  S390_FEAT_LONG_DISPLACEMENT_FAST,
>>>  S390_FEAT_ETF2_ENH,
>>>  S390_FEAT_STORE_CLOCK_FAST,
>>> @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>>>  if (kvm_enabled()) {
>>>  kvm_s390_get_host_cpu_model(_model, errp);
>>>  } else {
>>> -/* TCG emulates a z900 (with some optional additional 
>>> features) */
>>> -max_model.def = _cpu_defs[0];
>>> -bitmap_copy(max_model.features, max_model.def->default_feat,
>>> +/* TCG emulates a z800 (with some optional additional 
>>> features) */
>>> +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
>>> +bitmap_copy(max_model.features, max_model.def->full_feat,
>>>  S390_FEAT_MAX);
>
> This is most likely wrong: you're indicating features here that are not
> available on tcg. esp. S390_FEAT_SIE_F2 and friends.
>
> I think should only copy the base features and add whatever else is
> available via add_qemu_cpu_model_features() as already done.

 The patch series added all the z800 features exposed via STFL/STFLE.
 Indeed the SIE features are missing, but anyway QEMU doesn't emulate SIE
 at all so the lack of these features are not exposed to the guest. In that
 regard QEMU already wrongly claim to emulate a z900.
>>
>> Please note that:
>>
>> a) SIE features were never part of the max model. QEMU never claimed
>> that. With your change one could suddenly do a -cpu z900,sie_f2=on,
>> which is wrong.
>>
>> b) The SIE_F2 feature tells the guest that the SIE instruction is
>> available. E.g. Linux will look at this bit and show SIE support in
>> /proc/cpuinfo and unlock the KVM module.
> 
> I understand your point. That said I doubt we will support the SIE
> instruction soon (it looks quite complicated and I can't find any doc).
> As we are going to emulate more facilities to QEMU, it will be more and
> more difficult to select a modern CPU. One will have to use -cpu,z900,
> etf2=on,ldisp=on,...,eimm=on. I think we have to provide users with an
> easier way to do that.
> 

Okay, I think you got something wrong here (which happens easily with
all these different model types).

CPU definitions:
- Base definition: Minimum features we expect to be around on such a CPU
- Default definition: Features we expect to be around in sane
  environments (== KVM on LPAR)
- Full definition: Maximum features that could be around on such a CPU
  (and there are ususally quite some missing, e.g. when running KVM
   under z/VM)

Only default and full definitions can ever change.

CPU models:
- Base model: maps to base definition == will never change
   -> e.g. "z900-base"
- Default model: maps to default definition == can change (but migration
  safe via compatibility machines!)
  -> e.g. "z900" or "qemu"
- Host model: only for KVM, maps to "max model"
- Max model: (not exposed yet directly for TCG) "maximum supported model
  + features"

Full models do not exist!
So in summary, what you want to do here is:

1. Get the definition of the z800 model. Copy the base features to the
   max model.
2. Add all features that are supported additionally and not in the base.
3. (Maybe add a hack like Thomas implemented to support some further
features that are exceeding the full model)

So, especially, the CPU model "z800" does not map to the full model, but
the default model. It will never contain SIE features (at least 

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[Aurelien Jarno]

On 2017-06-02 13:30, David Hildenbrand wrote:
> On 02.06.2017 10:09, Thomas Huth wrote:
> > On 01.06.2017 21:17, Aurelien Jarno wrote:
> >> On 2017-06-01 11:04, David Hildenbrand wrote:
> >>> On 01.06.2017 10:38, David Hildenbrand wrote:
>  On 01.06.2017 00:01, Aurelien Jarno wrote:
> > At the same time fix the TCG version of get_max_cpu_model to return the
> > maximum model like on KVM. Remove the ETF2 and long-displacement
> 
>  I don't understand the part
>  "fix the TCG version of get_max_cpu_model to return the maximum model
>  like on KVM".
> 
>  Can you elaborate?
> 
> > facilities from the additional features as it is included in the z800.
> >
> > Signed-off-by: Aurelien Jarno 
> > ---
> >  target/s390x/cpu_models.c | 13 ++---
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> > index fc3cb25cc3..c13bbd852c 100644
> > --- a/target/s390x/cpu_models.c
> > +++ b/target/s390x/cpu_models.c
> > @@ -668,8 +668,6 @@ static void 
> > add_qemu_cpu_model_features(S390FeatBitmap fbm)
> >  static const int feats[] = {
> >  S390_FEAT_STFLE,
> >  S390_FEAT_EXTENDED_IMMEDIATE,
> > -S390_FEAT_EXTENDED_TRANSLATION_2,
> > -S390_FEAT_LONG_DISPLACEMENT,
> >  S390_FEAT_LONG_DISPLACEMENT_FAST,
> >  S390_FEAT_ETF2_ENH,
> >  S390_FEAT_STORE_CLOCK_FAST,
> > @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
> >  if (kvm_enabled()) {
> >  kvm_s390_get_host_cpu_model(_model, errp);
> >  } else {
> > -/* TCG emulates a z900 (with some optional additional 
> > features) */
> > -max_model.def = _cpu_defs[0];
> > -bitmap_copy(max_model.features, max_model.def->default_feat,
> > +/* TCG emulates a z800 (with some optional additional 
> > features) */
> > +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
> > +bitmap_copy(max_model.features, max_model.def->full_feat,
> >  S390_FEAT_MAX);
> >>>
> >>> This is most likely wrong: you're indicating features here that are not
> >>> available on tcg. esp. S390_FEAT_SIE_F2 and friends.
> >>>
> >>> I think should only copy the base features and add whatever else is
> >>> available via add_qemu_cpu_model_features() as already done.
> >>
> >> The patch series added all the z800 features exposed via STFL/STFLE.
> >> Indeed the SIE features are missing, but anyway QEMU doesn't emulate SIE
> >> at all so the lack of these features are not exposed to the guest. In that
> >> regard QEMU already wrongly claim to emulate a z900.
> 
> Please note that:
> 
> a) SIE features were never part of the max model. QEMU never claimed
> that. With your change one could suddenly do a -cpu z900,sie_f2=on,
> which is wrong.
> 
> b) The SIE_F2 feature tells the guest that the SIE instruction is
> available. E.g. Linux will look at this bit and show SIE support in
> /proc/cpuinfo and unlock the KVM module.

I understand your point. That said I doubt we will support the SIE
instruction soon (it looks quite complicated and I can't find any doc).
As we are going to emulate more facilities to QEMU, it will be more and
more difficult to select a modern CPU. One will have to use -cpu,z900,
etf2=on,ldisp=on,...,eimm=on. I think we have to provide users with an
easier way to do that.

One possibility would be to filter the features that are not emulated by
QEMU (like on the ppc64 target) or to allow the user to specify a higher
model provided the non emulated features are disabled. Something like
-cpu z800,sie_f2=off.

> Please, just don't add features to the MAX model that we don't implement.

Please note that QEMU does not fully implement S390_FEAT_ESAN3 (though
that is addressed in this patchset) and S390_FEAT_ZARCH, despite
claiming it.

> >  add_qemu_cpu_model_features(max_model.features);
> >  }
> > @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
> >  S390CPU *cpu = S390_CPU(obj);
> >  
> >  cpu->model = g_malloc0(sizeof(*cpu->model));
> > -/* TCG emulates a z900 (with some optional additional features) */
> > -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
> > sizeof(s390_qemu_cpu_defs));
> > +/* TCG emulates a z800 (with some optional additional features) */
> > +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
> > +   sizeof(s390_qemu_cpu_defs));
> 
>  No changing the qemu model without compatibility handling.
> 
> >>> Please have a look at the following mail for a possible solution:
> >>>
> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06030.html
> >>>
> >>> This could be moved to a separate patch. So this patch really 

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[Aurelien Jarno]

On 2017-06-01 21:56, David Hildenbrand wrote:
> On 01.06.2017 21:17, Aurelien Jarno wrote:
> > On 2017-06-01 10:38, David Hildenbrand wrote:
> >> On 01.06.2017 00:01, Aurelien Jarno wrote:
> >>> At the same time fix the TCG version of get_max_cpu_model to return the
> >>> maximum model like on KVM. Remove the ETF2 and long-displacement
> >>
> >> I don't understand the part
> >> "fix the TCG version of get_max_cpu_model to return the maximum model
> >> like on KVM".
> >>
> >> Can you elaborate?
> > 
> > Currently get_max_cpu_model returns the features of the base model, so
> > for example the one of a z900 even on a z800. This makes impossible to
> > enable the features that are provided by a z800 like etf2 or ldisp.
> > 
> 
> Right, you can always change the max_cpu_model, e.g. bumping up the
> version or adding new features, that is just fine.
> 
> > For what I understand from the KVM code (but I haven't tested), the
> > function return all the features that are supported by the current CPU,
> > not all the features that are supported by the base model of the current
> > CPU.
> 
> Correct, for KVM it is the detected model, that means: Base features +
> optional features.
> 
> 
> > 
> > 
> >>> facilities from the additional features as it is included in the z800.
> >>>
> >>> Signed-off-by: Aurelien Jarno 
> >>> ---
> >>>  target/s390x/cpu_models.c | 13 ++---
> >>>  1 file changed, 6 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> >>> index fc3cb25cc3..c13bbd852c 100644
> >>> --- a/target/s390x/cpu_models.c
> >>> +++ b/target/s390x/cpu_models.c
> >>> @@ -668,8 +668,6 @@ static void 
> >>> add_qemu_cpu_model_features(S390FeatBitmap fbm)
> >>>  static const int feats[] = {
> >>>  S390_FEAT_STFLE,
> >>>  S390_FEAT_EXTENDED_IMMEDIATE,
> >>> -S390_FEAT_EXTENDED_TRANSLATION_2,
> >>> -S390_FEAT_LONG_DISPLACEMENT,
> >>>  S390_FEAT_LONG_DISPLACEMENT_FAST,
> >>>  S390_FEAT_ETF2_ENH,
> >>>  S390_FEAT_STORE_CLOCK_FAST,
> >>> @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
> >>>  if (kvm_enabled()) {
> >>>  kvm_s390_get_host_cpu_model(_model, errp);
> >>>  } else {
> >>> -/* TCG emulates a z900 (with some optional additional features) 
> >>> */
> >>> -max_model.def = _cpu_defs[0];
> >>> -bitmap_copy(max_model.features, max_model.def->default_feat,
> >>> +/* TCG emulates a z800 (with some optional additional features) 
> >>> */
> >>> +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
> >>> +bitmap_copy(max_model.features, max_model.def->full_feat,
> >>>  S390_FEAT_MAX);
> >>>  add_qemu_cpu_model_features(max_model.features);
> >>>  }
> >>> @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
> >>>  S390CPU *cpu = S390_CPU(obj);
> >>>  
> >>>  cpu->model = g_malloc0(sizeof(*cpu->model));
> >>> -/* TCG emulates a z900 (with some optional additional features) */
> >>> -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
> >>> sizeof(s390_qemu_cpu_defs));
> >>> +/* TCG emulates a z800 (with some optional additional features) */
> >>> +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
> >>> +   sizeof(s390_qemu_cpu_defs));
> >>
> >> No changing the qemu model without compatibility handling.
> > 
> > This patch series is based on the patch from Thomas Huth. It means the
> > QEMU model is still based on a z900, but that it is possible to enable
> > some more features like etf2.
> 
> Thomas' code did neither change features nor the "model definition". It
> just allows for some more feature to be set. It is a hack.
> 
> I am pretty sure that expanding the "qemu" CPU model now (QMP
> query-cpu-model-expansion) will indicate a z800, not a z900.
> 
> See cpu_info_from_model(). And that is a problem, because the QEMU CPU
> model is a "migration-safe" CPU model, meaning it must remain equal for
> every compatibility machine.
> 

Ok, I get what you mean now.

Aurelien

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

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[David Hildenbrand]

On 02.06.2017 10:09, Thomas Huth wrote:
> On 01.06.2017 21:17, Aurelien Jarno wrote:
>> On 2017-06-01 11:04, David Hildenbrand wrote:
>>> On 01.06.2017 10:38, David Hildenbrand wrote:
 On 01.06.2017 00:01, Aurelien Jarno wrote:
> At the same time fix the TCG version of get_max_cpu_model to return the
> maximum model like on KVM. Remove the ETF2 and long-displacement

 I don't understand the part
 "fix the TCG version of get_max_cpu_model to return the maximum model
 like on KVM".

 Can you elaborate?

> facilities from the additional features as it is included in the z800.
>
> Signed-off-by: Aurelien Jarno 
> ---
>  target/s390x/cpu_models.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index fc3cb25cc3..c13bbd852c 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -668,8 +668,6 @@ static void 
> add_qemu_cpu_model_features(S390FeatBitmap fbm)
>  static const int feats[] = {
>  S390_FEAT_STFLE,
>  S390_FEAT_EXTENDED_IMMEDIATE,
> -S390_FEAT_EXTENDED_TRANSLATION_2,
> -S390_FEAT_LONG_DISPLACEMENT,
>  S390_FEAT_LONG_DISPLACEMENT_FAST,
>  S390_FEAT_ETF2_ENH,
>  S390_FEAT_STORE_CLOCK_FAST,
> @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>  if (kvm_enabled()) {
>  kvm_s390_get_host_cpu_model(_model, errp);
>  } else {
> -/* TCG emulates a z900 (with some optional additional features) 
> */
> -max_model.def = _cpu_defs[0];
> -bitmap_copy(max_model.features, max_model.def->default_feat,
> +/* TCG emulates a z800 (with some optional additional features) 
> */
> +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
> +bitmap_copy(max_model.features, max_model.def->full_feat,
>  S390_FEAT_MAX);
>>>
>>> This is most likely wrong: you're indicating features here that are not
>>> available on tcg. esp. S390_FEAT_SIE_F2 and friends.
>>>
>>> I think should only copy the base features and add whatever else is
>>> available via add_qemu_cpu_model_features() as already done.
>>
>> The patch series added all the z800 features exposed via STFL/STFLE.
>> Indeed the SIE features are missing, but anyway QEMU doesn't emulate SIE
>> at all so the lack of these features are not exposed to the guest. In that
>> regard QEMU already wrongly claim to emulate a z900.

Please note that:

a) SIE features were never part of the max model. QEMU never claimed
that. With your change one could suddenly do a -cpu z900,sie_f2=on,
which is wrong.

b) The SIE_F2 feature tells the guest that the SIE instruction is
available. E.g. Linux will look at this bit and show SIE support in
/proc/cpuinfo and unlock the KVM module.

Please, just don't add features to the MAX model that we don't implement.

>>
>>
>  add_qemu_cpu_model_features(max_model.features);
>  }
> @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
>  S390CPU *cpu = S390_CPU(obj);
>  
>  cpu->model = g_malloc0(sizeof(*cpu->model));
> -/* TCG emulates a z900 (with some optional additional features) */
> -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
> sizeof(s390_qemu_cpu_defs));
> +/* TCG emulates a z800 (with some optional additional features) */
> +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
> +   sizeof(s390_qemu_cpu_defs));

 No changing the qemu model without compatibility handling.

>>> Please have a look at the following mail for a possible solution:
>>>
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06030.html
>>>
>>> This could be moved to a separate patch. So this patch really should
>>> just care about the maximum model, not the qemu model.
>>
>> From what I understand from this thread, the patch from Thomas Huth was
>> finally considered acceptable. I am adding him in Cc: so that he can
>> comment.
> 
> In the 2nd version of my patch, I only changed the full_feat of the
> model definitions, but not the base_feat and default_feat fields, so
> that the CPU stays the same when you run QEMU without "-cpu" parameter
> (or simply with "-cpu qemu").
> 
> Consider that the following scenario should work: Start a QEMU v2.10
> with "-M s390-ccw-virtio-2.9" and a QEMU v2.9 with "-M
> s390-ccw-virtio-2.9 -incoming ...". Then it should be possible to
> migrate from the v2.10 to the v2.9 instance without problems. This won't
> work anymore if you changed the default feature bits unconditionally.
> 
>  Thomas
> 

The following can be used to make sure the model does not change:

s390x-softmmu/qemu-system-s390x -S -qmp stdio

-> { "execute": 

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[Thomas Huth]

On 01.06.2017 21:17, Aurelien Jarno wrote:
> On 2017-06-01 11:04, David Hildenbrand wrote:
>> On 01.06.2017 10:38, David Hildenbrand wrote:
>>> On 01.06.2017 00:01, Aurelien Jarno wrote:
 At the same time fix the TCG version of get_max_cpu_model to return the
 maximum model like on KVM. Remove the ETF2 and long-displacement
>>>
>>> I don't understand the part
>>> "fix the TCG version of get_max_cpu_model to return the maximum model
>>> like on KVM".
>>>
>>> Can you elaborate?
>>>
 facilities from the additional features as it is included in the z800.

 Signed-off-by: Aurelien Jarno 
 ---
  target/s390x/cpu_models.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

 diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
 index fc3cb25cc3..c13bbd852c 100644
 --- a/target/s390x/cpu_models.c
 +++ b/target/s390x/cpu_models.c
 @@ -668,8 +668,6 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
 fbm)
  static const int feats[] = {
  S390_FEAT_STFLE,
  S390_FEAT_EXTENDED_IMMEDIATE,
 -S390_FEAT_EXTENDED_TRANSLATION_2,
 -S390_FEAT_LONG_DISPLACEMENT,
  S390_FEAT_LONG_DISPLACEMENT_FAST,
  S390_FEAT_ETF2_ENH,
  S390_FEAT_STORE_CLOCK_FAST,
 @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
  if (kvm_enabled()) {
  kvm_s390_get_host_cpu_model(_model, errp);
  } else {
 -/* TCG emulates a z900 (with some optional additional features) */
 -max_model.def = _cpu_defs[0];
 -bitmap_copy(max_model.features, max_model.def->default_feat,
 +/* TCG emulates a z800 (with some optional additional features) */
 +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
 +bitmap_copy(max_model.features, max_model.def->full_feat,
  S390_FEAT_MAX);
>>
>> This is most likely wrong: you're indicating features here that are not
>> available on tcg. esp. S390_FEAT_SIE_F2 and friends.
>>
>> I think should only copy the base features and add whatever else is
>> available via add_qemu_cpu_model_features() as already done.
> 
> The patch series added all the z800 features exposed via STFL/STFLE.
> Indeed the SIE features are missing, but anyway QEMU doesn't emulate SIE
> at all so the lack of these features are not exposed to the guest. In that
> regard QEMU already wrongly claim to emulate a z900.
> 
> 
  add_qemu_cpu_model_features(max_model.features);
  }
 @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
  S390CPU *cpu = S390_CPU(obj);
  
  cpu->model = g_malloc0(sizeof(*cpu->model));
 -/* TCG emulates a z900 (with some optional additional features) */
 -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
 sizeof(s390_qemu_cpu_defs));
 +/* TCG emulates a z800 (with some optional additional features) */
 +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
 +   sizeof(s390_qemu_cpu_defs));
>>>
>>> No changing the qemu model without compatibility handling.
>>>
>> Please have a look at the following mail for a possible solution:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06030.html
>>
>> This could be moved to a separate patch. So this patch really should
>> just care about the maximum model, not the qemu model.
> 
> From what I understand from this thread, the patch from Thomas Huth was
> finally considered acceptable. I am adding him in Cc: so that he can
> comment.

In the 2nd version of my patch, I only changed the full_feat of the
model definitions, but not the base_feat and default_feat fields, so
that the CPU stays the same when you run QEMU without "-cpu" parameter
(or simply with "-cpu qemu").

Consider that the following scenario should work: Start a QEMU v2.10
with "-M s390-ccw-virtio-2.9" and a QEMU v2.9 with "-M
s390-ccw-virtio-2.9 -incoming ...". Then it should be possible to
migrate from the v2.10 to the v2.9 instance without problems. This won't
work anymore if you changed the default feature bits unconditionally.

 Thomas

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[David Hildenbrand]

On 01.06.2017 21:17, Aurelien Jarno wrote:
> On 2017-06-01 10:38, David Hildenbrand wrote:
>> On 01.06.2017 00:01, Aurelien Jarno wrote:
>>> At the same time fix the TCG version of get_max_cpu_model to return the
>>> maximum model like on KVM. Remove the ETF2 and long-displacement
>>
>> I don't understand the part
>> "fix the TCG version of get_max_cpu_model to return the maximum model
>> like on KVM".
>>
>> Can you elaborate?
> 
> Currently get_max_cpu_model returns the features of the base model, so
> for example the one of a z900 even on a z800. This makes impossible to
> enable the features that are provided by a z800 like etf2 or ldisp.
> 

Right, you can always change the max_cpu_model, e.g. bumping up the
version or adding new features, that is just fine.

> For what I understand from the KVM code (but I haven't tested), the
> function return all the features that are supported by the current CPU,
> not all the features that are supported by the base model of the current
> CPU.

Correct, for KVM it is the detected model, that means: Base features +
optional features.


> 
> 
>>> facilities from the additional features as it is included in the z800.
>>>
>>> Signed-off-by: Aurelien Jarno 
>>> ---
>>>  target/s390x/cpu_models.c | 13 ++---
>>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index fc3cb25cc3..c13bbd852c 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -668,8 +668,6 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
>>> fbm)
>>>  static const int feats[] = {
>>>  S390_FEAT_STFLE,
>>>  S390_FEAT_EXTENDED_IMMEDIATE,
>>> -S390_FEAT_EXTENDED_TRANSLATION_2,
>>> -S390_FEAT_LONG_DISPLACEMENT,
>>>  S390_FEAT_LONG_DISPLACEMENT_FAST,
>>>  S390_FEAT_ETF2_ENH,
>>>  S390_FEAT_STORE_CLOCK_FAST,
>>> @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>>>  if (kvm_enabled()) {
>>>  kvm_s390_get_host_cpu_model(_model, errp);
>>>  } else {
>>> -/* TCG emulates a z900 (with some optional additional features) */
>>> -max_model.def = _cpu_defs[0];
>>> -bitmap_copy(max_model.features, max_model.def->default_feat,
>>> +/* TCG emulates a z800 (with some optional additional features) */
>>> +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
>>> +bitmap_copy(max_model.features, max_model.def->full_feat,
>>>  S390_FEAT_MAX);
>>>  add_qemu_cpu_model_features(max_model.features);
>>>  }
>>> @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
>>>  S390CPU *cpu = S390_CPU(obj);
>>>  
>>>  cpu->model = g_malloc0(sizeof(*cpu->model));
>>> -/* TCG emulates a z900 (with some optional additional features) */
>>> -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
>>> sizeof(s390_qemu_cpu_defs));
>>> +/* TCG emulates a z800 (with some optional additional features) */
>>> +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
>>> +   sizeof(s390_qemu_cpu_defs));
>>
>> No changing the qemu model without compatibility handling.
> 
> This patch series is based on the patch from Thomas Huth. It means the
> QEMU model is still based on a z900, but that it is possible to enable
> some more features like etf2.

Thomas' code did neither change features nor the "model definition". It
just allows for some more feature to be set. It is a hack.

I am pretty sure that expanding the "qemu" CPU model now (QMP
query-cpu-model-expansion) will indicate a z800, not a z900.

See cpu_info_from_model(). And that is a problem, because the QEMU CPU
model is a "migration-safe" CPU model, meaning it must remain equal for
every compatibility machine.

Thanks.

> 
> Aurelien
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[Aurelien Jarno]

On 2017-06-01 11:04, David Hildenbrand wrote:
> On 01.06.2017 10:38, David Hildenbrand wrote:
> > On 01.06.2017 00:01, Aurelien Jarno wrote:
> >> At the same time fix the TCG version of get_max_cpu_model to return the
> >> maximum model like on KVM. Remove the ETF2 and long-displacement
> > 
> > I don't understand the part
> > "fix the TCG version of get_max_cpu_model to return the maximum model
> > like on KVM".
> > 
> > Can you elaborate?
> > 
> >> facilities from the additional features as it is included in the z800.
> >>
> >> Signed-off-by: Aurelien Jarno 
> >> ---
> >>  target/s390x/cpu_models.c | 13 ++---
> >>  1 file changed, 6 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> >> index fc3cb25cc3..c13bbd852c 100644
> >> --- a/target/s390x/cpu_models.c
> >> +++ b/target/s390x/cpu_models.c
> >> @@ -668,8 +668,6 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
> >> fbm)
> >>  static const int feats[] = {
> >>  S390_FEAT_STFLE,
> >>  S390_FEAT_EXTENDED_IMMEDIATE,
> >> -S390_FEAT_EXTENDED_TRANSLATION_2,
> >> -S390_FEAT_LONG_DISPLACEMENT,
> >>  S390_FEAT_LONG_DISPLACEMENT_FAST,
> >>  S390_FEAT_ETF2_ENH,
> >>  S390_FEAT_STORE_CLOCK_FAST,
> >> @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
> >>  if (kvm_enabled()) {
> >>  kvm_s390_get_host_cpu_model(_model, errp);
> >>  } else {
> >> -/* TCG emulates a z900 (with some optional additional features) */
> >> -max_model.def = _cpu_defs[0];
> >> -bitmap_copy(max_model.features, max_model.def->default_feat,
> >> +/* TCG emulates a z800 (with some optional additional features) */
> >> +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
> >> +bitmap_copy(max_model.features, max_model.def->full_feat,
> >>  S390_FEAT_MAX);
> 
> This is most likely wrong: you're indicating features here that are not
> available on tcg. esp. S390_FEAT_SIE_F2 and friends.
> 
> I think should only copy the base features and add whatever else is
> available via add_qemu_cpu_model_features() as already done.

The patch series added all the z800 features exposed via STFL/STFLE.
Indeed the SIE features are missing, but anyway QEMU doesn't emulate SIE
at all so the lack of these features are not exposed to the guest. In that
regard QEMU already wrongly claim to emulate a z900.


> >>  add_qemu_cpu_model_features(max_model.features);
> >>  }
> >> @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
> >>  S390CPU *cpu = S390_CPU(obj);
> >>  
> >>  cpu->model = g_malloc0(sizeof(*cpu->model));
> >> -/* TCG emulates a z900 (with some optional additional features) */
> >> -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
> >> sizeof(s390_qemu_cpu_defs));
> >> +/* TCG emulates a z800 (with some optional additional features) */
> >> +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
> >> +   sizeof(s390_qemu_cpu_defs));
> > 
> > No changing the qemu model without compatibility handling.
> > 
> Please have a look at the following mail for a possible solution:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06030.html
> 
> This could be moved to a separate patch. So this patch really should
> just care about the maximum model, not the qemu model.

From what I understand from this thread, the patch from Thomas Huth was
finally considered acceptable. I am adding him in Cc: so that he can
comment.

Aurelien

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

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[Aurelien Jarno]

On 2017-06-01 10:38, David Hildenbrand wrote:
> On 01.06.2017 00:01, Aurelien Jarno wrote:
> > At the same time fix the TCG version of get_max_cpu_model to return the
> > maximum model like on KVM. Remove the ETF2 and long-displacement
> 
> I don't understand the part
> "fix the TCG version of get_max_cpu_model to return the maximum model
> like on KVM".
> 
> Can you elaborate?

Currently get_max_cpu_model returns the features of the base model, so
for example the one of a z900 even on a z800. This makes impossible to
enable the features that are provided by a z800 like etf2 or ldisp.

For what I understand from the KVM code (but I haven't tested), the
function return all the features that are supported by the current CPU,
not all the features that are supported by the base model of the current
CPU.


> > facilities from the additional features as it is included in the z800.
> > 
> > Signed-off-by: Aurelien Jarno 
> > ---
> >  target/s390x/cpu_models.c | 13 ++---
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> > index fc3cb25cc3..c13bbd852c 100644
> > --- a/target/s390x/cpu_models.c
> > +++ b/target/s390x/cpu_models.c
> > @@ -668,8 +668,6 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
> > fbm)
> >  static const int feats[] = {
> >  S390_FEAT_STFLE,
> >  S390_FEAT_EXTENDED_IMMEDIATE,
> > -S390_FEAT_EXTENDED_TRANSLATION_2,
> > -S390_FEAT_LONG_DISPLACEMENT,
> >  S390_FEAT_LONG_DISPLACEMENT_FAST,
> >  S390_FEAT_ETF2_ENH,
> >  S390_FEAT_STORE_CLOCK_FAST,
> > @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
> >  if (kvm_enabled()) {
> >  kvm_s390_get_host_cpu_model(_model, errp);
> >  } else {
> > -/* TCG emulates a z900 (with some optional additional features) */
> > -max_model.def = _cpu_defs[0];
> > -bitmap_copy(max_model.features, max_model.def->default_feat,
> > +/* TCG emulates a z800 (with some optional additional features) */
> > +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
> > +bitmap_copy(max_model.features, max_model.def->full_feat,
> >  S390_FEAT_MAX);
> >  add_qemu_cpu_model_features(max_model.features);
> >  }
> > @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
> >  S390CPU *cpu = S390_CPU(obj);
> >  
> >  cpu->model = g_malloc0(sizeof(*cpu->model));
> > -/* TCG emulates a z900 (with some optional additional features) */
> > -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
> > sizeof(s390_qemu_cpu_defs));
> > +/* TCG emulates a z800 (with some optional additional features) */
> > +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
> > +   sizeof(s390_qemu_cpu_defs));
> 
> No changing the qemu model without compatibility handling.

This patch series is based on the patch from Thomas Huth. It means the
QEMU model is still based on a z900, but that it is possible to enable
some more features like etf2.

Aurelien

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

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[David Hildenbrand]

On 01.06.2017 10:38, David Hildenbrand wrote:
> On 01.06.2017 00:01, Aurelien Jarno wrote:
>> At the same time fix the TCG version of get_max_cpu_model to return the
>> maximum model like on KVM. Remove the ETF2 and long-displacement
> 
> I don't understand the part
> "fix the TCG version of get_max_cpu_model to return the maximum model
> like on KVM".
> 
> Can you elaborate?
> 
>> facilities from the additional features as it is included in the z800.
>>
>> Signed-off-by: Aurelien Jarno 
>> ---
>>  target/s390x/cpu_models.c | 13 ++---
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index fc3cb25cc3..c13bbd852c 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -668,8 +668,6 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
>> fbm)
>>  static const int feats[] = {
>>  S390_FEAT_STFLE,
>>  S390_FEAT_EXTENDED_IMMEDIATE,
>> -S390_FEAT_EXTENDED_TRANSLATION_2,
>> -S390_FEAT_LONG_DISPLACEMENT,
>>  S390_FEAT_LONG_DISPLACEMENT_FAST,
>>  S390_FEAT_ETF2_ENH,
>>  S390_FEAT_STORE_CLOCK_FAST,
>> @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>>  if (kvm_enabled()) {
>>  kvm_s390_get_host_cpu_model(_model, errp);
>>  } else {
>> -/* TCG emulates a z900 (with some optional additional features) */
>> -max_model.def = _cpu_defs[0];
>> -bitmap_copy(max_model.features, max_model.def->default_feat,
>> +/* TCG emulates a z800 (with some optional additional features) */
>> +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
>> +bitmap_copy(max_model.features, max_model.def->full_feat,
>>  S390_FEAT_MAX);

This is most likely wrong: you're indicating features here that are not
available on tcg. esp. S390_FEAT_SIE_F2 and friends.

I think should only copy the base features and add whatever else is
available via add_qemu_cpu_model_features() as already done.

>>  add_qemu_cpu_model_features(max_model.features);
>>  }
>> @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
>>  S390CPU *cpu = S390_CPU(obj);
>>  
>>  cpu->model = g_malloc0(sizeof(*cpu->model));
>> -/* TCG emulates a z900 (with some optional additional features) */
>> -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
>> sizeof(s390_qemu_cpu_defs));
>> +/* TCG emulates a z800 (with some optional additional features) */
>> +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
>> +   sizeof(s390_qemu_cpu_defs));
> 
> No changing the qemu model without compatibility handling.
> 
Please have a look at the following mail for a possible solution:

https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg06030.html

This could be moved to a separate patch. So this patch really should
just care about the maximum model, not the qemu model.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[David Hildenbrand]

On 01.06.2017 00:01, Aurelien Jarno wrote:
> At the same time fix the TCG version of get_max_cpu_model to return the
> maximum model like on KVM. Remove the ETF2 and long-displacement

I don't understand the part
"fix the TCG version of get_max_cpu_model to return the maximum model
like on KVM".

Can you elaborate?

> facilities from the additional features as it is included in the z800.
> 
> Signed-off-by: Aurelien Jarno 
> ---
>  target/s390x/cpu_models.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index fc3cb25cc3..c13bbd852c 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -668,8 +668,6 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
> fbm)
>  static const int feats[] = {
>  S390_FEAT_STFLE,
>  S390_FEAT_EXTENDED_IMMEDIATE,
> -S390_FEAT_EXTENDED_TRANSLATION_2,
> -S390_FEAT_LONG_DISPLACEMENT,
>  S390_FEAT_LONG_DISPLACEMENT_FAST,
>  S390_FEAT_ETF2_ENH,
>  S390_FEAT_STORE_CLOCK_FAST,
> @@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>  if (kvm_enabled()) {
>  kvm_s390_get_host_cpu_model(_model, errp);
>  } else {
> -/* TCG emulates a z900 (with some optional additional features) */
> -max_model.def = _cpu_defs[0];
> -bitmap_copy(max_model.features, max_model.def->default_feat,
> +/* TCG emulates a z800 (with some optional additional features) */
> +max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
> +bitmap_copy(max_model.features, max_model.def->full_feat,
>  S390_FEAT_MAX);
>  add_qemu_cpu_model_features(max_model.features);
>  }
> @@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
>  S390CPU *cpu = S390_CPU(obj);
>  
>  cpu->model = g_malloc0(sizeof(*cpu->model));
> -/* TCG emulates a z900 (with some optional additional features) */
> -memcpy(_qemu_cpu_defs, _cpu_defs[0], 
> sizeof(s390_qemu_cpu_defs));
> +/* TCG emulates a z800 (with some optional additional features) */
> +memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
> +   sizeof(s390_qemu_cpu_defs));

No changing the qemu model without compatibility handling.

Thanks!

-- 

Thanks,

David

[Qemu-devel] [PATCH v3 30/30] target/s390x: update maximum TCG model to z800

[Aurelien Jarno]

At the same time fix the TCG version of get_max_cpu_model to return the
maximum model like on KVM. Remove the ETF2 and long-displacement
facilities from the additional features as it is included in the z800.

Signed-off-by: Aurelien Jarno 
---
 target/s390x/cpu_models.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index fc3cb25cc3..c13bbd852c 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -668,8 +668,6 @@ static void add_qemu_cpu_model_features(S390FeatBitmap fbm)
 static const int feats[] = {
 S390_FEAT_STFLE,
 S390_FEAT_EXTENDED_IMMEDIATE,
-S390_FEAT_EXTENDED_TRANSLATION_2,
-S390_FEAT_LONG_DISPLACEMENT,
 S390_FEAT_LONG_DISPLACEMENT_FAST,
 S390_FEAT_ETF2_ENH,
 S390_FEAT_STORE_CLOCK_FAST,
@@ -696,9 +694,9 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
 if (kvm_enabled()) {
 kvm_s390_get_host_cpu_model(_model, errp);
 } else {
-/* TCG emulates a z900 (with some optional additional features) */
-max_model.def = _cpu_defs[0];
-bitmap_copy(max_model.features, max_model.def->default_feat,
+/* TCG emulates a z800 (with some optional additional features) */
+max_model.def = s390_find_cpu_def(0x2066, 7, 3, NULL);
+bitmap_copy(max_model.features, max_model.def->full_feat,
 S390_FEAT_MAX);
 add_qemu_cpu_model_features(max_model.features);
 }
@@ -956,8 +954,9 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
 S390CPU *cpu = S390_CPU(obj);
 
 cpu->model = g_malloc0(sizeof(*cpu->model));
-/* TCG emulates a z900 (with some optional additional features) */
-memcpy(_qemu_cpu_defs, _cpu_defs[0], sizeof(s390_qemu_cpu_defs));
+/* TCG emulates a z800 (with some optional additional features) */
+memcpy(_qemu_cpu_defs, s390_find_cpu_def(0x2066, 7, 3, NULL),
+   sizeof(s390_qemu_cpu_defs));
 add_qemu_cpu_model_features(s390_qemu_cpu_defs.full_feat);
 cpu->model->def = _qemu_cpu_defs;
 bitmap_copy(cpu->model->features, cpu->model->def->default_feat,
-- 
2.11.0