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