[Qemu-devel] [PATCH v1 for-2.12 0/2] s390x/kvm: last minute fixes

[Qemu-devel] [PATCH v1 for-2.12 0/2] s390x/kvm: last minute fixes

[David Hildenbrand]

Patch 1 only applies to old KVM instances without the virtual memory access
IOCTL in KVM.

Patch 2 could be problematic when toggling PER in a restart interrupt.
Watchpoint recomputation only applies to TCG.

David Hildenbrand (2):
  s390x/mmu: don't overwrite pending exception in mmu translate
  s390x: load_psw() should only exchange the PSW for KVM

 target/s390x/helper.c     | 10 ++++++----
 target/s390x/mmu_helper.c |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v1 for-2.12 1/2] s390x/mmu: don't overwrite pending exception in mmu translate

[David Hildenbrand]

If we already triggered another exception, don't overwrite it with a
protection exception.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 1deeb6e6e4..a25deef5dd 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -325,7 +325,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
 
     r = mmu_translate_region(env, vaddr, asc, asce, level, raddr, flags, rw,
                              exc);
-    if (rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
+    if (!r && rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
         trigger_prot_fault(env, vaddr, asc, rw, exc);
         return -1;
     }
-- 
2.14.3

Re: [Qemu-devel] [PATCH v1 for-2.12 1/2] s390x/mmu: don't overwrite pending exception in mmu translate

[Thomas Huth]

On 09.04.2018 13:30, David Hildenbrand wrote:
> If we already triggered another exception, don't overwrite it with a
> protection exception.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 1deeb6e6e4..a25deef5dd 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -325,7 +325,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>  
>      r = mmu_translate_region(env, vaddr, asc, asce, level, raddr, flags, rw,
>                               exc);
> -    if (rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
> +    if (!r && rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
>          trigger_prot_fault(env, vaddr, asc, rw, exc);
>          return -1;
>      }

Looks right.

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

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.12 1/2] s390x/mmu: don't overwrite pending exception in mmu translate

[Christian Borntraeger]

On 04/09/2018 01:30 PM, David Hildenbrand wrote:
> If we already triggered another exception, don't overwrite it with a
> protection exception.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>


Can you add your description from the cover letter regarding
"only applies to old KVM instances without the virtual memory access
IOCTL in KVM."

with that
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/mmu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 1deeb6e6e4..a25deef5dd 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -325,7 +325,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>  
>      r = mmu_translate_region(env, vaddr, asc, asce, level, raddr, flags, rw,
>                               exc);
> -    if (rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
> +    if (!r && rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
>          trigger_prot_fault(env, vaddr, asc, rw, exc);
>          return -1;
>      }
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.12 1/2] s390x/mmu: don't overwrite pending exception in mmu translate

[Cornelia Huck]

On Mon, 9 Apr 2018 13:50:03 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 04/09/2018 01:30 PM, David Hildenbrand wrote:
> > If we already triggered another exception, don't overwrite it with a
> > protection exception.
> > 
> > Signed-off-by: David Hildenbrand <david@redhat.com>  
> 
> 
> Can you add your description from the cover letter regarding
> "only applies to old KVM instances without the virtual memory access
> IOCTL in KVM."

Will add that.

> 
> with that
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  target/s390x/mmu_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> > index 1deeb6e6e4..a25deef5dd 100644
> > --- a/target/s390x/mmu_helper.c
> > +++ b/target/s390x/mmu_helper.c
> > @@ -325,7 +325,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
> >  
> >      r = mmu_translate_region(env, vaddr, asc, asce, level, raddr, flags, rw,
> >                               exc);
> > -    if (rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
> > +    if (!r && rw == MMU_DATA_STORE && !(*flags & PAGE_WRITE)) {
> >          trigger_prot_fault(env, vaddr, asc, rw, exc);
> >          return -1;
> >      }
> >   
> 

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

[David Hildenbrand]

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

[Christian Borntraeger]

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

[David Hildenbrand]

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

[Christian Borntraeger]

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

[David Hildenbrand]

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

[Christian Borntraeger]

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

[Cornelia Huck]

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

[Thomas Huth]

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>

Re: [Qemu-devel] [PATCH v1 for-2.12 0/2] s390x/kvm: last minute fixes

[Cornelia Huck]

On Mon,  9 Apr 2018 13:30:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> Patch 1 only applies to old KVM instances without the virtual memory access
> IOCTL in KVM.
> 
> Patch 2 could be problematic when toggling PER in a restart interrupt.
> Watchpoint recomputation only applies to TCG.
> 
> David Hildenbrand (2):
>   s390x/mmu: don't overwrite pending exception in mmu translate
>   s390x: load_psw() should only exchange the PSW for KVM
> 
>  target/s390x/helper.c     | 10 ++++++----
>  target/s390x/mmu_helper.c |  2 +-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 

Thanks, queued to s390-fixes.