[Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[David Hildenbrand]

For now, the kernel does not properly indicate configured CPU subfunctions
to the guest, but simply uses the host values (as support in KVM is still
missing). That's why we missed to model the PTFF subfunctions that come
with Multiple-epoch facility.

Let's properly add these, along with a new feature group.

Signed-off-by: David Hildenbrand <david@redhat.com>
---

v1 -> v2:
- s/MEPOCH/MULTIPLE_EPOCH/ (only internally visible)
- Add the features to the z14 full model
- Clear the features if Multiple-epoch facility is not installed

 target/s390x/cpu_features.c     |  5 +++++
 target/s390x/cpu_features_def.h |  4 ++++
 target/s390x/gen-features.c     | 11 +++++++++++
 target/s390x/kvm.c              |  8 ++++++++
 4 files changed, 28 insertions(+)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 85d10b5710..a5619f2893 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -156,8 +156,12 @@ static const S390FeatDef s390_features[] = {
     FEAT_INIT("ptff-qpc", S390_FEAT_TYPE_PTFF, 3, "PTFF Query Physical Clock"),
     FEAT_INIT("ptff-qui", S390_FEAT_TYPE_PTFF, 4, "PTFF Query UTC Information"),
     FEAT_INIT("ptff-qtou", S390_FEAT_TYPE_PTFF, 5, "PTFF Query TOD Offset User"),
+    FEAT_INIT("ptff-qsie", S390_FEAT_TYPE_PTFF, 10, "PTFF Query Steering Information Extended"),
+    FEAT_INIT("ptff-qtoue", S390_FEAT_TYPE_PTFF, 13, "PTFF Query TOD Offset User Extended"),
     FEAT_INIT("ptff-sto", S390_FEAT_TYPE_PTFF, 65, "PTFF Set TOD Offset"),
     FEAT_INIT("ptff-stou", S390_FEAT_TYPE_PTFF, 69, "PTFF Set TOD Offset User"),
+    FEAT_INIT("ptff-stoe", S390_FEAT_TYPE_PTFF, 73, "PTFF Set TOD Offset Extended"),
+    FEAT_INIT("ptff-stoue", S390_FEAT_TYPE_PTFF, 77, "PTFF Set TOD Offset User Extended"),
 
     FEAT_INIT("kmac-dea", S390_FEAT_TYPE_KMAC, 1, "KMAC DEA"),
     FEAT_INIT("kmac-tdea-128", S390_FEAT_TYPE_KMAC, 2, "KMAC TDEA-128"),
@@ -445,6 +449,7 @@ static S390FeatGroupDef s390_feature_groups[] = {
     FEAT_GROUP_INIT("plo", PLO, "Perform-locked-operation facility"),
     FEAT_GROUP_INIT("tods", TOD_CLOCK_STEERING, "Tod-clock-steering facility"),
     FEAT_GROUP_INIT("gen13ptff", GEN13_PTFF, "PTFF enhancements introduced with z13"),
+    FEAT_GROUP_INIT("mepochptff", MULTIPLE_EPOCH_PTFF, "PTFF enhancements introduced with Multiple-epoch facility"),
     FEAT_GROUP_INIT("msa", MSA, "Message-security-assist facility"),
     FEAT_GROUP_INIT("msa1", MSA_EXT_1, "Message-security-assist-extension 1 facility"),
     FEAT_GROUP_INIT("msa2", MSA_EXT_2, "Message-security-assist-extension 2 facility"),
diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
index 4d930871b4..7c5915c7b2 100644
--- a/target/s390x/cpu_features_def.h
+++ b/target/s390x/cpu_features_def.h
@@ -151,8 +151,12 @@ typedef enum {
     S390_FEAT_PTFF_QPT,
     S390_FEAT_PTFF_QUI,
     S390_FEAT_PTFF_QTOU,
+    S390_FEAT_PTFF_QSIE,
+    S390_FEAT_PTFF_QTOUE,
     S390_FEAT_PTFF_STO,
     S390_FEAT_PTFF_STOU,
+    S390_FEAT_PTFF_STOE,
+    S390_FEAT_PTFF_STOUE,
 
     /* KMAC */
     S390_FEAT_KMAC_DEA,
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 58b6ac484e..716b06f726 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -59,6 +59,12 @@
     S390_FEAT_PTFF_QTOU, \
     S390_FEAT_PTFF_STOU
 
+#define S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF \
+    S390_FEAT_PTFF_QSIE, \
+    S390_FEAT_PTFF_QTOUE, \
+    S390_FEAT_PTFF_STOE, \
+    S390_FEAT_PTFF_STOUE
+
 #define S390_FEAT_GROUP_MSA \
     S390_FEAT_MSA, \
     S390_FEAT_KMAC_DEA, \
@@ -219,6 +225,9 @@ static uint16_t group_TOD_CLOCK_STEERING[] = {
 static uint16_t group_GEN13_PTFF[] = {
     S390_FEAT_GROUP_GEN13_PTFF,
 };
+static uint16_t group_MULTIPLE_EPOCH_PTFF[] = {
+    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
+};
 static uint16_t group_MSA[] = {
     S390_FEAT_GROUP_MSA,
 };
@@ -466,6 +475,7 @@ static uint16_t full_GEN14_GA1[] = {
     S390_FEAT_CMM_NT,
     S390_FEAT_HPMA2,
     S390_FEAT_SIE_KSS,
+    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
 };
 
 /* Default features (in order of release)
@@ -665,6 +675,7 @@ static FeatGroupDefSpec FeatGroupDef[] = {
     FEAT_GROUP_INITIALIZER(PLO),
     FEAT_GROUP_INITIALIZER(TOD_CLOCK_STEERING),
     FEAT_GROUP_INITIALIZER(GEN13_PTFF),
+    FEAT_GROUP_INITIALIZER(MULTIPLE_EPOCH_PTFF),
     FEAT_GROUP_INITIALIZER(MSA),
     FEAT_GROUP_INITIALIZER(MSA_EXT_1),
     FEAT_GROUP_INITIALIZER(MSA_EXT_2),
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index bfd14723f1..deb870921b 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         return;
     }
 
+    /* PTFF subfunctions might be indicated although kernel support missing */
+    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
+        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
+        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
+        clear_bit(S390_FEAT_PTFF_STOE, model->features);
+        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
+    }
+
     /* with cpu model support, CMM is only indicated if really available */
     if (kvm_s390_cmma_available()) {
         set_bit(S390_FEAT_CMM, model->features);
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[no-reply]

Hi,

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

Type: series
Message-id: 20180205102935.14736-1-david@redhat.com
Subject: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20180205102659.60552-1-marcel@redhat.com -> patchew/20180205102659.60552-1-marcel@redhat.com
 * [new tag]               patchew/20180205102935.14736-1-david@redhat.com -> patchew/20180205102935.14736-1-david@redhat.com
Switched to a new branch 'test'
7c277a23c1 s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

=== OUTPUT BEGIN ===
Checking PATCH 1/1: s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility...
ERROR: line over 90 characters
#25: FILE: target/s390x/cpu_features.c:159:
+    FEAT_INIT("ptff-qsie", S390_FEAT_TYPE_PTFF, 10, "PTFF Query Steering Information Extended"),

ERROR: line over 90 characters
#26: FILE: target/s390x/cpu_features.c:160:
+    FEAT_INIT("ptff-qtoue", S390_FEAT_TYPE_PTFF, 13, "PTFF Query TOD Offset User Extended"),

WARNING: line over 80 characters
#29: FILE: target/s390x/cpu_features.c:163:
+    FEAT_INIT("ptff-stoe", S390_FEAT_TYPE_PTFF, 73, "PTFF Set TOD Offset Extended"),

WARNING: line over 80 characters
#30: FILE: target/s390x/cpu_features.c:164:
+    FEAT_INIT("ptff-stoue", S390_FEAT_TYPE_PTFF, 77, "PTFF Set TOD Offset User Extended"),

ERROR: line over 90 characters
#38: FILE: target/s390x/cpu_features.c:452:
+    FEAT_GROUP_INIT("mepochptff", MULTIPLE_EPOCH_PTFF, "PTFF enhancements introduced with Multiple-epoch facility"),

ERROR: Macros with complex values should be enclosed in parenthesis
#67: FILE: target/s390x/gen-features.c:62:
+#define S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF \
+    S390_FEAT_PTFF_QSIE, \
+    S390_FEAT_PTFF_QTOUE, \
+    S390_FEAT_PTFF_STOE, \
+    S390_FEAT_PTFF_STOUE

total: 4 errors, 2 warnings, 80 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[Christian Borntraeger]

Looks sane on a z14.
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>


On 02/05/2018 11:29 AM, David Hildenbrand wrote:
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>          return;
>      }
> 
> +    /* PTFF subfunctions might be indicated although kernel support missing */
> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
> +    }
> +
>      /* with cpu model support, CMM is only indicated if really available */
>      if (kvm_s390_cmma_available()) {
>          set_bit(S390_FEAT_CMM, model->features);
> 

Do you also want to add something to check_consistency ?

Right now the following user error 
-cpu z14,mepoch=off,mepochptff=on
is accepted.
On the other hand we also have no consistency checks for other subfunctions.

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[David Hildenbrand]

On 05.02.2018 12:22, Christian Borntraeger wrote:
> Looks sane on a z14.
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 
> On 02/05/2018 11:29 AM, David Hildenbrand wrote:
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>          return;
>>      }
>>
>> +    /* PTFF subfunctions might be indicated although kernel support missing */
>> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
>> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
>> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
>> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
>> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
>> +    }
>> +
>>      /* with cpu model support, CMM is only indicated if really available */
>>      if (kvm_s390_cmma_available()) {
>>          set_bit(S390_FEAT_CMM, model->features);
>>
> 
> Do you also want to add something to check_consistency ?
> 
> Right now the following user error 
> -cpu z14,mepoch=off,mepochptff=on
> is accepted.
> On the other hand we also have no consistency checks for other subfunctions.
> 

Thought about that, but that implies that a CPU model runable now, will
not run without warnings. Especially if migrating. We could add such
checks if we would push this into stable.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[Cornelia Huck]

On Mon, 5 Feb 2018 12:27:33 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 05.02.2018 12:22, Christian Borntraeger wrote:
> > Looks sane on a z14.
> > Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > 
> > 
> > On 02/05/2018 11:29 AM, David Hildenbrand wrote:  
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
> >>          return;
> >>      }
> >>
> >> +    /* PTFF subfunctions might be indicated although kernel support missing */
> >> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
> >> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
> >> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
> >> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
> >> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
> >> +    }
> >> +
> >>      /* with cpu model support, CMM is only indicated if really available */
> >>      if (kvm_s390_cmma_available()) {
> >>          set_bit(S390_FEAT_CMM, model->features);
> >>  
> > 
> > Do you also want to add something to check_consistency ?
> > 
> > Right now the following user error 
> > -cpu z14,mepoch=off,mepochptff=on
> > is accepted.
> > On the other hand we also have no consistency checks for other subfunctions.
> >   
> 
> Thought about that, but that implies that a CPU model runable now, will
> not run without warnings. Especially if migrating. We could add such
> checks if we would push this into stable.
> 

So, adding this check for the z14 stuff would work iff pushed into
stable - but for the other subfunctions the ship has already sailed?

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[David Hildenbrand]

On 05.02.2018 13:22, Cornelia Huck wrote:
> On Mon, 5 Feb 2018 12:27:33 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 05.02.2018 12:22, Christian Borntraeger wrote:
>>> Looks sane on a z14.
>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>
>>>
>>> On 02/05/2018 11:29 AM, David Hildenbrand wrote:  
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>>          return;
>>>>      }
>>>>
>>>> +    /* PTFF subfunctions might be indicated although kernel support missing */
>>>> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
>>>> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
>>>> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
>>>> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
>>>> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
>>>> +    }
>>>> +
>>>>      /* with cpu model support, CMM is only indicated if really available */
>>>>      if (kvm_s390_cmma_available()) {
>>>>          set_bit(S390_FEAT_CMM, model->features);
>>>>  
>>>
>>> Do you also want to add something to check_consistency ?
>>>
>>> Right now the following user error 
>>> -cpu z14,mepoch=off,mepochptff=on
>>> is accepted.
>>> On the other hand we also have no consistency checks for other subfunctions.
>>>   
>>
>> Thought about that, but that implies that a CPU model runable now, will
>> not run without warnings. Especially if migrating. We could add such
>> checks if we would push this into stable.
>>
> 
> So, adding this check for the z14 stuff would work iff pushed into
> stable - but for the other subfunctions the ship has already sailed?
> 

I don't know if we really have problems with other subfunctions. We
could also add consistency checks there (the problem here is that we
actually have to add missing subfunctions). So it is easier to check for
consistency with already existing subfunctions.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[Cornelia Huck]

On Mon, 5 Feb 2018 13:37:17 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 05.02.2018 13:22, Cornelia Huck wrote:
> > On Mon, 5 Feb 2018 12:27:33 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 05.02.2018 12:22, Christian Borntraeger wrote:  
> >>> Looks sane on a z14.
> >>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>
> >>>
> >>> On 02/05/2018 11:29 AM, David Hildenbrand wrote:    
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
> >>>>          return;
> >>>>      }
> >>>>
> >>>> +    /* PTFF subfunctions might be indicated although kernel support missing */
> >>>> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
> >>>> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
> >>>> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
> >>>> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
> >>>> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
> >>>> +    }
> >>>> +
> >>>>      /* with cpu model support, CMM is only indicated if really available */
> >>>>      if (kvm_s390_cmma_available()) {
> >>>>          set_bit(S390_FEAT_CMM, model->features);
> >>>>    
> >>>
> >>> Do you also want to add something to check_consistency ?
> >>>
> >>> Right now the following user error 
> >>> -cpu z14,mepoch=off,mepochptff=on
> >>> is accepted.
> >>> On the other hand we also have no consistency checks for other subfunctions.
> >>>     
> >>
> >> Thought about that, but that implies that a CPU model runable now, will
> >> not run without warnings. Especially if migrating. We could add such
> >> checks if we would push this into stable.

I'm currently wondering whether this change would actually be
applicable and useful for stable. Given the way stable is usually used,
probably not.

> >>  
> > 
> > So, adding this check for the z14 stuff would work iff pushed into
> > stable - but for the other subfunctions the ship has already sailed?
> >   
> 
> I don't know if we really have problems with other subfunctions. We
> could also add consistency checks there (the problem here is that we
> actually have to add missing subfunctions). So it is easier to check for
> consistency with already existing subfunctions.

Hm, so not really worth the hassle, just keep this as-is (and apply
this patch as-is)?

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[David Hildenbrand]

On 06.02.2018 18:19, Cornelia Huck wrote:
> On Mon, 5 Feb 2018 13:37:17 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 05.02.2018 13:22, Cornelia Huck wrote:
>>> On Mon, 5 Feb 2018 12:27:33 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 05.02.2018 12:22, Christian Borntraeger wrote:  
>>>>> Looks sane on a z14.
>>>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>
>>>>>
>>>>> On 02/05/2018 11:29 AM, David Hildenbrand wrote:    
>>>>>> --- a/target/s390x/kvm.c
>>>>>> +++ b/target/s390x/kvm.c
>>>>>> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>>>>          return;
>>>>>>      }
>>>>>>
>>>>>> +    /* PTFF subfunctions might be indicated although kernel support missing */
>>>>>> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
>>>>>> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
>>>>>> +    }
>>>>>> +
>>>>>>      /* with cpu model support, CMM is only indicated if really available */
>>>>>>      if (kvm_s390_cmma_available()) {
>>>>>>          set_bit(S390_FEAT_CMM, model->features);
>>>>>>    
>>>>>
>>>>> Do you also want to add something to check_consistency ?
>>>>>
>>>>> Right now the following user error 
>>>>> -cpu z14,mepoch=off,mepochptff=on
>>>>> is accepted.
>>>>> On the other hand we also have no consistency checks for other subfunctions.
>>>>>     
>>>>
>>>> Thought about that, but that implies that a CPU model runable now, will
>>>> not run without warnings. Especially if migrating. We could add such
>>>> checks if we would push this into stable.
> 
> I'm currently wondering whether this change would actually be
> applicable and useful for stable. Given the way stable is usually used,
> probably not.
> 
>>>>  
>>>
>>> So, adding this check for the z14 stuff would work iff pushed into
>>> stable - but for the other subfunctions the ship has already sailed?
>>>   
>>
>> I don't know if we really have problems with other subfunctions. We
>> could also add consistency checks there (the problem here is that we
>> actually have to add missing subfunctions). So it is easier to check for
>> consistency with already existing subfunctions.
> 
> Hm, so not really worth the hassle, just keep this as-is (and apply
> this patch as-is)?
> 

Think this would be best. (if we would have considered this earlier, we
would now have "mepoch-base" (now "mepoch") and "mepoch" as
"mepoch-base,ptff-stoe,ptff-stoue..."), just as e.g. handled for the
tod-clock-steering features. But unfortunately we missed that.

Such bugs happen as the features are merged before there is chance to
rewiew documentation (before an updated PoP is out). It is what it is.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[Christian Borntraeger]

On 02/06/2018 06:19 PM, Cornelia Huck wrote:
> On Mon, 5 Feb 2018 13:37:17 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 05.02.2018 13:22, Cornelia Huck wrote:
>>> On Mon, 5 Feb 2018 12:27:33 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 05.02.2018 12:22, Christian Borntraeger wrote:  
>>>>> Looks sane on a z14.
>>>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>
>>>>>
>>>>> On 02/05/2018 11:29 AM, David Hildenbrand wrote:    
>>>>>> --- a/target/s390x/kvm.c
>>>>>> +++ b/target/s390x/kvm.c
>>>>>> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>>>>          return;
>>>>>>      }
>>>>>>
>>>>>> +    /* PTFF subfunctions might be indicated although kernel support missing */
>>>>>> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
>>>>>> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
>>>>>> +    }
>>>>>> +
>>>>>>      /* with cpu model support, CMM is only indicated if really available */
>>>>>>      if (kvm_s390_cmma_available()) {
>>>>>>          set_bit(S390_FEAT_CMM, model->features);
>>>>>>    
>>>>>
>>>>> Do you also want to add something to check_consistency ?
>>>>>
>>>>> Right now the following user error 
>>>>> -cpu z14,mepoch=off,mepochptff=on
>>>>> is accepted.
>>>>> On the other hand we also have no consistency checks for other subfunctions.
>>>>>     
>>>>
>>>> Thought about that, but that implies that a CPU model runable now, will
>>>> not run without warnings. Especially if migrating. We could add such
>>>> checks if we would push this into stable.
> 
> I'm currently wondering whether this change would actually be
> applicable and useful for stable. Given the way stable is usually used,
> probably not.

I think its not necessary right now.
Currently we do not handle the subfunction in the kernel (we still rely on
the IBC) and I think we really do not want to go down that path unless really
necessary.

Re: [Qemu-devel] [qemu-s390x] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[Christian Borntraeger]

On 02/05/2018 11:29 AM, David Hildenbrand wrote:
> For now, the kernel does not properly indicate configured CPU subfunctions
> to the guest, but simply uses the host values (as support in KVM is still
> missing). That's why we missed to model the PTFF subfunctions that come
> with Multiple-epoch facility.
> 
> Let's properly add these, along with a new feature group.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
> 
> v1 -> v2:
> - s/MEPOCH/MULTIPLE_EPOCH/ (only internally visible)
> - Add the features to the z14 full model
> - Clear the features if Multiple-epoch facility is not installed
> 
>  target/s390x/cpu_features.c     |  5 +++++
>  target/s390x/cpu_features_def.h |  4 ++++
>  target/s390x/gen-features.c     | 11 +++++++++++
>  target/s390x/kvm.c              |  8 ++++++++
>  4 files changed, 28 insertions(+)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 85d10b5710..a5619f2893 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -156,8 +156,12 @@ static const S390FeatDef s390_features[] = {
>      FEAT_INIT("ptff-qpc", S390_FEAT_TYPE_PTFF, 3, "PTFF Query Physical Clock"),
>      FEAT_INIT("ptff-qui", S390_FEAT_TYPE_PTFF, 4, "PTFF Query UTC Information"),
>      FEAT_INIT("ptff-qtou", S390_FEAT_TYPE_PTFF, 5, "PTFF Query TOD Offset User"),
> +    FEAT_INIT("ptff-qsie", S390_FEAT_TYPE_PTFF, 10, "PTFF Query Steering Information Extended"),
> +    FEAT_INIT("ptff-qtoue", S390_FEAT_TYPE_PTFF, 13, "PTFF Query TOD Offset User Extended"),
>      FEAT_INIT("ptff-sto", S390_FEAT_TYPE_PTFF, 65, "PTFF Set TOD Offset"),
>      FEAT_INIT("ptff-stou", S390_FEAT_TYPE_PTFF, 69, "PTFF Set TOD Offset User"),
> +    FEAT_INIT("ptff-stoe", S390_FEAT_TYPE_PTFF, 73, "PTFF Set TOD Offset Extended"),
> +    FEAT_INIT("ptff-stoue", S390_FEAT_TYPE_PTFF, 77, "PTFF Set TOD Offset User Extended"),
> 
>      FEAT_INIT("kmac-dea", S390_FEAT_TYPE_KMAC, 1, "KMAC DEA"),
>      FEAT_INIT("kmac-tdea-128", S390_FEAT_TYPE_KMAC, 2, "KMAC TDEA-128"),
> @@ -445,6 +449,7 @@ static S390FeatGroupDef s390_feature_groups[] = {
>      FEAT_GROUP_INIT("plo", PLO, "Perform-locked-operation facility"),
>      FEAT_GROUP_INIT("tods", TOD_CLOCK_STEERING, "Tod-clock-steering facility"),
>      FEAT_GROUP_INIT("gen13ptff", GEN13_PTFF, "PTFF enhancements introduced with z13"),
> +    FEAT_GROUP_INIT("mepochptff", MULTIPLE_EPOCH_PTFF, "PTFF enhancements introduced with Multiple-epoch facility"),
>      FEAT_GROUP_INIT("msa", MSA, "Message-security-assist facility"),
>      FEAT_GROUP_INIT("msa1", MSA_EXT_1, "Message-security-assist-extension 1 facility"),
>      FEAT_GROUP_INIT("msa2", MSA_EXT_2, "Message-security-assist-extension 2 facility"),
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index 4d930871b4..7c5915c7b2 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -151,8 +151,12 @@ typedef enum {
>      S390_FEAT_PTFF_QPT,
>      S390_FEAT_PTFF_QUI,
>      S390_FEAT_PTFF_QTOU,
> +    S390_FEAT_PTFF_QSIE,
> +    S390_FEAT_PTFF_QTOUE,
>      S390_FEAT_PTFF_STO,
>      S390_FEAT_PTFF_STOU,
> +    S390_FEAT_PTFF_STOE,
> +    S390_FEAT_PTFF_STOUE,
> 
>      /* KMAC */
>      S390_FEAT_KMAC_DEA,
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 58b6ac484e..716b06f726 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -59,6 +59,12 @@
>      S390_FEAT_PTFF_QTOU, \
>      S390_FEAT_PTFF_STOU
> 
> +#define S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF \
> +    S390_FEAT_PTFF_QSIE, \
> +    S390_FEAT_PTFF_QTOUE, \
> +    S390_FEAT_PTFF_STOE, \
> +    S390_FEAT_PTFF_STOUE
> +
>  #define S390_FEAT_GROUP_MSA \
>      S390_FEAT_MSA, \
>      S390_FEAT_KMAC_DEA, \
> @@ -219,6 +225,9 @@ static uint16_t group_TOD_CLOCK_STEERING[] = {
>  static uint16_t group_GEN13_PTFF[] = {
>      S390_FEAT_GROUP_GEN13_PTFF,
>  };
> +static uint16_t group_MULTIPLE_EPOCH_PTFF[] = {
> +    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
> +};
>  static uint16_t group_MSA[] = {
>      S390_FEAT_GROUP_MSA,
>  };
> @@ -466,6 +475,7 @@ static uint16_t full_GEN14_GA1[] = {
>      S390_FEAT_CMM_NT,
>      S390_FEAT_HPMA2,
>      S390_FEAT_SIE_KSS,
> +    S390_FEAT_GROUP_MULTIPLE_EPOCH_PTFF,
>  };
> 
>  /* Default features (in order of release)
> @@ -665,6 +675,7 @@ static FeatGroupDefSpec FeatGroupDef[] = {
>      FEAT_GROUP_INITIALIZER(PLO),
>      FEAT_GROUP_INITIALIZER(TOD_CLOCK_STEERING),
>      FEAT_GROUP_INITIALIZER(GEN13_PTFF),
> +    FEAT_GROUP_INITIALIZER(MULTIPLE_EPOCH_PTFF),
>      FEAT_GROUP_INITIALIZER(MSA),
>      FEAT_GROUP_INITIALIZER(MSA_EXT_1),
>      FEAT_GROUP_INITIALIZER(MSA_EXT_2),
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index bfd14723f1..deb870921b 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>          return;
>      }
> 
> +    /* PTFF subfunctions might be indicated although kernel support missing */
> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
> +    }
> +
>      /* with cpu model support, CMM is only indicated if really available */
>      if (kvm_s390_cmma_available()) {
>          set_bit(S390_FEAT_CMM, model->features);
> 

Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[Cornelia Huck]

On Mon,  5 Feb 2018 11:29:35 +0100
David Hildenbrand <david@redhat.com> wrote:

> For now, the kernel does not properly indicate configured CPU subfunctions
> to the guest, but simply uses the host values (as support in KVM is still
> missing). That's why we missed to model the PTFF subfunctions that come
> with Multiple-epoch facility.
> 
> Let's properly add these, along with a new feature group.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 
> v1 -> v2:
> - s/MEPOCH/MULTIPLE_EPOCH/ (only internally visible)
> - Add the features to the z14 full model
> - Clear the features if Multiple-epoch facility is not installed
> 
>  target/s390x/cpu_features.c     |  5 +++++
>  target/s390x/cpu_features_def.h |  4 ++++
>  target/s390x/gen-features.c     | 11 +++++++++++
>  target/s390x/kvm.c              |  8 ++++++++
>  4 files changed, 28 insertions(+)

Thanks, applied.