Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Jul 2023 17:13:36 +0800
From:   Chao Gao <chao.gao@intel.com>
To:     Yang Weijiang <weijiang.yang@intel.com>
CC:     <seanjc@google.com>, <pbonzini@redhat.com>, <peterz@infradead.org>,
        <john.allen@amd.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <rick.p.edgecombe@intel.com>,
        <binbin.wu@linux.intel.com>
Subject: Re: [PATCH v4 11/20] KVM:x86: Save and reload GUEST_SSP to/from SMRAM
Message-ID: <ZL5AwOBYN1JV7I4W@chao-email>
References: <20230721030352.72414-1-weijiang.yang@intel.com>
 <20230721030352.72414-12-weijiang.yang@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230721030352.72414-12-weijiang.yang@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?fCE+3fPZofiOvMRKXva/0sVZze0h5NDBGy7WO/yzY4BEYdNIKRfrb930+X5X?=
 =?us-ascii?Q?a+XrW7apK729dIdtmco6n6G7z2zU7dZPNQu0eGHyU3zRYhgtSehYRoNUv3l9?=
 =?us-ascii?Q?om97KtfJ6aqj3xyD+FhQuBbEZCe/FnHeQVWqhpB8AVsxloa6rg/WRGZGrqJ2?=
 =?us-ascii?Q?P8BWa+JQRfX2hYPHwLvWRHVgxSFo8BzUJugCeLIvnMhAUu1MzAchRYI+V++M?=
 =?us-ascii?Q?4UxVrArVp2kxHpZPRsyCaMJDBrYZZuO2/bQvlB7HwYVLNoSs2OpNuiTcrrVN?=
 =?us-ascii?Q?DalcDg+Z2NlywYrX0f+NQ77D8Z85FQMmfy+/38GGHi8BOYbCJyXkf+sKJF8Z?=
 =?us-ascii?Q?HYMoVbmI+fDMw/lPFsFbK2aNuqJkWpfZEuDp5lYx2fh7uZqBvSDLENkKPSw2?=
 =?us-ascii?Q?UzzB+gtiBI5WY1akH4cQDl1FFhT2vomkl4dnValjeQlXDZkqZQtHyUA5KiSo?=
 =?us-ascii?Q?8ICS0+dqQKiXsLZSSkQp3QjyZClvcE9tLMqSGO4L621ZGjC7/hi/s1yxHNq/?=
 =?us-ascii?Q?K82VxSwjmfCCB2fLNjjdHXjekVRZJBh6ClQij4OXei8IrTLoo/RCaBGr/iY1?=
 =?us-ascii?Q?YCw95Z9eh6FqenkiyRGebm7KoH6vjvwDuoRVhecVJLj05p5JdRrZaL4wmsf/?=
 =?us-ascii?Q?gHckWEKfYTqqnd38Zz+NR0miCREUsDOgP+6KMH7Ik8+5rGW1SwkVaxM7iNM9?=
 =?us-ascii?Q?JDMGvkqmCzwgN1TI6LJuprXKDzz8qAeRBDV2KaQEWT2QnDYDRjROM/FenLB+?=
 =?us-ascii?Q?SX08Cp3R312ZNMroHyXcID63KZJtjD76J4A632GYS3sXaGsAzJkQlWt95Xcm?=
 =?us-ascii?Q?wpVoU4VUYgd46vCWPQcm0czpoDc/XGRh8fqdsLwb///5ombZ2lvWPS0iCtxd?=
 =?us-ascii?Q?OB/XqnjenKtQvL5nfr5Ki7sh5w7zrM9q1BiZU+QDzJFAJ7HKiaGhg87IPVYh?=
 =?us-ascii?Q?zSoNKK+7zkzRRX4olDHUSEqT9neZGGWRV2yTgbLKr77B30CtugWuMVHgKc7F?=
 =?us-ascii?Q?5rtyVYAtj2tGCmR1dxjvmmo0rQ+PvIQ9gX+WhpX4bgKxrKhr+NdPj7Wrqxya?=
 =?us-ascii?Q?kEv+Znu4e4mtUIAgzPeSBSfjh7j+sIbyqY8ohuf1BIhWdUMNIBe7y4s18fyz?=
 =?us-ascii?Q?SuLoljT2ePQOW1FI5axqBc7/j6Tti6Gh/4kjhStSKCA7bQhFGlgoYScAudd0?=
 =?us-ascii?Q?DO2jPsAw2rUY9nm+u4UcE/Yv07J3iwqPQpKlvNkUjWhGSwg0hf3Fniqj5AQa?=
 =?us-ascii?Q?JxXkPijfxFE5Tyd1D2IiRTYgWWSublimCKG/IngeLaMbJAsZCcYUMZfUcA7W?=
 =?us-ascii?Q?dJpaippHWtnOvMmVrouXQoPdxxifHcdDouPPreoDHYpxtBCAW6HqBGe7co86?=
 =?us-ascii?Q?fZ3M3EAIXfSsLwl+bxpAO8DOTCox84AWd7sMmxIYB+XUQGkX5N6/K/X8vhjN?=
 =?us-ascii?Q?CKE6xk7YsA/olLXxyu89JAWGCotUWyMC/+vrjvfexQcBc1DX4BWvk10ZKP+R?=
 =?us-ascii?Q?FWT1bSkOZJtVpio/OZJs+aPBgBAB6keJXdvks1Y49E0CH18Vz8g1qUBZZcGX?=
 =?us-ascii?Q?qwzNTBRQoJZlOa1NS+hKihQz0YZr6lEr4rvOs8fE?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1dc3c05d-25d0-41ca-7c0f-08db8c26488f
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB6780.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jul 2023 09:13:46.0609
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dO6rawIZ7rQwQud9CUORIZGPI1Q9ok02dlNwWjsnFXkKK1YKtBF3aAZHr508Y49HzV2DS8YtOZsxHwJbLMoRpA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4634
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jul 20, 2023 at 11:03:43PM -0400, Yang Weijiang wrote:
>Save GUEST_SSP to SMRAM on SMI and reload it on RSM.
>KVM emulates architectural behavior when guest enters/leaves SMM
>mode, i.e., save registers to SMRAM at the entry of SMM and reload
>them at the exit of SMM. Per SDM, GUEST_SSP is defined as one of

To me, GUEST_SSP is confusing here. From QEMU's perspective, it reads/writes
the SSP register. People may confuse it with the GUEST_SSP in nVMCS field.
I prefer to rename it to MSR_KVM_SSP.

>the fields in SMRAM for 64-bit mode, so handle the state accordingly.
>
>Check HF_SMM_MASK to determine whether kvm_cet_is_msr_accessible()
>is called in SMM mode so that kvm_{set,get}_msr() works in SMM mode.
>
>Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>---
> arch/x86/kvm/smm.c | 17 +++++++++++++++++
> arch/x86/kvm/smm.h |  2 +-
> arch/x86/kvm/x86.c | 12 +++++++++++-
> 3 files changed, 29 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
>index b42111a24cc2..a4e19d72224f 100644
>--- a/arch/x86/kvm/smm.c
>+++ b/arch/x86/kvm/smm.c
>@@ -309,6 +309,15 @@ void enter_smm(struct kvm_vcpu *vcpu)
> 
> 	kvm_smm_changed(vcpu, true);
> 
>+#ifdef CONFIG_X86_64
>+	if (guest_can_use(vcpu, X86_FEATURE_SHSTK)) {
>+		u64 data;
>+
>+		if (!kvm_get_msr(vcpu, MSR_KVM_GUEST_SSP, &data))
>+			smram.smram64.ssp = data;

I don't think it is correct to continue if kvm fails to read the MSR.

how about:
		if (kvm_get_msr(vcpu, MSR_KVM_GUEST_SSP, &smram.smram64.ssp))
			goto error;
>+	}
>+#endif
>+
> 	if (kvm_vcpu_write_guest(vcpu, vcpu->arch.smbase + 0xfe00, &smram, sizeof(smram)))
> 		goto error;
> 
>@@ -586,6 +595,14 @@ int emulator_leave_smm(struct x86_emulate_ctxt *ctxt)
> 	if ((vcpu->arch.hflags & HF_SMM_INSIDE_NMI_MASK) == 0)
> 		static_call(kvm_x86_set_nmi_mask)(vcpu, false);
> 
>+#ifdef CONFIG_X86_64
>+	if (guest_can_use(vcpu, X86_FEATURE_SHSTK)) {
>+		u64 data = smram.smram64.ssp;
>+

>+		if (is_noncanonical_address(data, vcpu) && IS_ALIGNED(data, 4))

shouldn't the checks be already done inside kvm_set_msr()?

>+			kvm_set_msr(vcpu, MSR_KVM_GUEST_SSP, data);

please handle the failure case. Probably just return X86EMUL_UNHANDLEABLE like other
failure paths in this function.

>+	}
>+#endif
> 	kvm_smm_changed(vcpu, false);
> 
> 	/*
>diff --git a/arch/x86/kvm/smm.h b/arch/x86/kvm/smm.h
>index a1cf2ac5bd78..b3efef7cb1dc 100644
>--- a/arch/x86/kvm/smm.h
>+++ b/arch/x86/kvm/smm.h
>@@ -116,7 +116,7 @@ struct kvm_smram_state_64 {
> 	u32 smbase;
> 	u32 reserved4[5];
> 
>-	/* ssp and svm_* fields below are not implemented by KVM */
>+	/* svm_* fields below are not implemented by KVM */

move this comment one line downward

> 	u64 ssp;
> 	u64 svm_guest_pat;
> 	u64 svm_host_efer;
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index f7558f0f6fc0..70d7c80889d6 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -3653,8 +3653,18 @@ static bool kvm_cet_is_msr_accessible(struct kvm_vcpu *vcpu,
> 		if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK))
> 			return false;
> 
>-		if (msr->index == MSR_KVM_GUEST_SSP)
>+		/*
>+		 * This MSR is synthesized mainly for userspace access during
>+		 * Live Migration, it also can be accessed in SMM mode by VMM.
>+		 * Guest is not allowed to access this MSR.
>+		 */
>+		if (msr->index == MSR_KVM_GUEST_SSP) {
>+			if (IS_ENABLED(CONFIG_X86_64) &&
>+			    !!(vcpu->arch.hflags & HF_SMM_MASK))

use is_smm() instead.

>+				return true;
>+
> 			return msr->host_initiated;
>+		}
> 
> 		return msr->host_initiated ||
> 			guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>-- 
>2.27.0
>