s390x: fixes for SIGP and DIAG 308
cover s390x: fixes for SIGP and DIAG 308
First one also applies to KVM SIGP handling. The other two only apply to TCG (allowing to IPL from disk with more than 1 VCPU- I never tested that before).
David Hildenbrand (3):
s390x: fix storing CPU status (again)
s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG) s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)
target/s390x/diag.c | 4 ---- target/s390x/helper.c | 2 +- target/s390x/misc_helper.c | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-)
Signature · 1 line
2.13.6
PATCH 1/3 s390x: fix storing CPU status (again)
Looks like the last fix + cleanup introduced another bug. (for now Linux guests don't seem to care) - we store the crs into ars.
--- target/s390x/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
target/s390x/helper.c +1 -1
@@ -279,7 +279,7 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch) sa->ars[i] =cpu_to_be32 (cpu->env.aregs[i]); }for (i =0 ; i <16 ; ++i) { - sa->ars[i] =cpu_to_be64 (cpu->env.cregs[i]); + sa->crs[i] =cpu_to_be64 (cpu->env.cregs[i]); }cpu_physical_memory_unmap (sa, len,1 , len);
Signature · 1 line
2.13.6
Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.11 1/3] s390x: fix storing CPU status (again)
Quoted 15 lines · David Hildenbrand
Looks like the last fix + cleanup introduced another bug. (for now Linux guests don't seem to care) - we store the crs into ars.
Fixes 947a38bd6f13 ("s390x/kvm: fix and cleanup storing CPU status")Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)target/s390x/helper.c +0
-0
target/s390x/helper.c int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch) · line 285
Quoted 8 lines
target/s390x/helper.c +1
-1@@ -279,7 +279,7 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch) sa->ars[i] =cpu_to_be32 (cpu->env.aregs[i]); }for (i =0 ; i <16 ; ++i) { - sa->ars[i] =cpu_to_be64 (cpu->env.cregs[i]); + sa->crs[i] =cpu_to_be64 (cpu->env.cregs[i]); }cpu_physical_memory_unmap (sa, len,1 , len);
Re: [Qemu-devel] [PATCH v1 for-2.11 1/3] s390x: fix storing CPU status (again)
In reply to David HildenbrandQuoted 5 lines · David Hildenbrand
Looks like the last fix + cleanup introduced another bug. (for now Linux guests don't seem to care) - we store the crs into ars.
Fixes 947a38bd6f13 ("s390x/kvm: fix and cleanup storing CPU status")Signed-off-by David Hildenbrand <[email protected]>
Quoted 18 lines
--- target/s390x/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)target/s390x/helper.c +1
-1@@ -279,7 +279,7 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch) sa->ars[i] =cpu_to_be32 (cpu->env.aregs[i]); }for (i =0 ; i <16 ; ++i) { - sa->ars[i] =cpu_to_be64 (cpu->env.cregs[i]); + sa->crs[i] =cpu_to_be64 (cpu->env.cregs[i]); }cpu_physical_memory_unmap (sa, len,1 , len);
PATCH 2/3 s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
In reply to David HildenbrandCurrently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it. run_on_cpu() will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.
--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)
target/s390x/diag.c +0 -4
@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t; -pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL );
@@ -37,7 +36,6 @@ static int modified_clear_reset(S390CPU *cpu)s390_crypto_reset (); scc->load_normal (CPU (cpu));cpu_synchronize_all_post_reset (); -resume_all_vcpus ();return 0 ; }
@@ -53,7 +51,6 @@ static int load_normal_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t; -pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_reset,RUN_ON_CPU_NULL );
@@ -63,7 +60,6 @@ static int load_normal_reset(S390CPU *cpu) scc->initial_cpu_reset (CPU (cpu)); scc->load_normal (CPU (cpu));cpu_synchronize_all_post_reset (); -resume_all_vcpus ();return 0 ; }
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
Quoted 19 lines · David Hildenbrand
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 32
Quoted 7 lines
target/s390x/diag.c +0
-1@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t; -pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL );
I think you also need to fix the run_on_cpu to be a async_run_on_cpu as you would otherwise hang waiting for run_on_cpu to finish on a single-threaded TCG run (as you are in the only vCPU context).
If it is important that the source vCPU doesn't continue you can schedule it's work afterwards with a async_safe_run_on_cpu which will complete after all other vCPUs have executed their work.
Quoted 25 lines
target/s390x/diag.c +0
-4@@ -37,7 +36,6 @@ static int modified_clear_reset(S390CPU *cpu)s390_crypto_reset (); scc->load_normal (CPU (cpu));cpu_synchronize_all_post_reset (); -resume_all_vcpus ();return 0 ; }@@ -53,7 +51,6 @@ static int load_normal_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t; -pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_reset,RUN_ON_CPU_NULL );@@ -63,7 +60,6 @@ static int load_normal_reset(S390CPU *cpu) scc->initial_cpu_reset (CPU (cpu)); scc->load_normal (CPU (cpu));cpu_synchronize_all_post_reset (); -resume_all_vcpus ();return 0 ; } --
Quoted 1 line
2.13.6
--
Alex Bennée
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
Quoted 25 lines · Alex Bennée
Quoted 22 lines · David Hildenbrand
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t;
Quoted 8 lines
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 32
Quoted 4 lines
target/s390x/diag.c +0
-1-pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL );I think you also need to fix the
run_on_cputo be aasync_run_on_cpuas you would otherwise hang waiting forrun_on_cputo finish on a single-threaded TCG run (as you are in the only vCPU context).
No, it works just fine for single threaded TCG. run_on_cpu() can deal with single threaded TCG just fine. (otherwise e.g. SIGP code also wouldn't work)
In do_run_on_cpu, the following code always directly triggers for single threaded tcg:
if (qemu_cpu_is_self(cpu)) {
func(cpu, data);
return;
}Quoted 5 lines
If it is important that the source vCPU doesn't continue you can schedule it's work afterwards with a
async_safe_run_on_cpuwhich will complete after all other vCPUs have executed their work.
It is important. Introducing async helpers at a point where sync is needed sounds strange.
Thanks!
Quoted 8 lines
Quoted 3 lines
--
2.13.6--
Alex Bennée
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
Quoted 42 lines · David Hildenbrand
Quoted 25 lines · Alex Bennée
Quoted 22 lines · David Hildenbrand
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t;Quoted 8 lines
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 32
Quoted 4 lines
target/s390x/diag.c +0
-1-pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL );I think you also need to fix the
run_on_cputo be aasync_run_on_cpuas you would otherwise hang waiting forrun_on_cputo finish on a single-threaded TCG run (as you are in the only vCPU context).No, it works just fine for single threaded TCG.
run_on_cpu()can deal with single threaded TCG just fine. (otherwise e.g. SIGP code also wouldn't work)In
do_run_on_cpu, the following code always directly triggers for single threaded tcg:
Quoted 7 lines
if (qemu_cpu_is_self(cpu)) {
func(cpu, data);
return;
}For -smp 1 it's fine, but have you tested --accel thread=single -smp 2?
Quoted 9 lines
Quoted 6 lines
If it is important that the source vCPU doesn't continue you can schedule it's work afterwards with a
async_safe_run_on_cpuwhich will complete after all other vCPUs have executed their work.It is important. Introducing async helpers at a point where sync is needed sounds strange.
Well the helper that schedulules the final async helper also needs to exit the run loop at that point, otherwise you are right it would attempt to execute a few more instructions in the block.
Quoted 13 lines
Thanks!
Quoted 8 lines
Quoted 3 lines
--
2.13.6--
Alex Bennée
Quoted 5 lines
--
Thanks,
David / dhildenb
--
Alex Bennée
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
Quoted 47 lines · Alex Bennée
Quoted 44 lines · David Hildenbrand
Quoted 25 lines · Alex Bennée
Quoted 22 lines · David Hildenbrand
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t;Quoted 8 lines
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 32
Quoted 4 lines
target/s390x/diag.c +0
-1-pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL );I think you also need to fix the
run_on_cputo be aasync_run_on_cpuas you would otherwise hang waiting forrun_on_cputo finish on a single-threaded TCG run (as you are in the only vCPU context).No, it works just fine for single threaded TCG.
run_on_cpu()can deal with single threaded TCG just fine. (otherwise e.g. SIGP code also wouldn't work)In
do_run_on_cpu, the following code always directly triggers for single threaded tcg:if (qemu_cpu_is_self(cpu)) {
Quoted 7 lines
Quoted 5 lines
func(cpu, data); return; }For -smp 1 it's fine, but have you tested --accel thread=single -smp 2?
Yes, otherwise I would never have noticed this bug ;)
I use for my current single threaded setup:
"--accel thread=single -smp 4"
The point is: if run_on_cpu() would not be able to cope with this very simple problem, it would basically be useless.
instead of scheduling work, it simply executes all these functions directly for single threaded TCG. For multi threaded TCG it actually schedules work, that's why I notice the missing iolock (see patch following this one)
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
If it is important that the source vCPU doesn't continue you can schedule it's work afterwards with a
async_safe_run_on_cpuwhich will complete after all other vCPUs have executed their work.
Quoted 7 lines
Quoted 2 lines
It is important. Introducing async helpers at a point where sync is needed sounds strange.
Well the helper that schedulules the final async helper also needs to exit the run loop at that point, otherwise you are right it would attempt to execute a few more instructions in the block.
And the nice thing about run_on_cpu here is for single threaded TCG that not a single work has to be scheduled.
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
In reply to David HildenbrandQuoted 42 lines · David Hildenbrand
Quoted 25 lines · Alex Bennée
Quoted 22 lines · David Hildenbrand
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t;Quoted 8 lines
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 32
Quoted 4 lines
target/s390x/diag.c +0
-1-pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL );I think you also need to fix the
run_on_cputo be aasync_run_on_cpuas you would otherwise hang waiting forrun_on_cputo finish on a single-threaded TCG run (as you are in the only vCPU context).No, it works just fine for single threaded TCG.
run_on_cpu()can deal with single threaded TCG just fine. (otherwise e.g. SIGP code also wouldn't work)In
do_run_on_cpu, the following code always directly triggers for single threaded tcg:
Quoted 7 lines
if (qemu_cpu_is_self(cpu)) {
func(cpu, data);
return;
}Sorry ignore what I said, qemu_cpu_is_self tests the underlying thread not the vCPU which they all share.
--
Alex Bennée
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
In reply to David HildenbrandQuoted 19 lines · David Hildenbrand
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 30
Quoted 5 lines
target/s390x/diag.c +0
-1@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t; -pause_all_vcpus ();cpu_synchronize_all_states ();
Hmm, thinking about this whole synchronization thingy, I guess it could happen that a VCPU starts running again after the cpu_synchronize_all_states() but before the reset. So we should do that in one shot. Resulting in something like the following. Comments?
Quoted 1 line
From
14440f775e65bb88c1a95705fc9b438f4022f332Mon Sep 17 00:00:00 2001
From: David Hildenbrand <[email protected]>
Date: Wed, 15 Nov 2017 17:30:02 +0100
Subject: [PATCH v1] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded
TCG)
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it. run_on_cpu() will make sure that the CPUs exits KVM/TCG, where they get stopped. Once stopped, they will no longer run. As we have to proper synchronize before and after the reset, and have to hinder the VCPU from going back into TCG/KVM execution after the sync, let's do the synchronization in the same shot.
Optimize so we don't do one additional round of synchronization on the running CPU.
--- target/s390x/diag.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-)
target/s390x/diag.c +28 -14
@@ -19,51 +19,65 @@#include "exec/exec-all.h" #include "hw/watchdog/wdt_diag288.h" #include "sysemu/cpus.h" +#include "sysemu/hw_accel.h" #include "hw/s390x/ipl.h" #include "hw/s390x/s390-virtio-ccw.h" +static void s390_do_sync_cpu_full_reset (CPUState *cs,run_on_cpu_data arg) +{ +cpu_synchronize_state (cs); +cpu_reset (cs); +cpu_synchronize_post_reset (cs); +} +static int modified_clear_reset (S390CPU *cpu) {S390CPUClass *scc =S390_CPU_GET_CLASS (cpu); +CPUState *cs =CPU (cpu);CPUState *t; -pause_all_vcpus (); -cpu_synchronize_all_states ();CPU_FOREACH (t) { -run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL ); +if (t != cs) { +run_on_cpu (t, s390_do_sync_cpu_full_reset,RUN_ON_CPU_NULL ); + } }s390_cmma_reset ();subsystem_reset ();s390_crypto_reset (); - scc->load_normal (CPU (cpu)); -cpu_synchronize_all_post_reset (); -resume_all_vcpus (); +cpu_synchronize_state (cs); +cpu_reset (cs); + scc->load_normal (cs); +cpu_synchronize_post_reset (cs);return 0 ; } -static inline void s390_do_cpu_reset (CPUState *cs,run_on_cpu_data arg) +static void s390_do_sync_cpu_reset (CPUState *cs,run_on_cpu_data arg) {S390CPUClass *scc =S390_CPU_GET_CLASS (cs); +cpu_synchronize_state (cs); scc->cpu_reset (cs); +cpu_synchronize_post_reset (cs); }static int load_normal_reset (S390CPU *cpu) {S390CPUClass *scc =S390_CPU_GET_CLASS (cpu); +CPUState *cs =CPU (cpu);CPUState *t; -pause_all_vcpus (); -cpu_synchronize_all_states ();CPU_FOREACH (t) { -run_on_cpu (t, s390_do_cpu_reset,RUN_ON_CPU_NULL ); +if (t != cs) { +run_on_cpu (t, s390_do_sync_cpu_reset,RUN_ON_CPU_NULL ); + } }s390_cmma_reset ();subsystem_reset (); - scc->initial_cpu_reset (CPU (cpu)); - scc->load_normal (CPU (cpu)); -cpu_synchronize_all_post_reset (); -resume_all_vcpus (); +cpu_synchronize_state (cs); +/* scc->initital_cpu_reset() is a superset of scc->cpu_reset() */ + scc->initial_cpu_reset (cs); + scc->load_normal (cs); +cpu_synchronize_post_reset (cs);return 0 ; }
Signature · 9 lines
2.13.6 -- Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
In reply to David HildenbrandPlease change the subject. In busy times I tend to ignore tcg patches. This code is clearly kvm and tcg.
So what about "s390x/diag:" as prefix?
Quoted 19 lines · David Hildenbrand
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 30
Quoted 4 lines
target/s390x/diag.c +0
-1@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t; -pause_all_vcpus ();
I did this to prevent a "still running CPU to restart an already stopped one". Are we sure that this can not happen?
Quoted 27 lines
target/s390x/diag.c +0
-3cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_full_reset,RUN_ON_CPU_NULL );@@ -37,7 +36,6 @@ static int modified_clear_reset(S390CPU *cpu)s390_crypto_reset (); scc->load_normal (CPU (cpu));cpu_synchronize_all_post_reset (); -resume_all_vcpus ();return 0 ; }@@ -53,7 +51,6 @@ static int load_normal_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t; -pause_all_vcpus ();cpu_synchronize_all_states ();CPU_FOREACH (t) {run_on_cpu (t, s390_do_cpu_reset,RUN_ON_CPU_NULL );@@ -63,7 +60,6 @@ static int load_normal_reset(S390CPU *cpu) scc->initial_cpu_reset (CPU (cpu)); scc->load_normal (CPU (cpu));cpu_synchronize_all_post_reset (); -resume_all_vcpus ();return 0 ; }
Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)
Quoted 3 lines · Christian Borntraeger
Please change the subject. In busy times I tend to ignore tcg patches. This code is clearly kvm and tcg.
So what about "s390x/diag:" as prefix?
Right, it was a fix for TCG, that's why I added TCG only. But it should have been purely s390x or s390x/diag.
Quoted 24 lines · David Hildenbrand
Quoted 22 lines
Currently, single threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
pause_all_vcpus()/resume_all_vcpus() should not be called from a VCPU.
And there is also no need for it.
run_on_cpu()will make sure that the CPUs exit KVM/TCG, where they get stopped. Once stopped, they will no longer run.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 4 ---- 1 file changed, 4 deletions(-)target/s390x/diag.c +0
-0@@ -27,7 +27,6 @@ static int modified_clear_reset(S390CPU *cpu)S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUState *t;
Quoted 5 lines
target/s390x/diag.c static int modified_clear_reset(S390CPU *cpu) · line 30
Quoted 1 line
target/s390x/diag.c +0
-1-pause_all_vcpus ();I did this to prevent a "still running CPU to restart an already stopped one". Are we sure that this can not happen?
Interesting question. I thought it would not be a problem but the way locking with run_on_cpu() is handled is tricky. Now I am worried about a couple of deadlocks.
pause_all_vcpus() actually gives up the qemu_global_mutex, so anybody waiting for that mutex can continue.
We have a deadlock if two CPUs at the same time would call pause_all_vcpus(). E.g. two CPUs executing at the same time a DIAG 308 (unlikely).
run_on_cpu temporarily gives up the qemu_global_mutex. If two CPUs call a run_on_cpu at the same time against each other, we might also have a deadlock.
Two CPUs simultaneously trying to send a SIGP START/STOP/RESTART cannot run into a deadlock as they are protected by the SIGP mutex with a trylock.
So even with pause_all_vcpus() I think we have a problem if another CPU sends a SIGP to the issuing DIAG CPU and expects the run_on_cpu to trigger. Deadlock, but unlikely I assume?
Let's defer this patch for now, booting with 1 VCPU is not degraded and it looked easier than it is.
Maybe fixing pause_all_vcpus() to work with more than one CPU in single threaded TCG is an (easier) alternative and at least keeps the (tested) state. 2.12 material.
Signature · 3 lines
Thanks, David / dhildenb
PATCH 3/3 s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)
In reply to David HildenbrandCurrently, multi threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
As run_on_cpu() is used, we run into a deadlock after handling the reset. We need the iolock (just like KVM).
--- target/s390x/misc_helper.c | 2 ++ 1 file changed, 2 insertions(+)
target/s390x/misc_helper.c +2 -0
@@ -103,7 +103,9 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)break ;case 0x308 :/* ipl */ +qemu_mutex_lock_iothread ();handle_diag_308 (env, r1, r3); +qemu_mutex_unlock_iothread (); r =0 ;break ;case 0x288 :
Signature · 1 line
2.13.6
Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.11 3/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)
Quoted 17 lines · David Hildenbrand
Currently, multi threaded TCG with > 1 VCPU gets stuck during IPL, when the bios tries to switch to the loaded kernel via DIAG 308.
As
run_on_cpu()is used, we run into a deadlock after handling the reset. We need the iolock (just like KVM).Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/misc_helper.c | 2 ++ 1 file changed, 2 insertions(+)target/s390x/misc_helper.c +0
-0
target/s390x/misc_helper.c void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num) · line 111
Quoted 9 lines
target/s390x/misc_helper.c +2
-0@@ -103,7 +103,9 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)break ;case 0x308 :/* ipl */ +qemu_mutex_lock_iothread ();handle_diag_308 (env, r1, r3); +qemu_mutex_unlock_iothread (); r =0 ;break ;case 0x288 :
Quoted 9 lines · On Thu, 16 Nov 2017 18:05:23 +0100 David Hildenbrand <[email protected]> wrote:
First one also applies to KVM SIGP handling. The other two only apply to TCG (allowing to IPL from disk with more than 1 VCPU- I never tested that before).
David Hildenbrand (3):
s390x: fix storing CPU status (again)
s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG) s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)
Quoted 5 lines
target/s390x/diag.c | 4 ---- target/s390x/helper.c | 2 +- target/s390x/misc_helper.c | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-)
Thanks, queued to s390-fixes.
Quoted 12 lines · Cornelia Huck
Quoted 9 lines · On Thu, 16 Nov 2017 18:05:23 +0100 David Hildenbrand <[email protected]> wrote:
First one also applies to KVM SIGP handling. The other two only apply to TCG (allowing to IPL from disk with more than 1 VCPU- I never tested that before).
David Hildenbrand (3):
s390x: fix storing CPU status (again)
s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG) s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)
Quoted 8 lines
Quoted 5 lines
target/s390x/diag.c | 4 ---- target/s390x/helper.c | 2 +- target/s390x/misc_helper.c | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-)Thanks, queued to s390-fixes.
Think the second one needs a change, not sure if a CPU can directly run after the synchronization and before the reset. Will send a comment.
Signature · 3 lines
Thanks, David / dhildenb
Quoted 12 lines · Cornelia Huck
Quoted 9 lines · On Thu, 16 Nov 2017 18:05:23 +0100 David Hildenbrand <[email protected]> wrote:
First one also applies to KVM SIGP handling. The other two only apply to TCG (allowing to IPL from disk with more than 1 VCPU- I never tested that before).
David Hildenbrand (3):
s390x: fix storing CPU status (again)
s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG) s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)
Quoted 7 lines
Quoted 5 lines
target/s390x/diag.c | 4 ---- target/s390x/helper.c | 2 +- target/s390x/misc_helper.c | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-)Thanks, queued to s390-fixes.
Can you please give a chance to review this?
18 minutes is not ok, especially in hard freeze.
Quoted 19 lines · On Thu, 16 Nov 2017 21:55:02 +0100 Christian Borntraeger…
Quoted 18 lines · Cornelia Huck
Quoted 14 lines · On Thu, 16 Nov 2017 18:05:23 +0100 David Hildenbrand <[email protected]> wrote:
First one also applies to KVM SIGP handling. The other two only apply to TCG (allowing to IPL from disk with more than 1 VCPU- I never tested that before).
David Hildenbrand (3):
s390x: fix storing CPU status (again)
s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG) s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)target/s390x/diag.c | 4 ---- target/s390x/helper.c | 2 +- target/s390x/misc_helper.c | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-)
Quoted 5 lines
Quoted 1 line
Thanks, queued to s390-fixes.
Can you please give a chance to review this?
18 minutes is not ok, especially in hard freeze.
Just queued, not sent. I'm still rebasing.
Quoted 12 lines · On Thu, 16 Nov 2017 18:23:35 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 9 lines · On Thu, 16 Nov 2017 18:05:23 +0100 David Hildenbrand <[email protected]> wrote:
First one also applies to KVM SIGP handling. The other two only apply to TCG (allowing to IPL from disk with more than 1 VCPU- I never tested that before).
David Hildenbrand (3):
s390x: fix storing CPU status (again)
s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG) s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)
Quoted 7 lines
Quoted 5 lines
target/s390x/diag.c | 4 ---- target/s390x/helper.c | 2 +- target/s390x/misc_helper.c | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-)Thanks, queued to s390-fixes.
Patch 2 still needs discussion, I think. I'll dequeue it again and leave 1 + 3.
Comments?
Quoted 21 lines · Cornelia Huck
Quoted 18 lines · On Thu, 16 Nov 2017 18:23:35 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 14 lines · On Thu, 16 Nov 2017 18:05:23 +0100 David Hildenbrand <[email protected]> wrote:
First one also applies to KVM SIGP handling. The other two only apply to TCG (allowing to IPL from disk with more than 1 VCPU- I never tested that before).
David Hildenbrand (3):
s390x: fix storing CPU status (again)
s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG) s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)target/s390x/diag.c | 4 ---- target/s390x/helper.c | 2 +- target/s390x/misc_helper.c | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-)
Quoted 4 lines
Quoted 1 line
Thanks, queued to s390-fixes.
Patch 2 still needs discussion, I think. I'll dequeue it again and leave 1 + 3.
Yes.
Patch1 is certainly good and relevant for 2.11.
Patch2 needs more time as it has potential for adding regression No opinion on Patch3