Message ID | 20180406093552.13016-1-david@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v1,for-2.12] s390x/kvm: call cpu_synchronize_state() on every kvm_arch_handle_exit() | expand |
On 06.04.2018 11:35, David Hildenbrand wrote: > 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 <david@redhat.com> > --- > target/s390x/kvm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index f570896dc1..fb59d92def 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -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? Thomas
On 06.04.2018 11:40, Thomas Huth wrote: > On 06.04.2018 11:35, David Hildenbrand wrote: >> 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 <david@redhat.com> >> --- >> target/s390x/kvm.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index f570896dc1..fb59d92def 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -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? > > 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
On Fri, 6 Apr 2018 11:46:22 +0200 David Hildenbrand <david@redhat.com> wrote: > On 06.04.2018 11:40, Thomas Huth wrote: > > On 06.04.2018 11:35, David Hildenbrand wrote: > >> 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 <david@redhat.com> > >> --- > >> target/s390x/kvm.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > >> index f570896dc1..fb59d92def 100644 > >> --- a/target/s390x/kvm.c > >> +++ b/target/s390x/kvm.c > >> @@ -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? > > > > 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 > Let's just go with this one for 2.12? If we want to switch to the kvm_* variant, we can still do it for 2.13.
On 04/06/2018 11:35 AM, David Hildenbrand wrote: > 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 <david@redhat.com> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> ok for 2.12. > --- > target/s390x/kvm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index f570896dc1..fb59d92def 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -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); >
On Fri, 6 Apr 2018 11:35:52 +0200 David Hildenbrand <david@redhat.com> wrote: > 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 <david@redhat.com> > --- > target/s390x/kvm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index f570896dc1..fb59d92def 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -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); Thanks, queued to s390-fixes.
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index f570896dc1..fb59d92def 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -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 <david@redhat.com> --- target/s390x/kvm.c | 2 ++ 1 file changed, 2 insertions(+)