From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by smtp.lore.kernel.org (Postfix) with ESMTP id 85CA9C433F5 for ; Tue, 11 Jan 2022 19:04:54 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 10FEF4B25F; Tue, 11 Jan 2022 14:04:54 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id c3I64UMMNaoy; Tue, 11 Jan 2022 14:04:52 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id CCED24B244; Tue, 11 Jan 2022 14:04:52 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 1E2DD4B244 for ; Tue, 11 Jan 2022 14:04:51 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id gDjozxfICKBa for ; Tue, 11 Jan 2022 14:04:48 -0500 (EST) Received: from mail-pl1-f175.google.com (mail-pl1-f175.google.com [209.85.214.175]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id D5C034B1E7 for ; Tue, 11 Jan 2022 14:04:48 -0500 (EST) Received: by mail-pl1-f175.google.com with SMTP id w7so160315plp.13 for ; Tue, 11 Jan 2022 11:04:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=K0voZAURqPJsMSPgWRWSbzQgUhtAjQkdnnmPm2jJT2w=; b=OBsot1EYclSvooVusPEFiHcqQS0tLiqutA1hLvc4Q8dJ1Ly3d4NacG3jUTcwT8b5B0 yAzAAMtSPazCmF1twB9t7p7OjthaBlEEVkPWyFLjoGrBIC4wlTb5/+rRXf0vM/6rqMCU Ragb7afH7stdPZ3KqbuRhWmkPsSooAAmEnsXlw63cnann8b5t+woTMjdUi9ErVLDZadL SBMqnGpNjCY4JuVHxg3BCfGS2f1fwERk57a6Mqygc7EBur2rRRWKrn5R3WR0uUengxin T+1Z9+vSV6mbJNk442DU5o4tAuCOGgn5t6ttzvE2XufqHuwrPcbXryB0l8VXzxOzfmXG EbOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=K0voZAURqPJsMSPgWRWSbzQgUhtAjQkdnnmPm2jJT2w=; b=AccpcCvhLl+4adw7V/CMsoQUkQ3VGbKJtwKcbbUeaEgAlNpiRPDfwb7EtVXCApeIVi Pfaqh+3MAWRXFDbvVCLO8Dk+IxF3IQBeha7U+UT+AHCuZWDjMZV80BvUeV0gVj9kf+9n BvpBLAoly89xtiH0b2y+ubUrsqiy/GLUWKyFiy0njhkRHV3Vu/t2KetZakbRNgnBnryS 1MM2CmKMS8LxFgaD+EEq/7Me89RlZQxTy5rjHxCoyCsiBYQtk1s/z7vsTrzUZ5S+SZFp 721tCpinvBOM+tvVsOxaBgH61xfmIsGZ8J+VNmEitzK1V4A2jbTRUV4uFXy23vxAJEYE AySA== X-Gm-Message-State: AOAM5301nPDhFDVrBWA0bT/vmwmvMiJluiZTNf9mDPiRlX7TY81zqEA+ ajey5RO6R2YEl8fVnpO3ZIWC0Q== X-Google-Smtp-Source: ABdhPJxSyV27y218zwiIgly8FGJNiKN5fGPK6Fudoin6YBhcWN0ZlQZF3CdSOjibQ9Gqz01uCEpKuA== X-Received: by 2002:a63:7c5e:: with SMTP id l30mr5246852pgn.297.1641927887737; Tue, 11 Jan 2022 11:04:47 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id l22sm11246197pfc.167.2022.01.11.11.04.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Jan 2022 11:04:47 -0800 (PST) Date: Tue, 11 Jan 2022 19:04:42 +0000 From: Sean Christopherson To: Raghavendra Rao Ananta Subject: Re: [RFC PATCH v3 01/11] KVM: Capture VM start Message-ID: References: <20220104194918.373612-1-rananta@google.com> <20220104194918.373612-2-rananta@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Cc: kvm@vger.kernel.org, Marc Zyngier , Peter Shier , linux-kernel@vger.kernel.org, Will Deacon , Catalin Marinas , Paolo Bonzini , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Tue, Jan 11, 2022, Raghavendra Rao Ananta wrote: > On Tue, Jan 11, 2022 at 9:36 AM Sean Christopherson wrote: > > In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces > > unnecessary contention as it will serialize this bit of code if multiple vCPUs > > are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a > > VM will acquire kvm->lock and thus avoid contention once the VM is up and running. > > There's still a possibility that multiple vCPUs will contend for kvm->lock on their > > first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's > > a more elegant solution depending on the use case, e.g. a lockless approach might > > work (or it might not). > > > But is it safe to read kvm->vm_started without grabbing the lock in > the first place? Don't know, but that's my point. Without a consumer in generic KVM and due to my lack of arm64 knowledge, without a high-level description of how the flag will be used by arm64, it's really difficult to determine what's safe and what's not. For other architectures, it's an impossible question to answer because we don't know how the flag might be used. > use atomic_t maybe for this? No. An atomic_t is generally useful only if there are multiple writers that can possibly write different values. It's highly unlikely that simply switching to an atomic address the needs of arm64. > > > > > + kvm->vm_started = true; > > > > > + mutex_unlock(&kvm->lock); > > > > > > > > Lastly, why is this in generic KVM? > > > > > > > The v1 of the series originally had it in the arm specific code. > > > However, I was suggested to move it to the generic code since the book > > > keeping is not arch specific and could be helpful to others too [1]. > > > > I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, > > but I'm skeptical in this particular case. The code _is_ arch specific in that > > arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. > > versus a hypothetical x86 use case that might be completely ok with a lockless > > implementation. And it's not obvious that there's a plausible, safe use case > > outside of arm64, e.g. on x86, there is very, very little that is truly shared > > across the entire VM/system, most things are per-thread/core/package in some way, > > shape, or form. In other words, I'm a wary of providing something like this for > > x86 because odds are good that any use will be functionally incorrect. > I've been going back and forth on this. I've seen a couple of > variables declared in the generic struct and used only in the arch > code. vcpu->valid_wakeup for instance, which is used only by s390 > arch. Maybe I'm looking at it the wrong way as to what can and can't > go in the generic kvm code. Ya, valid_wakeup is an oddball, I don't know why it's in kvm_vcpu instead of arch code that's wrapped with e.g. kvm_arch_vcpu_valid_wakeup(). That said, valid_wakeup is consumed by generic KVM, i.e. has well defined semantics for how it is used, so it's purely a "this code is rather odd" issue. vm_started on the other hand is only produced by generic KVM, and so its required semantics are unclear. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 14205C433F5 for ; Tue, 11 Jan 2022 19:06:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=G3Q3/MqZcOvToKzXhRIBik+i54ovrZvVI2rjZQg+KGc=; b=ZoAS9RuCUoyC9Q QGbNQyrmtqoiEQwtaScueNse6I6s95ybobytuuvTPFzOHtO6+4wo7UZXx/r1XvQutVFxpIqHY8RwE 4q0J1aL2J0oEAeDy+RKZ/iLCGh7qVRa0+KmU10t7O5I0+FQza0x+N4IJXoAyMplwuZZhQv752tDAz PQHcCXCGKEjZ19fo3HCPNBiJAU0idVFgWfgzFlmd2byTFfJBLFJw2DJqrLufxvAtm1XUNPN+/MvX+ ag33hqhwVAKC4L3JpTwNxAmWD7xzrddMipy6C5F0WlicO4SSuZWs40JhgK2oJI5puogLI2yZ2ielz 9ARH/FYvu6Dc6EdKz4cg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n7MRw-00HNWi-Rm; Tue, 11 Jan 2022 19:04:53 +0000 Received: from mail-pl1-x62e.google.com ([2607:f8b0:4864:20::62e]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n7MRs-00HNVj-LZ for linux-arm-kernel@lists.infradead.org; Tue, 11 Jan 2022 19:04:51 +0000 Received: by mail-pl1-x62e.google.com with SMTP id t18so190397plg.9 for ; Tue, 11 Jan 2022 11:04:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=K0voZAURqPJsMSPgWRWSbzQgUhtAjQkdnnmPm2jJT2w=; b=OBsot1EYclSvooVusPEFiHcqQS0tLiqutA1hLvc4Q8dJ1Ly3d4NacG3jUTcwT8b5B0 yAzAAMtSPazCmF1twB9t7p7OjthaBlEEVkPWyFLjoGrBIC4wlTb5/+rRXf0vM/6rqMCU Ragb7afH7stdPZ3KqbuRhWmkPsSooAAmEnsXlw63cnann8b5t+woTMjdUi9ErVLDZadL SBMqnGpNjCY4JuVHxg3BCfGS2f1fwERk57a6Mqygc7EBur2rRRWKrn5R3WR0uUengxin T+1Z9+vSV6mbJNk442DU5o4tAuCOGgn5t6ttzvE2XufqHuwrPcbXryB0l8VXzxOzfmXG EbOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=K0voZAURqPJsMSPgWRWSbzQgUhtAjQkdnnmPm2jJT2w=; b=DQjNZ86ShkSxg51uxARAocNOlujmzTJ/J151PrfjiEzBfLACvhoyjKUldHeYUXV3Rk y6bbZOVfnRxgRpN5+m69ieSZ/MyheWpQMl9BhhT+RRdYZhOlsGtJSEe656P2staKRW6O O1vP1OOq7S/JjgkVpd5xeg/0+QsmWj2rnUV+rlbgQngJOSqxzpvRnezcw0xV/E5c0mAu rczYl65nXTYGQ2F3Bllq5NpEi154eZQcZcQ6jpaTqP54L+KITdk9ylGH3oekSzZgAVja MWMvr684Jub9c6Iv0TjGhe7RZ17VSuQAHaHpMLWR/13VjuRUbOwv7T+cxFIY3cAvHZK3 3Fmw== X-Gm-Message-State: AOAM532WVDvdQ37XEcqA6EDylcf5xERekFs1XXlNjKBSlTXz0EZP4RoW IT326IKB8WROlJVdo7G+7XqWvA== X-Google-Smtp-Source: ABdhPJxSyV27y218zwiIgly8FGJNiKN5fGPK6Fudoin6YBhcWN0ZlQZF3CdSOjibQ9Gqz01uCEpKuA== X-Received: by 2002:a63:7c5e:: with SMTP id l30mr5246852pgn.297.1641927887737; Tue, 11 Jan 2022 11:04:47 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id l22sm11246197pfc.167.2022.01.11.11.04.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Jan 2022 11:04:47 -0800 (PST) Date: Tue, 11 Jan 2022 19:04:42 +0000 From: Sean Christopherson To: Raghavendra Rao Ananta Cc: Marc Zyngier , Andrew Jones , James Morse , Alexandru Elisei , Suzuki K Poulose , kvm@vger.kernel.org, Catalin Marinas , Peter Shier , linux-kernel@vger.kernel.org, Paolo Bonzini , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH v3 01/11] KVM: Capture VM start Message-ID: References: <20220104194918.373612-1-rananta@google.com> <20220104194918.373612-2-rananta@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220111_110448_734139_CE983A50 X-CRM114-Status: GOOD ( 33.13 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jan 11, 2022, Raghavendra Rao Ananta wrote: > On Tue, Jan 11, 2022 at 9:36 AM Sean Christopherson wrote: > > In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces > > unnecessary contention as it will serialize this bit of code if multiple vCPUs > > are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a > > VM will acquire kvm->lock and thus avoid contention once the VM is up and running. > > There's still a possibility that multiple vCPUs will contend for kvm->lock on their > > first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's > > a more elegant solution depending on the use case, e.g. a lockless approach might > > work (or it might not). > > > But is it safe to read kvm->vm_started without grabbing the lock in > the first place? Don't know, but that's my point. Without a consumer in generic KVM and due to my lack of arm64 knowledge, without a high-level description of how the flag will be used by arm64, it's really difficult to determine what's safe and what's not. For other architectures, it's an impossible question to answer because we don't know how the flag might be used. > use atomic_t maybe for this? No. An atomic_t is generally useful only if there are multiple writers that can possibly write different values. It's highly unlikely that simply switching to an atomic address the needs of arm64. > > > > > + kvm->vm_started = true; > > > > > + mutex_unlock(&kvm->lock); > > > > > > > > Lastly, why is this in generic KVM? > > > > > > > The v1 of the series originally had it in the arm specific code. > > > However, I was suggested to move it to the generic code since the book > > > keeping is not arch specific and could be helpful to others too [1]. > > > > I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, > > but I'm skeptical in this particular case. The code _is_ arch specific in that > > arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. > > versus a hypothetical x86 use case that might be completely ok with a lockless > > implementation. And it's not obvious that there's a plausible, safe use case > > outside of arm64, e.g. on x86, there is very, very little that is truly shared > > across the entire VM/system, most things are per-thread/core/package in some way, > > shape, or form. In other words, I'm a wary of providing something like this for > > x86 because odds are good that any use will be functionally incorrect. > I've been going back and forth on this. I've seen a couple of > variables declared in the generic struct and used only in the arch > code. vcpu->valid_wakeup for instance, which is used only by s390 > arch. Maybe I'm looking at it the wrong way as to what can and can't > go in the generic kvm code. Ya, valid_wakeup is an oddball, I don't know why it's in kvm_vcpu instead of arch code that's wrapped with e.g. kvm_arch_vcpu_valid_wakeup(). That said, valid_wakeup is consumed by generic KVM, i.e. has well defined semantics for how it is used, so it's purely a "this code is rather odd" issue. vm_started on the other hand is only produced by generic KVM, and so its required semantics are unclear. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3B155C433EF for ; Tue, 11 Jan 2022 19:04:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345855AbiAKTEt (ORCPT ); Tue, 11 Jan 2022 14:04:49 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37408 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238845AbiAKTEs (ORCPT ); Tue, 11 Jan 2022 14:04:48 -0500 Received: from mail-pl1-x636.google.com (mail-pl1-x636.google.com [IPv6:2607:f8b0:4864:20::636]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 75014C061748 for ; Tue, 11 Jan 2022 11:04:48 -0800 (PST) Received: by mail-pl1-x636.google.com with SMTP id h1so175144pls.11 for ; Tue, 11 Jan 2022 11:04:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=K0voZAURqPJsMSPgWRWSbzQgUhtAjQkdnnmPm2jJT2w=; b=OBsot1EYclSvooVusPEFiHcqQS0tLiqutA1hLvc4Q8dJ1Ly3d4NacG3jUTcwT8b5B0 yAzAAMtSPazCmF1twB9t7p7OjthaBlEEVkPWyFLjoGrBIC4wlTb5/+rRXf0vM/6rqMCU Ragb7afH7stdPZ3KqbuRhWmkPsSooAAmEnsXlw63cnann8b5t+woTMjdUi9ErVLDZadL SBMqnGpNjCY4JuVHxg3BCfGS2f1fwERk57a6Mqygc7EBur2rRRWKrn5R3WR0uUengxin T+1Z9+vSV6mbJNk442DU5o4tAuCOGgn5t6ttzvE2XufqHuwrPcbXryB0l8VXzxOzfmXG EbOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=K0voZAURqPJsMSPgWRWSbzQgUhtAjQkdnnmPm2jJT2w=; b=egVAgXbtIof3B0hkK3NZ2mQkIRX+6RNiG3EFNnhdZ9rJa550dzhaWFhQNzbzaRE+VX 1CgiZE+i9w+vKQzYUNzoIa8kOgDH4FqPxSy+ow8OBQSKpmaqWFLuUwfGvO+TGwcnIj57 cFmD/+i1ELutFt/5DpsTaTG3pMSyec0/bXztJysA3XP88H4PXJnbLv4NQRsHqLbq3JLc K2aKcQra+9naQMHpwgKRJdoNLPUfDeMTN+d5+RpZNYWgJWdohUou+CIknA6qg1+3sCtF N9B7hUHwIqCnXt0A2X1gaY3elx0RB8oQH//xeFRImP8z9s/s7VYsvwbPL+9Lnq0csvWd mxrA== X-Gm-Message-State: AOAM530HsoPwWKaqA29V3uaCGhJJfjVrgtJ5rYIuYsM7wjKxioAYag1M gsTLjHd1djNM+Td8XeMFck3B1Q== X-Google-Smtp-Source: ABdhPJxSyV27y218zwiIgly8FGJNiKN5fGPK6Fudoin6YBhcWN0ZlQZF3CdSOjibQ9Gqz01uCEpKuA== X-Received: by 2002:a63:7c5e:: with SMTP id l30mr5246852pgn.297.1641927887737; Tue, 11 Jan 2022 11:04:47 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id l22sm11246197pfc.167.2022.01.11.11.04.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Jan 2022 11:04:47 -0800 (PST) Date: Tue, 11 Jan 2022 19:04:42 +0000 From: Sean Christopherson To: Raghavendra Rao Ananta Cc: Marc Zyngier , Andrew Jones , James Morse , Alexandru Elisei , Suzuki K Poulose , kvm@vger.kernel.org, Catalin Marinas , Peter Shier , linux-kernel@vger.kernel.org, Paolo Bonzini , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH v3 01/11] KVM: Capture VM start Message-ID: References: <20220104194918.373612-1-rananta@google.com> <20220104194918.373612-2-rananta@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, Jan 11, 2022, Raghavendra Rao Ananta wrote: > On Tue, Jan 11, 2022 at 9:36 AM Sean Christopherson wrote: > > In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces > > unnecessary contention as it will serialize this bit of code if multiple vCPUs > > are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a > > VM will acquire kvm->lock and thus avoid contention once the VM is up and running. > > There's still a possibility that multiple vCPUs will contend for kvm->lock on their > > first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's > > a more elegant solution depending on the use case, e.g. a lockless approach might > > work (or it might not). > > > But is it safe to read kvm->vm_started without grabbing the lock in > the first place? Don't know, but that's my point. Without a consumer in generic KVM and due to my lack of arm64 knowledge, without a high-level description of how the flag will be used by arm64, it's really difficult to determine what's safe and what's not. For other architectures, it's an impossible question to answer because we don't know how the flag might be used. > use atomic_t maybe for this? No. An atomic_t is generally useful only if there are multiple writers that can possibly write different values. It's highly unlikely that simply switching to an atomic address the needs of arm64. > > > > > + kvm->vm_started = true; > > > > > + mutex_unlock(&kvm->lock); > > > > > > > > Lastly, why is this in generic KVM? > > > > > > > The v1 of the series originally had it in the arm specific code. > > > However, I was suggested to move it to the generic code since the book > > > keeping is not arch specific and could be helpful to others too [1]. > > > > I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, > > but I'm skeptical in this particular case. The code _is_ arch specific in that > > arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. > > versus a hypothetical x86 use case that might be completely ok with a lockless > > implementation. And it's not obvious that there's a plausible, safe use case > > outside of arm64, e.g. on x86, there is very, very little that is truly shared > > across the entire VM/system, most things are per-thread/core/package in some way, > > shape, or form. In other words, I'm a wary of providing something like this for > > x86 because odds are good that any use will be functionally incorrect. > I've been going back and forth on this. I've seen a couple of > variables declared in the generic struct and used only in the arch > code. vcpu->valid_wakeup for instance, which is used only by s390 > arch. Maybe I'm looking at it the wrong way as to what can and can't > go in the generic kvm code. Ya, valid_wakeup is an oddball, I don't know why it's in kvm_vcpu instead of arch code that's wrapped with e.g. kvm_arch_vcpu_valid_wakeup(). That said, valid_wakeup is consumed by generic KVM, i.e. has well defined semantics for how it is used, so it's purely a "this code is rather odd" issue. vm_started on the other hand is only produced by generic KVM, and so its required semantics are unclear.