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

["Alex Bennée" <alex.bennee@linaro.org>]

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