s390x/kvm: call cpu_synchronize_state() on every kvm_arch_handle_exit()

Manually having to use cpu_synchronize_state() is error prone. And as Christian Borntraeger discovered, e.g. handle_diag() is currently missing a cpu_synchronize_state(), as decode_basedisp_s() uses a general purpose register value internally.

So let's do an overall cpu_synchronize_state(), which fixes at least the one mentioned BUG. We will clean up the superfluous cpu_synchronize_state() calls later.

We now also call it (although maybe not neded) for
- KVM_EXIT_S390_RESET -> s390_reipl_request()
- KVM_EXIT_DEBUG -> kvm_arch_handle_debug_exit()
- unmanagable/unimplemented intercepts
- ICPT_CPU_STOP -> do_stop_interrupt() -> cpu gets halted
- Scenarios where we inject an operation exception
- handle_stsi()

I don't think any of these are performance critical. Especially as we have all information directly contained in kvm_run, there are no additional IOCTLs to issue on modern kernels.

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

---
 target/s390x/kvm.c | 2 ++
 1 file changed, 2 insertions(+)

target/s390x/kvm.c +0 -0

index f570896dc1..fb59d92def 100644

Quoted 32 lines · David Hildenbrand

Manually having to use cpu_synchronize_state() is error prone. And as Christian Borntraeger discovered, e.g. handle_diag() is currently missing a cpu_synchronize_state(), as decode_basedisp_s() uses a general purpose register value internally.

So let's do an overall cpu_synchronize_state(), which fixes at least the one mentioned BUG. We will clean up the superfluous cpu_synchronize_state() calls later.

We now also call it (although maybe not neded) for
- KVM_EXIT_S390_RESET -> s390_reipl_request()
- KVM_EXIT_DEBUG -> kvm_arch_handle_debug_exit()
- unmanagable/unimplemented intercepts
- ICPT_CPU_STOP -> do_stop_interrupt() -> cpu gets halted
- Scenarios where we inject an operation exception
- handle_stsi()

I don't think any of these are performance critical. Especially as we have all information directly contained in kvm_run, there are no additional IOCTLs to issue on modern kernels.

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

---
 target/s390x/kvm.c | 2 ++
 1 file changed, 2 insertions(+)

target/s390x/kvm.c +0 -0

index f570896dc1..fb59d92def 100644
@@ -1778,6 +1778,8 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 

target/s390x/kvm.c int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run) · line 1781

Quoted 3 lines

target/s390x/kvm.c +1 -0

     qemu_mutex_lock_iothread();
 
+    cpu_synchronize_state(cs);

Since we're in kvm.c here, maybe rather call kvm_cpu_synchronize_state() directly to avoid the wrapper function?

Thomas

Quoted 40 lines · Thomas Huth

Quoted 31 lines · David Hildenbrand

Manually having to use cpu_synchronize_state() is error prone. And as Christian Borntraeger discovered, e.g. handle_diag() is currently missing a cpu_synchronize_state(), as decode_basedisp_s() uses a general purpose register value internally.

So let's do an overall cpu_synchronize_state(), which fixes at least the one mentioned BUG. We will clean up the superfluous cpu_synchronize_state() calls later.

We now also call it (although maybe not neded) for
- KVM_EXIT_S390_RESET -> s390_reipl_request()
- KVM_EXIT_DEBUG -> kvm_arch_handle_debug_exit()
- unmanagable/unimplemented intercepts
- ICPT_CPU_STOP -> do_stop_interrupt() -> cpu gets halted
- Scenarios where we inject an operation exception
- handle_stsi()

I don't think any of these are performance critical. Especially as we have all information directly contained in kvm_run, there are no additional IOCTLs to issue on modern kernels.

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

---
 target/s390x/kvm.c | 2 ++
 1 file changed, 2 insertions(+)

target/s390x/kvm.c +0 -0

index f570896dc1..fb59d92def 100644

target/s390x/kvm.c int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run) · line 1781

Quoted 4 lines

target/s390x/kvm.c +1 -0

@@ -1778,6 +1778,8 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 
     qemu_mutex_lock_iothread();
 
+    cpu_synchronize_state(cs);  

Since we're in kvm.c here, maybe rather call kvm_cpu_synchronize_state() directly to avoid the wrapper function?

Quoted 2 lines

Thomas

No strong opinion. I can see that kvm_cpu_synchronize_state()
- is not used in target/s390x/kvm.c yet
- is very rarely used in kvm code in general

Manually having to use cpu_synchronize_state() is error prone. And as Christian Borntraeger discovered, e.g. handle_diag() is currently missing a cpu_synchronize_state(), as decode_basedisp_s() uses a general purpose register value internally.

So let's do an overall cpu_synchronize_state(), which fixes at least the one mentioned BUG. We will clean up the superfluous cpu_synchronize_state() calls later.

We now also call it (although maybe not neded) for
- KVM_EXIT_S390_RESET -> s390_reipl_request()
- KVM_EXIT_DEBUG -> kvm_arch_handle_debug_exit()
- unmanagable/unimplemented intercepts
- ICPT_CPU_STOP -> do_stop_interrupt() -> cpu gets halted
- Scenarios where we inject an operation exception
- handle_stsi()

I don't think any of these are performance critical. Especially as we have all information directly contained in kvm_run, there are no additional IOCTLs to issue on modern kernels.

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

---
 target/s390x/kvm.c | 2 ++
 1 file changed, 2 insertions(+)

target/s390x/kvm.c +2 -0

index f570896dc1..fb59d92def 100644
@@ -1778,6 +1778,8 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 
     qemu_mutex_lock_iothread();
 
+    cpu_synchronize_state(cs);
+
     switch (run->exit_reason) {
         case KVM_EXIT_S390_SIEIC:
             ret = handle_intercept(cpu);

Manually having to use cpu_synchronize_state() is error prone. And as Christian Borntraeger discovered, e.g. handle_diag() is currently missing a cpu_synchronize_state(), as decode_basedisp_s() uses a general purpose register value internally.

So let's do an overall cpu_synchronize_state(), which fixes at least the one mentioned BUG. We will clean up the superfluous cpu_synchronize_state() calls later.

We now also call it (although maybe not neded) for
- KVM_EXIT_S390_RESET -> s390_reipl_request()
- KVM_EXIT_DEBUG -> kvm_arch_handle_debug_exit()
- unmanagable/unimplemented intercepts
- ICPT_CPU_STOP -> do_stop_interrupt() -> cpu gets halted
- Scenarios where we inject an operation exception
- handle_stsi()

I don't think any of these are performance critical. Especially as we have all information directly contained in kvm_run, there are no additional IOCTLs to issue on modern kernels.

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

---
 target/s390x/kvm.c | 2 ++
 1 file changed, 2 insertions(+)

target/s390x/kvm.c +0 -0

index f570896dc1..fb59d92def 100644