Message ID | 20230914063325.85503-2-weijiang.yang@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp239946vqi; Thu, 14 Sep 2023 03:12:16 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE2GZQf66XHfTWtg6JjIsIwueI/cYFMyjd5SMBAnxQb418GmpQWwMLXJp3fL8YOIhHM37SV X-Received: by 2002:a05:6a00:1805:b0:68f:dfda:1814 with SMTP id y5-20020a056a00180500b0068fdfda1814mr5700564pfa.18.1694686335889; Thu, 14 Sep 2023 03:12:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1694686335; cv=none; d=google.com; s=arc-20160816; b=RbF3f5l2rU/Fhswrys9WEEkDlTIYCJ+rPECH4v2cEAOeKB7sLqkddK7PEMC5dXGQ4k i75htRz6KlH+Jy/acilVGqm96ctLV87iBXiWZU+zUUMmRpvFsgTEbLDE/kyRuo2b7tTU p9tCh6K38mGTuOOEE+FdyFVXxE/sQMAdi76ZSdWGle1zC0EjHQXXgX2DTYuQ9fgCj4xF P/hzSiJyqBylYDNIa9QDgQ1mgY/ZP3G8jj+rxmbLtUu8/gA0COJqx0AIuvXJLNNQraHP 4AaLN1yr3zOBA968k5u31Z4bn01H+zj+GNH7pymw7LpD0keTlwkZLjiFeuxVOE7VGdwk X/Cg== 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 :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=xVABnXeOkMZlJo+ZffrqSfE/4k2ea3Z1/0rzSIi4N90=; fh=jQqUhNQZPAXcZ44u72Wu3jv2pQizzn2Be2T8mkd1RwU=; b=CbGb9d2UrNldRdE+IXB7YZ8bMKkWSSgpQ3vDworxfTO4T5xOLeQIBd8Ad0w91BtgeK O5zsUFB+g7RmPe6tbS0BJgcEXBVkjaaZro0icxnK1/Rax+AvOHLMZ6FZI86C2LjOFNMf f3EiKRjY3Nau0japLu3Sy4mm40CZH1Xxfn888yacXSQi6r3O02avTb9ClOfrxLYhZsLq W4LbluNcazyobS9IAJ8iNQ/9Q0Ez2ibTYSpKNAxWRC2jRDHODqqzTDNWGDgKPaqcLiJC WsHVX7egGLtZ01ziET+hdykdCagjwdGLhdU6/RqB2+JWdjOdaI5c3YuwwgtJHSQZ390/ 97qQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=FHzUbTRa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from howler.vger.email (howler.vger.email. [2620:137:e000::3:4]) by mx.google.com with ESMTPS id e15-20020a6558cf000000b0056531bfc660si1256199pgu.143.2023.09.14.03.12.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Sep 2023 03:12:15 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=FHzUbTRa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id CFF9284C41EB; Thu, 14 Sep 2023 02:38:29 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237305AbjINJiY (ORCPT <rfc822;chrisfriedt@gmail.com> + 35 others); Thu, 14 Sep 2023 05:38:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40060 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235825AbjINJiU (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 14 Sep 2023 05:38:20 -0400 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.88]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 14FDE1BEF; Thu, 14 Sep 2023 02:38:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1694684296; x=1726220296; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=0H4/DsmRdq6ZAqAGPRiCyRQ3eI7Btuj1gad9cmvRYIo=; b=FHzUbTRatf9MQriLjNnfG0O8qD+rH/kpYfWsRys1cg5LkHPeJ1fmN5N/ f+1j/bWGSvw1vTuBo+vSe8MWPY0cy0UcdaD3HPEUaEqCCFRPTWwXgQplB FNpCa3Q/mKeYEmBYeYdwktRIoes82ZHExpNAJg+gmLGL/Jn/ivI5TVV35 M/fp7egW/8zwrLoX5cVe4FG9zHkvy+h+uByaREa8LQqFBtWNKfjNLzHMM saGuaf5VUwZ3K/tCAXsaiM1+JIXLE9QhRF8Pzu973JJh7lBqEIyw59OfK QrNUsEcCGwIub7ihsD90eaLuEznhLhJWc9bWLl6EBzzq7uh+JZIdyvV9s w==; X-IronPort-AV: E=McAfee;i="6600,9927,10832"; a="409857303" X-IronPort-AV: E=Sophos;i="6.02,145,1688454000"; d="scan'208";a="409857303" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2023 02:38:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10832"; a="747656209" X-IronPort-AV: E=Sophos;i="6.02,145,1688454000"; d="scan'208";a="747656209" Received: from embargo.jf.intel.com ([10.165.9.183]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2023 02:38:15 -0700 From: Yang Weijiang <weijiang.yang@intel.com> To: seanjc@google.com, pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Cc: dave.hansen@intel.com, peterz@infradead.org, chao.gao@intel.com, rick.p.edgecombe@intel.com, weijiang.yang@intel.com, john.allen@amd.com Subject: [PATCH v6 01/25] x86/fpu/xstate: Manually check and add XFEATURE_CET_USER xstate bit Date: Thu, 14 Sep 2023 02:33:01 -0400 Message-Id: <20230914063325.85503-2-weijiang.yang@intel.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20230914063325.85503-1-weijiang.yang@intel.com> References: <20230914063325.85503-1-weijiang.yang@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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 (howler.vger.email [0.0.0.0]); Thu, 14 Sep 2023 02:38:29 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777007419173748827 X-GMAIL-MSGID: 1777007419173748827 |
Series |
Enable CET Virtualization
|
|
Commit Message
Yang, Weijiang
Sept. 14, 2023, 6:33 a.m. UTC
Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't
reflect true dependency between CET features and the xstate bit, instead
manually check and add the bit back if either SHSTK or IBT is supported.
Both user mode shadow stack and indirect branch tracking features depend
on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode
xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary.
Although in real world a platform with IBT but no SHSTK is rare, but in
virtualization world it's common, guest SHSTK and IBT can be controlled
independently via userspace app.
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
arch/x86/kernel/fpu/xstate.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
Comments
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > Remove XFEATURE_CET_USER entry from dependency array as the entry > doesn't > reflect true dependency between CET features and the xstate bit, > instead > manually check and add the bit back if either SHSTK or IBT is > supported. > > Both user mode shadow stack and indirect branch tracking features > depend > on XFEATURE_CET_USER bit in XSS to automatically save/restore user > mode > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever > necessary. > > Although in real world a platform with IBT but no SHSTK is rare, but > in > virtualization world it's common, guest SHSTK and IBT can be > controlled > independently via userspace app. Nit, not sure we can assert it's common yet. It's true in general that guests can have CPUID combinations that don't appear in real world of course. Is that what you meant? Also, this doesn't discuss the real main reason for this patch, and that is that KVM will soon use the xfeature for user ibt, and so there will now be a reason to have XFEATURE_CET_USER depend on IBT. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> Otherwise: Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
On 9/15/2023 6:39 AM, Edgecombe, Rick P wrote: > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: >> Remove XFEATURE_CET_USER entry from dependency array as the entry >> doesn't >> reflect true dependency between CET features and the xstate bit, >> instead >> manually check and add the bit back if either SHSTK or IBT is >> supported. >> >> Both user mode shadow stack and indirect branch tracking features >> depend >> on XFEATURE_CET_USER bit in XSS to automatically save/restore user >> mode >> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever >> necessary. >> >> Although in real world a platform with IBT but no SHSTK is rare, but >> in >> virtualization world it's common, guest SHSTK and IBT can be >> controlled >> independently via userspace app. > Nit, not sure we can assert it's common yet. It's true in general that > guests can have CPUID combinations that don't appear in real world of > course. Is that what you meant? Yes, guest CPUID features can be configured by userspace flexibly. > > Also, this doesn't discuss the real main reason for this patch, and > that is that KVM will soon use the xfeature for user ibt, and so there > will now be a reason to have XFEATURE_CET_USER depend on IBT. This is one justification for Linux OS, another reason is there's non-Linux OS which is using the user IBT feature. I should make the reasons clearer in changelog, thanks for pointing it out! >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > Otherwise: > > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > Tested-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
On Fri, 2023-09-15 at 10:32 +0800, Yang, Weijiang wrote: > > > > Also, this doesn't discuss the real main reason for this patch, and > > that is that KVM will soon use the xfeature for user ibt, and so > > there > > will now be a reason to have XFEATURE_CET_USER depend on IBT. > > This is one justification for Linux OS, another reason is there's > non-Linux > OS which is using the user IBT feature. I should make the reasons > clearer > in changelog, thanks for pointing it out! The point I was trying to make was today (before this series) nothing on the system can use user IBT. Not the host, and not in any guest because KVM doesn't support it. So the added xfeature dependency on IBT was not previously needed. It is being added only for KVM CET support (which, yes, may run on guests with non-standard CPUID).
On 9/16/2023 12:35 AM, Edgecombe, Rick P wrote: > On Fri, 2023-09-15 at 10:32 +0800, Yang, Weijiang wrote: >>> Also, this doesn't discuss the real main reason for this patch, and >>> that is that KVM will soon use the xfeature for user ibt, and so >>> there >>> will now be a reason to have XFEATURE_CET_USER depend on IBT. >> This is one justification for Linux OS, another reason is there's >> non-Linux >> OS which is using the user IBT feature. I should make the reasons >> clearer >> in changelog, thanks for pointing it out! > The point I was trying to make was today (before this series) nothing > on the system can use user IBT. Not the host, and not in any guest > because KVM doesn't support it. So the added xfeature dependency on IBT > was not previously needed. It is being added only for KVM CET support > (which, yes, may run on guests with non-standard CPUID). Agree, I'll highlight this in changelog, thanks!
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: > Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't > reflect true dependency between CET features and the xstate bit, instead > manually check and add the bit back if either SHSTK or IBT is supported. > > Both user mode shadow stack and indirect branch tracking features depend > on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode > xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. > > Although in real world a platform with IBT but no SHSTK is rare, but in > virtualization world it's common, guest SHSTK and IBT can be controlled > independently via userspace app. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kernel/fpu/xstate.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index cadf68737e6b..12c8cb278346 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { > [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, > [XFEATURE_PKRU] = X86_FEATURE_OSPKE, > [XFEATURE_PASID] = X86_FEATURE_ENQCMD, > - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, > [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, > [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, > }; > @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) > fpu_kernel_cfg.max_features &= ~BIT_ULL(i); > } > > + /* > + * Manually add CET user mode xstate bit if either SHSTK or IBT is > + * available. Both features depend on the xstate bit to save/restore > + * CET user mode state. > + */ > + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) > + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); > + > if (!cpu_feature_enabled(X86_FEATURE_XFD)) > fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; > The goal of the xsave_cpuid_features is to disable xfeature state bits which are enabled in CPUID, but their parent feature bit (e.g X86_FEATURE_AVX512) is disabled in CPUID, something that should not happen on real CPU, but can happen if the user explicitly disables the feature on the kernel command line and/or due to virtualization. However the above code does the opposite, it will enable XFEATURE_CET_USER xsaves component, when in fact, it might be disabled in the CPUID (and one can say that in theory such configuration is even useful, since the kernel can still context switch CET msrs manually). So I think that the code should do this instead: if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); Best regards, Maxim Levitsky
On 11/1/2023 1:43 AM, Maxim Levitsky wrote: > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote: >> Remove XFEATURE_CET_USER entry from dependency array as the entry doesn't >> reflect true dependency between CET features and the xstate bit, instead >> manually check and add the bit back if either SHSTK or IBT is supported. >> >> Both user mode shadow stack and indirect branch tracking features depend >> on XFEATURE_CET_USER bit in XSS to automatically save/restore user mode >> xstate registers, i.e., IA32_U_CET and IA32_PL3_SSP whenever necessary. >> >> Although in real world a platform with IBT but no SHSTK is rare, but in >> virtualization world it's common, guest SHSTK and IBT can be controlled >> independently via userspace app. >> >> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> >> --- >> arch/x86/kernel/fpu/xstate.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c >> index cadf68737e6b..12c8cb278346 100644 >> --- a/arch/x86/kernel/fpu/xstate.c >> +++ b/arch/x86/kernel/fpu/xstate.c >> @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { >> [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, >> [XFEATURE_PKRU] = X86_FEATURE_OSPKE, >> [XFEATURE_PASID] = X86_FEATURE_ENQCMD, >> - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, >> [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, >> [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, >> }; >> @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) >> fpu_kernel_cfg.max_features &= ~BIT_ULL(i); >> } >> >> + /* >> + * Manually add CET user mode xstate bit if either SHSTK or IBT is >> + * available. Both features depend on the xstate bit to save/restore >> + * CET user mode state. >> + */ >> + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) >> + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); >> + >> if (!cpu_feature_enabled(X86_FEATURE_XFD)) >> fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC; >> > > The goal of the xsave_cpuid_features is to disable xfeature state bits which are enabled > in CPUID, but their parent feature bit (e.g X86_FEATURE_AVX512) is disabled in CPUID, > something that should not happen on real CPU, but can happen if the user explicitly > disables the feature on the kernel command line and/or due to virtualization. > > However the above code does the opposite, it will enable XFEATURE_CET_USER xsaves component, > when in fact, it might be disabled in the CPUID (and one can say that in theory such > configuration is even useful, since the kernel can still context switch CET msrs manually). > > > So I think that the code should do this instead: > > if (!boot_cpu_has(X86_FEATURE_SHSTK) && !boot_cpu_has(X86_FEATURE_IBT)) > fpu_kernel_cfg.max_features &= ~BIT_ULL(XFEATURE_CET_USER); Hi, Maxim, Thanks a lot for the comments on the series! I'll will check and reply them after finish an urgent task at hand. Yeah, it looks good to me and makes the handling logic more consistent! > Best regards, > Maxim Levitsky > > > >
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index cadf68737e6b..12c8cb278346 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -73,7 +73,6 @@ static unsigned short xsave_cpuid_features[] __initdata = { [XFEATURE_PT_UNIMPLEMENTED_SO_FAR] = X86_FEATURE_INTEL_PT, [XFEATURE_PKRU] = X86_FEATURE_OSPKE, [XFEATURE_PASID] = X86_FEATURE_ENQCMD, - [XFEATURE_CET_USER] = X86_FEATURE_SHSTK, [XFEATURE_XTILE_CFG] = X86_FEATURE_AMX_TILE, [XFEATURE_XTILE_DATA] = X86_FEATURE_AMX_TILE, }; @@ -798,6 +797,14 @@ void __init fpu__init_system_xstate(unsigned int legacy_size) fpu_kernel_cfg.max_features &= ~BIT_ULL(i); } + /* + * Manually add CET user mode xstate bit if either SHSTK or IBT is + * available. Both features depend on the xstate bit to save/restore + * CET user mode state. + */ + if (boot_cpu_has(X86_FEATURE_SHSTK) || boot_cpu_has(X86_FEATURE_IBT)) + fpu_kernel_cfg.max_features |= BIT_ULL(XFEATURE_CET_USER); + if (!cpu_feature_enabled(X86_FEATURE_XFD)) fpu_kernel_cfg.max_features &= ~XFEATURE_MASK_USER_DYNAMIC;