diff mbox series

[02/25] x86/cpufeatures: Add SGX1 and SGX2 sub-features

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

Commit Message

Kai Huang March 1, 2021, 9:44 a.m. UTC
From: Sean Christopherson <seanjc@google.com>

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.
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.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Acked-by: Dave Hansen <dave.hansen@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   | 2 ++
 arch/x86/kernel/cpu/scattered.c    | 2 ++
 3 files changed, 6 insertions(+)

Comments

Borislav Petkov March 1, 2021, 10 a.m. UTC | #1
On Mon, Mar 01, 2021 at 10:44:29PM +1300, Kai Huang wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> 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.
> 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.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Acked-by: Dave Hansen <dave.hansen@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   | 2 ++
>  arch/x86/kernel/cpu/scattered.c    | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index cc96e26d69f7..9502c445a3e9 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -290,6 +290,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) /* "" Basic SGX */
> +#define X86_FEATURE_SGX2        	(11*32+ 9) /* SGX Enclave Dynamic Memory Management (EDMM) */

"sgx1" is not gonna show in /proc/cpuinfo but "sgx2" will. Because...?

Also, you send a patchset once a week - not after two days. Please limit
your spamming.
Kai Huang March 1, 2021, 10:19 a.m. UTC | #2
On Mon, 2021-03-01 at 11:00 +0100, Borislav Petkov wrote:
> On Mon, Mar 01, 2021 at 10:44:29PM +1300, Kai Huang wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > 
> > 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.
> > 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.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > Acked-by: Dave Hansen <dave.hansen@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   | 2 ++
> >  arch/x86/kernel/cpu/scattered.c    | 2 ++
> >  3 files changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> > index cc96e26d69f7..9502c445a3e9 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -290,6 +290,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) /* "" Basic SGX */
> > +#define X86_FEATURE_SGX2        	(11*32+ 9) /* SGX Enclave Dynamic Memory Management (EDMM) */
> 
> "sgx1" is not gonna show in /proc/cpuinfo but "sgx2" will. Because...?

There's already X86_FEATURE_SGX, which shows "sgx" in /proc/cpuinfo. Showing "sgx1"
doesn't add anything. "sgx2" is useful because it adds additional functionality.

Both Sean and Paolo suggested this. Please see below discussion:

https://www.spinics.net/lists/kvm/msg234533.html

> 
> Also, you send a patchset once a week - not after two days. Please limit
> your spamming.
> 

OK. Thanks for reminding.
Borislav Petkov March 1, 2021, 10:30 a.m. UTC | #3
On Mon, Mar 01, 2021 at 11:19:15PM +1300, Kai Huang wrote:
> "sgx2" is useful because it adds additional functionality.

Useful for what?

People have got to start explaining "why" something is useful and put
that "why" in the commit message.
Kai Huang March 1, 2021, 10:40 a.m. UTC | #4
On Mon, 2021-03-01 at 11:30 +0100, Borislav Petkov wrote:
> On Mon, Mar 01, 2021 at 11:19:15PM +1300, Kai Huang wrote:
> > "sgx2" is useful because it adds additional functionality.
> 
> Useful for what?

SGX2 means "Enclave Dynamic Memory Management", which supports dynamically
adding/removing EPC pages, plus changing page permission, after enclave is
initialized. So it allows enclave author to write enclave in more flexible way, and
also utilize EPC resource more efficiently.

> 
> People have got to start explaining "why" something is useful and put
> that "why" in the commit message.
> 

Yes I can add "why" into commit message, but isn't the comment after X86_FEATURE_SGX2
enough? I think people who are interested in SGX will know what SGX2 is and why SGX2
is useful.
Borislav Petkov March 1, 2021, 10:53 a.m. UTC | #5
On Mon, Mar 01, 2021 at 11:40:02PM +1300, Kai Huang wrote:
> SGX2 means "Enclave Dynamic Memory Management", which supports dynamically
> adding/removing EPC pages, plus changing page permission, after enclave is
> initialized. So it allows enclave author to write enclave in more flexible way, and
> also utilize EPC resource more efficiently.

So how is the enclave author going to use "sgx2" in /proc/cpuinfo? Query
it to know whether it can start adding/removing EPC pages or is this
going to be used by scripts?

> Yes I can add "why" into commit message, but isn't the comment after X86_FEATURE_SGX2
> enough? I think people who are interested in SGX will know what SGX2 is and why SGX2
> is useful.

The point is, the commit message should say how this flag in
/proc/cpuinfo is going to be used - not what people interested in sgx
might and might not know.

/proc/cpuinfo is user-visible ABI - you can't add stuff to it just because.
Kai Huang March 1, 2021, 11:28 a.m. UTC | #6
On Mon, 2021-03-01 at 11:53 +0100, Borislav Petkov wrote:
> On Mon, Mar 01, 2021 at 11:40:02PM +1300, Kai Huang wrote:
> > SGX2 means "Enclave Dynamic Memory Management", which supports dynamically
> > adding/removing EPC pages, plus changing page permission, after enclave is
> > initialized. So it allows enclave author to write enclave in more flexible way, and
> > also utilize EPC resource more efficiently.
> 
> So how is the enclave author going to use "sgx2" in /proc/cpuinfo? Query
> it to know whether it can start adding/removing EPC pages or is this
> going to be used by scripts?

I think some script can utilize /proc/cpuinfo. For instance, admin can have
automation tool/script to deploy enclave (with sgx2) apps, and that script can check
whether platform supports sgx2 or not, before it can deploy those enclave apps. Or
enclave author may just want to check /proc/cpuinfo to know whether the machine can
be used for testing sgx2 enclave or not.

However this is just my 2 cents.

> 
> > Yes I can add "why" into commit message, but isn't the comment after X86_FEATURE_SGX2
> > enough? I think people who are interested in SGX will know what SGX2 is and why SGX2
> > is useful.
> 
> The point is, the commit message should say how this flag in
> /proc/cpuinfo is going to be used - not what people interested in sgx
> might and might not know.
> 
> /proc/cpuinfo is user-visible ABI - you can't add stuff to it just because.
> 

Thanks for the explaining. Will do.
Borislav Petkov March 1, 2021, 11:32 a.m. UTC | #7
On Tue, Mar 02, 2021 at 12:28:27AM +1300, Kai Huang wrote:
> I think some script can utilize /proc/cpuinfo. For instance, admin can have
> automation tool/script to deploy enclave (with sgx2) apps, and that script can check
> whether platform supports sgx2 or not, before it can deploy those enclave apps. Or
> enclave author may just want to check /proc/cpuinfo to know whether the machine can
> be used for testing sgx2 enclave or not.

This doesn't sound like a concrete use of this. So you can hide it
initially with "" until you guys have a use case. Exposing it later is
always easy vs exposing it now and then not being able to change it
anymore.
Kai Huang March 1, 2021, 11:43 a.m. UTC | #8
On Mon, 2021-03-01 at 12:32 +0100, Borislav Petkov wrote:
> On Tue, Mar 02, 2021 at 12:28:27AM +1300, Kai Huang wrote:
> > I think some script can utilize /proc/cpuinfo. For instance, admin can have
> > automation tool/script to deploy enclave (with sgx2) apps, and that script can check
> > whether platform supports sgx2 or not, before it can deploy those enclave apps. Or
> > enclave author may just want to check /proc/cpuinfo to know whether the machine can
> > be used for testing sgx2 enclave or not.
> 
> This doesn't sound like a concrete use of this. So you can hide it
> initially with "" until you guys have a use case. Exposing it later is
> always easy vs exposing it now and then not being able to change it
> anymore.
> 

Hi Haitao, Jarkko,

Do you have more concrete use case of needing "sgx2" in /proc/cpuinfo?

Hi Boris,

To confirm, if we suppress both "sgx1" and "sgx2" in /proc/cpuinfo, we don't need to
add "why to suppress" in commit message, right?
Borislav Petkov March 1, 2021, 11:58 a.m. UTC | #9
On Tue, Mar 02, 2021 at 12:43:06AM +1300, Kai Huang wrote:
> To confirm, if we suppress both "sgx1" and "sgx2" in /proc/cpuinfo, we
> don't need to add "why to suppress" in commit message, right?

You should always explain in a patch why you're doing the change. So
that a reviewer knows. And then people in the future can follow why
you've made that decision. Always.
Kai Huang March 1, 2021, 12:08 p.m. UTC | #10
On Mon, 2021-03-01 at 12:58 +0100, Borislav Petkov wrote:
> On Tue, Mar 02, 2021 at 12:43:06AM +1300, Kai Huang wrote:
> > To confirm, if we suppress both "sgx1" and "sgx2" in /proc/cpuinfo, we
> > don't need to add "why to suppress" in commit message, right?
> 
> You should always explain in a patch why you're doing the change. So
> that a reviewer knows. And then people in the future can follow why
> you've made that decision. Always.

Got it. Thanks.
Haitao Huang March 2, 2021, 3:48 p.m. UTC | #11
On Mon, 01 Mar 2021 05:43:06 -0600, Kai Huang <kai.huang@intel.com> wrote:

> On Mon, 2021-03-01 at 12:32 +0100, Borislav Petkov wrote:
>> On Tue, Mar 02, 2021 at 12:28:27AM +1300, Kai Huang wrote:
>> > I think some script can utilize /proc/cpuinfo. For instance, admin  
>> can have
>> > automation tool/script to deploy enclave (with sgx2) apps, and that  
>> script can check
>> > whether platform supports sgx2 or not, before it can deploy those  
>> enclave apps. Or
>> > enclave author may just want to check /proc/cpuinfo to know whether  
>> the machine can
>> > be used for testing sgx2 enclave or not.
>>
>> This doesn't sound like a concrete use of this. So you can hide it
>> initially with "" until you guys have a use case. Exposing it later is
>> always easy vs exposing it now and then not being able to change it
>> anymore.
>>
>
> Hi Haitao, Jarkko,
>
> Do you have more concrete use case of needing "sgx2" in /proc/cpuinfo?
>

I don't have specific use cases so far. But I think users would expect all  
sub-features supported by the cpu in /proc/cpuinfo. And it is a more  
convenient and readily available tool than cpuid for verifying sgx2  
enabled in HW. So it'd be just nice for cpuinfo to be consistent with  
cpuid output.

Thanks
Haitao
Dave Hansen March 2, 2021, 3:58 p.m. UTC | #12
On 3/2/21 7:48 AM, Haitao Huang wrote:
> 
> Hi Haitao, Jarkko,
> 
> Do you have more concrete use case of needing "sgx2" in /proc/cpuinfo?

Kai, please remove it from your series.  I'm not hearing any arguments
remotely close enough to what Boris would require in order to keep it.
Sean Christopherson March 2, 2021, 4:02 p.m. UTC | #13
On Tue, Mar 02, 2021, Kai Huang wrote:
> On Mon, 2021-03-01 at 12:32 +0100, Borislav Petkov wrote:
> > On Tue, Mar 02, 2021 at 12:28:27AM +1300, Kai Huang wrote:
> > > I think some script can utilize /proc/cpuinfo. For instance, admin can have
> > > automation tool/script to deploy enclave (with sgx2) apps, and that script can check
> > > whether platform supports sgx2 or not, before it can deploy those enclave apps. Or
> > > enclave author may just want to check /proc/cpuinfo to know whether the machine can
> > > be used for testing sgx2 enclave or not.
> > 
> > This doesn't sound like a concrete use of this. So you can hide it
> > initially with "" until you guys have a use case. Exposing it later is
> > always easy vs exposing it now and then not being able to change it
> > anymore.
> > 
> 
> Hi Haitao, Jarkko,
> 
> Do you have more concrete use case of needing "sgx2" in /proc/cpuinfo?

The KVM use case is to query /proc/cpuinfo to see if sgx2 can be enabled in a
guest.

The counter-argument to that is we might want sgx2 in /proc/cpuinfo to mean sgx2
is enabled in hardware _and_ supported by the kernel.  Userspace can grep for
sgx in /proc/cpuinfo, and use cpuid to discover sgx2, so it's not a blocker.

That being said, adding some form of capability/versioning to SGX seems
inevitable, not sure it's worth witholding sgx2 from /proc/cpuinfo.
Borislav Petkov March 2, 2021, 5:53 p.m. UTC | #14
On March 2, 2021 5:02:13 PM GMT+01:00, Sean Christopherson <seanjc@google.com> wrote:
>The KVM use case is to query /proc/cpuinfo to see if sgx2 can be
>enabled in a
>guest.

You mean before the guest ia created? I sure hope there's a better way to query HV-supported features than grepping /proc/cpuinfo...

>The counter-argument to that is we might want sgx2 in /proc/cpuinfo to
>mean sgx2
>is enabled in hardware _and_ supported by the kernel.  Userspace can
>grep for
>sgx in /proc/cpuinfo, and use cpuid to discover sgx2, so it's not a
>blocker.

Question is, what exactly that flag should denote: that EDMM is supported in the HV and guests can do the dynamic thing of adding/rwmoving EPC pages? Is that the only feature behind SGX2?

>That being said, adding some form of capability/versioning to SGX seems
>inevitable, not sure it's worth witholding sgx2 from /proc/cpuinfo.

See what I typed earlier - no objections from me if a proper use case is identified and written down.

Thx.
Kai Huang March 2, 2021, 6:27 p.m. UTC | #15
On Tue, 2021-03-02 at 18:53 +0100, Boris Petkov wrote:
> On March 2, 2021 5:02:13 PM GMT+01:00, Sean Christopherson <seanjc@google.com> wrote:
> > The KVM use case is to query /proc/cpuinfo to see if sgx2 can be
> > enabled in a
> > guest.
> 
> You mean before the guest ia created? I sure hope there's a better way to query HV-supported features than grepping /proc/cpuinfo...
> 
> > The counter-argument to that is we might want sgx2 in /proc/cpuinfo to
> > mean sgx2
> > is enabled in hardware _and_ supported by the kernel.  Userspace can
> > grep for
> > sgx in /proc/cpuinfo, and use cpuid to discover sgx2, so it's not a
> > blocker.
> 
> Question is, what exactly that flag should denote: that EDMM is supported in the HV and guests can do the dynamic thing of adding/rwmoving EPC pages? Is that the only feature behind SGX2?

Yes SGX2 == EDMM. Other sub-features, such as VMM oversubscription, have other CPUID
bits.

> 
> > That being said, adding some form of capability/versioning to SGX seems
> > inevitable, not sure it's worth witholding sgx2 from /proc/cpuinfo.
> 
> See what I typed earlier - no objections from me if a proper use case is identified and written down.
> 
> Thx.
Kai Huang March 7, 2021, 10:49 p.m. UTC | #16
On Tue, 2021-03-02 at 07:58 -0800, Dave Hansen wrote:
> On 3/2/21 7:48 AM, Haitao Huang wrote:
> > 
> > Hi Haitao, Jarkko,
> > 
> > Do you have more concrete use case of needing "sgx2" in /proc/cpuinfo?
> 
> Kai, please remove it from your series.  I'm not hearing any arguments
> remotely close enough to what Boris would require in order to keep it.

Yes I will do. Thanks.
Jarkko Sakkinen March 10, 2021, 3:30 p.m. UTC | #17
On Tue, Mar 02, 2021 at 07:58:37AM -0800, Dave Hansen wrote:
> On 3/2/21 7:48 AM, Haitao Huang wrote:
> > 
> > Hi Haitao, Jarkko,
> > 
> > Do you have more concrete use case of needing "sgx2" in /proc/cpuinfo?
> 
> Kai, please remove it from your series.  I'm not hearing any arguments
> remotely close enough to what Boris would require in order to keep it.

Agreed.

/Jarkko
diff mbox series

Patch

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index cc96e26d69f7..9502c445a3e9 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -290,6 +290,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) /* "" Basic SGX */
+#define X86_FEATURE_SGX2        	(11*32+ 9) /* SGX Enclave Dynamic Memory Management (EDMM) */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
 #define X86_FEATURE_AVX_VNNI		(12*32+ 4) /* AVX VNNI instructions */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index d40f8e0a54ce..defda61f372d 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -73,6 +73,8 @@  static const struct cpuid_dep cpuid_deps[] = {
 	{ 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/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 972ec3bfa9c0..21d1f062895a 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 },