Message ID | a99e9c23310c79f2f4175c1af4c4cbcef913c3e5.1618196135.git.kai.huang@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM SGX virtualization support (KVM part) | expand |
Hi Kai, I love your patch! Yet something to improve: [auto build test ERROR on kvm/queue] [also build test ERROR on next-20210409] [cannot apply to vhost/linux-next v5.12-rc7] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Kai-Huang/KVM-SGX-virtualization-support-KVM-part/20210412-122425 base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue config: x86_64-rhel-8.3-kselftests (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/66e235131b59a03ed48f6f6343de43ba9786e32d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Kai-Huang/KVM-SGX-virtualization-support-KVM-part/20210412-122425 git checkout 66e235131b59a03ed48f6f6343de43ba9786e32d # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from arch/x86/kvm/cpuid.c:22: arch/x86/kvm/cpuid.h: In function '__feature_translate': arch/x86/kvm/cpuid.h:128:21: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 128 | if (x86_feature == X86_FEATURE_SGX1) | ^~~~~~~~~~~~~~~~ | X86_FEATURE_SGX arch/x86/kvm/cpuid.h:128:21: note: each undeclared identifier is reported only once for each function it appears in arch/x86/kvm/cpuid.h:130:26: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 130 | else if (x86_feature == X86_FEATURE_SGX2) | ^~~~~~~~~~~~~~~~ | X86_FEATURE_SGX In file included from arch/x86/include/asm/thread_info.h:53, from include/linux/thread_info.h:58, from arch/x86/include/asm/preempt.h:7, from include/linux/preempt.h:78, from include/linux/percpu.h:6, from include/linux/context_tracking_state.h:5, from include/linux/hardirq.h:5, from include/linux/kvm_host.h:7, from arch/x86/kvm/cpuid.c:12: arch/x86/kvm/cpuid.c: In function 'kvm_set_cpu_caps': >> arch/x86/kvm/cpuid.c:57:32: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) | ^~~~~~~~~~~~ arch/x86/include/asm/cpufeature.h:121:24: note: in definition of macro 'cpu_has' 121 | (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 : \ | ^~~ arch/x86/kvm/cpuid.c:57:19: note: in expansion of macro 'boot_cpu_has' 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) | ^~~~~~~~~~~~ arch/x86/kvm/cpuid.c:500:3: note: in expansion of macro 'SF' 500 | SF(SGX1) | SF(SGX2) | ^~ >> arch/x86/kvm/cpuid.c:57:32: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) | ^~~~~~~~~~~~ arch/x86/include/asm/cpufeature.h:121:24: note: in definition of macro 'cpu_has' 121 | (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 : \ | ^~~ arch/x86/kvm/cpuid.c:57:19: note: in expansion of macro 'boot_cpu_has' 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) | ^~~~~~~~~~~~ arch/x86/kvm/cpuid.c:500:14: note: in expansion of macro 'SF' 500 | SF(SGX1) | SF(SGX2) | ^~ arch/x86/kvm/cpuid.c: In function '__do_cpuid_func': >> arch/x86/kvm/cpuid.c:838:17: error: 'SGX_MISC_EXINFO' undeclared (first use in this function) 838 | entry->ebx &= SGX_MISC_EXINFO; | ^~~~~~~~~~~~~~~ >> arch/x86/kvm/cpuid.c:851:17: error: 'SGX_ATTR_DEBUG' undeclared (first use in this function) 851 | entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | | ^~~~~~~~~~~~~~ >> arch/x86/kvm/cpuid.c:851:34: error: 'SGX_ATTR_MODE64BIT' undeclared (first use in this function) 851 | entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | | ^~~~~~~~~~~~~~~~~~ >> arch/x86/kvm/cpuid.c:852:31: error: 'SGX_ATTR_EINITTOKENKEY' undeclared (first use in this function) 852 | /* PROVISIONKEY | */ SGX_ATTR_EINITTOKENKEY | | ^~~~~~~~~~~~~~~~~~~~~~ >> arch/x86/kvm/cpuid.c:853:10: error: 'SGX_ATTR_KSS' undeclared (first use in this function) 853 | SGX_ATTR_KSS; | ^~~~~~~~~~~~ -- In file included from arch/x86/kvm/vmx/vmx.c:51: arch/x86/kvm/cpuid.h: In function '__feature_translate': arch/x86/kvm/cpuid.h:128:21: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 128 | if (x86_feature == X86_FEATURE_SGX1) | ^~~~~~~~~~~~~~~~ | X86_FEATURE_SGX arch/x86/kvm/cpuid.h:128:21: note: each undeclared identifier is reported only once for each function it appears in arch/x86/kvm/cpuid.h:130:26: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 130 | else if (x86_feature == X86_FEATURE_SGX2) | ^~~~~~~~~~~~~~~~ | X86_FEATURE_SGX arch/x86/kvm/vmx/vmx.c: In function 'vmx_set_cpu_caps': >> arch/x86/kvm/vmx/vmx.c:7375:21: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 7375 | kvm_cpu_cap_clear(X86_FEATURE_SGX1); | ^~~~~~~~~~~~~~~~ | X86_FEATURE_SGX >> arch/x86/kvm/vmx/vmx.c:7376:21: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? 7376 | kvm_cpu_cap_clear(X86_FEATURE_SGX2); | ^~~~~~~~~~~~~~~~ | X86_FEATURE_SGX vim +57 arch/x86/kvm/cpuid.c 4344ee981e2199 Paolo Bonzini 2013-10-02 55 87382003e35559 Sean Christopherson 2019-12-17 56 #define F feature_bit cb4de96e0cca64 Sean Christopherson 2021-04-12 @57 #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) 5c404cabd1b5c1 Paolo Bonzini 2014-12-03 58 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Paolo, Obviously this series requires x86 part patches (which is on tip/x86/sgx) to work. Please let me know how do you want to proceed? On Mon, 2021-04-12 at 17:51 +0800, kernel test robot wrote: > Hi Kai, > > I love your patch! Yet something to improve: > > [auto build test ERROR on kvm/queue] > [also build test ERROR on next-20210409] > [cannot apply to vhost/linux-next v5.12-rc7] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Kai-Huang/KVM-SGX-virtualization-support-KVM-part/20210412-122425 > base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue > config: x86_64-rhel-8.3-kselftests (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > reproduce (this is a W=1 build): > # https://github.com/0day-ci/linux/commit/66e235131b59a03ed48f6f6343de43ba9786e32d > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Kai-Huang/KVM-SGX-virtualization-support-KVM-part/20210412-122425 > git checkout 66e235131b59a03ed48f6f6343de43ba9786e32d > # save the attached .config to linux build tree > make W=1 ARCH=x86_64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > In file included from arch/x86/kvm/cpuid.c:22: > arch/x86/kvm/cpuid.h: In function '__feature_translate': > arch/x86/kvm/cpuid.h:128:21: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 128 | if (x86_feature == X86_FEATURE_SGX1) > | ^~~~~~~~~~~~~~~~ > | X86_FEATURE_SGX > arch/x86/kvm/cpuid.h:128:21: note: each undeclared identifier is reported only once for each function it appears in > arch/x86/kvm/cpuid.h:130:26: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 130 | else if (x86_feature == X86_FEATURE_SGX2) > | ^~~~~~~~~~~~~~~~ > | X86_FEATURE_SGX > In file included from arch/x86/include/asm/thread_info.h:53, > from include/linux/thread_info.h:58, > from arch/x86/include/asm/preempt.h:7, > from include/linux/preempt.h:78, > from include/linux/percpu.h:6, > from include/linux/context_tracking_state.h:5, > from include/linux/hardirq.h:5, > from include/linux/kvm_host.h:7, > from arch/x86/kvm/cpuid.c:12: > arch/x86/kvm/cpuid.c: In function 'kvm_set_cpu_caps': > > > arch/x86/kvm/cpuid.c:57:32: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) > | ^~~~~~~~~~~~ > arch/x86/include/asm/cpufeature.h:121:24: note: in definition of macro 'cpu_has' > 121 | (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 : \ > | ^~~ > arch/x86/kvm/cpuid.c:57:19: note: in expansion of macro 'boot_cpu_has' > 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) > | ^~~~~~~~~~~~ > arch/x86/kvm/cpuid.c:500:3: note: in expansion of macro 'SF' > 500 | SF(SGX1) | SF(SGX2) > | ^~ > > > arch/x86/kvm/cpuid.c:57:32: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) > | ^~~~~~~~~~~~ > arch/x86/include/asm/cpufeature.h:121:24: note: in definition of macro 'cpu_has' > 121 | (__builtin_constant_p(bit) && REQUIRED_MASK_BIT_SET(bit) ? 1 : \ > | ^~~ > arch/x86/kvm/cpuid.c:57:19: note: in expansion of macro 'boot_cpu_has' > 57 | #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) > | ^~~~~~~~~~~~ > arch/x86/kvm/cpuid.c:500:14: note: in expansion of macro 'SF' > 500 | SF(SGX1) | SF(SGX2) > | ^~ > arch/x86/kvm/cpuid.c: In function '__do_cpuid_func': > > > arch/x86/kvm/cpuid.c:838:17: error: 'SGX_MISC_EXINFO' undeclared (first use in this function) > 838 | entry->ebx &= SGX_MISC_EXINFO; > | ^~~~~~~~~~~~~~~ > > > arch/x86/kvm/cpuid.c:851:17: error: 'SGX_ATTR_DEBUG' undeclared (first use in this function) > 851 | entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > | ^~~~~~~~~~~~~~ > > > arch/x86/kvm/cpuid.c:851:34: error: 'SGX_ATTR_MODE64BIT' undeclared (first use in this function) > 851 | entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > | ^~~~~~~~~~~~~~~~~~ > > > arch/x86/kvm/cpuid.c:852:31: error: 'SGX_ATTR_EINITTOKENKEY' undeclared (first use in this function) > 852 | /* PROVISIONKEY | */ SGX_ATTR_EINITTOKENKEY | > | ^~~~~~~~~~~~~~~~~~~~~~ > > > arch/x86/kvm/cpuid.c:853:10: error: 'SGX_ATTR_KSS' undeclared (first use in this function) > 853 | SGX_ATTR_KSS; > | ^~~~~~~~~~~~ > -- > In file included from arch/x86/kvm/vmx/vmx.c:51: > arch/x86/kvm/cpuid.h: In function '__feature_translate': > arch/x86/kvm/cpuid.h:128:21: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 128 | if (x86_feature == X86_FEATURE_SGX1) > | ^~~~~~~~~~~~~~~~ > | X86_FEATURE_SGX > arch/x86/kvm/cpuid.h:128:21: note: each undeclared identifier is reported only once for each function it appears in > arch/x86/kvm/cpuid.h:130:26: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 130 | else if (x86_feature == X86_FEATURE_SGX2) > | ^~~~~~~~~~~~~~~~ > | X86_FEATURE_SGX > arch/x86/kvm/vmx/vmx.c: In function 'vmx_set_cpu_caps': > > > arch/x86/kvm/vmx/vmx.c:7375:21: error: 'X86_FEATURE_SGX1' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 7375 | kvm_cpu_cap_clear(X86_FEATURE_SGX1); > | ^~~~~~~~~~~~~~~~ > | X86_FEATURE_SGX > > > arch/x86/kvm/vmx/vmx.c:7376:21: error: 'X86_FEATURE_SGX2' undeclared (first use in this function); did you mean 'X86_FEATURE_SGX'? > 7376 | kvm_cpu_cap_clear(X86_FEATURE_SGX2); > | ^~~~~~~~~~~~~~~~ > | X86_FEATURE_SGX > > > vim +57 arch/x86/kvm/cpuid.c > > 4344ee981e2199 Paolo Bonzini 2013-10-02 55 > 87382003e35559 Sean Christopherson 2019-12-17 56 #define F feature_bit > cb4de96e0cca64 Sean Christopherson 2021-04-12 @57 #define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) > 5c404cabd1b5c1 Paolo Bonzini 2014-12-03 58 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 12/04/21 06:21, Kai Huang wrote: > @@ -4377,6 +4380,15 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx) > if (!vcpu->kvm->arch.bus_lock_detection_enabled) > exec_control &= ~SECONDARY_EXEC_BUS_LOCK_DETECTION; > > + if (cpu_has_vmx_encls_vmexit() && nested) { > + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX)) > + vmx->nested.msrs.secondary_ctls_high |= > + SECONDARY_EXEC_ENCLS_EXITING; > + else > + vmx->nested.msrs.secondary_ctls_high &= > + ~SECONDARY_EXEC_ENCLS_EXITING; > + } > + This is incorrect, I've removed it. The MSRs can only be written by userspace. If SGX is disabled in the guest CPUID, nested_vmx_exit_handled_encls can just do: if (!guest_cpuid_has(vcpu, X86_FEATURE_SGX) || !nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING)) return false; and the useless ENCLS exiting bitmap in vmcs12 will be ignored. Paolo
On Sat, 2021-04-17 at 16:11 +0200, Paolo Bonzini wrote: > On 12/04/21 06:21, Kai Huang wrote: > > @@ -4377,6 +4380,15 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx) > > if (!vcpu->kvm->arch.bus_lock_detection_enabled) > > exec_control &= ~SECONDARY_EXEC_BUS_LOCK_DETECTION; > > > > > > + if (cpu_has_vmx_encls_vmexit() && nested) { > > + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX)) > > + vmx->nested.msrs.secondary_ctls_high |= > > + SECONDARY_EXEC_ENCLS_EXITING; > > + else > > + vmx->nested.msrs.secondary_ctls_high &= > > + ~SECONDARY_EXEC_ENCLS_EXITING; > > + } > > + > > This is incorrect, I've removed it. The MSRs can only be written by > userspace. > > If SGX is disabled in the guest CPUID, nested_vmx_exit_handled_encls can > just do: > > if (!guest_cpuid_has(vcpu, X86_FEATURE_SGX) || > !nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING)) > return false; > > and the useless ENCLS exiting bitmap in vmcs12 will be ignored. > > Paolo > Thanks for queuing this series! Looks good to me. However if I read code correctly, in this way a side effect would be vmx->nested.msrs.secondary_ctls_high will always have SECONDARY_EXEC_ENCLS_EXITING bit set, even SGX is not exposed to guest, which means a guest can set this even SGX is not present, but I think it is OK since ENCLS exiting bitmap in vmcs12 will be ignored anyway in nested_vmx_exit_handled_encls() as you mentioned above. Anyway, I have tested this code and it works at my side (by creating L2 with SGX support and running SGX workloads inside it). Sean, please also comment if you have any.
On Mon, Apr 19, 2021, Kai Huang wrote: > On Sat, 2021-04-17 at 16:11 +0200, Paolo Bonzini wrote: > > On 12/04/21 06:21, Kai Huang wrote: > > > @@ -4377,6 +4380,15 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx) > > > if (!vcpu->kvm->arch.bus_lock_detection_enabled) > > > exec_control &= ~SECONDARY_EXEC_BUS_LOCK_DETECTION; > > > > > > > > > + if (cpu_has_vmx_encls_vmexit() && nested) { > > > + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX)) > > > + vmx->nested.msrs.secondary_ctls_high |= > > > + SECONDARY_EXEC_ENCLS_EXITING; > > > + else > > > + vmx->nested.msrs.secondary_ctls_high &= > > > + ~SECONDARY_EXEC_ENCLS_EXITING; > > > + } > > > + > > > > This is incorrect, I've removed it. The MSRs can only be written by > > userspace. vmx_compute_secondary_exec_control() violates that left, right, and center, it's just buried down in vmx_adjust_secondary_exec_control(). This is an open coded version of that helper, sans the actual update to exec_control since ENCLS needs to be conditionally intercepted even when it's exposed to the guest. > > If SGX is disabled in the guest CPUID, nested_vmx_exit_handled_encls can > > just do: > > > > if (!guest_cpuid_has(vcpu, X86_FEATURE_SGX) || > > !nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING)) > > return false; > > > > and the useless ENCLS exiting bitmap in vmcs12 will be ignored. > > > > Paolo > > > > Thanks for queuing this series! > > Looks good to me. However if I read code correctly, in this way a side effect would be > vmx->nested.msrs.secondary_ctls_high will always have SECONDARY_EXEC_ENCLS_EXITING bit > set, even SGX is not exposed to guest, which means a guest can set this even SGX is not > present, but I think it is OK since ENCLS exiting bitmap in vmcs12 will be ignored anyway > in nested_vmx_exit_handled_encls() as you mentioned above. > > Anyway, I have tested this code and it works at my side (by creating L2 with SGX support > and running SGX workloads inside it). > > Sean, please also comment if you have any. > >
On 19/04/21 17:16, Sean Christopherson wrote: > On Mon, Apr 19, 2021, Kai Huang wrote: >> On Sat, 2021-04-17 at 16:11 +0200, Paolo Bonzini wrote: >>> On 12/04/21 06:21, Kai Huang wrote: >>>> @@ -4377,6 +4380,15 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx) >>>> if (!vcpu->kvm->arch.bus_lock_detection_enabled) >>>> exec_control &= ~SECONDARY_EXEC_BUS_LOCK_DETECTION; >>>> >>>> >>>> + if (cpu_has_vmx_encls_vmexit() && nested) { >>>> + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX)) >>>> + vmx->nested.msrs.secondary_ctls_high |= >>>> + SECONDARY_EXEC_ENCLS_EXITING; >>>> + else >>>> + vmx->nested.msrs.secondary_ctls_high &= >>>> + ~SECONDARY_EXEC_ENCLS_EXITING; >>>> + } >>>> + >>> >>> This is incorrect, I've removed it. The MSRs can only be written by >>> userspace. > > vmx_compute_secondary_exec_control() violates that left, right, and center, it's > just buried down in vmx_adjust_secondary_exec_control(). This is an open coded > version of that helper, sans the actual update to exec_control since ENCLS needs > to be conditionally intercepted even when it's exposed to the guest. Hmm, that's true. I'll place back the version that you had. Paolo >>> If SGX is disabled in the guest CPUID, nested_vmx_exit_handled_encls can >>> just do: >>> >>> if (!guest_cpuid_has(vcpu, X86_FEATURE_SGX) || >>> !nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING)) >>> return false; >>> >>> and the useless ENCLS exiting bitmap in vmcs12 will be ignored. >>> >>> Paolo >>> >> >> Thanks for queuing this series! >> >> Looks good to me. However if I read code correctly, in this way a side effect would be >> vmx->nested.msrs.secondary_ctls_high will always have SECONDARY_EXEC_ENCLS_EXITING bit >> set, even SGX is not exposed to guest, which means a guest can set this even SGX is not >> present, but I think it is OK since ENCLS exiting bitmap in vmcs12 will be ignored anyway >> in nested_vmx_exit_handled_encls() as you mentioned above. >> >> Anyway, I have tested this code and it works at my side (by creating L2 with SGX support >> and running SGX workloads inside it). >> >> Sean, please also comment if you have any. >> >> >
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index a0e7be9ed449..a0d45607b702 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -18,6 +18,7 @@ #include <asm/processor.h> #include <asm/user.h> #include <asm/fpu/xstate.h> +#include <asm/sgx.h> #include "cpuid.h" #include "lapic.h" #include "mmu.h" @@ -171,6 +172,21 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) vcpu->arch.guest_supported_xcr0 = (best->eax | ((u64)best->edx << 32)) & supported_xcr0; + /* + * Bits 127:0 of the allowed SECS.ATTRIBUTES (CPUID.0x12.0x1) enumerate + * the supported XSAVE Feature Request Mask (XFRM), i.e. the enclave's + * requested XCR0 value. The enclave's XFRM must be a subset of XCRO + * at the time of EENTER, thus adjust the allowed XFRM by the guest's + * supported XCR0. Similar to XCR0 handling, FP and SSE are forced to + * '1' even on CPUs that don't support XSAVE. + */ + best = kvm_find_cpuid_entry(vcpu, 0x12, 0x1); + if (best) { + best->ecx &= vcpu->arch.guest_supported_xcr0 & 0xffffffff; + best->edx &= vcpu->arch.guest_supported_xcr0 >> 32; + best->ecx |= XFEATURE_MASK_FPSSE; + } + kvm_update_pv_runtime(vcpu); vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); @@ -429,7 +445,7 @@ void kvm_set_cpu_caps(void) ); kvm_cpu_cap_mask(CPUID_7_0_EBX, - F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) | + F(FSGSBASE) | F(SGX) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) | F(BMI2) | F(ERMS) | F(INVPCID) | F(RTM) | 0 /*MPX*/ | F(RDSEED) | F(ADX) | F(SMAP) | F(AVX512IFMA) | F(AVX512F) | F(AVX512PF) | F(AVX512ER) | F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(AVX512DQ) | @@ -440,7 +456,8 @@ void kvm_set_cpu_caps(void) F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) | F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | - F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/ + F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/ | + F(SGX_LC) ); /* Set LA57 based on hardware capability. */ if (cpuid_ecx(7) & F(LA57)) @@ -479,6 +496,10 @@ void kvm_set_cpu_caps(void) F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1) | F(XSAVES) ); + kvm_cpu_cap_init(CPUID_12_EAX, + SF(SGX1) | SF(SGX2) + ); + kvm_cpu_cap_mask(CPUID_8000_0001_ECX, F(LAHF_LM) | F(CMP_LEGACY) | 0 /*SVM*/ | 0 /* ExtApicSpace */ | F(CR8_LEGACY) | F(ABM) | F(SSE4A) | F(MISALIGNSSE) | @@ -800,6 +821,38 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) entry->edx = 0; } break; + case 0x12: + /* Intel SGX */ + if (!kvm_cpu_cap_has(X86_FEATURE_SGX)) { + entry->eax = entry->ebx = entry->ecx = entry->edx = 0; + break; + } + + /* + * Index 0: Sub-features, MISCSELECT (a.k.a extended features) + * and max enclave sizes. The SGX sub-features and MISCSELECT + * are restricted by kernel and KVM capabilities (like most + * feature flags), while enclave size is unrestricted. + */ + cpuid_entry_override(entry, CPUID_12_EAX); + entry->ebx &= SGX_MISC_EXINFO; + + entry = do_host_cpuid(array, function, 1); + if (!entry) + goto out; + + /* + * Index 1: SECS.ATTRIBUTES. ATTRIBUTES are restricted a la + * feature flags. Advertise all supported flags, including + * privileged attributes that require explicit opt-in from + * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is + * expected to derive it from supported XCR0. + */ + entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | + /* PROVISIONKEY | */ SGX_ATTR_EINITTOKENKEY | + SGX_ATTR_KSS; + entry->ebx &= 0; + break; /* Intel PT */ case 0x14: if (!kvm_cpu_cap_has(X86_FEATURE_INTEL_PT)) { diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index baaea9bbb8e5..8de1920da4f8 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -11,6 +11,7 @@ #include "mmu.h" #include "nested.h" #include "pmu.h" +#include "sgx.h" #include "trace.h" #include "vmx.h" #include "x86.h" @@ -2300,6 +2301,9 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12) if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST)) exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST; + if (exec_control & SECONDARY_EXEC_ENCLS_EXITING) + vmx_write_encls_bitmap(&vmx->vcpu, vmcs12); + secondary_exec_controls_set(vmx, exec_control); } @@ -5716,6 +5720,20 @@ static bool nested_vmx_exit_handled_cr(struct kvm_vcpu *vcpu, return false; } +static bool nested_vmx_exit_handled_encls(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + u32 encls_leaf; + + if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING)) + return false; + + encls_leaf = kvm_rax_read(vcpu); + if (encls_leaf > 62) + encls_leaf = 63; + return vmcs12->encls_exiting_bitmap & BIT_ULL(encls_leaf); +} + static bool nested_vmx_exit_handled_vmcs_access(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, gpa_t bitmap) { @@ -5812,9 +5830,6 @@ static bool nested_vmx_l0_wants_exit(struct kvm_vcpu *vcpu, case EXIT_REASON_VMFUNC: /* VM functions are emulated through L2->L0 vmexits. */ return true; - case EXIT_REASON_ENCLS: - /* SGX is never exposed to L1 */ - return true; default: break; } @@ -5938,6 +5953,8 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu, case EXIT_REASON_TPAUSE: return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE); + case EXIT_REASON_ENCLS: + return nested_vmx_exit_handled_encls(vcpu, vmcs12); default: return true; } @@ -6513,6 +6530,9 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps) msrs->secondary_ctls_high |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; + if (enable_sgx) + msrs->secondary_ctls_high |= SECONDARY_EXEC_ENCLS_EXITING; + /* miscellaneous data */ rdmsr(MSR_IA32_VMX_MISC, msrs->misc_low, diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h index 197148d76b8f..184418baeb3c 100644 --- a/arch/x86/kvm/vmx/nested.h +++ b/arch/x86/kvm/vmx/nested.h @@ -244,6 +244,11 @@ static inline bool nested_exit_on_intr(struct kvm_vcpu *vcpu) PIN_BASED_EXT_INTR_MASK; } +static inline bool nested_cpu_has_encls_exit(struct vmcs12 *vmcs12) +{ + return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING); +} + /* * if fixed0[i] == 1: val[i] must be 1 * if fixed1[i] == 0: val[i] must be 0 diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c index 2eed5da91698..6693ebdc0770 100644 --- a/arch/x86/kvm/vmx/sgx.c +++ b/arch/x86/kvm/vmx/sgx.c @@ -5,11 +5,13 @@ #include "cpuid.h" #include "kvm_cache_regs.h" +#include "nested.h" #include "sgx.h" #include "vmx.h" #include "x86.h" -bool __read_mostly enable_sgx; +bool __read_mostly enable_sgx = 1; +module_param_named(sgx, enable_sgx, bool, 0444); /* Initial value of guest's virtual SGX_LEPUBKEYHASHn MSRs */ static u64 sgx_pubkey_hash[4] __ro_after_init; @@ -422,3 +424,79 @@ void vcpu_setup_sgx_lepubkeyhash(struct kvm_vcpu *vcpu) memcpy(vmx->msr_ia32_sgxlepubkeyhash, sgx_pubkey_hash, sizeof(sgx_pubkey_hash)); } + +/* + * ECREATE must be intercepted to enforce MISCSELECT, ATTRIBUTES and XFRM + * restrictions if the guest's allowed-1 settings diverge from hardware. + */ +static bool sgx_intercept_encls_ecreate(struct kvm_vcpu *vcpu) +{ + struct kvm_cpuid_entry2 *guest_cpuid; + u32 eax, ebx, ecx, edx; + + if (!vcpu->kvm->arch.sgx_provisioning_allowed) + return true; + + guest_cpuid = kvm_find_cpuid_entry(vcpu, 0x12, 0); + if (!guest_cpuid) + return true; + + cpuid_count(0x12, 0, &eax, &ebx, &ecx, &edx); + if (guest_cpuid->ebx != ebx || guest_cpuid->edx != edx) + return true; + + guest_cpuid = kvm_find_cpuid_entry(vcpu, 0x12, 1); + if (!guest_cpuid) + return true; + + cpuid_count(0x12, 1, &eax, &ebx, &ecx, &edx); + if (guest_cpuid->eax != eax || guest_cpuid->ebx != ebx || + guest_cpuid->ecx != ecx || guest_cpuid->edx != edx) + return true; + + return false; +} + +void vmx_write_encls_bitmap(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) +{ + /* + * There is no software enable bit for SGX that is virtualized by + * hardware, e.g. there's no CR4.SGXE, so when SGX is disabled in the + * guest (either by the host or by the guest's BIOS) but enabled in the + * host, trap all ENCLS leafs and inject #UD/#GP as needed to emulate + * the expected system behavior for ENCLS. + */ + u64 bitmap = -1ull; + + /* Nothing to do if hardware doesn't support SGX */ + if (!cpu_has_vmx_encls_vmexit()) + return; + + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX) && + sgx_enabled_in_guest_bios(vcpu)) { + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX1)) { + bitmap &= ~GENMASK_ULL(ETRACK, ECREATE); + if (sgx_intercept_encls_ecreate(vcpu)) + bitmap |= (1 << ECREATE); + } + + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX2)) + bitmap &= ~GENMASK_ULL(EMODT, EAUG); + + /* + * Trap and execute EINIT if launch control is enabled in the + * host using the guest's values for launch control MSRs, even + * if the guest's values are fixed to hardware default values. + * The MSRs are not loaded/saved on VM-Enter/VM-Exit as writing + * the MSRs is extraordinarily expensive. + */ + if (boot_cpu_has(X86_FEATURE_SGX_LC)) + bitmap |= (1 << EINIT); + + if (!vmcs12 && is_guest_mode(vcpu)) + vmcs12 = get_vmcs12(vcpu); + if (vmcs12 && nested_cpu_has_encls_exit(vmcs12)) + bitmap |= vmcs12->encls_exiting_bitmap; + } + vmcs_write64(ENCLS_EXITING_BITMAP, bitmap); +} diff --git a/arch/x86/kvm/vmx/sgx.h b/arch/x86/kvm/vmx/sgx.h index 6502fa52c7e9..a400888b376d 100644 --- a/arch/x86/kvm/vmx/sgx.h +++ b/arch/x86/kvm/vmx/sgx.h @@ -4,6 +4,9 @@ #include <linux/kvm_host.h> +#include "capabilities.h" +#include "vmx_ops.h" + #ifdef CONFIG_X86_SGX_KVM extern bool __read_mostly enable_sgx; @@ -11,11 +14,21 @@ int handle_encls(struct kvm_vcpu *vcpu); void setup_default_sgx_lepubkeyhash(void); void vcpu_setup_sgx_lepubkeyhash(struct kvm_vcpu *vcpu); + +void vmx_write_encls_bitmap(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12); #else #define enable_sgx 0 static inline void setup_default_sgx_lepubkeyhash(void) { } static inline void vcpu_setup_sgx_lepubkeyhash(struct kvm_vcpu *vcpu) { } + +static inline void vmx_write_encls_bitmap(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + /* Nothing to do if hardware doesn't support SGX */ + if (cpu_has_vmx_encls_vmexit()) + vmcs_write64(ENCLS_EXITING_BITMAP, -1ull); +} #endif #endif /* __KVM_X86_SGX_H */ diff --git a/arch/x86/kvm/vmx/vmcs12.c b/arch/x86/kvm/vmx/vmcs12.c index c8e51c004f78..034adb6404dc 100644 --- a/arch/x86/kvm/vmx/vmcs12.c +++ b/arch/x86/kvm/vmx/vmcs12.c @@ -50,6 +50,7 @@ const unsigned short vmcs_field_to_offset_table[] = { FIELD64(VMREAD_BITMAP, vmread_bitmap), FIELD64(VMWRITE_BITMAP, vmwrite_bitmap), FIELD64(XSS_EXIT_BITMAP, xss_exit_bitmap), + FIELD64(ENCLS_EXITING_BITMAP, encls_exiting_bitmap), FIELD64(GUEST_PHYSICAL_ADDRESS, guest_physical_address), FIELD64(VMCS_LINK_POINTER, vmcs_link_pointer), FIELD64(GUEST_IA32_DEBUGCTL, guest_ia32_debugctl), diff --git a/arch/x86/kvm/vmx/vmcs12.h b/arch/x86/kvm/vmx/vmcs12.h index 80232daf00ff..13494956d0e9 100644 --- a/arch/x86/kvm/vmx/vmcs12.h +++ b/arch/x86/kvm/vmx/vmcs12.h @@ -69,7 +69,8 @@ struct __packed vmcs12 { u64 vm_function_control; u64 eptp_list_address; u64 pml_address; - u64 padding64[3]; /* room for future expansion */ + u64 encls_exiting_bitmap; + u64 padding64[2]; /* room for future expansion */ /* * To allow migration of L1 (complete with its L2 guests) between * machines of different natural widths (32 or 64 bit), we cannot have @@ -256,6 +257,7 @@ static inline void vmx_check_vmcs12_offsets(void) CHECK_OFFSET(vm_function_control, 296); CHECK_OFFSET(eptp_list_address, 304); CHECK_OFFSET(pml_address, 312); + CHECK_OFFSET(encls_exiting_bitmap, 320); CHECK_OFFSET(cr0_guest_host_mask, 344); CHECK_OFFSET(cr4_guest_host_mask, 352); CHECK_OFFSET(cr0_read_shadow, 360); diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index ccf49f596f28..184dbfbca348 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2222,6 +2222,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) vmx->msr_ia32_feature_control = data; if (msr_info->host_initiated && data == 0) vmx_leave_nested(vcpu); + + /* SGX may be enabled/disabled by guest's firmware */ + vmx_write_encls_bitmap(vcpu, NULL); break; case MSR_IA32_SGXLEPUBKEYHASH0 ... MSR_IA32_SGXLEPUBKEYHASH3: if (!msr_info->host_initiated && @@ -4377,6 +4380,15 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx) if (!vcpu->kvm->arch.bus_lock_detection_enabled) exec_control &= ~SECONDARY_EXEC_BUS_LOCK_DETECTION; + if (cpu_has_vmx_encls_vmexit() && nested) { + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX)) + vmx->nested.msrs.secondary_ctls_high |= + SECONDARY_EXEC_ENCLS_EXITING; + else + vmx->nested.msrs.secondary_ctls_high &= + ~SECONDARY_EXEC_ENCLS_EXITING; + } + vmx->secondary_exec_control = exec_control; } @@ -4467,8 +4479,7 @@ static void init_vmcs(struct vcpu_vmx *vmx) vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1); } - if (cpu_has_vmx_encls_vmexit()) - vmcs_write64(ENCLS_EXITING_BITMAP, -1ull); + vmx_write_encls_bitmap(&vmx->vcpu, NULL); if (vmx_pt_mode_is_host_guest()) { memset(&vmx->pt_desc, 0, sizeof(vmx->pt_desc)); @@ -7325,6 +7336,19 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) set_cr4_guest_host_mask(vmx); + vmx_write_encls_bitmap(vcpu, NULL); + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX)) + vmx->msr_ia32_feature_control_valid_bits |= FEAT_CTL_SGX_ENABLED; + else + vmx->msr_ia32_feature_control_valid_bits &= ~FEAT_CTL_SGX_ENABLED; + + if (guest_cpuid_has(vcpu, X86_FEATURE_SGX_LC)) + vmx->msr_ia32_feature_control_valid_bits |= + FEAT_CTL_SGX_LC_ENABLED; + else + vmx->msr_ia32_feature_control_valid_bits &= + ~FEAT_CTL_SGX_LC_ENABLED; + /* Refresh #PF interception to account for MAXPHYADDR changes. */ vmx_update_exception_bitmap(vcpu); } @@ -7345,6 +7369,13 @@ static __init void vmx_set_cpu_caps(void) if (vmx_pt_mode_is_host_guest()) kvm_cpu_cap_check_and_set(X86_FEATURE_INTEL_PT); + if (!enable_sgx) { + kvm_cpu_cap_clear(X86_FEATURE_SGX); + kvm_cpu_cap_clear(X86_FEATURE_SGX_LC); + kvm_cpu_cap_clear(X86_FEATURE_SGX1); + kvm_cpu_cap_clear(X86_FEATURE_SGX2); + } + if (vmx_umip_emulated()) kvm_cpu_cap_set(X86_FEATURE_UMIP);