| Message ID | 8db93a26b3cbb67e309d05600811dd6a37b34433.1296133797.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
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;
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
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
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
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.
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
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
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.
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
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.
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;
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(-)