Re: [Qemu-devel] [kvm-devel] [PATCH v2] kvm-ppc: halt secondary cpus when guest reset

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-01-10 00:17, Scott Wood wrote:
> On 01/09/2012 04:39 PM, Alexander Graf wrote:
>>
>> On 09.01.2012, at 22:23, Scott Wood wrote:
>>
>>> On 01/09/2012 01:41 AM, Liu Yu wrote:
>>>> When guest reset, we need to halt secondary cpus until guest kick them.
>>>> This already works for tcg. The patch add the support for kvm.
>>>>
>>>> There are two cases for kvm:
>>>>
>>>> 1. If not use in-kernel mpic. We return env->halted in
>>>> kvm_arch_process_async_events(), in order to tell kvm common code
>>>> whether we request to halt.
>>>>
>>>> 2. If use in-kernel mpic. Even we request to halt,
>>>> the common code cpu_thread_is_idle() refuses to halt,
>>>> because if vcpu halts while use in-kernel mpic, then there's
>>>> no external interrupt handled in qemu to wake up the vcpu thread.
>>>> In this case, we set stopped to pause the sencondaries instead,
>>>> And resume secondaries when primary cpu access ppce500_spin.
>>>>
>>>> Signed-off-by: Liu Yu <yu.liu@freescale.com>
>>>> ---
>>>> v2:
>>>> Add comment and modify commit log
>>>>
>>>> hw/ppce500_spin.c |    1 +
>>>> target-ppc/kvm.c  |   15 ++++++++++++++-
>>>> 2 files changed, 15 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>>> index cccd940..2b52728 100644
>>>> --- a/hw/ppce500_spin.c
>>>> +++ b/hw/ppce500_spin.c
>>>> @@ -112,6 +112,7 @@ static void spin_kick(void *data)
>>>>
>>>>     env->halted = 0;
>>>>     env->exception_index = -1;
>>>> +    env->stopped = 0;
>>>>     qemu_cpu_kick(env);
>>>> }
>>>>
>>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>>> index 429349f..b7c70e2 100644
>>>> --- a/target-ppc/kvm.c
>>>> +++ b/target-ppc/kvm.c
>>>> @@ -504,7 +504,20 @@ void kvm_arch_post_run(CPUState *env, struct kvm_run *run)
>>>>
>>>> int kvm_arch_process_async_events(CPUState *env)
>>>> {
>>>> -    return 0;
>>>> +    if (kvm_irqchip_in_kernel()) {
>>>> +        if (env->halted == 1 && env->exception_index == EXCP_HLT) {
>>>> +            /* We're here because secondary vcpus are requested to halt
>>>> +             * before get kicked. But it's not allowed to halt vcpu when
>>>> +             * kvm_irqchip_in_kernel() is true. Instead, set stopped=1
>>>> +             * so that vcpu thread can be paused until guest kick them */
>>>> +            env->stopped = 1;
>>>> +            return 1;
>>>> +        } else {
>>>> +            return 0;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return env->halted;
>>>> }
>>>
>>> Alex, is there a better way to deal with the IRQ chip issue?
>>
>> To be honest, I'm not sure what the issue really is.
> 
> If irqchip is enabled, env->halted won't result in a CPU being
> considered idle -- since QEMU won't see the interrupt that wakes the
> vcpu, and the idling is handled in the kernel.  In this case we're
> waiting for MMIO rather than an interrupt, and it's the kernel that
> doesn't know what's going on.
> 
> It seems wrong to use env->stopped, though, as a spin-table release
> should not override a user's explicit request to stop a CPU.  It might
> be OK (though a bit ugly) if the only usage of env->stopped is through
> pause_all_vcpus(), and the boot thread is the first one to be kicked
> (though in theory the boot cpu could wake another cpu, and that could
> wake a cpu that comes before it, causing a race with pause_all_vcpus()).
> 
> If it is OK to use env->stopped, is there any reason not to always use
> it (versus just with irqchip)?

Why don't you wait in the kernel with in-kernel irqchip under all
condition (except pausing VCPUs, of course) on PPC? Just like x86 does.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux