[Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[David Hildenbrand]

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(-)

-- 
2.13.6

[Qemu-devel] [PATCH v1 for-2.11 1/3] s390x: fix storing CPU status (again)

[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 <david@redhat.com>
---
 target/s390x/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index f78983dd6a..246ba20f0d 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -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);
-- 
2.13.6

[Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)

[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 <david@redhat.com>
---
 target/s390x/diag.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index dbbb9e886f..52bc348808 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -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;
 }
 
-- 
2.13.6

[Qemu-devel] [PATCH v1 for-2.11 3/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)

[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 <david@redhat.com>
---
 target/s390x/misc_helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 4afd90b969..d272851e1c 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -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:
-- 
2.13.6

Re: [Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[Cornelia Huck]

On Thu, 16 Nov 2017 18:05:23 +0100
David Hildenbrand <david@redhat.com> 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(-)
> 

Thanks, queued to s390-fixes.

Re: [Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[David Hildenbrand]

On 16.11.2017 18:23, Cornelia Huck wrote:
> On Thu, 16 Nov 2017 18:05:23 +0100
> David Hildenbrand <david@redhat.com> 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(-)
>>
> 
> 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.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)

[Alex Bennée]

David Hildenbrand <david@redhat.com> writes:

> 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 <david@redhat.com>
> ---
>  target/s390x/diag.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index dbbb9e886f..52bc348808 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -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.


> @@ -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;
>  }
>
> --
> 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)

[David Hildenbrand]

On 16.11.2017 18:05, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  target/s390x/diag.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index dbbb9e886f..52bc348808 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -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?



>From 14440f775e65bb88c1a95705fc9b438f4022f332 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
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.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/diag.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index dbbb9e886f..8396124a5f 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -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;
 }

-- 
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)

[David Hildenbrand]

On 16.11.2017 18:37, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> 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 <david@redhat.com>
>> ---
>>  target/s390x/diag.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index dbbb9e886f..52bc348808 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -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).

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;

    }

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

It is important. Introducing async helpers at a point where sync is
needed sounds strange.

Thanks!

>>
>> --
>> 2.13.6
> 
> 
> --
> Alex Bennée
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)

[Alex Bennée]

David Hildenbrand <david@redhat.com> writes:

> On 16.11.2017 18:37, Alex Bennée wrote:
>>
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> 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 <david@redhat.com>
>>> ---
>>>  target/s390x/diag.c | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>>> index dbbb9e886f..52bc348808 100644
>>> --- a/target/s390x/diag.c
>>> +++ b/target/s390x/diag.c
>>> @@ -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).
>
> 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;
>
>     }

For -smp 1 it's fine, but have you tested --accel thread=single -smp 2?

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

>
> Thanks!
>
>>>
>>> --
>>> 2.13.6
>>
>>
>> --
>> Alex Bennée
>>
>
>
> --
>
> 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)

[Alex Bennée]

David Hildenbrand <david@redhat.com> writes:

> On 16.11.2017 18:37, Alex Bennée wrote:
>>
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> 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 <david@redhat.com>
>>> ---
>>>  target/s390x/diag.c | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>>> index dbbb9e886f..52bc348808 100644
>>> --- a/target/s390x/diag.c
>>> +++ b/target/s390x/diag.c
>>> @@ -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).
>
> 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;
>
>     }

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)

[David Hildenbrand]

On 16.11.2017 19:12, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 16.11.2017 18:37, Alex Bennée wrote:
>>>
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> 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 <david@redhat.com>
>>>> ---
>>>>  target/s390x/diag.c | 4 ----
>>>>  1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>>>> index dbbb9e886f..52bc348808 100644
>>>> --- a/target/s390x/diag.c
>>>> +++ b/target/s390x/diag.c
>>>> @@ -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).
>>
>> 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;
>>
>>     }
> 
> 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)

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

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.11 1/3] s390x: fix storing CPU status (again)

[Thomas Huth]

On 16.11.2017 18:05, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  target/s390x/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index f78983dd6a..246ba20f0d 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -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);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.11 3/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (MTTCG)

[Thomas Huth]

On 16.11.2017 18:05, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  target/s390x/misc_helper.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 4afd90b969..d272851e1c 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -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:
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[Christian Borntraeger]

On 11/16/2017 06:23 PM, Cornelia Huck wrote:
> On Thu, 16 Nov 2017 18:05:23 +0100
> David Hildenbrand <david@redhat.com> 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(-)
>>
> 
> Thanks, queued to s390-fixes.

Can you please give a chance to review this?
18 minutes is not ok, especially in hard freeze.

Re: [Qemu-devel] [PATCH v1 for-2.11 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)

[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?

On 11/16/2017 06:05 PM, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
>  target/s390x/diag.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index dbbb9e886f..52bc348808 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -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?


>      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;
>  }
> 

Re: [Qemu-devel] [PATCH v1 for-2.11 1/3] s390x: fix storing CPU status (again)

[Christian Borntraeger]

On 11/16/2017 06:05 PM, David Hildenbrand wrote:
> 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 <david@redhat.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>  target/s390x/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index f78983dd6a..246ba20f0d 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -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 2/3] s390x/tcg: fix DIAG 308 with > 1 VCPU (single threaded TCG)

[David Hildenbrand]

On 16.11.2017 21:57, Christian Borntraeger wrote:
> 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.

> 
> On 11/16/2017 06:05 PM, David Hildenbrand wrote:
>> 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 <david@redhat.com>
>> ---
>>  target/s390x/diag.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index dbbb9e886f..52bc348808 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -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?

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.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[Cornelia Huck]

On Thu, 16 Nov 2017 21:55:02 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 11/16/2017 06:23 PM, Cornelia Huck wrote:
> > On Thu, 16 Nov 2017 18:05:23 +0100
> > David Hildenbrand <david@redhat.com> 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(-)
> >>  
> > 
> > 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.

Re: [Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[Cornelia Huck]

On Thu, 16 Nov 2017 18:23:35 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 16 Nov 2017 18:05:23 +0100
> David Hildenbrand <david@redhat.com> 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(-)
> >   
> 
> Thanks, queued to s390-fixes.

Patch 2 still needs discussion, I think. I'll dequeue it again and
leave 1 + 3.

Comments?

Re: [Qemu-devel] [PATCH v1 for-2.11 0/3] s390x: fixes for SIGP and DIAG 308

[Christian Borntraeger]

On 11/17/2017 09:10 AM, Cornelia Huck wrote:
> On Thu, 16 Nov 2017 18:23:35 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Thu, 16 Nov 2017 18:05:23 +0100
>> David Hildenbrand <david@redhat.com> 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(-)
>>>   
>>
>> 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