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

Manually having to use cpu_synchronize_state() is error prone. I don't think that the performance impact is that huge if we simply synchronize the state on every kvm_arch_handle_exit() call. This makes the code easier to maintain.

We now also call it (although not neded) for
- KVM_EXIT_S390_RESET -> s390_reipl_request()
- KVM_EXIT_DEBUG -> kvm_arch_handle_debug_exit()
- unmanagable/unimplemented intercepts
- ICPT_WAITPSW -> s390_handle_wait() -> cpu gets halted

- ICPT_CPU_STOP -> do_stop_interrupt() -> cpu gets halted
- Scenarios where we inject an operation exception
- handle_sigp() on the source CPU

- handle_stsi()

Manually having to use cpu_synchronize_state() is error prone. I don't think that the performance impact is that huge if we simply synchronize the state on every kvm_arch_handle_exit() call. This makes the code easier to maintain.

We now also call it (although not neded) for
- KVM_EXIT_S390_RESET -> s390_reipl_request()
- KVM_EXIT_DEBUG -> kvm_arch_handle_debug_exit()
- unmanagable/unimplemented intercepts
- ICPT_WAITPSW -> s390_handle_wait() -> cpu gets halted
- ICPT_CPU_STOP -> do_stop_interrupt() -> cpu gets halted
- Scenarios where we inject an operation exception
- handle_sigp() on the source CPU
- 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.

Quoted 15 lines · David Hildenbrand

Manually having to use cpu_synchronize_state() is error prone. I don't think that the performance impact is that huge if we simply synchronize the state on every kvm_arch_handle_exit() call. This makes the code easier to maintain.

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

Quoted 3 lines

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.

We had other issues in the past in other (common code) places. For example see

commit 79ca7a1b898eb97c4192f3c78027a0f20485e7b4

Author:     Christian Borntraeger <[email protected]>
AuthorDate: Tue Mar 7 15:19:08 2017 +0100
Commit:     Paolo Bonzini <[email protected]>
CommitDate: Tue Mar 14 13:26:36 2017 +0100

exec: add cpu_synchronize_state to cpu_memory_rw_debug

so we might consider going even further.....But this will be tricky.

FWIW, I think your patch even fixes a bug:

--- snip ----
static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)

{
    int r = 0;
    uint16_t func_code;

    /*
     * For any diagnose call we support, bits 48-63 of the resulting
     * address specify the function code; the remainder is ignored.
     */
    func_code = decode_basedisp_rs(&cpu->env, ipb, NULL) & DIAG_KVM_CODE_MASK;

---->

	static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
	                                       uint8_t *ar)
	{
	    hwaddr addr = 0;
	    uint8_t reg;
	    
	    reg = ipb >> 28;
	    if (reg > 0) {
	        addr = env->regs[reg];

----> we do the sync_regs after this in the diag handler! So currently we only handle the case with base reg == 0 correctly. So
diag x,y,0x500(0)
works

but things like
lghi 1,0x500
diag x,y,0(1)

not unless I miss something.

[...]

Quoted 4 lines

target/s390x/kvm.c +0 -0

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

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

Quoted 6 lines

target/s390x/kvm.c +2 -0

+    cpu_synchronize_state(CPU(cpu));
+
     switch (run->exit_reason) {
         case KVM_EXIT_S390_SIEIC:
             ret = handle_intercept(cpu);

Does it make sense to do this hunk NOW for 2.12 (cc stable) and queue your full patch for 2.13?

On 03/26/2018 11:20 AM, David Hildenbrand wrote:

FWIW, I think your patch even fixes a bug:

--- snip ----
static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)

{
    int r = 0;
    uint16_t func_code;

    /*
     * For any diagnose call we support, bits 48-63 of the resulting
     * address specify the function code; the remainder is ignored.
     */
    func_code = decode_basedisp_rs(&cpu->env, ipb, NULL) & DIAG_KVM_CODE_MASK;

---->

	static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
	                                       uint8_t *ar)
	{
	    hwaddr addr = 0;
	    uint8_t reg;
	    
	    reg = ipb >> 28;
	    if (reg > 0) {
	        addr = env->regs[reg];

----> we do the sync_regs after this in the diag handler! So currently we only handle the case with base reg == 0 correctly. So
diag x,y,0x500(0)
works

but things like
lghi 1,0x500
diag x,y,0(1)

not unless I miss something.

[...]

Quoted 4 lines

target/s390x/kvm.c +0 -0

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

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

Quoted 6 lines

target/s390x/kvm.c +2 -0

+    cpu_synchronize_state(CPU(cpu));
+
     switch (run->exit_reason) {
         case KVM_EXIT_S390_SIEIC:
             ret = handle_intercept(cpu);

Does it make sense to do this hunk NOW for 2.12 (cc stable) and queue your full patch for 2.13?

Quoted 15 lines · David Hildenbrand

Manually having to use cpu_synchronize_state() is error prone. I don't think that the performance impact is that huge if we simply synchronize the state on every kvm_arch_handle_exit() call. This makes the code easier to maintain.

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

Quoted 3 lines

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.

We had other issues in the past in other (common code) places. For example see

commit 79ca7a1b898eb97c4192f3c78027a0f20485e7b4

Author:     Christian Borntraeger <[email protected]>
AuthorDate: Tue Mar 7 15:19:08 2017 +0100
Commit:     Paolo Bonzini <[email protected]>
CommitDate: Tue Mar 14 13:26:36 2017 +0100

exec: add cpu_synchronize_state to cpu_memory_rw_debug

so we might consider going even further.....But this will be tricky.

FWIW, I think your patch even fixes a bug:

--- snip ----
static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)

{
    int r = 0;
    uint16_t func_code;

    /*
     * For any diagnose call we support, bits 48-63 of the resulting
     * address specify the function code; the remainder is ignored.
     */
    func_code = decode_basedisp_rs(&cpu->env, ipb, NULL) & DIAG_KVM_CODE_MASK;

---->

	static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
	                                       uint8_t *ar)
	{
	    hwaddr addr = 0;
	    uint8_t reg;
	    
	    reg = ipb >> 28;
	    if (reg > 0) {
	        addr = env->regs[reg];

----> we do the sync_regs after this in the diag handler! So currently we only handle the case with base reg == 0 correctly. So
diag x,y,0x500(0)
works

but things like
lghi 1,0x500
diag x,y,0(1)

not unless I miss something.

Quoted 6 lines · Christian Borntraeger

So currently we only handle the case with base reg == 0 correctly. So
diag x,y,0x500(0)
works

Quoted 5 lines

but things like
lghi 1,0x500
diag x,y,0(1)

not unless I miss something.

FWIW: Sounds like a good idea for a new kvm-unit-test... any volunteers?