From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] cpu: smp_wmb before lauching cpus. Date: Fri, 06 Jul 2012 10:14:54 +0200 Message-ID: <4FF69E7E.7060203@siemens.com> References: <1341454689-8129-1-git-send-email-qemulist@gmail.com> <4FF53856.6030901@web.de> <4FF56988.8060207@siemens.com> <4FF58156.9050601@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: "qemu-devel@nongnu.org" , Anthony Liguori , "kvm@vger.kernel.org" To: liu ping fan Return-path: Received: from david.siemens.de ([192.35.17.14]:24561 "EHLO david.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751852Ab2GFIPE (ORCPT ); Fri, 6 Jul 2012 04:15:04 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 2012-07-06 09:46, liu ping fan wrote: > On Thu, Jul 5, 2012 at 7:58 PM, Jan Kiszka wrote: >> On 2012-07-05 13:02, liu ping fan wrote: >>> On Thu, Jul 5, 2012 at 6:16 PM, Jan Kiszka wrote: >>>> On 2012-07-05 12:10, liu ping fan wrote: >>>>> On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka wrote: >>>>>> On 2012-07-05 04:18, Liu Ping Fan wrote: >>>>>>> Vcpu state must be set completely before receiving INIT-IPI,SIPI >>>>>>> >>>>>>> Signed-off-by: Liu Ping Fan >>>>>>> --- >>>>>>> kvm.h | 1 + >>>>>>> 1 files changed, 1 insertions(+), 0 deletions(-) >>>>>>> >>>>>>> diff --git a/kvm.h b/kvm.h >>>>>>> index 9c7b0ea..5b3c228 100644 >>>>>>> --- a/kvm.h >>>>>>> +++ b/kvm.h >>>>>>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env) >>>>>>> { >>>>>>> if (kvm_enabled()) { >>>>>>> kvm_cpu_synchronize_post_init(env); >>>>>>> + smp_wmb(); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> >>>>>> >>>>>> In theory, there should be no vcpu kick-off after this without some >>>>>> locking operations involved that imply barriers. Did you see real >>>>> >>>>> Yeah, but what if it is non-x86? >>>> >>>> The locking I'm referring to is arch independent. >>>> >>>>>> inconsistencies without this explicit one? >>>> >>>> Again: Did you see real issues or is this based on static analysis? >>>> >>> Just on static analysis >> >> Then please describe - also for the changelog - at least one case in >> details where this is needed. >> > I dived into code. And yes, as you said, ACPI eject does involve some > locking operation. So the only thing left is for starting up. There > seems no potential lock operation from cpu_synchronize_all_post_init() > to resume_all_vcpus(). There is no difference in this scenario as well: the vcpu is waiting on a condvar for the initial kick-off and will synchronize with the main thread via the global mutex. When in doubt, try using a debugger. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59147) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sn3gz-0005kP-5I for qemu-devel@nongnu.org; Fri, 06 Jul 2012 04:15:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sn3gr-0004F6-Ck for qemu-devel@nongnu.org; Fri, 06 Jul 2012 04:15:08 -0400 Received: from david.siemens.de ([192.35.17.14]:33163) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sn3gr-0004E0-3I for qemu-devel@nongnu.org; Fri, 06 Jul 2012 04:15:01 -0400 Message-ID: <4FF69E7E.7060203@siemens.com> Date: Fri, 06 Jul 2012 10:14:54 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1341454689-8129-1-git-send-email-qemulist@gmail.com> <4FF53856.6030901@web.de> <4FF56988.8060207@siemens.com> <4FF58156.9050601@siemens.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] cpu: smp_wmb before lauching cpus. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: "qemu-devel@nongnu.org" , Anthony Liguori , "kvm@vger.kernel.org" On 2012-07-06 09:46, liu ping fan wrote: > On Thu, Jul 5, 2012 at 7:58 PM, Jan Kiszka wrote: >> On 2012-07-05 13:02, liu ping fan wrote: >>> On Thu, Jul 5, 2012 at 6:16 PM, Jan Kiszka wrote: >>>> On 2012-07-05 12:10, liu ping fan wrote: >>>>> On Thu, Jul 5, 2012 at 2:46 PM, Jan Kiszka wrote: >>>>>> On 2012-07-05 04:18, Liu Ping Fan wrote: >>>>>>> Vcpu state must be set completely before receiving INIT-IPI,SIPI >>>>>>> >>>>>>> Signed-off-by: Liu Ping Fan >>>>>>> --- >>>>>>> kvm.h | 1 + >>>>>>> 1 files changed, 1 insertions(+), 0 deletions(-) >>>>>>> >>>>>>> diff --git a/kvm.h b/kvm.h >>>>>>> index 9c7b0ea..5b3c228 100644 >>>>>>> --- a/kvm.h >>>>>>> +++ b/kvm.h >>>>>>> @@ -198,6 +198,7 @@ static inline void cpu_synchronize_post_init(CPUArchState *env) >>>>>>> { >>>>>>> if (kvm_enabled()) { >>>>>>> kvm_cpu_synchronize_post_init(env); >>>>>>> + smp_wmb(); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> >>>>>> >>>>>> In theory, there should be no vcpu kick-off after this without some >>>>>> locking operations involved that imply barriers. Did you see real >>>>> >>>>> Yeah, but what if it is non-x86? >>>> >>>> The locking I'm referring to is arch independent. >>>> >>>>>> inconsistencies without this explicit one? >>>> >>>> Again: Did you see real issues or is this based on static analysis? >>>> >>> Just on static analysis >> >> Then please describe - also for the changelog - at least one case in >> details where this is needed. >> > I dived into code. And yes, as you said, ACPI eject does involve some > locking operation. So the only thing left is for starting up. There > seems no potential lock operation from cpu_synchronize_all_post_init() > to resume_all_vcpus(). There is no difference in this scenario as well: the vcpu is waiting on a condvar for the initial kick-off and will synchronize with the main thread via the global mutex. When in doubt, try using a debugger. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux