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

[David Hildenbrand <david@redhat.com>]

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