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

[Scott Wood]

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 <address@hidden>
>>> ---
>>> 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)?

-Scott