| Message ID | 20190320162119.4469-9-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel SGX1 support | expand |
On Wed, 2019-03-20 at 18:21 +0200, Jarkko Sakkinen wrote: > From: Sean Christopherson <sean.j.christopherson@intel.com> > > Similar to other large Intel features such as VMX and TXT, SGX must be > explicitly enabled in IA32_FEATURE_CONTROL MSR to be truly usable. > Clear all SGX related capabilities if SGX is not fully enabled in > IA32_FEATURE_CONTROL or if the SGX1 instruction set isn't supported > (impossible on bare metal, theoretically possible in a VM if the VMM is > doing something weird). > > Like SGX itself, SGX Launch Control must be explicitly enabled via a > flag in IA32_FEATURE_CONTROL. Clear the SGX_LC capability if Launch > Control is not fully enabled (or obviously if SGX itself is disabled). > > Note that clearing X86_FEATURE_SGX_LC creates a bit of a conundrum > regarding the SGXLEPUBKEYHASH MSRs, as it may be desirable to read the > MSRs even if they are not writable, e.g. to query the configured key, > but clearing the capability leaves no breadcrum for discerning whether > or not the MSRs exist. But, such usage will be rare (KVM is the only > known case at this time) and not performance critical, so it's not > unreasonable to require the use of rdmsr_safe(). Clearing the cap bit > eliminates the need for an additional flag to track whether or not > Launch Control is truly enabled, which is what we care about the vast > majority of the time. [Resend. Somehow my last reply doesn't show up in my mailbox so not sure whether I sent it successfully or not. Sorry if you receving duplicated mails.] However this is not consistent with HW behavior. If LC feature flag is not present, then MSRs should have hash of Intel's key, which is not always the case here, when you expose SGX to KVM. Enclave in KVM guest will get unexpected EINIT error when launing Intel enclave, if on HW MSRs are configured to 3rd party value but locked to readonly. My opition is we already have enough cases that violates HW behavior in SGX virtualization, let's not have one more. Besides, why do we "need an additional flag to track whether or not Launch Control is truly enabled"? Doesn't driver only need to know whether MSRs are writable? Thanks, -Kai > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/kernel/cpu/intel.c | 39 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index fc3c07fe7df5..702497f34a96 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -596,6 +596,42 @@ static void detect_tme(struct cpuinfo_x86 *c) > c->x86_phys_bits -= keyid_bits; > } > > +static void __maybe_unused detect_sgx(struct cpuinfo_x86 *c) > +{ > + unsigned long long fc; > + > + rdmsrl(MSR_IA32_FEATURE_CONTROL, fc); > + if (!(fc & FEATURE_CONTROL_LOCKED)) { > + pr_err_once("sgx: The feature control MSR is not locked\n"); > + goto err_unsupported; > + } > + > + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) { > + pr_err_once("sgx: SGX is not enabled in IA32_FEATURE_CONTROL MSR\n"); > + goto err_unsupported; > + } > + > + if (!cpu_has(c, X86_FEATURE_SGX1)) { > + pr_err_once("sgx: SGX1 instruction set is not supported\n"); > + goto err_unsupported; > + } > + > + if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) { > + pr_info_once("sgx: The launch control MSRs are not writable\n"); > + goto err_msrs_rdonly; > + } > + > + return; > + > +err_unsupported: > + setup_clear_cpu_cap(X86_FEATURE_SGX); > + setup_clear_cpu_cap(X86_FEATURE_SGX1); > + setup_clear_cpu_cap(X86_FEATURE_SGX2); > + > +err_msrs_rdonly: > + setup_clear_cpu_cap(X86_FEATURE_SGX_LC); > +} > + > static void init_intel_energy_perf(struct cpuinfo_x86 *c) > { > u64 epb; > @@ -763,6 +799,9 @@ static void init_intel(struct cpuinfo_x86 *c) > if (cpu_has(c, X86_FEATURE_TME)) > detect_tme(c); > > + if (IS_ENABLED(CONFIG_INTEL_SGX) && cpu_has(c, X86_FEATURE_SGX)) > + detect_sgx(c); > + > init_intel_energy_perf(c); > > init_intel_misc_features(c);
On Tue, Mar 26, 2019 at 05:17:40AM -0700, Huang, Kai wrote: > On Wed, 2019-03-20 at 18:21 +0200, Jarkko Sakkinen wrote: > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Similar to other large Intel features such as VMX and TXT, SGX must be > > explicitly enabled in IA32_FEATURE_CONTROL MSR to be truly usable. > > Clear all SGX related capabilities if SGX is not fully enabled in > > IA32_FEATURE_CONTROL or if the SGX1 instruction set isn't supported > > (impossible on bare metal, theoretically possible in a VM if the VMM is > > doing something weird). > > > > Like SGX itself, SGX Launch Control must be explicitly enabled via a > > flag in IA32_FEATURE_CONTROL. Clear the SGX_LC capability if Launch > > Control is not fully enabled (or obviously if SGX itself is disabled). > > > > Note that clearing X86_FEATURE_SGX_LC creates a bit of a conundrum > > regarding the SGXLEPUBKEYHASH MSRs, as it may be desirable to read the > > MSRs even if they are not writable, e.g. to query the configured key, > > but clearing the capability leaves no breadcrum for discerning whether > > or not the MSRs exist. But, such usage will be rare (KVM is the only > > known case at this time) and not performance critical, so it's not > > unreasonable to require the use of rdmsr_safe(). Clearing the cap bit > > eliminates the need for an additional flag to track whether or not > > Launch Control is truly enabled, which is what we care about the vast > > majority of the time. > > [Resend. Somehow my last reply doesn't show up in my mailbox so not sure whether I sent it > successfully or not. Sorry if you receving duplicated mails.] > > However this is not consistent with HW behavior. If LC feature flag is not present, then MSRs should > have hash of Intel's key, which is not always the case here, when you expose SGX to KVM. Enclave in > KVM guest will get unexpected EINIT error when launing Intel enclave, if on HW MSRs are configured > to 3rd party value but locked to readonly. Intel doesn't have a singular key. The internal reset value of the LE pubkey hash MSRs is micro-architectural, i.e. can change without warning on any given processor. All current processors with SGX support may use the same reset value, but it's not something that customers should/can rely on. That being said, this in no way impacts KVM's ability to virtualize SGX, e.g. KVM can directly do CPUID and {RD,WR}MSR to probe the capabilities of the platform as needed. > My opition is we already have enough cases that violates HW behavior in > SGX virtualization, let's not have one more. What are the other cases? > Besides, why do we "need an additional flag to track whether or not > Launch Control is truly enabled"? Doesn't driver only need to know whether > MSRs are writable? Yes, and that's why we're overloading X86_FEATURE_SGX_LC to be set if and only if SGX_LC is supported *and* enabled, e.g. so that the kernel can simply check X86_FEATURE_SGX_LC without having to also probe the MSRs.
> > On Tue, Mar 26, 2019 at 05:17:40AM -0700, Huang, Kai wrote: > > On Wed, 2019-03-20 at 18:21 +0200, Jarkko Sakkinen wrote: > > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > Similar to other large Intel features such as VMX and TXT, SGX must > > > be explicitly enabled in IA32_FEATURE_CONTROL MSR to be truly usable. > > > Clear all SGX related capabilities if SGX is not fully enabled in > > > IA32_FEATURE_CONTROL or if the SGX1 instruction set isn't supported > > > (impossible on bare metal, theoretically possible in a VM if the VMM > > > is doing something weird). > > > > > > Like SGX itself, SGX Launch Control must be explicitly enabled via a > > > flag in IA32_FEATURE_CONTROL. Clear the SGX_LC capability if Launch > > > Control is not fully enabled (or obviously if SGX itself is disabled). > > > > > > Note that clearing X86_FEATURE_SGX_LC creates a bit of a conundrum > > > regarding the SGXLEPUBKEYHASH MSRs, as it may be desirable to read > > > the MSRs even if they are not writable, e.g. to query the configured > > > key, but clearing the capability leaves no breadcrum for discerning > > > whether or not the MSRs exist. But, such usage will be rare (KVM is > > > the only known case at this time) and not performance critical, so > > > it's not unreasonable to require the use of rdmsr_safe(). Clearing > > > the cap bit eliminates the need for an additional flag to track > > > whether or not Launch Control is truly enabled, which is what we > > > care about the vast majority of the time. > > > > [Resend. Somehow my last reply doesn't show up in my mailbox so not > > sure whether I sent it successfully or not. Sorry if you receving > > duplicated mails.] > > > > However this is not consistent with HW behavior. If LC feature flag is > > not present, then MSRs should have hash of Intel's key, which is not > > always the case here, when you expose SGX to KVM. Enclave in KVM guest > > will get unexpected EINIT error when launing Intel enclave, if on HW MSRs > are configured to 3rd party value but locked to readonly. > > Intel doesn't have a singular key. The internal reset value of the LE pubkey > hash MSRs is micro-architectural, i.e. can change without warning on any > given processor. All current processors with SGX support may use the same > reset value, but it's not something that customers should/can rely on. I don't think update of Intel hash would be frequent, like you said all current processors with SGX support may should be using the same value. Thus I am expecting I can run Intel SDK on KVM guest if LC feature flag is cleared. Even Intel has updated the key, SDK should mention those impacted machine should run SDK starting from some new version, so SDK should continue to run. > > That being said, this in no way impacts KVM's ability to virtualize SGX, e.g. > KVM can directly do CPUID and {RD,WR}MSR to probe the capabilities of the > platform as needed. I am not following. KVM can do whatever it wants, but it cannot change the fact that KVM guest cannot run intel enclave if platform's MSRs are configured to 3rd party and locked. Or am I misunderstanding? > > > My opition is we already have enough cases that violates HW behavior > > in SGX virtualization, let's not have one more. > > What are the other cases? One example is EPCM + EPC should be power of 2. And EPC migration (sudden loss of EPC). And if we apply some policy to restrict enclave during EINIT trap, etc. But those are not the point. I should probably have not mentioned them. > > > Besides, why do we "need an additional flag to track whether or not > > Launch Control is truly enabled"? Doesn't driver only need to know > > whether MSRs are writable? > > Yes, and that's why we're overloading X86_FEATURE_SGX_LC to be set if and > only if SGX_LC is supported *and* enabled, e.g. so that the kernel can simply > check X86_FEATURE_SGX_LC without having to also probe the MSRs. OK. But IMO if what I mentioned above is correct, then that part should overweight the benefit you can get here. Anyway who cares to do less/more thing in driver probe? Thanks, -Kai
On Tue, Mar 26, 2019 at 02:25:52PM -0700, Huang, Kai wrote: > > > > That being said, this in no way impacts KVM's ability to virtualize SGX, e.g. > > KVM can directly do CPUID and {RD,WR}MSR to probe the capabilities of the > > platform as needed. > > I am not following. KVM can do whatever it wants, but it cannot change the > fact that KVM guest cannot run intel enclave if platform's MSRs are > configured to 3rd party and locked. > > Or am I misunderstanding? What does that have to do with this patch? The only thing this patch does is clear a *software* bit that says "SGX LC is enabled" so that the kernel can make the reasonable assumption that the MSRs are writable when X86_FEATURE_SGX_LC=1. > > > > > > My opition is we already have enough cases that violates HW behavior > > > in SGX virtualization, let's not have one more. > > > > What are the other cases? > > One example is EPCM + EPC should be power of 2. Again, that's a micro-architecture detail due to the use of range registers to carve out the EPC. The only thing the SDM states regarding size and alignment is: The EPC is divided into EPC pages. An EPC page is 4KB in size and always aligned on a 4KB boundary. > And EPC migration (sudden loss of EPC). Unfortunate, yes, but it does not violate the architecture in any way. You can argue that it adds a wrinkle that isn't explicitly documented in the SDM, but virtualization is full of things like that. > And if we apply some policy to restrict enclave during EINIT trap, etc. > > But those are not the point. I should probably have not mentioned them. > > > > > > Besides, why do we "need an additional flag to track whether or not > > > Launch Control is truly enabled"? Doesn't driver only need to know > > > whether MSRs are writable? > > > > Yes, and that's why we're overloading X86_FEATURE_SGX_LC to be set if and > > only if SGX_LC is supported *and* enabled, e.g. so that the kernel can simply > > check X86_FEATURE_SGX_LC without having to also probe the MSRs. > > OK. > > But IMO if what I mentioned above is correct, then that part should > overweight the benefit you can get here. Half dozen of one, six of the other. If we clear the flag, then KVM has to manually probe the MSRs. If we don't clear the flag, then the driver has to manually probe the MSRs. Alternatively we could add another flag, but then we end up with X86_FEATURE_SGX_LC and X86_FEATURE_SGX_LE_PUBKEY_HASH_WRITABLE, or maybe X86_FEATURE_SGX_LC_REALLY_TRULY_ENABLED. > > Anyway who cares to do less/more thing in driver probe? Exactly. KVM will probe the MSRs once during its own loading.
> On Tue, Mar 26, 2019 at 02:25:52PM -0700, Huang, Kai wrote: > > > > > > That being said, this in no way impacts KVM's ability to virtualize SGX, e.g. > > > KVM can directly do CPUID and {RD,WR}MSR to probe the capabilities > > > of the platform as needed. > > > > I am not following. KVM can do whatever it wants, but it cannot change > > the fact that KVM guest cannot run intel enclave if platform's MSRs > > are configured to 3rd party and locked. > > > > Or am I misunderstanding? > > What does that have to do with this patch? The only thing this patch does is > clear a *software* bit that says "SGX LC is enabled" so that the kernel can > make the reasonable assumption that the MSRs are writable when > X86_FEATURE_SGX_LC=1. Sorted out offline discussion with you. Will let you handle :) Thanks, -Kai
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index fc3c07fe7df5..702497f34a96 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -596,6 +596,42 @@ static void detect_tme(struct cpuinfo_x86 *c) c->x86_phys_bits -= keyid_bits; } +static void __maybe_unused detect_sgx(struct cpuinfo_x86 *c) +{ + unsigned long long fc; + + rdmsrl(MSR_IA32_FEATURE_CONTROL, fc); + if (!(fc & FEATURE_CONTROL_LOCKED)) { + pr_err_once("sgx: The feature control MSR is not locked\n"); + goto err_unsupported; + } + + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) { + pr_err_once("sgx: SGX is not enabled in IA32_FEATURE_CONTROL MSR\n"); + goto err_unsupported; + } + + if (!cpu_has(c, X86_FEATURE_SGX1)) { + pr_err_once("sgx: SGX1 instruction set is not supported\n"); + goto err_unsupported; + } + + if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) { + pr_info_once("sgx: The launch control MSRs are not writable\n"); + goto err_msrs_rdonly; + } + + return; + +err_unsupported: + setup_clear_cpu_cap(X86_FEATURE_SGX); + setup_clear_cpu_cap(X86_FEATURE_SGX1); + setup_clear_cpu_cap(X86_FEATURE_SGX2); + +err_msrs_rdonly: + setup_clear_cpu_cap(X86_FEATURE_SGX_LC); +} + static void init_intel_energy_perf(struct cpuinfo_x86 *c) { u64 epb; @@ -763,6 +799,9 @@ static void init_intel(struct cpuinfo_x86 *c) if (cpu_has(c, X86_FEATURE_TME)) detect_tme(c); + if (IS_ENABLED(CONFIG_INTEL_SGX) && cpu_has(c, X86_FEATURE_SGX)) + detect_sgx(c); + init_intel_energy_perf(c); init_intel_misc_features(c);