| Message ID | bbfc8c833a62e4b55220834320829df1e17aff41.1614590788.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM SGX virtualization support | expand |
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.
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.
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.
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.
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.
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.
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.
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?
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.
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.
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
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.
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.
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.
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.
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.
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 --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 },