Message ID | 20231030141728.1406118-1-nik.borisov@suse.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp2250222vqb; Mon, 30 Oct 2023 07:17:51 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEmxQFiIxgt73dO6S74jLcGGU2QZIJD2IT6GchCp7BLH4LMDv60U1F86Fa7Td8/qGLBpQ+g X-Received: by 2002:a17:902:e848:b0:1cc:4559:ff with SMTP id t8-20020a170902e84800b001cc455900ffmr4198630plg.13.1698675470953; Mon, 30 Oct 2023 07:17:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698675470; cv=none; d=google.com; s=arc-20160816; b=SiPEl3NAXOui7jovWbDDJdXzmHgCVRuqm2qn0y/9AZCwPHNgcoBQ+omaCuLf2gl4uZ aKQ73v9HhhSGgzFUiJlJSgfRl699r5gqPoh2Y0sH5KdmFU9mWrgOOdJoHU0hGvoF25xK GcsgTI0Q3FeiWnULU6mfFsrOkZSumUyK9FVQHgf10fQKMhlCBpVainPo5WqpGnoM7T0M aYCZdphY2OlRQZjBN6qvXv5XhyRnx+blBoVdJB/FoJJy8IoWAA9/HUS0UXzwvLMeFT8W Vg3jThfaRKzQwXyodQ5UgcH7Uc4JO7LBU1kM9j+YacwKMPdJHdv9w+1a+hwjFZ3SR1wT Ehkg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=JL0o6ejOmiqyX7RlEJrZ7RAI8Gjvf0EzzlogCece80k=; fh=LkO7p7q30BFfSoovj2Eo8otZDk7Xt7A9HoE19DR418E=; b=XRQcezlPYZrfK1OF40GBpZfY+lcjpl6m9yEVl9jtmi7p746dLnGHL727OWjvLMnqWl hWmrFrKb7HbYnLICWSm77XxcCn6UISaosF5ci0yBtLxFy6kS65qX1rfvUcTAzRIl2Veg d9biDRlczeu9yuy1od7/I7vQDpZTAM/uCXJv7a3IaTDb1Nov8znuHRfQsKd2Go9c5fAO NHiPWwJIIXPJeUA/09EHfmOmlAJw5+LyPci6xYI3yFpK1oe/FQFhpvOPy+WF7bbFills 74DLjLpgwmHhzLjrHPaRHjEZ4oSgpMN7vVVgBWKWkSB3A9H7ILeRmNiE1uiI/kVMcDZ4 sJvA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@suse.com header.s=susede1 header.b=Wjl2lgZq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=suse.com Received: from groat.vger.email (groat.vger.email. [2620:137:e000::3:5]) by mx.google.com with ESMTPS id l12-20020a170902e2cc00b001c7342b46e4si4959922plc.23.2023.10.30.07.17.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Oct 2023 07:17:50 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5; Authentication-Results: mx.google.com; dkim=pass header.i=@suse.com header.s=susede1 header.b=Wjl2lgZq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=suse.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id AE0968062AC3; Mon, 30 Oct 2023 07:17:48 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233592AbjJ3ORh (ORCPT <rfc822;zxc52fgh@gmail.com> + 31 others); Mon, 30 Oct 2023 10:17:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53732 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231577AbjJ3ORf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 30 Oct 2023 10:17:35 -0400 Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 04C9BC0; Mon, 30 Oct 2023 07:17:33 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id B59E521906; Mon, 30 Oct 2023 14:17:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1698675451; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=JL0o6ejOmiqyX7RlEJrZ7RAI8Gjvf0EzzlogCece80k=; b=Wjl2lgZqWUg/a9AE3QuJHK6+NDQzHMDQTy31U33Llwff6vIw9Uw3qZJEK6BBLQK7obO/sZ t2Mtq/tO6UUvCOg2E8yhcLo1TzdTXF2lTFc0SzApsN+9MC+1pLb4PGPjwQK+Xh8b3RevrF dn4G3KmXXo2GEKtZOC/O72raxnLbkZw= Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 66803138F8; Mon, 30 Oct 2023 14:17:31 +0000 (UTC) Received: from dovecot-director1.suse.de ([192.168.254.64]) by imap2.suse-dmz.suse.de with ESMTPSA id jifVFvu6P2VsOgAAMHmgww (envelope-from <nik.borisov@suse.com>); Mon, 30 Oct 2023 14:17:31 +0000 From: Nikolay Borisov <nik.borisov@suse.com> To: seanjc@google.com Cc: pbonzini@redhat.com, x86@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Nikolay Borisov <nik.borisov@suse.com> Subject: [PATCH] KVM: x86: User mutex guards to eliminate __kvm_x86_vendor_init() Date: Mon, 30 Oct 2023 16:17:28 +0200 Message-Id: <20231030141728.1406118-1-nik.borisov@suse.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Mon, 30 Oct 2023 07:17:48 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781190331054958791 X-GMAIL-MSGID: 1781190331054958791 |
Series |
KVM: x86: User mutex guards to eliminate __kvm_x86_vendor_init()
|
|
Commit Message
Nikolay Borisov
Oct. 30, 2023, 2:17 p.m. UTC
Current separation between (__){0,1}kvm_x86_vendor_init() is superfluos as
the the underscore version doesn't have any other callers.
Instead, use the newly added cleanup infrastructure to ensure that
kvm_x86_vendor_init() holds the vendor_module_lock throughout its
exectuion and that in case of error in the middle it's released. No
functional changes.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
arch/x86/kvm/x86.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
Comments
On Mon, Oct 30, 2023, Nikolay Borisov wrote: > Current separation between (__){0,1}kvm_x86_vendor_init() is superfluos as superfluous But this intro is actively misleading. The double-underscore variant most definitely isn't superfluous, e.g. it eliminates the need for gotos reduces the probability of incorrect error codes, bugs in the error handling, etc. It _becomes_ superflous after switching to guard(mutex). IMO, this is one of the instances where the "problem, then solution" appoach is counter-productive. If there are no objections, I'll massage the change log to the below when applying (for 6.8, in a few weeks). Use the recently introduced guard(mutex) infrastructure acquire and automatically release vendor_module_lock when the guard goes out of scope. Drop the inner __kvm_x86_vendor_init(), its sole purpose was to simplify releasing vendor_module_lock in error paths. No functional change intended. > the the underscore version doesn't have any other callers. > > Instead, use the newly added cleanup infrastructure to ensure that > kvm_x86_vendor_init() holds the vendor_module_lock throughout its > exectuion and that in case of error in the middle it's released. No > functional changes.
On 30.10.23 г. 18:07 ч., Sean Christopherson wrote: > On Mon, Oct 30, 2023, Nikolay Borisov wrote: >> Current separation between (__){0,1}kvm_x86_vendor_init() is superfluos as > > superfluous > > But this intro is actively misleading. The double-underscore variant most definitely > isn't superfluous, e.g. it eliminates the need for gotos reduces the probability > of incorrect error codes, bugs in the error handling, etc. It _becomes_ superflous > after switching to guard(mutex). > > IMO, this is one of the instances where the "problem, then solution" appoach is > counter-productive. If there are no objections, I'll massage the change log to > the below when applying (for 6.8, in a few weeks). > > Use the recently introduced guard(mutex) infrastructure acquire and > automatically release vendor_module_lock when the guard goes out of scope. > Drop the inner __kvm_x86_vendor_init(), its sole purpose was to simplify > releasing vendor_module_lock in error paths. > > No functional change intended. Thanks, I'm fine with this changelog. > >> the the underscore version doesn't have any other callers. >> >> Instead, use the newly added cleanup infrastructure to ensure that >> kvm_x86_vendor_init() holds the vendor_module_lock throughout its >> exectuion and that in case of error in the middle it's released. No >> functional changes. >
On 10/30/23 17:07, Sean Christopherson wrote: > On Mon, Oct 30, 2023, Nikolay Borisov wrote: >> Current separation between (__){0,1}kvm_x86_vendor_init() is superfluos as > > superfluous > > But this intro is actively misleading. The double-underscore variant most definitely > isn't superfluous, e.g. it eliminates the need for gotos reduces the probability > of incorrect error codes, bugs in the error handling, etc. It _becomes_ superflous > after switching to guard(mutex). > > IMO, this is one of the instances where the "problem, then solution" appoach is > counter-productive. If there are no objections, I'll massage the change log to > the below when applying (for 6.8, in a few weeks). I think this is a "Speak Now or Forever Rest in Peace" situation. I'm going to wait a couple days more for reviews to come in, post a v14 myself, and apply the series to kvm/next as soon as Linus merges the 6.7 changes. The series will be based on the 6.7 tags/for-linus, and when 6.7-rc1 comes up, I'll do this to straighten the history: git checkout kvm/next git tag -s -f kvm-gmem HEAD git reset --hard v6.7-rc1 git merge tags/kvm-gmem # fix conflict with Christian Brauner's VFS series git commit git push kvm 6.8 is not going to be out for four months, and I'm pretty sure that anything discovered within "a few weeks" can be applied on top, and the heaviness of a 35-patch series will outweigh any imperfections by a long margin). (Full disclosure: this is _also_ because I want to apply this series to the RHEL kernel, and Red Hat has a high level of disdain for non-upstream patches. But it's mostly because I want all dependencies to be able to move on and be developed on top of stock kvm/next). Paolo
On Mon, Oct 30, 2023, Paolo Bonzini wrote: > On 10/30/23 17:07, Sean Christopherson wrote: > > On Mon, Oct 30, 2023, Nikolay Borisov wrote: > > > Current separation between (__){0,1}kvm_x86_vendor_init() is superfluos as > > > > superfluous > > > > But this intro is actively misleading. The double-underscore variant most definitely > > isn't superfluous, e.g. it eliminates the need for gotos reduces the probability > > of incorrect error codes, bugs in the error handling, etc. It _becomes_ superflous > > after switching to guard(mutex). > > > > IMO, this is one of the instances where the then solution problem appoach is > > counter-productive. If there are no objections, I'll massage the change log to > > the below when applying (for 6.8, in a few weeks). > > I think this is a "Speak Now or Forever Rest in Peace" situation. I'm going > to wait a couple days more for reviews to come in, post a v14 myself, and > apply the series to kvm/next as soon as Linus merges the 6.7 changes. The > series will be based on the 6.7 tags/for-linus, and when 6.7-rc1 comes up, > I'll do this to straighten the history: Heh, I'm pretty sure you meant to respond to the guest_memfd series. > git checkout kvm/next > git tag -s -f kvm-gmem HEAD > git reset --hard v6.7-rc1 > git merge tags/kvm-gmem > # fix conflict with Christian Brauner's VFS series > git commit > git push kvm > > 6.8 is not going to be out for four months, and I'm pretty sure that > anything discovered within "a few weeks" can be applied on top, and the > heaviness of a 35-patch series will outweigh any imperfections by a long > margin). > > (Full disclosure: this is _also_ because I want to apply this series to the > RHEL kernel, and Red Hat has a high level of disdain for non-upstream > patches. But it's mostly because I want all dependencies to be able to move > on and be developed on top of stock kvm/next).
On 10/30/23 18:36, Sean Christopherson wrote: >>> If there are no objections, I'll massage the change log to >>> the below when applying (for 6.8, in a few weeks). >> >> I think this is a "Speak Now or Forever Rest in Peace" situation. I'm going >> to wait a couple days more for reviews to come in, post a v14 myself, and >> apply the series to kvm/next as soon as Linus merges the 6.7 changes. The >> series will be based on the 6.7 tags/for-linus, and when 6.7-rc1 comes up, >> I'll do this to straighten the history: > > Heh, I'm pretty sure you meant to respond to the guest_memfd series. Well, it was the "in a few weeks" that almost caused me a panic attack. :) But yeah, I soon got to the conclusion that this required a wider diffusion and reposted there. Paolo
On Mon, 2023-10-30 at 18:17 +0200, Nikolay Borisov wrote: > > On 30.10.23 г. 18:07 ч., Sean Christopherson wrote: > > On Mon, Oct 30, 2023, Nikolay Borisov wrote: > > > Current separation between (__){0,1}kvm_x86_vendor_init() is superfluos as > > > > superfluous > > > > But this intro is actively misleading. The double-underscore variant most definitely > > isn't superfluous, e.g. it eliminates the need for gotos reduces the probability > > of incorrect error codes, bugs in the error handling, etc. It _becomes_ superflous > > after switching to guard(mutex). > > > > IMO, this is one of the instances where the "problem, then solution" appoach is > > counter-productive. If there are no objections, I'll massage the change log to > > the below when applying (for 6.8, in a few weeks). > > > > Use the recently introduced guard(mutex) infrastructure acquire and > > automatically release vendor_module_lock when the guard goes out of scope. > > Drop the inner __kvm_x86_vendor_init(), its sole purpose was to simplify > > releasing vendor_module_lock in error paths. > > > > No functional change intended. > > Thanks, I'm fine with this changelog. > > Reviewed-by: Kai Huang <kai.huang@intel.com>
On 30.10.23 г. 18:07 ч., Sean Christopherson wrote: > On Mon, Oct 30, 2023, Nikolay Borisov wrote: >> Current separation between (__){0,1}kvm_x86_vendor_init() is >> superfluos as > > superfluous > > But this intro is actively misleading. The double-underscore variant > most definitely > isn't superfluous, e.g. it eliminates the need for gotos reduces the > probability > of incorrect error codes, bugs in the error handling, etc. It _becomes_ > superflous > after switching to guard(mutex). > > IMO, this is one of the instances where the "problem, then solution" > appoach is > counter-productive. If there are no objections, I'll massage the change > log to > the below when applying (for 6.8, in a few weeks). > > Use the recently introduced guard(mutex) infrastructure acquire and > automatically release vendor_module_lock when the guard goes out of > scope. > Drop the inner __kvm_x86_vendor_init(), its sole purpose was to simplify > releasing vendor_module_lock in error paths. > > No functional change intended. > >> the the underscore version doesn't have any other callers. >> Has this fallen through the cracks as I don't see it in 6.7?
On Sat, Dec 09, 2023, Nikolay Borisov wrote: > > > On 30.10.23 г. 18:07 ч., Sean Christopherson wrote: > > On Mon, Oct 30, 2023, Nikolay Borisov wrote: > > > Current separation between (__){0,1}kvm_x86_vendor_init() is > > > superfluos as > > > > superfluous > > > > But this intro is actively misleading. The double-underscore variant > > most definitely > > isn't superfluous, e.g. it eliminates the need for gotos reduces the > > probability > > of incorrect error codes, bugs in the error handling, etc. It _becomes_ > > superflous > > after switching to guard(mutex). > > > > IMO, this is one of the instances where the then solution problem > > appoach is > > counter-productive. If there are no objections, I'll massage the change > > log to > > the below when applying (for 6.8, in a few weeks). > > > > Use the recently introduced guard(mutex) infrastructure acquire and > > automatically release vendor_module_lock when the guard goes out of > > scope. > > Drop the inner __kvm_x86_vendor_init(), its sole purpose was to simplify > > releasing vendor_module_lock in error paths. > > > > No functional change intended. > > > > > the the underscore version doesn't have any other callers. > > > > > > Has this fallen through the cracks as I don't see it in 6.7? As above, I have this tagged for inclusion in 6.8, not 6.7. Though admittedly, this one did actually fall through the cracks as I moved it to the wrong mailbox when Paolo usurped the thread for unrelated guest_memfd stuff. Anyways, I do plan on grabbing this for 6.8, I'm just buried in non-upstream stuff right now.
On Mon, 30 Oct 2023 16:17:28 +0200, Nikolay Borisov wrote: > Current separation between (__){0,1}kvm_x86_vendor_init() is superfluos as > the the underscore version doesn't have any other callers. > > Instead, use the newly added cleanup infrastructure to ensure that > kvm_x86_vendor_init() holds the vendor_module_lock throughout its > exectuion and that in case of error in the middle it's released. No > functional changes. > > [...] Applied to kvm-x86 misc, thanks! [1/1] KVM: x86: Use mutex guards to eliminate __kvm_x86_vendor_init() https://github.com/kvm-x86/linux/commit/955997e88017 -- https://github.com/kvm-x86/linux/tree/next
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 41cce5031126..cd7c2d0f88cb 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9446,11 +9446,13 @@ static void kvm_x86_check_cpu_compat(void *ret) *(int *)ret = kvm_x86_check_processor_compatibility(); } -static int __kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) +int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) { u64 host_pat; int r, cpu; + guard(mutex)(&vendor_module_lock); + if (kvm_x86_ops.hardware_enable) { pr_err("already loaded vendor module '%s'\n", kvm_x86_ops.name); return -EEXIST; @@ -9580,17 +9582,6 @@ static int __kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) kmem_cache_destroy(x86_emulator_cache); return r; } - -int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) -{ - int r; - - mutex_lock(&vendor_module_lock); - r = __kvm_x86_vendor_init(ops); - mutex_unlock(&vendor_module_lock); - - return r; -} EXPORT_SYMBOL_GPL(kvm_x86_vendor_init); void kvm_x86_vendor_exit(void)