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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 49C14C25B50 for ; Fri, 20 Jan 2023 23:17:27 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9ECB46B0071; Fri, 20 Jan 2023 18:17:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 975EF6B0073; Fri, 20 Jan 2023 18:17:26 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7EFC66B0074; Fri, 20 Jan 2023 18:17:26 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 68A746B0071 for ; Fri, 20 Jan 2023 18:17:26 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 3EE0080B23 for ; Fri, 20 Jan 2023 23:17:26 +0000 (UTC) X-FDA: 80376740892.16.6D33B43 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf02.hostedemail.com (Postfix) with ESMTP id 84FF88001A for ; Fri, 20 Jan 2023 23:17:24 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=HAucgctX; spf=pass (imf02.hostedemail.com: domain of jarkko@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jarkko@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1674256644; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=PIm6yliyB6cmgABc57nkKBtikSxib/VmQeTUgA1ExYQ=; b=opie5GcMWIzFPEqQWg3j785+I0E29Q4hQGGLDLzroJ7cedh2xvFObw/sP+8wop1dvBJvWi ReHUK5dP3NIfmzE1g25oLmTQVaDJqEvMXeCSDucP1Sbb6YRmYnlelA3szdyPxxDD1Bi9Lf fHuDHGXTn4FoOIASwzMT0qk0zI2yFLI= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=HAucgctX; spf=pass (imf02.hostedemail.com: domain of jarkko@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jarkko@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1674256644; a=rsa-sha256; cv=none; b=uOJPd0P6tvxI22E/VF1EcMX4q78F9tGl7HMnxWGeFA31KfxKwVUlU4hBgB+2I+ChgjHyH1 Hr4u2d7w7EQTrsIuv22K2uuCEzYa2eYfZy4UWbLt0hK0jfLzjAyN/Ciiss8sS+JZc7jnDr 4Qsbkv9+Yp43cPRyxD3kqrMTw7ifw8w= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8F761620DC; Fri, 20 Jan 2023 23:17:23 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7510DC433D2; Fri, 20 Jan 2023 23:17:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1674256643; bh=SoqwK6Lbe/mngQK8/Z0v1FXpq/aJOBcKcs9SVP31/ss=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HAucgctXzN9/951JWEJjbzvwMgSm5mu/FK37EvGSRiIsqcGwiwm0Sd2MMd+k58d9G Fp0qyJPu11GHZFkhDwIPCnhjqIlM0o13n/mq14ncbX/1ZNxhnX5nQdbv2s9DDIINdU zkynKCEzKf2AjpTOkByQy3zeIem0IvE7iniX+PDZ2Zcga80SXcyT6Cx9V/XJYFmqMW 8xlCk1YB8+/16Wtm8akBQmfuMqE/uj4FKd8v2julqdoNeZv+InV1oBUkWNUHg/n+Y7 w3Xu/Cx89Yyy8Ab+QikkvjG1ytxt2d3Pki9e+jRBI/Cop2LOoXGWd8r6aIjK57H9Ce WGCs8eUeLB8uQ== Date: Fri, 20 Jan 2023 23:17:20 +0000 From: Jarkko Sakkinen To: "Kalra, Ashish" Cc: Michael Roth , kvm@vger.kernel.org, linux-coco@lists.linux.dev, linux-mm@kvack.org, linux-crypto@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com, jroedel@suse.de, thomas.lendacky@amd.com, hpa@zytor.com, ardb@kernel.org, pbonzini@redhat.com, seanjc@google.com, vkuznets@redhat.com, wanpengli@tencent.com, jmattson@google.com, luto@kernel.org, dave.hansen@linux.intel.com, slp@redhat.com, pgonda@google.com, peterz@infradead.org, srinivas.pandruvada@linux.intel.com, rientjes@google.com, dovmurik@linux.ibm.com, tobin@ibm.com, bp@alien8.de, vbabka@suse.cz, kirill@shutemov.name, ak@linux.intel.com, tony.luck@intel.com, marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com, dgilbert@redhat.com, harald@profian.com, Brijesh Singh , Pavan Kumar Paluri Subject: Re: [PATCH RFC v7 37/64] KVM: SVM: Add KVM_SNP_INIT command Message-ID: References: <20221214194056.161492-1-michael.roth@amd.com> <20221214194056.161492-38-michael.roth@amd.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="Mwn5vxNlX4YIa3fN" Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 84FF88001A X-Rspam-User: X-Stat-Signature: hwgp5dwobh874oetgsn9qqznyaf5ojqk X-HE-Tag: 1674256644-790253 X-HE-Meta: U2FsdGVkX1/b3//Vnjr6m3KjII1KcAOeGu/vivL3Q+ujBpq1J8kDriECdsvqDAzvQZ2ApEPlnuMugwjwy/DIsg5nnLik3HpJR91qSpCI/9ckg4BijVcGYO4DQ/froyIwPUGukr6RqqIpmHUcwBkWSq9Hw1k2v+5OtM9J4VxwTGZglMJ8CMg6hBZbQhjLEDaUsNazw1jKD/x3mKP91axiUd72ItfVe7rhl0wVO50XHfGM6+4zNcXzpaTTF6TGcDxjKCPse4vUDCEilXufzNUf9KHXZchUm0p9Zfj8t4Vq+nYY0ia/01OM+82md9sZap8FQDGe8gYqVEVmtcT9JRHIOiPQr5KSUbiuXmXHk9rxu7aUjPdZM7gS5/a+6bRwD3/vxy7w2y3J5eFeWOD2ZHetZ8tB++ecS9VWRLR9LtLIBp2jzR0hAPKUGxFc9/TSeUopD7S789sIQUZUbED/K0TOskPmLe/xOo6alTorKdxTLEqN2Nn9cFdAXv6p6id+R0q8TXxni3tiXqa1M07GTXRMtdsUCHVHoUo2yB6K+bb1QZH7N/QdqFfzz2f0RU9cAGJ8DznATufV15orz/Hq4bG7peBSH1sRhsUDn6fdazvvO4CoO3hVdPtX3NsCUaPG0Xzcuvxii6mFwKv2EhBfQcvg7qiYMvbvEZPJdGL2pEYJUglTMcMshLMTNvJzR1Wlji6m3mTWaSIpjvGcC0Jby2ws2YrU8x+yDyW+d9FIkMhSxFYrBb7BLGtAOPZT0JfolT9rAbfawqAYdAnKqA1CLCPo3m4R8tV7fnsvG+vSD+UjYdkBd86EzwzdIXurY3Kel3a//FpHCugb+cL+wjQSNTTy1j9lRaaaRRk+7PRIujY0tqLaDjPu2YoACqzYs/PB6kpC69x7hbJ4r8cDAB9DxRRbBMC7rdHGcYRFAFtDdiK3CEuTdYBDSyeqdJhH+/hT+mAYTwDRj4CA4YDlBW1Ytff oiCGGgAD ymy2oBI6n91RGYGq/ApAehCef91ykjdLZ0hrmLnnUKpalD9H2a/Uy/+awWsZUhW7ToiY7pKsl3u3qmqHYccgtkHrQElbBAzQ7lV9ILAW5LDCNWPl/hr9FWadDeD3lrJe+raEJqCn8Beo3/CC3GF1+kH34nUTVrro8DTdp89WCm80KtWSmVYfhEyshrMfnwrYpMmCJsvrXXVaAFLKtALfZv8nOoWuyMNB89ltuVVnFvdWCpZH3F8AM7rhCp8TY3/MA+4/FX5d/kcCViGLFI9+mYKytFJTZ7NLp5VC1tf8SWnDr15UToaM0X3UDM3PGm8Oue33pjfW7WgbUurtIN+QVUNnPsxfMbsjFIuopeyUasaa+jrB7mQxuiiY2FGg0TrVq9HzvejB/htElxC0GTqgwKKMpfr7/dyZyumQPzSeNRYy7hmZx2Tet4FmdG0LLX+74xDASKFZHeZsWAhu5a6aXTUg2pK7WzIpAXPsx74Tx3Cp4/1s= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: --Mwn5vxNlX4YIa3fN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Jan 05, 2023 at 05:37:20PM -0600, Kalra, Ashish wrote: > Hello Jarkko, > > On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote: > > On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote: > > > static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > > > { > > > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > > > @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > > > return ret; > > > sev->active = true; > > > - sev->es_active = argp->id == KVM_SEV_ES_INIT; > > > + sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT); > > > + sev->snp_active = argp->id == KVM_SEV_SNP_INIT; > > > asid = sev_asid_new(sev); > > > if (asid < 0) > > > goto e_no_asid; > > > sev->asid = asid; > > > - ret = sev_platform_init(&argp->error); > > > + if (sev->snp_active) { > > > + ret = verify_snp_init_flags(kvm, argp); > > > + if (ret) > > > + goto e_free; > > > + > > > + ret = sev_snp_init(&argp->error, false); > > > + } else { > > > + ret = sev_platform_init(&argp->error); > > > + } > > > > Couldn't sev_snp_init() and sev_platform_init() be called unconditionally > > in order? > > > > Since there is a hardware constraint that SNP init needs to always happen > > before platform init, shouldn't SNP init happen as part of > > __sev_platform_init_locked() instead? > > > > On Genoa there is currently an issue that if we do an SNP_INIT before an > SEV_INIT and then attempt to launch a SEV guest that may fail, so we need to > keep SNP INIT and SEV INIT separate. > > We need to provide a way to run (existing) SEV guests on a system that > supports SNP without doing an SNP_INIT at all. > > This is done using psp_init_on_probe parameter of the CCP module to avoid > doing either SNP/SEV firmware initialization during module load and then > defer the firmware initialization till someone launches a guest of one > flavor or the other. > > And then sev_guest_init() does either SNP or SEV firmware init depending on > the type of the guest being launched. OK, got it, thank you. I have not noticed the init_on_probe for sev_snp_init() before. Was it in earlier patch set version? The benefit of having everything in __sev_platform_init_lock() would be first less risk of shooting yourself into foot, and also no need to pass init_on_probe to sev_snp_init() as it would be internal to sev-dev.c, and no need for special cases for callers. It is in my opinion internals of the SEV driver to guarantee the order. E.g. changes to svm/sev.c would be then quite trivial. > > I found these call sites for __sev_platform_init_locked(), none of which > > follow the correct call order: > > > > * sev_guest_init() > > As explained above, this call site is important for deferring the firmware > initialization to an actual guest launch. > > > * sev_ioctl_do_pek_csr > > * sev_ioctl_do_pdh_export() > > * sev_ioctl_do_pek_import() > > * sev_ioctl_do_pek_pdh_gen() What happens if any of these are called before sev_guest_init()? They only call __sev_platform_init_locked(). > > * sev_pci_init() > > > > For me it looks like a bit flakky API use to have sev_snp_init() as an API > > call. > > > > I would suggest to make SNP init internal to the ccp driver and take care > > of the correct orchestration over there. > > > > Due to Genoa issue, we may still need SNP init and SEV init to be invoked > separately outside the CCP driver. > > > Also, how it currently works in this patch set, if the firmware did not > > load correctly, SNP init halts the whole system. The version check needs > > to be in all call paths. > > > > Yes, i agree with that. Attached the fix I sent in private earlier. > Thanks, > Ashish BR, Jarkko --Mwn5vxNlX4YIa3fN Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-crypto-ccp-Prevent-a-spurious-SEV_CMD_SNP_INIT-trigg.patch" >From f24054af9eeeb0314bbd0c37bd97ff38e2ded717 Mon Sep 17 00:00:00 2001 From: Jarkko Sakkinen Date: Sun, 4 Dec 2022 06:17:07 +0000 Subject: [PATCH] crypto: ccp: Prevent a spurious SEV_CMD_SNP_INIT triggered by sev_guest_init() Move the firmware version check from sev_pci_init() to sev_snp_init(). Signed-off-by: Jarkko Sakkinen --- drivers/crypto/ccp/sev-dev.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 7c5698bde655..08787d998f15 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1387,6 +1387,12 @@ static int __sev_snp_init_locked(int *error) if (sev->snp_initialized) return 0; + if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) { + dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n", + SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); + return 0; + } + /* * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h * across all cores. @@ -2319,25 +2325,19 @@ void sev_pci_init(void) } } + rc = sev_snp_init(&error, true); + if (rc) + /* + * Don't abort the probe if SNP INIT failed, + * continue to initialize the legacy SEV firmware. + */ + dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error); + /* * If boot CPU supports SNP, then first attempt to initialize * the SNP firmware. */ if (cpu_feature_enabled(X86_FEATURE_SEV_SNP)) { - if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) { - dev_err(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n", - SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR); - } else { - rc = sev_snp_init(&error, true); - if (rc) { - /* - * Don't abort the probe if SNP INIT failed, - * continue to initialize the legacy SEV firmware. - */ - dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error); - } - } - /* * Allocate the intermediate buffers used for the legacy command handling. */ -- 2.38.1 --Mwn5vxNlX4YIa3fN--