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 17:45:57 +0100 Message-ID: <4D46E745.7090004@siemens.com> References: <8db93a26b3cbb67e309d05600811dd6a37b34433.1296133797.git.jan.kiszka@siemens.com> <4D468A24.4080800@redhat.com> <4D469ED6.9050805@siemens.com> <4D46B369.3050404@siemens.com> <4D46D7F2.3040502@siemens.com> <20110131163858.GA18827@redhat.com> <4D46E634.30109@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , Marcelo Tosatti , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" To: Gleb Natapov Return-path: Received: from goliath.siemens.de ([192.35.17.28]:15622 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753927Ab1AaQqL (ORCPT ); Mon, 31 Jan 2011 11:46:11 -0500 In-Reply-To: <4D46E634.30109@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >>>>>>> --- >>>>>>> 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 -- 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=60294 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pjwt7-0001hE-M3 for qemu-devel@nongnu.org; Mon, 31 Jan 2011 11:46:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pjwt6-00019v-9d for qemu-devel@nongnu.org; Mon, 31 Jan 2011 11:46:01 -0500 Received: from goliath.siemens.de ([192.35.17.28]:15575) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pjwt5-00019d-UX for qemu-devel@nongnu.org; Mon, 31 Jan 2011 11:46:00 -0500 Message-ID: <4D46E745.7090004@siemens.com> Date: Mon, 31 Jan 2011 17:45:57 +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> <4D46D7F2.3040502@siemens.com> <20110131163858.GA18827@redhat.com> <4D46E634.30109@siemens.com> In-Reply-To: <4D46E634.30109@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: Gleb Natapov Cc: Marcelo Tosatti , Avi Kivity , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" 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 >>>>>>> --- >>>>>>> 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 -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux