1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v1 for-2.12 0/2] s390x/kvm: last minute fixes
  4. View patch

[Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM

David Hildenbrand posted 2 patches 7 years, 10 months ago
[Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by David Hildenbrand 7 years, 10 months ago 
Let's simplify it a bit. On some weird circumstances we would have tried
to recompute watchpoints when running under KVM.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 615fa24ab9..e8548f340a 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
 
     env->psw.addr = addr;
     env->psw.mask = mask;
-    if (tcg_enabled()) {
-        env->cc_op = (mask >> 44) & 3;
+
+    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
+    if (!tcg_enabled()) {
+        return;
     }
+    env->cc_op = (mask >> 44) & 3;
 
     if ((old_mask ^ mask) & PSW_MASK_PER) {
         s390_cpu_recompute_watchpoints(CPU(s390_env_get_cpu(env)));
     }
 
-    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
-    if (tcg_enabled() && (mask & PSW_MASK_WAIT)) {
+    if (mask & PSW_MASK_WAIT) {
         s390_handle_wait(s390_env_get_cpu(env));
     }
 }
-- 
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by Christian Borntraeger 7 years, 10 months ago 

On 04/09/2018 01:30 PM, David Hildenbrand wrote:
> Let's simplify it a bit. On some weird circumstances we would have tried
> to recompute watchpoints when running under KVM.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 615fa24ab9..e8548f340a 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
> 
>      env->psw.addr = addr;
>      env->psw.mask = mask;
> -    if (tcg_enabled()) {
> -        env->cc_op = (mask >> 44) & 3;
> +
> +    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
> +    if (!tcg_enabled()) {
> +        return;
>      }
> +    env->cc_op = (mask >> 44) & 3;

Do we have any call path were KVM could call load_psw?

> 
>      if ((old_mask ^ mask) & PSW_MASK_PER) {
>          s390_cpu_recompute_watchpoints(CPU(s390_env_get_cpu(env)));
>      }
> 
> -    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
> -    if (tcg_enabled() && (mask & PSW_MASK_WAIT)) {
> +    if (mask & PSW_MASK_WAIT) {
>          s390_handle_wait(s390_env_get_cpu(env));
>      }
>  }
>
Re: [Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by David Hildenbrand 7 years, 10 months ago 
On 09.04.2018 13:35, Christian Borntraeger wrote:
> 
> 
> On 04/09/2018 01:30 PM, David Hildenbrand wrote:
>> Let's simplify it a bit. On some weird circumstances we would have tried
>> to recompute watchpoints when running under KVM.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/helper.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>> index 615fa24ab9..e8548f340a 100644
>> --- a/target/s390x/helper.c
>> +++ b/target/s390x/helper.c
>> @@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>>
>>      env->psw.addr = addr;
>>      env->psw.mask = mask;
>> -    if (tcg_enabled()) {
>> -        env->cc_op = (mask >> 44) & 3;
>> +
>> +    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
>> +    if (!tcg_enabled()) {
>> +        return;
>>      }
>> +    env->cc_op = (mask >> 44) & 3;
> 
> Do we have any call path were KVM could call load_psw?

do_restart_interrupt()

SIGP while the target CPU is stopped.

> 
>>
>>      if ((old_mask ^ mask) & PSW_MASK_PER) {
>>          s390_cpu_recompute_watchpoints(CPU(s390_env_get_cpu(env)));
>>      }
>>
>> -    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
>> -    if (tcg_enabled() && (mask & PSW_MASK_WAIT)) {
>> +    if (mask & PSW_MASK_WAIT) {
>>          s390_handle_wait(s390_env_get_cpu(env));
>>      }
>>  }
>>
> 


-- 

Thanks,

David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by Christian Borntraeger 7 years, 10 months ago 

On 04/09/2018 01:36 PM, David Hildenbrand wrote:
> On 09.04.2018 13:35, Christian Borntraeger wrote:
>>
>>
>> On 04/09/2018 01:30 PM, David Hildenbrand wrote:
>>> Let's simplify it a bit. On some weird circumstances we would have tried
>>> to recompute watchpoints when running under KVM.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  target/s390x/helper.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>>> index 615fa24ab9..e8548f340a 100644
>>> --- a/target/s390x/helper.c
>>> +++ b/target/s390x/helper.c
>>> @@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>>>
>>>      env->psw.addr = addr;
>>>      env->psw.mask = mask;
>>> -    if (tcg_enabled()) {
>>> -        env->cc_op = (mask >> 44) & 3;
>>> +
>>> +    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
>>> +    if (!tcg_enabled()) {
>>> +        return;
>>>      }
>>> +    env->cc_op = (mask >> 44) & 3;
>>
>> Do we have any call path were KVM could call load_psw?
> 
> do_restart_interrupt()
> 
> SIGP while the target CPU is stopped.

makes sense. Can you add that to the patch description? that makes it easier to understand
what can really go wrong without this patch.

> 
>>
>>>
>>>      if ((old_mask ^ mask) & PSW_MASK_PER) {
>>>          s390_cpu_recompute_watchpoints(CPU(s390_env_get_cpu(env)));
>>>      }
>>>
>>> -    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
>>> -    if (tcg_enabled() && (mask & PSW_MASK_WAIT)) {
>>> +    if (mask & PSW_MASK_WAIT) {
>>>          s390_handle_wait(s390_env_get_cpu(env));
>>>      }
>>>  }
>>>
>>
> 
>
Re: [Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by David Hildenbrand 7 years, 10 months ago 
On 09.04.2018 13:40, Christian Borntraeger wrote:
> 
> 
> On 04/09/2018 01:36 PM, David Hildenbrand wrote:
>> On 09.04.2018 13:35, Christian Borntraeger wrote:
>>>
>>>
>>> On 04/09/2018 01:30 PM, David Hildenbrand wrote:
>>>> Let's simplify it a bit. On some weird circumstances we would have tried
>>>> to recompute watchpoints when running under KVM.



>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  target/s390x/helper.c | 10 ++++++----
>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>>>> index 615fa24ab9..e8548f340a 100644
>>>> --- a/target/s390x/helper.c
>>>> +++ b/target/s390x/helper.c
>>>> @@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>>>>
>>>>      env->psw.addr = addr;
>>>>      env->psw.mask = mask;
>>>> -    if (tcg_enabled()) {
>>>> -        env->cc_op = (mask >> 44) & 3;
>>>> +
>>>> +    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
>>>> +    if (!tcg_enabled()) {
>>>> +        return;
>>>>      }
>>>> +    env->cc_op = (mask >> 44) & 3;
>>>
>>> Do we have any call path were KVM could call load_psw?
>>
>> do_restart_interrupt()
>>
>> SIGP while the target CPU is stopped.
> 
> makes sense. Can you add that to the patch description? that makes it easier to understand
> what can really go wrong without this patch.

Sure!

Conny, when (and if ;) ) picking this up, can you change the description to

"Let's simplify it a bit. On some weird circumstances we would have
tried to recompute watchpoints when running under KVM. load_psw() is
called from do_restart_interrupt() during a SIGP RESTART if the target
CPU is STOPPED. Let's touch watchpoints only in the TCG case - where
they are used for PER emulation."


-- 

Thanks,

David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by Christian Borntraeger 7 years, 10 months ago 

On 04/09/2018 01:44 PM, David Hildenbrand wrote:
> On 09.04.2018 13:40, Christian Borntraeger wrote:
>>
>>
>> On 04/09/2018 01:36 PM, David Hildenbrand wrote:
>>> On 09.04.2018 13:35, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 04/09/2018 01:30 PM, David Hildenbrand wrote:
>>>>> Let's simplify it a bit. On some weird circumstances we would have tried
>>>>> to recompute watchpoints when running under KVM.
> 
> 
> 
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  target/s390x/helper.c | 10 ++++++----
>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
>>>>> index 615fa24ab9..e8548f340a 100644
>>>>> --- a/target/s390x/helper.c
>>>>> +++ b/target/s390x/helper.c
>>>>> @@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>>>>>
>>>>>      env->psw.addr = addr;
>>>>>      env->psw.mask = mask;
>>>>> -    if (tcg_enabled()) {
>>>>> -        env->cc_op = (mask >> 44) & 3;
>>>>> +
>>>>> +    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
>>>>> +    if (!tcg_enabled()) {
>>>>> +        return;
>>>>>      }
>>>>> +    env->cc_op = (mask >> 44) & 3;
>>>>
>>>> Do we have any call path were KVM could call load_psw?
>>>
>>> do_restart_interrupt()
>>>
>>> SIGP while the target CPU is stopped.
>>
>> makes sense. Can you add that to the patch description? that makes it easier to understand
>> what can really go wrong without this patch.
> 
> Sure!
> 
> Conny, when (and if ;) ) picking this up, can you change the description to
> 
> "Let's simplify it a bit. On some weird circumstances we would have
> tried to recompute watchpoints when running under KVM. load_psw() is
> called from do_restart_interrupt() during a SIGP RESTART if the target
> CPU is STOPPED. Let's touch watchpoints only in the TCG case - where
> they are used for PER emulation."
> 
With that:

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Re: [Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by Cornelia Huck 7 years, 10 months ago 
On Mon, 9 Apr 2018 13:44:30 +0200
David Hildenbrand <david@redhat.com> wrote:

> Conny, when (and if ;) ) picking this up, can you change the description to
> 
> "Let's simplify it a bit. On some weird circumstances we would have
> tried to recompute watchpoints when running under KVM. load_psw() is
> called from do_restart_interrupt() during a SIGP RESTART if the target
> CPU is STOPPED. Let's touch watchpoints only in the TCG case - where
> they are used for PER emulation."

Will do. Patch looks good to me, but waiting for some review.
Re: [Qemu-devel] [PATCH v1 for-2.12 2/2] s390x: load_psw() should only exchange the PSW for KVM
Posted by Thomas Huth 7 years, 10 months ago 
On 09.04.2018 13:30, David Hildenbrand wrote:
> Let's simplify it a bit. On some weird circumstances we would have tried
> to recompute watchpoints when running under KVM.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 615fa24ab9..e8548f340a 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>  
>      env->psw.addr = addr;
>      env->psw.mask = mask;
> -    if (tcg_enabled()) {
> -        env->cc_op = (mask >> 44) & 3;
> +
> +    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
> +    if (!tcg_enabled()) {
> +        return;
>      }
> +    env->cc_op = (mask >> 44) & 3;
>  
>      if ((old_mask ^ mask) & PSW_MASK_PER) {
>          s390_cpu_recompute_watchpoints(CPU(s390_env_get_cpu(env)));
>      }
>  
> -    /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
> -    if (tcg_enabled() && (mask & PSW_MASK_WAIT)) {
> +    if (mask & PSW_MASK_WAIT) {
>          s390_handle_wait(s390_env_get_cpu(env));
>      }
>  }
> 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.