1. Patchew
  2. QEMU
  3. [PATCH 0/2] s390x: pv: Diag318 fixes
  4. View patch

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

Janosch Frank posted 2 patches 5 years, 3 months ago
Maintainers: Thomas Huth <thuth@redhat.com>, David Hildenbrand <david@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Richard Henderson <rth@twiddle.net>, Halil Pasic <pasic@linux.ibm.com>
There is a newer version of this series
[PATCH 2/2] s390x: pv: Fix diag318 PV fencing
Posted by Janosch Frank 5 years, 3 months ago 
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 2/2] s390x: pv: Fix diag318 PV fencing
Posted by David Hildenbrand 5 years, 3 months ago 
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
Posted by Christian Borntraeger 5 years, 3 months ago 

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
Posted by David Hildenbrand 5 years, 3 months ago 
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 2/2] s390x: pv: Fix diag318 PV fencing
Posted by Janosch Frank 5 years, 3 months ago 
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?
Re: [PATCH 2/2] s390x: pv: Fix diag318 PV fencing
Posted by David Hildenbrand 5 years, 3 months ago 
>> 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.