From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 17/22] kvm: Move irqchip event processing out of inner loop Date: Mon, 31 Jan 2011 16:40:34 +0100 Message-ID: <4D46D7F2.3040502@siemens.com> References: <8db93a26b3cbb67e309d05600811dd6a37b34433.1296133797.git.jan.kiszka@siemens.com> <4D468A24.4080800@redhat.com> <4D469ED6.9050805@siemens.com> <4D46B369.3050404@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" To: Avi Kivity Return-path: Received: from thoth.sbs.de ([192.35.17.2]:18322 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756060Ab1AaPkv (ORCPT ); Mon, 31 Jan 2011 10:40:51 -0500 In-Reply-To: <4D46B369.3050404@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >>>> --- >>>> 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 -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33300 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pjvrr-0001Jv-QT for qemu-devel@nongnu.org; Mon, 31 Jan 2011 10:40:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pjvrq-0006ko-JV for qemu-devel@nongnu.org; Mon, 31 Jan 2011 10:40:39 -0500 Received: from thoth.sbs.de ([192.35.17.2]:18245) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pjvrq-0006ie-7B for qemu-devel@nongnu.org; Mon, 31 Jan 2011 10:40:38 -0500 Message-ID: <4D46D7F2.3040502@siemens.com> Date: Mon, 31 Jan 2011 16:40:34 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <8db93a26b3cbb67e309d05600811dd6a37b34433.1296133797.git.jan.kiszka@siemens.com> <4D468A24.4080800@redhat.com> <4D469ED6.9050805@siemens.com> <4D46B369.3050404@siemens.com> In-Reply-To: <4D46B369.3050404@siemens.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 17/22] kvm: Move irqchip event processing out of inner loop List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: Marcelo Tosatti , "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" 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 >>>> --- >>>> 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 -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux