Skip to contentSkip to conversation

s390x/kvm: last minute fixes

PATCH In review David Hildenbrand 4 participants 14 messages Apr 9, 2018

lore
Share & tools
lore

David Hildenbrand

author
#0 lore raw

cover s390x/kvm: last minute fixes

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(-)
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH 1/2 s390x/mmu: don't overwrite pending exception in mmu translate

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

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
target/s390x/mmu_helper.c +1 -1
index 1deeb6e6e4..a25deef5dd 100644
@@ -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;
     }
Signature · 1 line
2.14.3
Reviewed-by on v1 1/2
#2 lore raw

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

Quoted 13 lines · David Hildenbrand

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

Signed-off-by David Hildenbrand <[email protected]>
---
 target/s390x/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
target/s390x/mmu_helper.c +0 -0
index 1deeb6e6e4..a25deef5dd 100644

target/s390x/mmu_helper.c static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr, · line 331

Quoted 8 lines
target/s390x/mmu_helper.c +1 -1
@@ -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 <[email protected]>
Acked-by on v1 1/2
#3 lore raw

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

In reply to David Hildenbrand
Quoted 4 lines · David Hildenbrand

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

Signed-off-by David Hildenbrand <[email protected]>

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 <[email protected]>
Quoted 18 lines
---
 target/s390x/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
target/s390x/mmu_helper.c +1 -1
index 1deeb6e6e4..a25deef5dd 100644
@@ -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;
     }
#4 lore raw

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

Quoted 10 lines · On Mon, 9 Apr 2018 13:50:03 +0200 Christian Borntraeger…
Quoted 4 lines · David Hildenbrand

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

Signed-off-by David Hildenbrand <[email protected]>

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.

Quoted 22 lines

with that

Acked-by Christian Borntraeger <[email protected]>
Quoted 18 lines
---
 target/s390x/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
target/s390x/mmu_helper.c +1 -1
index 1deeb6e6e4..a25deef5dd 100644
@@ -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;
     }
author
#5 lore raw

PATCH 2/2 s390x: load_psw() should only exchange the PSW for KVM

In reply to 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 <[email protected]>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
target/s390x/helper.c +6 -4
index 615fa24ab9..e8548f340a 100644
@@ -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));
     }
 }
Signature · 1 line
2.14.3
#6 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
target/s390x/helper.c +0 -0
index 615fa24ab9..e8548f340a 100644
@@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)

target/s390x/helper.c void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr) · line 111

Quoted 10 lines
target/s390x/helper.c +5 -2
     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?

Quoted 12 lines
target/s390x/helper.c +1 -2
     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));
     }
 }
author
#7 lore raw

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

Quoted 17 lines · Christian Borntraeger
Quoted 14 lines · 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 <[email protected]>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
target/s390x/helper.c +0 -0
index 615fa24ab9..e8548f340a 100644
@@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
Quoted 12 lines

target/s390x/helper.c void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr) · line 111

Quoted 10 lines
target/s390x/helper.c +5 -2
     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.

Quoted 14 lines
Quoted 13 lines
target/s390x/helper.c +1 -2
     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));
     }
 }
Signature · 3 lines
Thanks,

David / dhildenb
#8 lore raw

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

Quoted 31 lines · David Hildenbrand
Quoted 17 lines · Christian Borntraeger
Quoted 14 lines · 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 <[email protected]>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
target/s390x/helper.c +0 -0
index 615fa24ab9..e8548f340a 100644
@@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
Quoted 12 lines

target/s390x/helper.c void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr) · line 111

Quoted 10 lines
target/s390x/helper.c +5 -2
     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?

Quoted 3 lines

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.

Quoted 17 lines
Quoted 15 lines
Quoted 14 lines
target/s390x/helper.c +1 -2
     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));
     }
 }
author
#9 lore raw

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

Quoted 37 lines · Christian Borntraeger
Quoted 34 lines · David Hildenbrand
Quoted 32 lines · Christian Borntraeger
Quoted 17 lines · 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 <[email protected]>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
target/s390x/helper.c +0 -0
index 615fa24ab9..e8548f340a 100644
@@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)

target/s390x/helper.c void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr) · line 111

Quoted 10 lines
target/s390x/helper.c +5 -2
     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?

Quoted 6 lines
Quoted 3 lines

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."

Signature · 3 lines
Thanks,

David / dhildenb
Acked-by
#10 lore raw

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

Quoted 47 lines · David Hildenbrand
Quoted 37 lines · Christian Borntraeger
Quoted 34 lines · David Hildenbrand
Quoted 32 lines · Christian Borntraeger
Quoted 17 lines · 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 <[email protected]>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
target/s390x/helper.c +0 -0
index 615fa24ab9..e8548f340a 100644
@@ -103,16 +103,18 @@ void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)

target/s390x/helper.c void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr) · line 111

Quoted 10 lines
target/s390x/helper.c +5 -2
     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?

Quoted 6 lines
Quoted 3 lines

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!

Quoted 8 lines

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 <[email protected]>
#11 lore raw

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

In reply to David Hildenbrand
Quoted 7 lines · On Mon, 9 Apr 2018 13:44:30 +0200 David Hildenbrand <[email protected]> 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.

Reviewed-by on v1 2/2
#12 lore raw

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

In reply to David Hildenbrand
Quoted 26 lines · 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 <[email protected]>
---
 target/s390x/helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
target/s390x/helper.c +5 -2
index 615fa24ab9..e8548f340a 100644
@@ -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;

target/s390x/helper.c void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr) · line 120

Quoted 10 lines
target/s390x/helper.c +1 -2
     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 <[email protected]>
#13 lore raw
In reply to David Hildenbrand
Quoted 6 lines · On Mon, 9 Apr 2018 13:30:17 +0200 David Hildenbrand <[email protected]> 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.

Quoted 8 lines

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.