Patchwork [17/22] kvm: Move irqchip event processing out of inner loop

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 27, 2011, 1:10 p.m.
Message ID <8db93a26b3cbb67e309d05600811dd6a37b34433.1296133797.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/80678/
State New
Headers show

Comments

Jan Kiszka - Jan. 27, 2011, 1:10 p.m.
Align with qemu-kvm and prepare for IO exit fix: There is no need to run
kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
this service processes will first cause an exit from kvm_cpu_exec
anyway. And we will have to reenter the kernel on IO exits
unconditionally, something that the current logic prevents.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
Avi Kivity - Jan. 31, 2011, 10:08 a.m.
On 01/27/2011 03:10 PM, Jan Kiszka wrote:
> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
> this service processes will first cause an exit from kvm_cpu_exec
> anyway. And we will have to reenter the kernel on IO exits
> unconditionally, something that the current logic prevents.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>   kvm-all.c |   11 ++++++-----
>   1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 5bfa8c0..46ecc1c 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
>
>       DPRINTF("kvm_cpu_exec()\n");
>
> +    if (kvm_arch_process_irqchip_events(env)) {
> +        env->exit_request = 0;
> +        env->exception_index = EXCP_HLT;
> +        return 0;
> +    }
> +
>       do {
>   #ifndef CONFIG_IOTHREAD
>           if (env->exit_request) {
> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
>           }

We check for ->exit_request here

>   #endif
>
> -        if (kvm_arch_process_irqchip_events(env)) {
> -            ret = 0;
> -            break;
> -        }
> -

But this checks for ->interrupt_request.  What ensures that we exit when 
->interrupt_request is set?

>           if (env->kvm_vcpu_dirty) {
>               kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
>               env->kvm_vcpu_dirty = 0;
Jan Kiszka - Jan. 31, 2011, 11:36 a.m.
On 2011-01-31 11:08, Avi Kivity wrote:
> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
>> this service processes will first cause an exit from kvm_cpu_exec
>> anyway. And we will have to reenter the kernel on IO exits
>> unconditionally, something that the current logic prevents.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>   kvm-all.c |   11 ++++++-----
>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 5bfa8c0..46ecc1c 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
>>
>>       DPRINTF("kvm_cpu_exec()\n");
>>
>> +    if (kvm_arch_process_irqchip_events(env)) {
>> +        env->exit_request = 0;
>> +        env->exception_index = EXCP_HLT;
>> +        return 0;
>> +    }
>> +
>>       do {
>>   #ifndef CONFIG_IOTHREAD
>>           if (env->exit_request) {
>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
>>           }
> 
> We check for ->exit_request here
> 
>>   #endif
>>
>> -        if (kvm_arch_process_irqchip_events(env)) {
>> -            ret = 0;
>> -            break;
>> -        }
>> -
> 
> But this checks for ->interrupt_request.  What ensures that we exit when 
> ->interrupt_request is set?

Good question, need to check again. But if that turns out to be an
issue, qemu-kvm would be broken as well. I'm just aligning the code here.

Jan
Jan Kiszka - Jan. 31, 2011, 1:04 p.m.
On 2011-01-31 12:36, Jan Kiszka wrote:
> On 2011-01-31 11:08, Avi Kivity wrote:
>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
>>> this service processes will first cause an exit from kvm_cpu_exec
>>> anyway. And we will have to reenter the kernel on IO exits
>>> unconditionally, something that the current logic prevents.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>> ---
>>>   kvm-all.c |   11 ++++++-----
>>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 5bfa8c0..46ecc1c 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
>>>
>>>       DPRINTF("kvm_cpu_exec()\n");
>>>
>>> +    if (kvm_arch_process_irqchip_events(env)) {
>>> +        env->exit_request = 0;
>>> +        env->exception_index = EXCP_HLT;
>>> +        return 0;
>>> +    }
>>> +
>>>       do {
>>>   #ifndef CONFIG_IOTHREAD
>>>           if (env->exit_request) {
>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
>>>           }
>>
>> We check for ->exit_request here
>>
>>>   #endif
>>>
>>> -        if (kvm_arch_process_irqchip_events(env)) {
>>> -            ret = 0;
>>> -            break;
>>> -        }
>>> -
>>
>> But this checks for ->interrupt_request.  What ensures that we exit when 
>> ->interrupt_request is set?
> 
> Good question, need to check again. But if that turns out to be an
> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
> 

The only thing we miss by moving process_irqchip_events is a self-INIT
of an AP - if such thing exists in real life. In that case, the AP would
cause a reset of itself, followed by a transition to HALT state.

A self-SIPI has no effect as A) a CPU can't send a SIPI from
wait-on-SIPI state and B) SIPIs are ignored in any other state.

Will post a version that additionally checks for pending INIT as well
and injects a self-ipi then.

Jan
Jan Kiszka - Jan. 31, 2011, 3:40 p.m.
On 2011-01-31 14:04, Jan Kiszka wrote:
> On 2011-01-31 12:36, Jan Kiszka wrote:
>> On 2011-01-31 11:08, Avi Kivity wrote:
>>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
>>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
>>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
>>>> this service processes will first cause an exit from kvm_cpu_exec
>>>> anyway. And we will have to reenter the kernel on IO exits
>>>> unconditionally, something that the current logic prevents.
>>>>
>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>> ---
>>>>   kvm-all.c |   11 ++++++-----
>>>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>> index 5bfa8c0..46ecc1c 100644
>>>> --- a/kvm-all.c
>>>> +++ b/kvm-all.c
>>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
>>>>
>>>>       DPRINTF("kvm_cpu_exec()\n");
>>>>
>>>> +    if (kvm_arch_process_irqchip_events(env)) {
>>>> +        env->exit_request = 0;
>>>> +        env->exception_index = EXCP_HLT;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>>       do {
>>>>   #ifndef CONFIG_IOTHREAD
>>>>           if (env->exit_request) {
>>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
>>>>           }
>>>
>>> We check for ->exit_request here
>>>
>>>>   #endif
>>>>
>>>> -        if (kvm_arch_process_irqchip_events(env)) {
>>>> -            ret = 0;
>>>> -            break;
>>>> -        }
>>>> -
>>>
>>> But this checks for ->interrupt_request.  What ensures that we exit when 
>>> ->interrupt_request is set?
>>
>> Good question, need to check again. But if that turns out to be an
>> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
>>
> 
> The only thing we miss by moving process_irqchip_events is a self-INIT
> of an AP - if such thing exists in real life. In that case, the AP would
> cause a reset of itself, followed by a transition to HALT state.

I checked again with the Intel spec, and a self-INIT is invalid (at
least when specified via shorthand). So I'm under the impression now
that we can safely ignore this case and leave the patch as is.

Any different views?

Jan
Gleb Natapov - Jan. 31, 2011, 4:38 p.m.
On Mon, Jan 31, 2011 at 04:40:34PM +0100, Jan Kiszka wrote:
> On 2011-01-31 14:04, Jan Kiszka wrote:
> > On 2011-01-31 12:36, Jan Kiszka wrote:
> >> On 2011-01-31 11:08, Avi Kivity wrote:
> >>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
> >>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
> >>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
> >>>> this service processes will first cause an exit from kvm_cpu_exec
> >>>> anyway. And we will have to reenter the kernel on IO exits
> >>>> unconditionally, something that the current logic prevents.
> >>>>
> >>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>>> ---
> >>>>   kvm-all.c |   11 ++++++-----
> >>>>   1 files changed, 6 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/kvm-all.c b/kvm-all.c
> >>>> index 5bfa8c0..46ecc1c 100644
> >>>> --- a/kvm-all.c
> >>>> +++ b/kvm-all.c
> >>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
> >>>>
> >>>>       DPRINTF("kvm_cpu_exec()\n");
> >>>>
> >>>> +    if (kvm_arch_process_irqchip_events(env)) {
> >>>> +        env->exit_request = 0;
> >>>> +        env->exception_index = EXCP_HLT;
> >>>> +        return 0;
> >>>> +    }
> >>>> +
> >>>>       do {
> >>>>   #ifndef CONFIG_IOTHREAD
> >>>>           if (env->exit_request) {
> >>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
> >>>>           }
> >>>
> >>> We check for ->exit_request here
> >>>
> >>>>   #endif
> >>>>
> >>>> -        if (kvm_arch_process_irqchip_events(env)) {
> >>>> -            ret = 0;
> >>>> -            break;
> >>>> -        }
> >>>> -
> >>>
> >>> But this checks for ->interrupt_request.  What ensures that we exit when 
> >>> ->interrupt_request is set?
> >>
> >> Good question, need to check again. But if that turns out to be an
> >> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
> >>
> > 
> > The only thing we miss by moving process_irqchip_events is a self-INIT
> > of an AP - if such thing exists in real life. In that case, the AP would
> > cause a reset of itself, followed by a transition to HALT state.
> 
> I checked again with the Intel spec, and a self-INIT is invalid (at
> least when specified via shorthand). So I'm under the impression now
> that we can safely ignore this case and leave the patch as is.
> 
> Any different views?
> 
IIRC if you don't use shorthand you can send INIT to self.

--
			Gleb.
Jan Kiszka - Jan. 31, 2011, 4:41 p.m.
On 2011-01-31 17:38, Gleb Natapov wrote:
> On Mon, Jan 31, 2011 at 04:40:34PM +0100, Jan Kiszka wrote:
>> On 2011-01-31 14:04, Jan Kiszka wrote:
>>> On 2011-01-31 12:36, Jan Kiszka wrote:
>>>> On 2011-01-31 11:08, Avi Kivity wrote:
>>>>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
>>>>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
>>>>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
>>>>>> this service processes will first cause an exit from kvm_cpu_exec
>>>>>> anyway. And we will have to reenter the kernel on IO exits
>>>>>> unconditionally, something that the current logic prevents.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>   kvm-all.c |   11 ++++++-----
>>>>>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>>> index 5bfa8c0..46ecc1c 100644
>>>>>> --- a/kvm-all.c
>>>>>> +++ b/kvm-all.c
>>>>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
>>>>>>
>>>>>>       DPRINTF("kvm_cpu_exec()\n");
>>>>>>
>>>>>> +    if (kvm_arch_process_irqchip_events(env)) {
>>>>>> +        env->exit_request = 0;
>>>>>> +        env->exception_index = EXCP_HLT;
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +
>>>>>>       do {
>>>>>>   #ifndef CONFIG_IOTHREAD
>>>>>>           if (env->exit_request) {
>>>>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
>>>>>>           }
>>>>>
>>>>> We check for ->exit_request here
>>>>>
>>>>>>   #endif
>>>>>>
>>>>>> -        if (kvm_arch_process_irqchip_events(env)) {
>>>>>> -            ret = 0;
>>>>>> -            break;
>>>>>> -        }
>>>>>> -
>>>>>
>>>>> But this checks for ->interrupt_request.  What ensures that we exit when 
>>>>> ->interrupt_request is set?
>>>>
>>>> Good question, need to check again. But if that turns out to be an
>>>> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
>>>>
>>>
>>> The only thing we miss by moving process_irqchip_events is a self-INIT
>>> of an AP - if such thing exists in real life. In that case, the AP would
>>> cause a reset of itself, followed by a transition to HALT state.
>>
>> I checked again with the Intel spec, and a self-INIT is invalid (at
>> least when specified via shorthand). So I'm under the impression now
>> that we can safely ignore this case and leave the patch as is.
>>
>> Any different views?
>>
> IIRC if you don't use shorthand you can send INIT to self.

We didn't care so far (in qemu-kvm), do you think we should?

Jan
Jan Kiszka - Jan. 31, 2011, 4:45 p.m.
On 2011-01-31 17:41, Jan Kiszka wrote:
> On 2011-01-31 17:38, Gleb Natapov wrote:
>> On Mon, Jan 31, 2011 at 04:40:34PM +0100, Jan Kiszka wrote:
>>> On 2011-01-31 14:04, Jan Kiszka wrote:
>>>> On 2011-01-31 12:36, Jan Kiszka wrote:
>>>>> On 2011-01-31 11:08, Avi Kivity wrote:
>>>>>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
>>>>>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
>>>>>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
>>>>>>> this service processes will first cause an exit from kvm_cpu_exec
>>>>>>> anyway. And we will have to reenter the kernel on IO exits
>>>>>>> unconditionally, something that the current logic prevents.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>>>   kvm-all.c |   11 ++++++-----
>>>>>>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>>>> index 5bfa8c0..46ecc1c 100644
>>>>>>> --- a/kvm-all.c
>>>>>>> +++ b/kvm-all.c
>>>>>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
>>>>>>>
>>>>>>>       DPRINTF("kvm_cpu_exec()\n");
>>>>>>>
>>>>>>> +    if (kvm_arch_process_irqchip_events(env)) {
>>>>>>> +        env->exit_request = 0;
>>>>>>> +        env->exception_index = EXCP_HLT;
>>>>>>> +        return 0;
>>>>>>> +    }
>>>>>>> +
>>>>>>>       do {
>>>>>>>   #ifndef CONFIG_IOTHREAD
>>>>>>>           if (env->exit_request) {
>>>>>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
>>>>>>>           }
>>>>>>
>>>>>> We check for ->exit_request here
>>>>>>
>>>>>>>   #endif
>>>>>>>
>>>>>>> -        if (kvm_arch_process_irqchip_events(env)) {
>>>>>>> -            ret = 0;
>>>>>>> -            break;
>>>>>>> -        }
>>>>>>> -
>>>>>>
>>>>>> But this checks for ->interrupt_request.  What ensures that we exit when 
>>>>>> ->interrupt_request is set?
>>>>>
>>>>> Good question, need to check again. But if that turns out to be an
>>>>> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
>>>>>
>>>>
>>>> The only thing we miss by moving process_irqchip_events is a self-INIT
>>>> of an AP - if such thing exists in real life. In that case, the AP would
>>>> cause a reset of itself, followed by a transition to HALT state.
>>>
>>> I checked again with the Intel spec, and a self-INIT is invalid (at
>>> least when specified via shorthand). So I'm under the impression now
>>> that we can safely ignore this case and leave the patch as is.
>>>
>>> Any different views?
>>>
>> IIRC if you don't use shorthand you can send INIT to self.
> 
> We didn't care so far (in qemu-kvm), do you think we should?

...and the kernel model should have barked "INIT on a runnable vcpu" in
such cases (BTW, that's user triggerable and should likely be rate limited).

Jan
Gleb Natapov - Jan. 31, 2011, 4:50 p.m.
On Mon, Jan 31, 2011 at 05:41:24PM +0100, Jan Kiszka wrote:
> On 2011-01-31 17:38, Gleb Natapov wrote:
> > On Mon, Jan 31, 2011 at 04:40:34PM +0100, Jan Kiszka wrote:
> >> On 2011-01-31 14:04, Jan Kiszka wrote:
> >>> On 2011-01-31 12:36, Jan Kiszka wrote:
> >>>> On 2011-01-31 11:08, Avi Kivity wrote:
> >>>>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
> >>>>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
> >>>>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
> >>>>>> this service processes will first cause an exit from kvm_cpu_exec
> >>>>>> anyway. And we will have to reenter the kernel on IO exits
> >>>>>> unconditionally, something that the current logic prevents.
> >>>>>>
> >>>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>>>>> ---
> >>>>>>   kvm-all.c |   11 ++++++-----
> >>>>>>   1 files changed, 6 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/kvm-all.c b/kvm-all.c
> >>>>>> index 5bfa8c0..46ecc1c 100644
> >>>>>> --- a/kvm-all.c
> >>>>>> +++ b/kvm-all.c
> >>>>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
> >>>>>>
> >>>>>>       DPRINTF("kvm_cpu_exec()\n");
> >>>>>>
> >>>>>> +    if (kvm_arch_process_irqchip_events(env)) {
> >>>>>> +        env->exit_request = 0;
> >>>>>> +        env->exception_index = EXCP_HLT;
> >>>>>> +        return 0;
> >>>>>> +    }
> >>>>>> +
> >>>>>>       do {
> >>>>>>   #ifndef CONFIG_IOTHREAD
> >>>>>>           if (env->exit_request) {
> >>>>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
> >>>>>>           }
> >>>>>
> >>>>> We check for ->exit_request here
> >>>>>
> >>>>>>   #endif
> >>>>>>
> >>>>>> -        if (kvm_arch_process_irqchip_events(env)) {
> >>>>>> -            ret = 0;
> >>>>>> -            break;
> >>>>>> -        }
> >>>>>> -
> >>>>>
> >>>>> But this checks for ->interrupt_request.  What ensures that we exit when 
> >>>>> ->interrupt_request is set?
> >>>>
> >>>> Good question, need to check again. But if that turns out to be an
> >>>> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
> >>>>
> >>>
> >>> The only thing we miss by moving process_irqchip_events is a self-INIT
> >>> of an AP - if such thing exists in real life. In that case, the AP would
> >>> cause a reset of itself, followed by a transition to HALT state.
> >>
> >> I checked again with the Intel spec, and a self-INIT is invalid (at
> >> least when specified via shorthand). So I'm under the impression now
> >> that we can safely ignore this case and leave the patch as is.
> >>
> >> Any different views?
> >>
> > IIRC if you don't use shorthand you can send INIT to self.
> 
> We didn't care so far (in qemu-kvm), do you think we should?
> 
Doesn't kernel lapic emulation support this?

--
			Gleb.
Jan Kiszka - Jan. 31, 2011, 4:52 p.m.
On 2011-01-31 17:50, Gleb Natapov wrote:
> On Mon, Jan 31, 2011 at 05:41:24PM +0100, Jan Kiszka wrote:
>> On 2011-01-31 17:38, Gleb Natapov wrote:
>>> On Mon, Jan 31, 2011 at 04:40:34PM +0100, Jan Kiszka wrote:
>>>> On 2011-01-31 14:04, Jan Kiszka wrote:
>>>>> On 2011-01-31 12:36, Jan Kiszka wrote:
>>>>>> On 2011-01-31 11:08, Avi Kivity wrote:
>>>>>>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
>>>>>>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
>>>>>>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
>>>>>>>> this service processes will first cause an exit from kvm_cpu_exec
>>>>>>>> anyway. And we will have to reenter the kernel on IO exits
>>>>>>>> unconditionally, something that the current logic prevents.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>>>>>> ---
>>>>>>>>   kvm-all.c |   11 ++++++-----
>>>>>>>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>>>>> index 5bfa8c0..46ecc1c 100644
>>>>>>>> --- a/kvm-all.c
>>>>>>>> +++ b/kvm-all.c
>>>>>>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
>>>>>>>>
>>>>>>>>       DPRINTF("kvm_cpu_exec()\n");
>>>>>>>>
>>>>>>>> +    if (kvm_arch_process_irqchip_events(env)) {
>>>>>>>> +        env->exit_request = 0;
>>>>>>>> +        env->exception_index = EXCP_HLT;
>>>>>>>> +        return 0;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>>       do {
>>>>>>>>   #ifndef CONFIG_IOTHREAD
>>>>>>>>           if (env->exit_request) {
>>>>>>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
>>>>>>>>           }
>>>>>>>
>>>>>>> We check for ->exit_request here
>>>>>>>
>>>>>>>>   #endif
>>>>>>>>
>>>>>>>> -        if (kvm_arch_process_irqchip_events(env)) {
>>>>>>>> -            ret = 0;
>>>>>>>> -            break;
>>>>>>>> -        }
>>>>>>>> -
>>>>>>>
>>>>>>> But this checks for ->interrupt_request.  What ensures that we exit when 
>>>>>>> ->interrupt_request is set?
>>>>>>
>>>>>> Good question, need to check again. But if that turns out to be an
>>>>>> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
>>>>>>
>>>>>
>>>>> The only thing we miss by moving process_irqchip_events is a self-INIT
>>>>> of an AP - if such thing exists in real life. In that case, the AP would
>>>>> cause a reset of itself, followed by a transition to HALT state.
>>>>
>>>> I checked again with the Intel spec, and a self-INIT is invalid (at
>>>> least when specified via shorthand). So I'm under the impression now
>>>> that we can safely ignore this case and leave the patch as is.
>>>>
>>>> Any different views?
>>>>
>>> IIRC if you don't use shorthand you can send INIT to self.
>>
>> We didn't care so far (in qemu-kvm), do you think we should?
>>
> Doesn't kernel lapic emulation support this?

See the my other mail: It supports it, but it apparently doesn't expects
this to happen.

Jan
Gleb Natapov - Jan. 31, 2011, 4:56 p.m.
On Mon, Jan 31, 2011 at 05:52:13PM +0100, Jan Kiszka wrote:
> On 2011-01-31 17:50, Gleb Natapov wrote:
> > On Mon, Jan 31, 2011 at 05:41:24PM +0100, Jan Kiszka wrote:
> >> On 2011-01-31 17:38, Gleb Natapov wrote:
> >>> On Mon, Jan 31, 2011 at 04:40:34PM +0100, Jan Kiszka wrote:
> >>>> On 2011-01-31 14:04, Jan Kiszka wrote:
> >>>>> On 2011-01-31 12:36, Jan Kiszka wrote:
> >>>>>> On 2011-01-31 11:08, Avi Kivity wrote:
> >>>>>>> On 01/27/2011 03:10 PM, Jan Kiszka wrote:
> >>>>>>>> Align with qemu-kvm and prepare for IO exit fix: There is no need to run
> >>>>>>>> kvm_arch_process_irqchip_events in the inner VCPU loop. Any state change
> >>>>>>>> this service processes will first cause an exit from kvm_cpu_exec
> >>>>>>>> anyway. And we will have to reenter the kernel on IO exits
> >>>>>>>> unconditionally, something that the current logic prevents.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>>>>>>> ---
> >>>>>>>>   kvm-all.c |   11 ++++++-----
> >>>>>>>>   1 files changed, 6 insertions(+), 5 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/kvm-all.c b/kvm-all.c
> >>>>>>>> index 5bfa8c0..46ecc1c 100644
> >>>>>>>> --- a/kvm-all.c
> >>>>>>>> +++ b/kvm-all.c
> >>>>>>>> @@ -892,6 +892,12 @@ int kvm_cpu_exec(CPUState *env)
> >>>>>>>>
> >>>>>>>>       DPRINTF("kvm_cpu_exec()\n");
> >>>>>>>>
> >>>>>>>> +    if (kvm_arch_process_irqchip_events(env)) {
> >>>>>>>> +        env->exit_request = 0;
> >>>>>>>> +        env->exception_index = EXCP_HLT;
> >>>>>>>> +        return 0;
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>>       do {
> >>>>>>>>   #ifndef CONFIG_IOTHREAD
> >>>>>>>>           if (env->exit_request) {
> >>>>>>>> @@ -901,11 +907,6 @@ int kvm_cpu_exec(CPUState *env)
> >>>>>>>>           }
> >>>>>>>
> >>>>>>> We check for ->exit_request here
> >>>>>>>
> >>>>>>>>   #endif
> >>>>>>>>
> >>>>>>>> -        if (kvm_arch_process_irqchip_events(env)) {
> >>>>>>>> -            ret = 0;
> >>>>>>>> -            break;
> >>>>>>>> -        }
> >>>>>>>> -
> >>>>>>>
> >>>>>>> But this checks for ->interrupt_request.  What ensures that we exit when 
> >>>>>>> ->interrupt_request is set?
> >>>>>>
> >>>>>> Good question, need to check again. But if that turns out to be an
> >>>>>> issue, qemu-kvm would be broken as well. I'm just aligning the code here.
> >>>>>>
> >>>>>
> >>>>> The only thing we miss by moving process_irqchip_events is a self-INIT
> >>>>> of an AP - if such thing exists in real life. In that case, the AP would
> >>>>> cause a reset of itself, followed by a transition to HALT state.
> >>>>
> >>>> I checked again with the Intel spec, and a self-INIT is invalid (at
> >>>> least when specified via shorthand). So I'm under the impression now
> >>>> that we can safely ignore this case and leave the patch as is.
> >>>>
> >>>> Any different views?
> >>>>
> >>> IIRC if you don't use shorthand you can send INIT to self.
> >>
> >> We didn't care so far (in qemu-kvm), do you think we should?
> >>
> > Doesn't kernel lapic emulation support this?
> 
> See the my other mail: It supports it, but it apparently doesn't expects
> this to happen.
> 
I saw it, but I do not understand why do we print this message. May be
it was used for debugging in early stages of KVM development.

--
			Gleb.

Patch

diff --git a/kvm-all.c b/kvm-all.c
index 5bfa8c0..46ecc1c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -892,6 +892,12 @@  int kvm_cpu_exec(CPUState *env)
 
     DPRINTF("kvm_cpu_exec()\n");
 
+    if (kvm_arch_process_irqchip_events(env)) {
+        env->exit_request = 0;
+        env->exception_index = EXCP_HLT;
+        return 0;
+    }
+
     do {
 #ifndef CONFIG_IOTHREAD
         if (env->exit_request) {
@@ -901,11 +907,6 @@  int kvm_cpu_exec(CPUState *env)
         }
 #endif
 
-        if (kvm_arch_process_irqchip_events(env)) {
-            ret = 0;
-            break;
-        }
-
         if (env->kvm_vcpu_dirty) {
             kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
             env->kvm_vcpu_dirty = 0;