[PATCH 0/2] s390x: pv: Diag318 fixes

[PATCH 0/2] s390x: pv: Diag318 fixes

[Janosch Frank]

Here are two fixes for the diag318 support that fix crashes when
booting PV guests.

We're working on extending our testing to catch problems like these
earlier.


Branch:
https://gitlab.com/frankja/qemu/-/commits/bb/frankja/diag318_fixes

CI:
https://gitlab.com/frankja/qemu/-/pipelines/205771933

Janosch Frank (2):
  s390x: pv: Remove sclp boundary checks
  s390x: pv: Fix diag318 PV fencing

 hw/s390x/sclp.c    | 15 ++++++++++-----
 target/s390x/kvm.c |  3 +--
 2 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/2] s390x: pv: Remove sclp boundary checks

[Janosch Frank]

The SCLP boundary cross check is done by the Ultravisor for a
protected guest, hence we don't need to do it. As QEMU doesn't get a
valid SCCB address in protected mode this is even problematic and can
lead to QEMU reporting a false boundary cross error.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Fixes: db13387ca0 ("s390/sclp: rework sclp boundary checks")
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 hw/s390x/sclp.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 00f1e4648d..0cf2290826 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -285,11 +285,6 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
         goto out_write;
     }
 
-    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length), code)) {
-        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
-        goto out_write;
-    }
-
     sclp_c->execute(sclp, work_sccb, code);
 out_write:
     s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
-- 
2.25.1

[PATCH 2/2] s390x: pv: Fix diag318 PV fencing

[Janosch Frank]

Diag318 fencing needs to be determined on the current VM PV state and
not on the state that the VM has when we create the CPU model.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 hw/s390x/sclp.c    | 10 ++++++++++
 target/s390x/kvm.c |  3 +--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 0cf2290826..69aba402d3 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -22,6 +22,7 @@
 #include "hw/s390x/event-facility.h"
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/ipl.h"
+#include "hw/s390x/pv.h"
 
 static inline SCLPDevice *get_sclp_device(void)
 {
@@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
         s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
                             &read_info->fac134);
+        /*
+         * Diag318 is not available in protected mode and will result
+         * in an operation exception. As we can dynamically move in
+         * and out of protected mode, we need to fence the feature
+         * here rather than when creating the CPU model.
+         */
+        if (s390_is_pv()) {
+            read_info->fac134 &= ~0x80;
+        }
     }
 
     read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index f13eff688c..baa070fdf7 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2498,8 +2498,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
      */
     set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
 
-    /* DIAGNOSE 0x318 is not supported under protected virtualization */
-    if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
+    if (kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
         set_bit(S390_FEAT_DIAG_318, model->features);
     }
 
-- 
2.25.1

Re: [PATCH 1/2] s390x: pv: Remove sclp boundary checks

[David Hildenbrand]

On 21.10.20 15:43, Janosch Frank wrote:
> The SCLP boundary cross check is done by the Ultravisor for a
> protected guest, hence we don't need to do it. As QEMU doesn't get a
> valid SCCB address in protected mode this is even problematic and can
> lead to QEMU reporting a false boundary cross error.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Fixes: db13387ca0 ("s390/sclp: rework sclp boundary checks")
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 00f1e4648d..0cf2290826 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -285,11 +285,6 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> -    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length), code)) {
> -        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> -        goto out_write;
> -    }
> -
>      sclp_c->execute(sclp, work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
> 

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] s390x: pv: Fix diag318 PV fencing

[David Hildenbrand]

On 21.10.20 15:43, Janosch Frank wrote:
> Diag318 fencing needs to be determined on the current VM PV state and
> not on the state that the VM has when we create the CPU model.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  hw/s390x/sclp.c    | 10 ++++++++++
>  target/s390x/kvm.c |  3 +--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 0cf2290826..69aba402d3 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -22,6 +22,7 @@
>  #include "hw/s390x/event-facility.h"
>  #include "hw/s390x/s390-pci-bus.h"
>  #include "hw/s390x/ipl.h"
> +#include "hw/s390x/pv.h"
>  
>  static inline SCLPDevice *get_sclp_device(void)
>  {
> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
>          s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
>                              &read_info->fac134);
> +        /*
> +         * Diag318 is not available in protected mode and will result
> +         * in an operation exception. As we can dynamically move in
> +         * and out of protected mode, we need to fence the feature
> +         * here rather than when creating the CPU model.
> +         */
> +        if (s390_is_pv()) {
> +            read_info->fac134 &= ~0x80;
> +        }

Hmm, I thought firmware would handle exposing cpu features and similar,
so we can't temper with it ....

Can we move that into s390_get_feat_block instead and check against the
feature bit instead?

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] s390x: pv: Fix diag318 PV fencing

[Christian Borntraeger]

On 21.10.20 16:14, David Hildenbrand wrote:
> On 21.10.20 15:43, Janosch Frank wrote:
>> Diag318 fencing needs to be determined on the current VM PV state and
>> not on the state that the VM has when we create the CPU model.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
>> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c    | 10 ++++++++++
>>  target/s390x/kvm.c |  3 +--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 0cf2290826..69aba402d3 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -22,6 +22,7 @@
>>  #include "hw/s390x/event-facility.h"
>>  #include "hw/s390x/s390-pci-bus.h"
>>  #include "hw/s390x/ipl.h"
>> +#include "hw/s390x/pv.h"
>>  
>>  static inline SCLPDevice *get_sclp_device(void)
>>  {
>> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
>>          s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
>>                              &read_info->fac134);
>> +        /*
>> +         * Diag318 is not available in protected mode and will result
>> +         * in an operation exception. As we can dynamically move in
>> +         * and out of protected mode, we need to fence the feature
>> +         * here rather than when creating the CPU model.
>> +         */
>> +        if (s390_is_pv()) {
>> +            read_info->fac134 &= ~0x80;
>> +        }
> 
> Hmm, I thought firmware would handle exposing cpu features and similar,
> so we can't temper with it ....

Only the stfle bits. 
> 
> Can we move that into s390_get_feat_block instead and check against the
> feature bit instead?

No because we want to have this active for !pv and disabled for pv but we switch
this multiple times when doing boot/reboot. 

Re: [PATCH 2/2] s390x: pv: Fix diag318 PV fencing

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2148 bytes --]

On 10/21/20 4:14 PM, David Hildenbrand wrote:
> On 21.10.20 15:43, Janosch Frank wrote:
>> Diag318 fencing needs to be determined on the current VM PV state and
>> not on the state that the VM has when we create the CPU model.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
>> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  hw/s390x/sclp.c    | 10 ++++++++++
>>  target/s390x/kvm.c |  3 +--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 0cf2290826..69aba402d3 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -22,6 +22,7 @@
>>  #include "hw/s390x/event-facility.h"
>>  #include "hw/s390x/s390-pci-bus.h"
>>  #include "hw/s390x/ipl.h"
>> +#include "hw/s390x/pv.h"
>>  
>>  static inline SCLPDevice *get_sclp_device(void)
>>  {
>> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>      if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
>>          s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
>>                              &read_info->fac134);
>> +        /*
>> +         * Diag318 is not available in protected mode and will result
>> +         * in an operation exception. As we can dynamically move in
>> +         * and out of protected mode, we need to fence the feature
>> +         * here rather than when creating the CPU model.
>> +         */
>> +        if (s390_is_pv()) {
>> +            read_info->fac134 &= ~0x80;
>> +        }
> 
> Hmm, I thought firmware would handle exposing cpu features and similar,
> so we can't temper with it ....

STFLE data is indeed provided by the Ultravisor, but SCLP is still done
by QEMU as that data is often not machine specific as visible in this case

> 
> Can we move that into s390_get_feat_block instead and check against the
> feature bit instead?
> 
You mean fence inside the s390_get_feat_block() function?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] s390x: pv: Fix diag318 PV fencing

[David Hildenbrand]

>> Can we move that into s390_get_feat_block instead and check against the
>> feature bit instead?
>>
> You mean fence inside the s390_get_feat_block() function?

Yes, in

s390_fill_feat_block()

and to make it clean even in

s390_has_feat()

based on s390_is_pv().

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] s390x: pv: Fix diag318 PV fencing

[David Hildenbrand]

On 21.10.20 16:18, Christian Borntraeger wrote:
> 
> 
> On 21.10.20 16:14, David Hildenbrand wrote:
>> On 21.10.20 15:43, Janosch Frank wrote:
>>> Diag318 fencing needs to be determined on the current VM PV state and
>>> not on the state that the VM has when we create the CPU model.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
>>> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> ---
>>>  hw/s390x/sclp.c    | 10 ++++++++++
>>>  target/s390x/kvm.c |  3 +--
>>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>>> index 0cf2290826..69aba402d3 100644
>>> --- a/hw/s390x/sclp.c
>>> +++ b/hw/s390x/sclp.c
>>> @@ -22,6 +22,7 @@
>>>  #include "hw/s390x/event-facility.h"
>>>  #include "hw/s390x/s390-pci-bus.h"
>>>  #include "hw/s390x/ipl.h"
>>> +#include "hw/s390x/pv.h"
>>>  
>>>  static inline SCLPDevice *get_sclp_device(void)
>>>  {
>>> @@ -142,6 +143,15 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>>      if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
>>>          s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
>>>                              &read_info->fac134);
>>> +        /*
>>> +         * Diag318 is not available in protected mode and will result
>>> +         * in an operation exception. As we can dynamically move in
>>> +         * and out of protected mode, we need to fence the feature
>>> +         * here rather than when creating the CPU model.
>>> +         */
>>> +        if (s390_is_pv()) {
>>> +            read_info->fac134 &= ~0x80;
>>> +        }
>>
>> Hmm, I thought firmware would handle exposing cpu features and similar,
>> so we can't temper with it ....
> 
> Only the stfle bits. 
>>
>> Can we move that into s390_get_feat_block instead and check against the
>> feature bit instead?
> 
> No because we want to have this active for !pv and disabled for pv but we switch
> this multiple times when doing boot/reboot. 

Keeping the s390_is_pv() condition of course.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] s390x: pv: Remove sclp boundary checks

[Halil Pasic]

On Wed, 21 Oct 2020 09:43:44 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> The SCLP boundary cross check is done by the Ultravisor for a
> protected guest, hence we don't need to do it. As QEMU doesn't get a
> valid SCCB address in protected mode this is even problematic and can
> lead to QEMU reporting a false boundary cross error.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Fixes: db13387ca0 ("s390/sclp: rework sclp boundary checks")
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>

Acked-by: Halil Pasic <pasic@linux.ibm.com>

> ---
>  hw/s390x/sclp.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 00f1e4648d..0cf2290826 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -285,11 +285,6 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> -    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length), code)) {
> -        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> -        goto out_write;
> -    }
> -
>      sclp_c->execute(sclp, work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,

[PATCH] s390x: pv: Fix diag318 PV fencing

[Janosch Frank]

Diag318 fencing needs to be determined on the current VM PV state and
not on the state that the VM has when we create the CPU model.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
---

If you're sure that this is what you want, then I'll send a v2 of the
patch set.

---
 target/s390x/cpu_features.c | 5 +++++
 target/s390x/cpu_features.h | 4 ++++
 target/s390x/cpu_models.c   | 4 ++++
 target/s390x/kvm.c          | 3 +--
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 31ea8df246..42fe0bf4ca 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "cpu_features.h"
+#include "hw/s390x/pv.h"
 
 #define DEF_FEAT(_FEAT, _NAME, _TYPE, _BIT, _DESC) \
     [S390_FEAT_##_FEAT] = {                        \
@@ -105,6 +106,10 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
         }
         feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
     }
+
+    if (type == S390_FEAT_TYPE_SCLP_FAC134 && s390_is_pv()) {
+        clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
+    }
 }
 
 void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index ef52ffce83..87463f064d 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -81,6 +81,10 @@ const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup group);
 
 #define BE_BIT_NR(BIT) (BIT ^ (BITS_PER_LONG - 1))
 
+static inline void clear_be_bit(unsigned int bit_nr, uint8_t *array)
+{
+    array[bit_nr / 8] &= ~(0x80 >> (bit_nr % 8));
+}
 static inline void set_be_bit(unsigned int bit_nr, uint8_t *array)
 {
     array[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index ca484bfda7..461e0b8f4a 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -29,6 +29,7 @@
 #include "hw/pci/pci.h"
 #endif
 #include "qapi/qapi-commands-machine-target.h"
+#include "hw/s390x/pv.h"
 
 #define CPUDEF_INIT(_type, _gen, _ec_ga, _mha_pow, _hmfai, _name, _desc) \
     {                                                                    \
@@ -238,6 +239,9 @@ bool s390_has_feat(S390Feat feat)
         }
         return 0;
     }
+    if (feat == S390_FEAT_DIAG_318 && s390_is_pv()) {
+        return false;
+    }
     return test_bit(feat, cpu->model->features);
 }
 
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index f13eff688c..baa070fdf7 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2498,8 +2498,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
      */
     set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
 
-    /* DIAGNOSE 0x318 is not supported under protected virtualization */
-    if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
+    if (kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
         set_bit(S390_FEAT_DIAG_318, model->features);
     }
 
-- 
2.25.1

Re: [PATCH 1/2] s390x: pv: Remove sclp boundary checks

[Thomas Huth]

On 21/10/2020 15.43, Janosch Frank wrote:
> The SCLP boundary cross check is done by the Ultravisor for a
> protected guest, hence we don't need to do it. As QEMU doesn't get a
> valid SCCB address in protected mode this is even problematic and can
> lead to QEMU reporting a false boundary cross error.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Fixes: db13387ca0 ("s390/sclp: rework sclp boundary checks")
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  hw/s390x/sclp.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 00f1e4648d..0cf2290826 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -285,11 +285,6 @@ int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
>          goto out_write;
>      }
>  
> -    if (!sccb_verify_boundary(sccb, be16_to_cpu(work_sccb->h.length), code)) {
> -        work_sccb->h.response_code = cpu_to_be16(SCLP_RC_SCCB_BOUNDARY_VIOLATION);
> -        goto out_write;
> -    }
> -
>      sclp_c->execute(sclp, work_sccb, code);
>  out_write:
>      s390_cpu_pv_mem_write(env_archcpu(env), 0, work_sccb,
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] s390x: pv: Fix diag318 PV fencing

[David Hildenbrand]

On 22.10.20 10:23, Janosch Frank wrote:
> Diag318 fencing needs to be determined on the current VM PV state and
> not on the state that the VM has when we create the CPU model.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
> ---
> 
> If you're sure that this is what you want, then I'll send a v2 of the
> patch set.

So that's going to be the first CPU feature that gets suppressed in PC
mode - which seems to be what we want.

diag318_needed() will return false, resulting in vmstate_diag318() not
being included in the migration stream (I know, we don't support
migration yet for PV).

I don't see where diag318 would get reset during a reipl - is it
expected to be persistent when switching in/out of PV, or when reipling
etc..?

> 
> ---
>  target/s390x/cpu_features.c | 5 +++++
>  target/s390x/cpu_features.h | 4 ++++
>  target/s390x/cpu_models.c   | 4 ++++
>  target/s390x/kvm.c          | 3 +--
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 31ea8df246..42fe0bf4ca 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -14,6 +14,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "cpu_features.h"
> +#include "hw/s390x/pv.h"
>  
>  #define DEF_FEAT(_FEAT, _NAME, _TYPE, _BIT, _DESC) \
>      [S390_FEAT_##_FEAT] = {                        \
> @@ -105,6 +106,10 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
>          }
>          feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
>      }
> +
> +    if (type == S390_FEAT_TYPE_SCLP_FAC134 && s390_is_pv()) {
> +        clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
> +    }
>  }
>  
>  void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index ef52ffce83..87463f064d 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -81,6 +81,10 @@ const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup group);
>  
>  #define BE_BIT_NR(BIT) (BIT ^ (BITS_PER_LONG - 1))
>  
> +static inline void clear_be_bit(unsigned int bit_nr, uint8_t *array)
> +{
> +    array[bit_nr / 8] &= ~(0x80 >> (bit_nr % 8));
> +}
>  static inline void set_be_bit(unsigned int bit_nr, uint8_t *array)
>  {
>      array[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index ca484bfda7..461e0b8f4a 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -29,6 +29,7 @@
>  #include "hw/pci/pci.h"
>  #endif
>  #include "qapi/qapi-commands-machine-target.h"
> +#include "hw/s390x/pv.h"
>  
>  #define CPUDEF_INIT(_type, _gen, _ec_ga, _mha_pow, _hmfai, _name, _desc) \
>      {                                                                    \
> @@ -238,6 +239,9 @@ bool s390_has_feat(S390Feat feat)
>          }
>          return 0;
>      }
> +    if (feat == S390_FEAT_DIAG_318 && s390_is_pv()) {
> +        return false;
> +    }
>      return test_bit(feat, cpu->model->features);
>  }
>  
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index f13eff688c..baa070fdf7 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2498,8 +2498,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>       */
>      set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>  
> -    /* DIAGNOSE 0x318 is not supported under protected virtualization */
> -    if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
> +    if (kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
>          set_bit(S390_FEAT_DIAG_318, model->features);
>      }
>  
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH] s390x: pv: Fix diag318 PV fencing

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 4497 bytes --]

On 10/22/20 10:32 AM, David Hildenbrand wrote:
> On 22.10.20 10:23, Janosch Frank wrote:
>> Diag318 fencing needs to be determined on the current VM PV state and
>> not on the state that the VM has when we create the CPU model.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
>> ---
>>
>> If you're sure that this is what you want, then I'll send a v2 of the
>> patch set.
> 
> So that's going to be the first CPU feature that gets suppressed in PC
> mode - which seems to be what we want.
> 
> diag318_needed() will return false, resulting in vmstate_diag318() not
> being included in the migration stream (I know, we don't support
> migration yet for PV).

Well either you have it and need to migrate it or you don't.
As it doesn't persist over IPLs, that should not be a problem, no?

> 
> I don't see where diag318 would get reset during a reipl - is it
> expected to be persistent when switching in/out of PV, or when reipling
> etc..?

That's actually another bug we need to address. Diag318 will need to be
reset on a diag308 reset instead of when doing the cpu resets...

> 
>>
>> ---
>>  target/s390x/cpu_features.c | 5 +++++
>>  target/s390x/cpu_features.h | 4 ++++
>>  target/s390x/cpu_models.c   | 4 ++++
>>  target/s390x/kvm.c          | 3 +--
>>  4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index 31ea8df246..42fe0bf4ca 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -14,6 +14,7 @@
>>  #include "qemu/osdep.h"
>>  #include "qemu/module.h"
>>  #include "cpu_features.h"
>> +#include "hw/s390x/pv.h"
>>  
>>  #define DEF_FEAT(_FEAT, _NAME, _TYPE, _BIT, _DESC) \
>>      [S390_FEAT_##_FEAT] = {                        \
>> @@ -105,6 +106,10 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
>>          }
>>          feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
>>      }
>> +
>> +    if (type == S390_FEAT_TYPE_SCLP_FAC134 && s390_is_pv()) {
>> +        clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
>> +    }
>>  }
>>  
>>  void s390_add_from_feat_block(S390FeatBitmap features, S390FeatType type,
>> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
>> index ef52ffce83..87463f064d 100644
>> --- a/target/s390x/cpu_features.h
>> +++ b/target/s390x/cpu_features.h
>> @@ -81,6 +81,10 @@ const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup group);
>>  
>>  #define BE_BIT_NR(BIT) (BIT ^ (BITS_PER_LONG - 1))
>>  
>> +static inline void clear_be_bit(unsigned int bit_nr, uint8_t *array)
>> +{
>> +    array[bit_nr / 8] &= ~(0x80 >> (bit_nr % 8));
>> +}
>>  static inline void set_be_bit(unsigned int bit_nr, uint8_t *array)
>>  {
>>      array[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index ca484bfda7..461e0b8f4a 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -29,6 +29,7 @@
>>  #include "hw/pci/pci.h"
>>  #endif
>>  #include "qapi/qapi-commands-machine-target.h"
>> +#include "hw/s390x/pv.h"
>>  
>>  #define CPUDEF_INIT(_type, _gen, _ec_ga, _mha_pow, _hmfai, _name, _desc) \
>>      {                                                                    \
>> @@ -238,6 +239,9 @@ bool s390_has_feat(S390Feat feat)
>>          }
>>          return 0;
>>      }
>> +    if (feat == S390_FEAT_DIAG_318 && s390_is_pv()) {
>> +        return false;
>> +    }
>>      return test_bit(feat, cpu->model->features);
>>  }
>>  
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index f13eff688c..baa070fdf7 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2498,8 +2498,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>       */
>>      set_bit(S390_FEAT_EXTENDED_LENGTH_SCCB, model->features);
>>  
>> -    /* DIAGNOSE 0x318 is not supported under protected virtualization */
>> -    if (!s390_is_pv() && kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
>> +    if (kvm_check_extension(kvm_state, KVM_CAP_S390_DIAG318)) {
>>          set_bit(S390_FEAT_DIAG_318, model->features);
>>      }
>>  
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] s390x: pv: Fix diag318 PV fencing

[David Hildenbrand]

On 22.10.20 10:39, Janosch Frank wrote:
> On 10/22/20 10:32 AM, David Hildenbrand wrote:
>> On 22.10.20 10:23, Janosch Frank wrote:
>>> Diag318 fencing needs to be determined on the current VM PV state and
>>> not on the state that the VM has when we create the CPU model.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
>>> ---
>>>
>>> If you're sure that this is what you want, then I'll send a v2 of the
>>> patch set.
>>
>> So that's going to be the first CPU feature that gets suppressed in PC
>> mode - which seems to be what we want.
>>
>> diag318_needed() will return false, resulting in vmstate_diag318() not
>> being included in the migration stream (I know, we don't support
>> migration yet for PV).
> 
> Well either you have it and need to migrate it or you don't.
> As it doesn't persist over IPLs, that should not be a problem, no?

Okay, was confused by no finding a proper reset from our central reset
handler. So this feels like the right thing to do.


-- 
Thanks,

David / dhildenb

Re: [PATCH] s390x: pv: Fix diag318 PV fencing

[Halil Pasic]

On Thu, 22 Oct 2020 04:23:12 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Diag318 fencing needs to be determined on the current VM PV state and
> not on the state that the VM has when we create the CPU model.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
> ---
> 
> If you're sure that this is what you want, then I'll send a v2 of the
> patch set.
> 
> ---
>  target/s390x/cpu_features.c | 5 +++++
>  target/s390x/cpu_features.h | 4 ++++
>  target/s390x/cpu_models.c   | 4 ++++
>  target/s390x/kvm.c          | 3 +--
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 31ea8df246..42fe0bf4ca 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -14,6 +14,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "cpu_features.h"
> +#include "hw/s390x/pv.h"
>  
>  #define DEF_FEAT(_FEAT, _NAME, _TYPE, _BIT, _DESC) \
>      [S390_FEAT_##_FEAT] = {                        \
> @@ -105,6 +106,10 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
>          }
>          feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
>      }
> +
> +    if (type == S390_FEAT_TYPE_SCLP_FAC134 && s390_is_pv()) {
> +        clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
> +    }
>  }

Sorry, I'm a little rusty with cpu models. Does this affect the outcome
of the corresponding QMP commands?

I would guess it does...

Regards,
Halil

Re: [PATCH] s390x: pv: Fix diag318 PV fencing

[David Hildenbrand]

On 22.10.20 11:54, Halil Pasic wrote:
> On Thu, 22 Oct 2020 04:23:12 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Diag318 fencing needs to be determined on the current VM PV state and
>> not on the state that the VM has when we create the CPU model.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Fixes: fabdada935 ("s390: guest support for diagnose 0x318")
>> ---
>>
>> If you're sure that this is what you want, then I'll send a v2 of the
>> patch set.
>>
>> ---
>>  target/s390x/cpu_features.c | 5 +++++
>>  target/s390x/cpu_features.h | 4 ++++
>>  target/s390x/cpu_models.c   | 4 ++++
>>  target/s390x/kvm.c          | 3 +--
>>  4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index 31ea8df246..42fe0bf4ca 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -14,6 +14,7 @@
>>  #include "qemu/osdep.h"
>>  #include "qemu/module.h"
>>  #include "cpu_features.h"
>> +#include "hw/s390x/pv.h"
>>  
>>  #define DEF_FEAT(_FEAT, _NAME, _TYPE, _BIT, _DESC) \
>>      [S390_FEAT_##_FEAT] = {                        \
>> @@ -105,6 +106,10 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
>>          }
>>          feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
>>      }
>> +
>> +    if (type == S390_FEAT_TYPE_SCLP_FAC134 && s390_is_pv()) {
>> +        clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
>> +    }
>>  }
> 
> Sorry, I'm a little rusty with cpu models. Does this affect the outcome
> of the corresponding QMP commands?
> 
> I would guess it does...

No, it shouldn't.


-- 
Thanks,

David / dhildenb