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

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

[Alexander Graf]

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.

But x86 does some weird things with an mp_state variable that I have no clue what it's about. Jan, do you know what's going on here?


Alex

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

-Scott

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

[Jan Kiszka]

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

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

[Scott Wood]

On 01/10/2012 03:38 AM, Jan Kiszka wrote:
> 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:
>>>> 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.

We do for normal idling.  This is a bit different, in that we're not
waiting for an interrupt, but for an MMIO that releases the cpu at
boot-time.

-Scott

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

[Jan Kiszka]

On 2012-01-10 18:43, Scott Wood wrote:
> On 01/10/2012 03:38 AM, Jan Kiszka wrote:
>> 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:
>>>>> 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.
> 
> We do for normal idling.  This is a bit different, in that we're not
> waiting for an interrupt, but for an MMIO that releases the cpu at
> boot-time.

Where is the state stored that declares a VCPU to wait for that event?
Where is it set, where removed?

What about implementing MP_STATE on PPC, at least those states that make
sense? Don't you need that anyway for normal HALT<->RUNNABLE transitions?

Jan

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

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

[Alexander Graf]

On 10.01.2012, at 18:52, Jan Kiszka wrote:

> On 2012-01-10 18:43, Scott Wood wrote:
>> On 01/10/2012 03:38 AM, Jan Kiszka wrote:
>>> 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:
>>>>>> 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.
>> 
>> We do for normal idling.  This is a bit different, in that we're not
>> waiting for an interrupt, but for an MMIO that releases the cpu at
>> boot-time.
> 
> Where is the state stored that declares a VCPU to wait for that event?
> Where is it set, where removed?

It's set in the reset callback of the vcpu in QEMU. Basically we're starting all secondary CPUs in "waiting for an MMIO to come" state to not waste CPU cycles. On real hardware, secondaries are running in a tight loop trying to check if they should wake up.

When the primary is good to go, it will then flip a bit in MMIO space (on real hardware this is just RAM) which gives us an event to wake up a secondary.

> What about implementing MP_STATE on PPC, at least those states that make
> sense? Don't you need that anyway for normal HALT<->RUNNABLE transitions?

The in-kernel code isn't upstream yet, so we're free to change things still. But yes, I guess doing it the same way as x86 does make sense here.


Alex

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

[Scott Wood]

On 01/10/2012 11:52 AM, Jan Kiszka wrote:
> On 2012-01-10 18:43, Scott Wood wrote:
>> On 01/10/2012 03:38 AM, Jan Kiszka wrote:
>>> 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:
>>>>>> 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.
>>
>> We do for normal idling.  This is a bit different, in that we're not
>> waiting for an interrupt, but for an MMIO that releases the cpu at
>> boot-time.
> 
> Where is the state stored that declares a VCPU to wait for that event?
> Where is it set, where removed?
> 
> What about implementing MP_STATE on PPC, at least those states that make
> sense? Don't you need that anyway for normal HALT<->RUNNABLE transitions?

On ppc, normal halt/runnable transitions are handled entirely in the
kernel, even without irqchip.

So, the idea is that on secondary VCPU creation, QEMU sets MP_STATE to
KVM_MP_STATE_UNITIALIZED, and KVM will hold the thread idle until the
MMIO is done and QEMU sets MP_STATE to KVM_MP_STATE_RUNNABLE?  It seems
excessive compared to QEMU being able to figure out for itself when it
doesn't want to run a VCPU thread, when the decision is based entirely
on things that are modeled in QEMU (which it will still need to do in
the non-KVM case).

-Scott

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

[Alexander Graf]

On 10.01.2012, at 23:43, Scott Wood wrote:

> On 01/10/2012 11:52 AM, Jan Kiszka wrote:
>> On 2012-01-10 18:43, Scott Wood wrote:
>>> On 01/10/2012 03:38 AM, Jan Kiszka wrote:
>>>> 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:
>>>>>>> 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.
>>> 
>>> We do for normal idling.  This is a bit different, in that we're not
>>> waiting for an interrupt, but for an MMIO that releases the cpu at
>>> boot-time.
>> 
>> Where is the state stored that declares a VCPU to wait for that event?
>> Where is it set, where removed?
>> 
>> What about implementing MP_STATE on PPC, at least those states that make
>> sense? Don't you need that anyway for normal HALT<->RUNNABLE transitions?
> 
> On ppc, normal halt/runnable transitions are handled entirely in the
> kernel, even without irqchip.
> 
> So, the idea is that on secondary VCPU creation, QEMU sets MP_STATE to
> KVM_MP_STATE_UNITIALIZED, and KVM will hold the thread idle until the
> MMIO is done and QEMU sets MP_STATE to KVM_MP_STATE_RUNNABLE?  It seems
> excessive compared to QEMU being able to figure out for itself when it
> doesn't want to run a VCPU thread, when the decision is based entirely
> on things that are modeled in QEMU (which it will still need to do in
> the non-KVM case).

I agree, but the closer we can stick with how x86 models it today the more generic code we have, the better.


Alex