diff mbox series

[RFC,v2,01/26] x86/cpufeatures: Add SGX1 and SGX2 sub-features

Message ID 87385f646120a3b5b34dc20480dbce77b8005acd.1610935432.git.kai.huang@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM SGX virtualization support | expand

Commit Message

Kai Huang Jan. 18, 2021, 3:26 a.m. UTC 
From: Sean Christopherson <seanjc@google.com>

Add SGX1 and SGX2 feature flags, via CPUID.0x12.0x0.EAX, as scattered
features.  As part of virtualizing SGX, KVM will expose the SGX CPUID
leafs to its guest, and to do so correctly needs to query hardware and
kernel support for SGX1 and SGX2.

Also add SGX related feature bits to CPUID dependency table to make
clearing SGX feature easier.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Co-developed-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 2 ++
 arch/x86/kernel/cpu/cpuid-deps.c   | 3 +++
 arch/x86/kernel/cpu/feat_ctl.c     | 1 -
 arch/x86/kernel/cpu/scattered.c    | 2 ++
 4 files changed, 7 insertions(+), 1 deletion(-)

Comments

Borislav Petkov Jan. 19, 2021, 4:19 p.m. UTC | #1 
On Mon, Jan 18, 2021 at 04:26:49PM +1300, Kai Huang wrote:
> diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
> index 3b1b01f2b248..7937a315f8cf 100644
> --- a/arch/x86/kernel/cpu/feat_ctl.c
> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> @@ -96,7 +96,6 @@ static void init_vmx_capabilities(struct cpuinfo_x86 *c)
>  static void clear_sgx_caps(void)
>  {
>  	setup_clear_cpu_cap(X86_FEATURE_SGX);
> -	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);

Why is that line being removed here?

Shouldn't this add SGX1 and SGX2 here instead as this function is
supposed to, well, *clear* sgx caps on feat_ctl setup failures or
"nosgx" cmdline?
Sean Christopherson Jan. 19, 2021, 6:03 p.m. UTC | #2 
On Tue, Jan 19, 2021, Borislav Petkov wrote:
> On Mon, Jan 18, 2021 at 04:26:49PM +1300, Kai Huang wrote:
> > diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
> > index 3b1b01f2b248..7937a315f8cf 100644
> > --- a/arch/x86/kernel/cpu/feat_ctl.c
> > +++ b/arch/x86/kernel/cpu/feat_ctl.c
> > @@ -96,7 +96,6 @@ static void init_vmx_capabilities(struct cpuinfo_x86 *c)
> >  static void clear_sgx_caps(void)
> >  {
> >  	setup_clear_cpu_cap(X86_FEATURE_SGX);
> > -	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
> 
> Why is that line being removed here?
> 
> Shouldn't this add SGX1 and SGX2 here instead as this function is
> supposed to, well, *clear* sgx caps on feat_ctl setup failures or
> "nosgx" cmdline?

Doesn't adding making the SGX sub-features depend on X86_FEATURE_SGX have the
same net effect?  Or am I misreading do_clear_cpu_cap()?

Though if we use the cpuid_deps table, I'd vote to get rid of clear_sgx_caps()
and call setup_clear_cpu_cap(X86_FEATURE_SGX) directly.  And probably change the
existing SGX_LC behavior and drop clear_sgx_caps() in a separate patch instead
of squeezing it into this one.
Kai Huang Jan. 19, 2021, 10:54 p.m. UTC | #3 
On Tue, 2021-01-19 at 10:03 -0800, Sean Christopherson wrote:
> On Tue, Jan 19, 2021, Borislav Petkov wrote:
> > On Mon, Jan 18, 2021 at 04:26:49PM +1300, Kai Huang wrote:
> > > diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
> > > index 3b1b01f2b248..7937a315f8cf 100644
> > > --- a/arch/x86/kernel/cpu/feat_ctl.c
> > > +++ b/arch/x86/kernel/cpu/feat_ctl.c
> > > @@ -96,7 +96,6 @@ static void init_vmx_capabilities(struct cpuinfo_x86 *c)
> > >  static void clear_sgx_caps(void)
> > >  {
> > >  	setup_clear_cpu_cap(X86_FEATURE_SGX);
> > > -	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
> > 
> > Why is that line being removed here?
> > 
> > Shouldn't this add SGX1 and SGX2 here instead as this function is
> > supposed to, well, *clear* sgx caps on feat_ctl setup failures or
> > "nosgx" cmdline?
> 
> Doesn't adding making the SGX sub-features depend on X86_FEATURE_SGX have the
> same net effect?  Or am I misreading do_clear_cpu_cap()?

On my testing machine with SGX, SGX_LC, and SGX1. I just double tested that clearing
X86_FEATURE_SGX also clears SGX1 and SGX_LC bits.

> 
> Though if we use the cpuid_deps table, I'd vote to get rid of clear_sgx_caps()
> and call setup_clear_cpu_cap(X86_FEATURE_SGX) directly. 
> 

Yes I can do that.

>  And probably change the
> existing SGX_LC behavior and drop clear_sgx_caps() in a separate patch instead
> of squeezing it into this one.

To double confirm, you want:

1) This patch to introduce SGX1, SGX2, and also put SGX1 and SGX2 in to CPUID
dependency table; 
2) A separate patch to put SGX_LC to CPUID dependency table too, and also git rid of
clear_sgx_caps()

Correct?

Btw, in your original patch, both SGX1 and SGX2 depends on SGX, but I changed to make
SGX2 depend on SGX1. However I still make SGX_LC depend on SGX, but not SGX1. Does
this make sense to you?
Borislav Petkov Jan. 20, 2021, 10:28 a.m. UTC | #4 
On Tue, Jan 19, 2021 at 10:03:14AM -0800, Sean Christopherson wrote:
> Doesn't adding making the SGX sub-features depend on X86_FEATURE_SGX have the
> same net effect?  Or am I misreading do_clear_cpu_cap()?

No, you're correct - I missed that.

> Though if we use the cpuid_deps table, I'd vote to get rid of clear_sgx_caps()
> and call setup_clear_cpu_cap(X86_FEATURE_SGX) directly.  And probably change the
> existing SGX_LC behavior and drop clear_sgx_caps() in a separate patch instead
> of squeezing it into this one.

Yah, sounds good.
Jarkko Sakkinen Jan. 20, 2021, 11:50 a.m. UTC | #5 
On Mon, Jan 18, 2021 at 04:26:49PM +1300, Kai Huang wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Add SGX1 and SGX2 feature flags, via CPUID.0x12.0x0.EAX, as scattered
> features.  As part of virtualizing SGX, KVM will expose the SGX CPUID
> leafs to its guest, and to do so correctly needs to query hardware and
> kernel support for SGX1 and SGX2.

This commit message is missing reasoning behind scattered vs. own word.

Please just document the reasoning, that's all.

/Jarkko
Kai Huang Jan. 20, 2021, 11:23 p.m. UTC | #6 
On Wed, 20 Jan 2021 13:50:12 +0200 Jarkko Sakkinen wrote:
> On Mon, Jan 18, 2021 at 04:26:49PM +1300, Kai Huang wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > 
> > Add SGX1 and SGX2 feature flags, via CPUID.0x12.0x0.EAX, as scattered
> > features.  As part of virtualizing SGX, KVM will expose the SGX CPUID
> > leafs to its guest, and to do so correctly needs to query hardware and
> > kernel support for SGX1 and SGX2.
> 
> This commit message is missing reasoning behind scattered vs. own word.
> 
> Please just document the reasoning, that's all.

OK. Will do. How about:

"Add SGX1 and SGX2 feature flags, via CPUID.0x12.0x0.EAX, as scattered
features, since adding a new leaf for only two bits would be wasteful."

?

> 
> /Jarkko
Jarkko Sakkinen Jan. 21, 2021, 1:01 a.m. UTC | #7 
On Thu, Jan 21, 2021 at 12:23:08PM +1300, Kai Huang wrote:
> On Wed, 20 Jan 2021 13:50:12 +0200 Jarkko Sakkinen wrote:
> > On Mon, Jan 18, 2021 at 04:26:49PM +1300, Kai Huang wrote:
> > > From: Sean Christopherson <seanjc@google.com>
> > > 
> > > Add SGX1 and SGX2 feature flags, via CPUID.0x12.0x0.EAX, as scattered
> > > features.  As part of virtualizing SGX, KVM will expose the SGX CPUID
> > > leafs to its guest, and to do so correctly needs to query hardware and
> > > kernel support for SGX1 and SGX2.
> > 
> > This commit message is missing reasoning behind scattered vs. own word.
> > 
> > Please just document the reasoning, that's all.
> 
> OK. Will do. How about:
> 
> "Add SGX1 and SGX2 feature flags, via CPUID.0x12.0x0.EAX, as scattered
> features, since adding a new leaf for only two bits would be wasteful."

For *me*, that would be sufficient.

/Jarkko
diff mbox series

Patch

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 84b887825f12..18b2d0c8bbbe 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -292,6 +292,8 @@ 
 #define X86_FEATURE_FENCE_SWAPGS_KERNEL	(11*32+ 5) /* "" LFENCE in kernel entry SWAPGS path */
 #define X86_FEATURE_SPLIT_LOCK_DETECT	(11*32+ 6) /* #AC for split lock */
 #define X86_FEATURE_PER_THREAD_MBA	(11*32+ 7) /* "" Per-thread Memory Bandwidth Allocation */
+#define X86_FEATURE_SGX1		(11*32+ 8) /* Software Guard Extensions sub-feature SGX1 */
+#define X86_FEATURE_SGX2        	(11*32+ 9) /* Software Guard Extensions sub-feature SGX2 */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
 #define X86_FEATURE_AVX512_BF16		(12*32+ 5) /* AVX512 BFLOAT16 instructions */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 42af31b64c2c..7d341bfe7f57 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -72,6 +72,9 @@  static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_AVX512_FP16,		X86_FEATURE_AVX512BW  },
 	{ X86_FEATURE_ENQCMD,			X86_FEATURE_XSAVES    },
 	{ X86_FEATURE_PER_THREAD_MBA,		X86_FEATURE_MBA       },
+	{ X86_FEATURE_SGX_LC,			X86_FEATURE_SGX       },
+	{ X86_FEATURE_SGX1,			X86_FEATURE_SGX       },
+	{ X86_FEATURE_SGX2,			X86_FEATURE_SGX1      },
 	{}
 };
 
diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
index 3b1b01f2b248..7937a315f8cf 100644
--- a/arch/x86/kernel/cpu/feat_ctl.c
+++ b/arch/x86/kernel/cpu/feat_ctl.c
@@ -96,7 +96,6 @@  static void init_vmx_capabilities(struct cpuinfo_x86 *c)
 static void clear_sgx_caps(void)
 {
 	setup_clear_cpu_cap(X86_FEATURE_SGX);
-	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
 }
 
 static int __init nosgx(char *str)
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 236924930bf0..fea0df867d18 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -36,6 +36,8 @@  static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_CDP_L2,		CPUID_ECX,  2, 0x00000010, 2 },
 	{ X86_FEATURE_MBA,		CPUID_EBX,  3, 0x00000010, 0 },
 	{ X86_FEATURE_PER_THREAD_MBA,	CPUID_ECX,  0, 0x00000010, 3 },
+	{ X86_FEATURE_SGX1,		CPUID_EAX,  0, 0x00000012, 0 },
+	{ X86_FEATURE_SGX2,		CPUID_EAX,  1, 0x00000012, 0 },
 	{ X86_FEATURE_HW_PSTATE,	CPUID_EDX,  7, 0x80000007, 0 },
 	{ X86_FEATURE_CPB,		CPUID_EDX,  9, 0x80000007, 0 },
 	{ X86_FEATURE_PROC_FEEDBACK,    CPUID_EDX, 11, 0x80000007, 0 },