From: Kai Huang <hidden> Date: 2021-03-09 01:39:40
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
This series is based against latest upstream kernel master branch.
You can also get the code from upstream branch of kvm-sgx repo on github:
https://github.com/intel/kvm-sgx.git upstream
It also requires Qemu changes to create VM with SGX support. You can find Qemu
repo here:
https://github.com/intel/qemu-sgx.git upstream
Please refer to README.md of above qemu-sgx repo for detail on how to create
guest with SGX support. At meantime, for your quick reference you can use below
command to create SGX guest:
#qemu-system-x86_64 -smp 4 -m 2G -drive file=<your_vm_image>,if=virtio \
-cpu host,+sgx_provisionkey \
-sgx-epc id=epc1,memdev=mem1 \
-object memory-backend-epc,id=mem1,size=64M,prealloc
Please note that the SGX relevant part is:
-cpu host,+sgx_provisionkey \
-sgx-epc id=epc1,memdev=mem1 \
-object memory-backend-epc,id=mem1,size=64M,prealloc
And you can change other parameters of your qemu command based on your needs.
=========
Changelog:
(Changelog here is for global changes. Please see each patch's changelog for
changes made to specific patch.)
v1->v2:
- No big change in design, structural of patch series, etc.
- Addressed Boris's comments regarding to suppressing both SGX1 and SGX2 in
/proc/cpuinfo, and improvement in feat_ctl.c when enabling SGX (patch 2
and 6).
- Addressed Sean's comments for both x86 part patches and KVM patches (patch 3,
5, 9, 12, 19, 21).
- Addressed Dave's comments in RFC v6 series (patch 13).
RFC->v1:
- Refined patch (x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()) to print
error msg that EPC page is leaked when EREMOVE failed, requested by Dave.
- Changelog history of all RFC series is removed in both this cover letter
and each individual patch, since majority of x86 part patches already got
Acked-by from Dave and Jarkko. And the changelogs are not quite useful from
my perspective.
=========
KVM SGX virtualization Overview
- Virtual EPC
SGX enclave memory is special and is reserved specifically for enclave use.
In bare-metal SGX enclaves, the kernel allocates enclave pages, copies data
into the pages with privileged instructions, then allows the enclave to start.
In this scenario, only initialized pages already assigned to an enclave are
mapped to userspace.
In virtualized environments, the hypervisor still needs to do the physical
enclave page allocation. The guest kernel is responsible for the data copying
(among other things). This means that the job of starting an enclave is now
split between hypervisor and guest.
This series introduces a new misc device: /dev/sgx_vepc. This device allows
the host to map *uninitialized* enclave memory into userspace, which can then
be passed into a guest.
While it might be *possible* to start a host-side enclave with /dev/sgx_enclave
and pass its memory into a guest, it would be wasteful and convoluted.
Implement the *raw* EPC allocation in the x86 core-SGX subsystem via
/dev/sgx_vepc rather than in KVM. Doing so has two major advantages:
- Does not require changes to KVM's uAPI, e.g. EPC gets handled as
just another memory backend for guests.
- EPC management is wholly contained in the SGX subsystem, e.g. SGX
does not have to export any symbols, changes to reclaim flows don't
need to be routed through KVM, SGX's dirty laundry doesn't have to
get aired out for the world to see, and so on and so forth.
The virtual EPC pages allocated to guests are currently not reclaimable.
Reclaiming EPC page used by enclave requires a special reclaim mechanism
separate from normal page reclaim, and that mechanism is not supported
for virutal EPC pages. Due to the complications of handling reclaim
conflicts between guest and host, reclaiming virtual EPC pages is
significantly more complex than basic support for SGX virtualization.
- Support SGX virtualization without SGX Flexible Launch Control
SGX hardware supports two "launch control" modes to limit which enclaves can
run. In the "locked" mode, the hardware prevents enclaves from running unless
they are blessed by a third party. In the unlocked mode, the kernel is in
full control of which enclaves can run. The bare-metal SGX code refuses to
launch enclaves unless it is in the unlocked mode.
This sgx_virt_epc driver does not have such a restriction. This allows guests
which are OK with the locked mode to use SGX, even if the host kernel refuses
to.
- Support exposing SGX2
Due to the same reason above, SGX2 feature detection is added to core SGX code
to allow KVM to expose SGX2 to guest, even currently SGX driver doesn't support
SGX2, because SGX2 can work just fine in guest w/o any interaction to host SGX
driver.
- Restricit SGX guest access to provisioning key
To grant guest being able to fully use SGX, guest needs to be able to access
provisioning key. The provisioning key is sensitive, and accessing to it should
be restricted. In bare-metal driver, allowing enclave to access provisioning key
is restricted by being able to open /dev/sgx_provision.
Add a new KVM_CAP_SGX_ATTRIBUTE to KVM uAPI to extend above mechanism to KVM
guests as well. When userspace hypervisor creates a new VM, the new cap is only
added to VM when userspace hypervisior is able to open /dev/sgx_provision,
following the same role as in bare-metal driver. KVM then traps ECREATE from
guest, and only allows ECREATE with provisioning key bit to run when guest
supports KVM_CAP_SGX_ATTRIBUTE.
Jarkko Sakkinen (1):
x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()
Kai Huang (3):
x86/cpufeatures: Make SGX_LC feature bit depend on SGX bit
x86/sgx: Initialize virtual EPC driver even when SGX driver is
disabled
x86/sgx: Add helper to update SGX_LEPUBKEYHASHn MSRs
Sean Christopherson (21):
x86/cpufeatures: Add SGX1 and SGX2 sub-features
x86/sgx: Add SGX_CHILD_PRESENT hardware error code
x86/sgx: Introduce virtual EPC for use by KVM guests
x86/cpu/intel: Allow SGX virtualization without Launch Control support
x86/sgx: Expose SGX architectural definitions to the kernel
x86/sgx: Move ENCLS leaf definitions to sgx.h
x86/sgx: Add SGX2 ENCLS leaf definitions (EAUG, EMODPR and EMODT)
x86/sgx: Add encls_faulted() helper
x86/sgx: Add helpers to expose ECREATE and EINIT to KVM
x86/sgx: Move provisioning device creation out of SGX driver
KVM: x86: Export kvm_mmu_gva_to_gpa_{read,write}() for SGX (VMX)
KVM: x86: Define new #PF SGX error code bit
KVM: x86: Add support for reverse CPUID lookup of scattered features
KVM: x86: Add reverse-CPUID lookup support for scattered SGX features
KVM: VMX: Add basic handling of VM-Exit from SGX enclave
KVM: VMX: Frame in ENCLS handler for SGX virtualization
KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions
KVM: VMX: Add emulation of SGX Launch Control LE hash MSRs
KVM: VMX: Add ENCLS[EINIT] handler to support SGX Launch Control (LC)
KVM: VMX: Enable SGX virtualization for SGX1, SGX2 and LC
KVM: x86: Add capability to grant VM access to privileged SGX
attribute
Documentation/virt/kvm/api.rst | 23 +
arch/x86/Kconfig | 12 +
arch/x86/include/asm/cpufeatures.h | 2 +
arch/x86/include/asm/kvm_host.h | 5 +
.../cpu/sgx/arch.h => include/asm/sgx.h} | 50 +-
arch/x86/include/asm/vmx.h | 1 +
arch/x86/include/uapi/asm/vmx.h | 1 +
arch/x86/kernel/cpu/cpuid-deps.c | 3 +
arch/x86/kernel/cpu/feat_ctl.c | 71 ++-
arch/x86/kernel/cpu/scattered.c | 2 +
arch/x86/kernel/cpu/sgx/Makefile | 1 +
arch/x86/kernel/cpu/sgx/driver.c | 17 -
arch/x86/kernel/cpu/sgx/encl.c | 29 +-
arch/x86/kernel/cpu/sgx/encls.h | 30 +-
arch/x86/kernel/cpu/sgx/ioctl.c | 23 +-
arch/x86/kernel/cpu/sgx/main.c | 94 +++-
arch/x86/kernel/cpu/sgx/sgx.h | 13 +-
arch/x86/kernel/cpu/sgx/virt.c | 370 ++++++++++++++
arch/x86/kvm/Makefile | 2 +
arch/x86/kvm/cpuid.c | 89 +++-
arch/x86/kvm/cpuid.h | 50 +-
arch/x86/kvm/vmx/nested.c | 28 +-
arch/x86/kvm/vmx/nested.h | 5 +
arch/x86/kvm/vmx/sgx.c | 481 ++++++++++++++++++
arch/x86/kvm/vmx/sgx.h | 34 ++
arch/x86/kvm/vmx/vmcs12.c | 1 +
arch/x86/kvm/vmx/vmcs12.h | 4 +-
arch/x86/kvm/vmx/vmx.c | 109 +++-
arch/x86/kvm/vmx/vmx.h | 2 +
arch/x86/kvm/x86.c | 23 +
include/uapi/linux/kvm.h | 1 +
tools/testing/selftests/sgx/defines.h | 2 +-
32 files changed, 1460 insertions(+), 118 deletions(-)
rename arch/x86/{kernel/cpu/sgx/arch.h => include/asm/sgx.h} (89%)
create mode 100644 arch/x86/kernel/cpu/sgx/virt.c
create mode 100644 arch/x86/kvm/vmx/sgx.c
create mode 100644 arch/x86/kvm/vmx/sgx.h
--
2.29.2
From: Kai Huang <hidden> Date: 2021-03-09 01:40:13
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.
Suppress both SGX1 and SGX2 from /proc/cpuinfo. SGX1 basically means
SGX, and for SGX2 there is no concrete use case of using it in
/proc/cpuinfo.
Signed-off-by: Sean Christopherson <seanjc@google.com>
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Changed to hide both SGX1 and SGX2 from /proc/cpuinfo, since no concrete
use case, per Boris.
- Refined commit msg to explain why to hide SGX1 and SGX2 in /proc/cpuinfo.
---
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(+)
From: Kai Huang <hidden> Date: 2021-03-09 01:40:13
From: Sean Christopherson <redacted>
SGX driver can accurately track how enclave pages are used. This
enables SECS to be specifically targeted and EREMOVE'd only after all
child pages have been EREMOVE'd. This ensures that SGX driver will
never encounter SGX_CHILD_PRESENT in normal operation.
Virtual EPC is different. The host does not track how EPC pages are
used by the guest, so it cannot guarantee EREMOVE success. It might,
for instance, encounter a SECS with a non-zero child count.
Add a definition of SGX_CHILD_PRESENT. It will be used exclusively by
the SGX virtualization driver to handle recoverable EREMOVE errors when
saniziting EPC pages after they are freed.
Signed-off-by: Sean Christopherson <redacted>
Acked-by: Dave Hansen <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/arch.h | 2 ++
1 file changed, 2 insertions(+)
From: Kai Huang <hidden> Date: 2021-03-09 01:40:13
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Merge original WARN() and pr_err_once() into one single WARN(), suggested
by Sean.
---
arch/x86/kernel/cpu/sgx/encl.c | 27 ++++++++++++++++++++++++---
arch/x86/kernel/cpu/sgx/main.c | 12 ++++--------
2 files changed, 28 insertions(+), 11 deletions(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:40:14
From: Sean Christopherson <redacted>
Add a misc device /dev/sgx_vepc to allow userspace to allocate "raw" EPC
without an associated enclave. The intended and only known use case for
raw EPC allocation is to expose EPC to a KVM guest, hence the 'vepc'
moniker, virt.{c,h} files and X86_SGX_KVM Kconfig.
SGX driver uses misc device /dev/sgx_enclave to support userspace to
create enclave. Each file descriptor from opening /dev/sgx_enclave
represents an enclave. Unlike SGX driver, KVM doesn't control how guest
uses EPC, therefore EPC allocated to KVM guest is not associated to an
enclave, and /dev/sgx_enclave is not suitable for allocating EPC for KVM
guest.
Having separate device nodes for SGX driver and KVM virtual EPC also
allows separate permission control for running host SGX enclaves and
KVM SGX guests.
To use /dev/sgx_vepc to allocate a virtual EPC instance with particular
size, the userspace hypervisor opens /dev/sgx_vepc, and uses mmap()
with the intended size to get an address range of virtual EPC. Then
it may use the address range to create one KVM memory slot as virtual
EPC for guest.
Implement the "raw" EPC allocation in the x86 core-SGX subsystem via
/dev/sgx_vepc rather than in KVM. Doing so has two major advantages:
- Does not require changes to KVM's uAPI, e.g. EPC gets handled as
just another memory backend for guests.
- EPC management is wholly contained in the SGX subsystem, e.g. SGX
does not have to export any symbols, changes to reclaim flows don't
need to be routed through KVM, SGX's dirty laundry doesn't have to
get aired out for the world to see, and so on and so forth.
The virtual EPC pages allocated to guests are currently not reclaimable.
Reclaiming EPC page used by enclave requires a special reclaim mechanism
separate from normal page reclaim, and that mechanism is not supported
for virutal EPC pages. Due to the complications of handling reclaim
conflicts between guest and host, reclaiming virtual EPC pages is
significantly more complex than basic support for SGX virtualization.
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Kai Huang <redacted>
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Addressed one typo in comments for last round of zapping zombie_secs_pages,
pointed out by Sean.
---
arch/x86/Kconfig | 12 ++
arch/x86/kernel/cpu/sgx/Makefile | 1 +
arch/x86/kernel/cpu/sgx/sgx.h | 9 ++
arch/x86/kernel/cpu/sgx/virt.c | 260 +++++++++++++++++++++++++++++++
4 files changed, 282 insertions(+)
create mode 100644 arch/x86/kernel/cpu/sgx/virt.c
From: Kai Huang <hidden> Date: 2021-03-09 01:40:14
Move SGX_LC feature bit to CPUID dependency table to make clearing all
SGX feature bits easier. Also remove clear_sgx_caps() since it is just
a wrapper of setup_clear_cpu_cap(X86_FEATURE_SGX) now.
Suggested-by: Sean Christopherson <seanjc@google.com>
Acked-by: Dave Hansen <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/cpuid-deps.c | 1 +
arch/x86/kernel/cpu/feat_ctl.c | 12 +++---------
2 files changed, 4 insertions(+), 9 deletions(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:40:45
From: Sean Christopherson <redacted>
The kernel will currently disable all SGX support if the hardware does
not support launch control. Make it more permissive to allow SGX
virtualization on systems without Launch Control support. This will
allow KVM to expose SGX to guests that have less-strict requirements on
the availability of flexible launch control.
Improve error message to distinguish between three cases. There are two
cases where SGX support is completely disabled:
1) SGX has been disabled completely by the BIOS
2) SGX LC is locked by the BIOS. Bare-metal support is disabled because
of LC unavailability. SGX virtualization is unavailable (because of
Kconfig).
One where it is partially available:
3) SGX LC is locked by the BIOS. Bare-metal support is disabled because
of LC unavailability. SGX virtualization is supported.
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Kai Huang <redacted>
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Changed order of declaration of variables so that they are in
reverse-christmas tree pattern.
- Removed enable_sgx_any, suggested by Boris.
---
arch/x86/kernel/cpu/feat_ctl.c | 59 +++++++++++++++++++++++++---------
1 file changed, 44 insertions(+), 15 deletions(-)
@@ -167,10 +177,29 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)}update_sgx:-if(!(msr&FEAT_CTL_SGX_ENABLED)||-!(msr&FEAT_CTL_SGX_LC_ENABLED)||!enable_sgx){-if(enable_sgx)-pr_err_once("SGX disabled by BIOS\n");+if(!(msr&FEAT_CTL_SGX_ENABLED)){+if(enable_sgx_kvm||enable_sgx_driver)+pr_err_once("SGX disabled by BIOS.\n");clear_cpu_cap(c,X86_FEATURE_SGX);+return;+}++/*+*VMXfeaturebitmaybeclearedduetobeingdisabledinBIOS,+*inwhichcaseSGXvirtualizationcannotbesupportedeither.+*/+if(!cpu_has(c,X86_FEATURE_VMX)&&enable_sgx_kvm){+pr_err_once("SGX virtualization disabled due to lack of VMX.\n");+enable_sgx_kvm=0;+}++if(!(msr&FEAT_CTL_SGX_LC_ENABLED)&&enable_sgx_driver){+if(!enable_sgx_kvm){+pr_err_once("SGX Launch Control is locked. Disable SGX.\n");+clear_cpu_cap(c,X86_FEATURE_SGX);+}else{+pr_err_once("SGX Launch Control is locked. Support SGX virtualization only.\n");+clear_cpu_cap(c,X86_FEATURE_SGX_LC);+}}}
From: Kai Huang <hidden> Date: 2021-03-09 01:40:45
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:40:46
From: Sean Christopherson <redacted>
Expose SGX architectural structures, as KVM will use many of the
architectural constants and structs to virtualize SGX.
Name the new header file as asm/sgx.h, rather than asm/sgx_arch.h, to
have single header to provide SGX facilities to share with other kernel
componments.
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Kai Huang <redacted>
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
.../cpu/sgx/arch.h => include/asm/sgx.h} | 20 ++++++++++++++-----
arch/x86/kernel/cpu/sgx/encl.c | 2 +-
arch/x86/kernel/cpu/sgx/sgx.h | 2 +-
tools/testing/selftests/sgx/defines.h | 2 +-
4 files changed, 18 insertions(+), 8 deletions(-)
rename arch/x86/{kernel/cpu/sgx/arch.h => include/asm/sgx.h} (95%)
diff --git a/arch/x86/kernel/cpu/sgx/arch.h b/arch/x86/include/asm/sgx.hsimilarity index 95%rename from arch/x86/kernel/cpu/sgx/arch.hrename to arch/x86/include/asm/sgx.hindex abf99bb71fdc..d4ad35f6319a 100644--- a/arch/x86/kernel/cpu/sgx/arch.h+++ b/arch/x86/include/asm/sgx.h
@@ -2,15 +2,20 @@/***Copyright(c)2016-20IntelCorporation.*-*ContainsdatastructuresdefinedbytheSGXarchitecture.Datastructures-*definedbytheLinuxsoftwarestackshouldnotbeplacedhere.+*IntelSoftwareGuardExtensions(SGX)support.*/-#ifndef _ASM_X86_SGX_ARCH_H-#define _ASM_X86_SGX_ARCH_H+#ifndef _ASM_X86_SGX_H+#define _ASM_X86_SGX_H#include<linux/bits.h>#include<linux/types.h>+/*+*ThisfilecontainsbothdatastructuresdefinedbySGXarchitectureandLinux+*definedsoftwaredatastructuresandfunctions.Thetwoshouldnotbemixed+*togetherforbetterreadibility.Thearchitecturaldefinitionscomefirst.+*/+/* The SGX specific CPUID function. */#define SGX_CPUID 0x12/* EPC enumeration. */
From: Kai Huang <hidden> Date: 2021-03-09 01:40:46
From: Sean Christopherson <redacted>
Move the ENCLS leaf definitions to sgx.h so that they can be used by
KVM.
Signed-off-by: Sean Christopherson <redacted>
Acked-by: Dave Hansen <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Removed "And because they're architectural." in commit msg, per Sean.
---
arch/x86/include/asm/sgx.h | 15 +++++++++++++++
arch/x86/kernel/cpu/sgx/encls.h | 15 ---------------
2 files changed, 15 insertions(+), 15 deletions(-)
@@ -27,6 +27,21 @@/* The bitmask for the EPC section type. */#define SGX_CPUID_EPC_MASK GENMASK(3, 0)+enumsgx_encls_function{+ECREATE=0x00,+EADD=0x01,+EINIT=0x02,+EREMOVE=0x03,+EDGBRD=0x04,+EDGBWR=0x05,+EEXTEND=0x06,+ELDU=0x08,+EBLOCK=0x09,+EPA=0x0A,+EWB=0x0B,+ETRACK=0x0C,+};+/***enumsgx_return_code-ThereturncodetypeforENCLS,ENCLUandENCLV*%SGX_NOT_TRACKED:PreviousETRACK'sshootdownsequencehasnot
From: Kai Huang <hidden> Date: 2021-03-09 01:41:17
From: Sean Christopherson <redacted>
Define the ENCLS leafs that are available with SGX2, also referred to as
Enclave Dynamic Memory Management (EDMM). The leafs will be used by KVM
to conditionally expose SGX2 capabilities to guests.
Signed-off-by: Sean Christopherson <redacted>
Acked-by: Dave Hansen <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/include/asm/sgx.h | 3 +++
1 file changed, 3 insertions(+)
From: Kai Huang <hidden> Date: 2021-03-09 01:41:17
Add a helper to update SGX_LEPUBKEYHASHn MSRs. SGX virtualization also
needs to update those MSRs based on guest's "virtual" SGX_LEPUBKEYHASHn
before EINIT from guest.
Acked-by: Dave Hansen <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Added WARN_ON_ONCE(preemptible()) check, per Sean.
---
arch/x86/kernel/cpu/sgx/ioctl.c | 5 ++---
arch/x86/kernel/cpu/sgx/main.c | 17 +++++++++++++++++
arch/x86/kernel/cpu/sgx/sgx.h | 2 ++
3 files changed, 21 insertions(+), 3 deletions(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:41:17
From: Sean Christopherson <redacted>
Add a helper to extract the fault indicator from an encoded ENCLS return
value. SGX virtualization will also need to detect ENCLS faults.
Signed-off-by: Sean Christopherson <redacted>
Acked-by: Dave Hansen <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/encls.h | 15 ++++++++++++++-
arch/x86/kernel/cpu/sgx/ioctl.c | 2 +-
2 files changed, 15 insertions(+), 2 deletions(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:41:17
From: Sean Christopherson <redacted>
And extract sgx_set_attribute() out of sgx_ioc_enclave_provision() and
export it as symbol for KVM to use.
Provisioning key is sensitive. SGX driver only allows to create enclave
which can access provisioning key when enclave creator has permission to
open /dev/sgx_provision. It should apply to VM as well, as provisioning
key is platform specific, thus unrestricted VM can also potentially
compromise provisioning key.
Move provisioning device creation out of sgx_drv_init() to sgx_init() as
preparation for adding SGX virtualization support, so that even SGX
driver is not enabled due to flexible launch control is not available,
SGX virtualization can still be enabled, and use it to restrict VM's
capability of being able to access provisioning key.
Signed-off-by: Sean Christopherson <redacted>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/include/asm/sgx.h | 3 ++
arch/x86/kernel/cpu/sgx/driver.c | 17 ----------
arch/x86/kernel/cpu/sgx/ioctl.c | 16 ++-------
arch/x86/kernel/cpu/sgx/main.c | 57 +++++++++++++++++++++++++++++++-
4 files changed, 61 insertions(+), 32 deletions(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:41:17
From: Sean Christopherson <redacted>
The host kernel must intercept ECREATE to impose policies on guests, and
intercept EINIT to be able to write guest's virtual SGX_LEPUBKEYHASH MSR
values to hardware before running guest's EINIT so it can run correctly
according to hardware behavior.
Provide wrappers around __ecreate() and __einit() to hide the ugliness
of overloading the ENCLS return value to encode multiple error formats
in a single int. KVM will trap-and-execute ECREATE and EINIT as part
of SGX virtualization, and reflect ENCLS execution result to guest by
setting up guest's GPRs, or on an exception, injecting the correct fault
based on return value of __ecreate() and __einit().
Use host userspace addresses (provided by KVM based on guest physical
address of ENCLS parameters) to execute ENCLS/EINIT when possible.
Accesses to both EPC and memory originating from ENCLS are subject to
segmentation and paging mechanisms. It's also possible to generate
kernel mappings for ENCLS parameters by resolving PFN but using
__uaccess_xx() is simpler.
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Tried to address Dave's comments:
- Refined comments around @secs in sgx_virt_ecreate().
- Refined commit msg to explain why to use userspace address for ENCLS,
instead of generating kernel mapping.
- Added access_ok() on userspace addresses, and give WARN() if check fails.
---
arch/x86/include/asm/sgx.h | 7 +++
arch/x86/kernel/cpu/sgx/virt.c | 110 +++++++++++++++++++++++++++++++++
2 files changed, 117 insertions(+)
@@ -258,3 +258,113 @@ int __init sgx_vepc_init(void)returnmisc_register(&sgx_vepc_dev);}++/**+*sgx_virt_ecreate()-RunECREATEonbehalfofguest+*@pageinfo:PointertoPAGEINFOstructure+*@secs:UserspacepointertoSECSpage+*@trapnr:trapnumberinjectedtoguestincaseofECREATEerror+*+*RunECREATEonbehalfofguestafterKVMtrapsECREATEforthepurpose+*ofenforcingpoliciesofguest'senclaves,andreturnthetrapnumber+*whichshouldbeinjectedtoguestincaseofanyECREATEerror.+*+*Return:+*-0:ECREATEwassuccessful.+*--EFAULT:ECREATEreturnederror.+*/+intsgx_virt_ecreate(structsgx_pageinfo*pageinfo,void__user*secs,+int*trapnr)+{+intret;++/*+*@secsisanuntrusted,userspace-providedaddress.Itcomesfrom+*KVMandisassumedtobeavalidpointerwhichpointssomewherein+*userspace.ThiscanfaultandcallSGXorotherfaulthandlerswhen+*userspacemapping@secsdoesn'texist.+*+*AddaWARN()tomakesure@secsisalreadyvaliduserspacepointer+*fromcaller(KVM),whoshouldalreadyhavehandledinvalidpointer+*case(forinstance,madebymaliciousguest).Allotherchecks,+*suchasalignmentof@secs,aredeferredtoENCLSitself.+*/+WARN_ON_ONCE(!access_ok(PTR_ALIGN_DOWN(secs,PAGE_SIZE),PAGE_SIZE));+__uaccess_begin();+ret=__ecreate(pageinfo,(void*)secs);+__uaccess_end();++if(encls_faulted(ret)){+*trapnr=ENCLS_TRAPNR(ret);+return-EFAULT;+}++/* ECREATE doesn't return an error code, it faults or succeeds. */+WARN_ON_ONCE(ret);+return0;+}+EXPORT_SYMBOL_GPL(sgx_virt_ecreate);++staticint__sgx_virt_einit(void__user*sigstruct,void__user*token,+void__user*secs)+{+intret;++/*+*Makesurealluserspacepointersfromcaller(KVM)arevalid.+*AllotherchecksdeferredtoENCLSitself.Alsoseecomment+*for@secsinsgx_virt_ecreate().+*/+WARN_ON_ONCE(!access_ok(PTR_ALIGN_DOWN(sigstruct,PAGE_SIZE),+PAGE_SIZE)||+!access_ok(PTR_ALIGN_DOWN(token,PAGE_SIZE),PAGE_SIZE)||+!access_ok(PTR_ALIGN_DOWN(secs,PAGE_SIZE),PAGE_SIZE));+__uaccess_begin();+ret=__einit((void*)sigstruct,(void*)token,(void*)secs);+__uaccess_end();++returnret;+}++/**+*sgx_virt_einit()-RunEINITonbehalfofguest+*@sigstruct:UserspacepointertoSIGSTRUCTstructure+*@token:UserspacepointertoEINITTOKENstructure+*@secs:UserspacepointertoSECSpage+*@lepubkeyhash:Pointertoguest's*virtual*SGX_LEPUBKEYHASHMSR+*values+*@trapnr:trapnumberinjectedtoguestincaseofEINITerror+*+*RunEINITonbehalfofguestafterKVMtrapsEINIT.IfSGX_LCisavailable+*inhost,SGXdrivermayrewritethehardwarevaluesatwish,thereforeKVM+*needstoupdatehardwarevaluestoguest'svirtualMSRvaluesinorderto+*ensureEINITisexecutedwithexpectedhardwarevalues.+*+*Return:+*-0:EINITwassuccessful.+*--EFAULT:EINITreturnederror.+*/+intsgx_virt_einit(void__user*sigstruct,void__user*token,+void__user*secs,u64*lepubkeyhash,int*trapnr)+{+intret;++if(!boot_cpu_has(X86_FEATURE_SGX_LC)){+ret=__sgx_virt_einit(sigstruct,token,secs);+}else{+preempt_disable();++sgx_update_lepubkeyhash(lepubkeyhash);++ret=__sgx_virt_einit(sigstruct,token,secs);+preempt_enable();+}++if(encls_faulted(ret)){+*trapnr=ENCLS_TRAPNR(ret);+return-EFAULT;+}++returnret;+}+EXPORT_SYMBOL_GPL(sgx_virt_einit);
From: Kai Huang <hidden> Date: 2021-03-09 01:41:48
From: Sean Christopherson <redacted>
Export the gva_to_gpa() helpers for use by SGX virtualization when
executing ENCLS[ECREATE] and ENCLS[EINIT] on behalf of the guest.
To execute ECREATE and EINIT, KVM must obtain the GPA of the target
Secure Enclave Control Structure (SECS) in order to get its
corresponding HVA.
Because the SECS must reside in the Enclave Page Cache (EPC), copying
the SECS's data to a host-controlled buffer via existing exported
helpers is not a viable option as the EPC is not readable or writable
by the kernel.
SGX virtualization will also use gva_to_gpa() to obtain HVAs for
non-EPC pages in order to pass user pointers directly to ECREATE and
EINIT, which avoids having to copy pages worth of data into the kernel.
Signed-off-by: Sean Christopherson <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kvm/x86.c | 2 ++
1 file changed, 2 insertions(+)
From: Kai Huang <hidden> Date: 2021-03-09 01:41:49
From: Sean Christopherson <seanjc@google.com>
Introduce a scheme that allows KVM's CPUID magic to support features
that are scattered in the kernel's feature words. To advertise and/or
query guest support for CPUID-based features, KVM requires the bit
number of an X86_FEATURE_* to match the bit number in its associated
CPUID entry. For scattered features, this does not hold true.
Add a framework to allow defining KVM-only words, stored in
kvm_cpu_caps after the shared kernel caps, that can be used to gather
the scattered feature bits by translating X86_FEATURE_* flags into their
KVM-defined feature.
Note, because reverse_cpuid_check() effectively forces kvm_cpu_caps
lookups to be resolved at compile time, there is no runtime cost for
translating from kernel-defined to kvm-defined features.
More details here: https://lkml.kernel.org/r/X/jxCOLG+HUO4QlZ@google.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kvm/cpuid.c | 32 +++++++++++++++++++++++++++-----
arch/x86/kvm/cpuid.h | 39 ++++++++++++++++++++++++++++++++++-----
2 files changed, 61 insertions(+), 10 deletions(-)
@@ -347,13 +348,13 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,returnr;}-static__always_inlinevoidkvm_cpu_cap_mask(enumcpuid_leafsleaf,u32mask)+/* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */+static__always_inlinevoid__kvm_cpu_cap_mask(enumcpuid_leafsleaf){conststructcpuid_regcpuid=x86_feature_cpuid(leaf*32);structkvm_cpuid_entry2entry;reverse_cpuid_check(leaf);-kvm_cpu_caps[leaf]&=mask;cpuid_count(cpuid.function,cpuid.index,&entry.eax,&entry.ebx,&entry.ecx,&entry.edx);
@@ -361,6 +362,26 @@ static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask)kvm_cpu_caps[leaf]&=*__cpuid_entry_get_reg(&entry,cpuid.reg);}+static__always_inlinevoidkvm_cpu_cap_mask(enumcpuid_leafsleaf,u32mask)+{+/* Use the "init" variant for scattered leafs. */+BUILD_BUG_ON(leaf>=NCAPINTS);++kvm_cpu_caps[leaf]&=mask;++__kvm_cpu_cap_mask(leaf);+}++static__always_inlinevoidkvm_cpu_cap_init(enumcpuid_leafsleaf,u32mask)+{+/* Use the "mask" variant for hardwared-defined leafs. */+BUILD_BUG_ON(leaf<NCAPINTS);++kvm_cpu_caps[leaf]=mask;++__kvm_cpu_cap_mask(leaf);+}+voidkvm_set_cpu_caps(void){unsignedintf_nx=is_efer_nx()?F(NX):0;
From: Kai Huang <hidden> Date: 2021-03-09 01:41:50
From: Sean Christopherson <seanjc@google.com>
Define a new KVM-only feature word for advertising and querying SGX
sub-features in CPUID.0x12.0x0.EAX. Because SGX1 and SGX2 are scattered
in the kernel's feature word, they need to be translated so that the
bit numbers match those of hardware.
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kvm/cpuid.h | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:41:50
From: Sean Christopherson <redacted>
Page faults that are signaled by the SGX Enclave Page Cache Map (EPCM),
as opposed to the traditional IA32/EPT page tables, set an SGX bit in
the error code to indicate that the #PF was induced by SGX. KVM will
need to emulate this behavior as part of its trap-and-execute scheme for
virtualizing SGX Launch Control, e.g. to inject SGX-induced #PFs if
EINIT faults in the host, and to support live migration.
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/include/asm/kvm_host.h | 2 ++
1 file changed, 2 insertions(+)
From: Kai Huang <hidden> Date: 2021-03-09 01:41:50
From: Sean Christopherson <redacted>
Introduce sgx.c and sgx.h, along with the framework for handling ENCLS
VM-Exits. Add a bool, enable_sgx, that will eventually be wired up to a
module param to control whether or not SGX virtualization is enabled at
runtime.
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kvm/Makefile | 2 ++
arch/x86/kvm/vmx/sgx.c | 50 ++++++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/sgx.h | 15 +++++++++++++
arch/x86/kvm/vmx/vmx.c | 9 +++++---
4 files changed, 73 insertions(+), 3 deletions(-)
create mode 100644 arch/x86/kvm/vmx/sgx.c
create mode 100644 arch/x86/kvm/vmx/sgx.h
From: Kai Huang <hidden> Date: 2021-03-09 01:41:50
From: Sean Christopherson <redacted>
Add support for handling VM-Exits that originate from a guest SGX
enclave. In SGX, an "enclave" is a new CPL3-only execution environment,
wherein the CPU and memory state is protected by hardware to make the
state inaccesible to code running outside of the enclave. When exiting
an enclave due to an asynchronous event (from the perspective of the
enclave), e.g. exceptions, interrupts, and VM-Exits, the enclave's state
is automatically saved and scrubbed (the CPU loads synthetic state), and
then reloaded when re-entering the enclave. E.g. after an instruction
based VM-Exit from an enclave, vmcs.GUEST_RIP will not contain the RIP
of the enclave instruction that trigered VM-Exit, but will instead point
to a RIP in the enclave's untrusted runtime (the guest userspace code
that coordinates entry/exit to/from the enclave).
To help a VMM recognize and handle exits from enclaves, SGX adds bits to
existing VMCS fields, VM_EXIT_REASON.VMX_EXIT_REASON_FROM_ENCLAVE and
GUEST_INTERRUPTIBILITY_INFO.GUEST_INTR_STATE_ENCLAVE_INTR. Define the
new architectural bits, and add a boolean to struct vcpu_vmx to cache
VMX_EXIT_REASON_FROM_ENCLAVE. Clear the bit in exit_reason so that
checks against exit_reason do not need to account for SGX, e.g.
"if (exit_reason == EXIT_REASON_EXCEPTION_NMI)" continues to work.
KVM is a largely a passive observer of the new bits, e.g. KVM needs to
account for the bits when propagating information to a nested VMM, but
otherwise doesn't need to act differently for the majority of VM-Exits
from enclaves.
The one scenario that is directly impacted is emulation, which is for
all intents and purposes impossible[1] since KVM does not have access to
the RIP or instruction stream that triggered the VM-Exit. The inability
to emulate is a non-issue for KVM, as most instructions that might
trigger VM-Exit unconditionally #UD in an enclave (before the VM-Exit
check. For the few instruction that conditionally #UD, KVM either never
sets the exiting control, e.g. PAUSE_EXITING[2], or sets it if and only
if the feature is not exposed to the guest in order to inject a #UD,
e.g. RDRAND_EXITING.
But, because it is still possible for a guest to trigger emulation,
e.g. MMIO, inject a #UD if KVM ever attempts emulation after a VM-Exit
from an enclave. This is architecturally accurate for instruction
VM-Exits, and for MMIO it's the least bad choice, e.g. it's preferable
to killing the VM. In practice, only broken or particularly stupid
guests should ever encounter this behavior.
Add a WARN in skip_emulated_instruction to detect any attempt to
modify the guest's RIP during an SGX enclave VM-Exit as all such flows
should either be unreachable or must handle exits from enclaves before
getting to skip_emulated_instruction.
[1] Impossible for all practical purposes. Not truly impossible
since KVM could implement some form of para-virtualization scheme.
[2] PAUSE_LOOP_EXITING only affects CPL0 and enclaves exist only at
CPL3, so we also don't need to worry about that interaction.
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Add comment to explain why injecting #UD when emulating guest
instruction due to VM-EXIT from enclave, per Sean.
---
arch/x86/include/asm/vmx.h | 1 +
arch/x86/include/uapi/asm/vmx.h | 1 +
arch/x86/kvm/vmx/nested.c | 2 ++
arch/x86/kvm/vmx/vmx.c | 45 +++++++++++++++++++++++++++++++--
4 files changed, 47 insertions(+), 2 deletions(-)
From: Kai Huang <hidden> Date: 2021-03-09 01:42:21
From: Sean Christopherson <redacted>
Add an ECREATE handler that will be used to intercept ECREATE for the
purpose of enforcing and enclave's MISCSELECT, ATTRIBUTES and XFRM, i.e.
to allow userspace to restrict SGX features via CPUID. ECREATE will be
intercepted when any of the aforementioned masks diverges from hardware
in order to enforce the desired CPUID model, i.e. inject #GP if the
guest attempts to set a bit that hasn't been enumerated as allowed-1 in
CPUID.
Note, access to the PROVISIONKEY is not yet supported.
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Kai Huang <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1-v2:
- Fixed 'contents' leak problem pointed out by Sean.
---
arch/x86/include/asm/kvm_host.h | 3 +
arch/x86/kvm/vmx/sgx.c | 263 ++++++++++++++++++++++++++++++++
2 files changed, 266 insertions(+)
@@ -11,6 +11,267 @@bool__read_mostlyenable_sgx;+/*+*ENCLS'smemoryoperandsuseafixedsegment(DS)andafixed+*addresssizebasedonthemode.Relatedprefixesareignored.+*/+staticintsgx_get_encls_gva(structkvm_vcpu*vcpu,unsignedlongoffset,+intsize,intalignment,gva_t*gva)+{+structkvm_segments;+boolfault;++/* Skip vmcs.GUEST_DS retrieval for 64-bit mode to avoid VMREADs. */+*gva=offset;+if(!is_long_mode(vcpu)){+vmx_get_segment(vcpu,&s,VCPU_SREG_DS);+*gva+=s.base;+}++if(!IS_ALIGNED(*gva,alignment)){+fault=true;+}elseif(likely(is_long_mode(vcpu))){+fault=is_noncanonical_address(*gva,vcpu);+}else{+*gva&=0xffffffff;+fault=(s.unusable)||+(s.type!=2&&s.type!=3)||+(*gva>s.limit)||+((s.base!=0||s.limit!=0xffffffff)&&+(((u64)*gva+size-1)>s.limit+1));+}+if(fault)+kvm_inject_gp(vcpu,0);+returnfault?-EINVAL:0;+}++staticvoidsgx_handle_emulation_failure(structkvm_vcpu*vcpu,u64addr,+unsignedintsize)+{+vcpu->run->exit_reason=KVM_EXIT_INTERNAL_ERROR;+vcpu->run->internal.suberror=KVM_INTERNAL_ERROR_EMULATION;+vcpu->run->internal.ndata=2;+vcpu->run->internal.data[0]=addr;+vcpu->run->internal.data[1]=size;+}++staticintsgx_read_hva(structkvm_vcpu*vcpu,unsignedlonghva,void*data,+unsignedintsize)+{+if(__copy_from_user(data,(void__user*)hva,size)){+sgx_handle_emulation_failure(vcpu,hva,size);+return-EFAULT;+}++return0;+}++staticintsgx_gva_to_gpa(structkvm_vcpu*vcpu,gva_tgva,boolwrite,+gpa_t*gpa)+{+structx86_exceptionex;++if(write)+*gpa=kvm_mmu_gva_to_gpa_write(vcpu,gva,&ex);+else+*gpa=kvm_mmu_gva_to_gpa_read(vcpu,gva,&ex);++if(*gpa==UNMAPPED_GVA){+kvm_inject_emulated_page_fault(vcpu,&ex);+return-EFAULT;+}++return0;+}++staticintsgx_gpa_to_hva(structkvm_vcpu*vcpu,gpa_tgpa,unsignedlong*hva)+{+*hva=kvm_vcpu_gfn_to_hva(vcpu,PFN_DOWN(gpa));+if(kvm_is_error_hva(*hva)){+sgx_handle_emulation_failure(vcpu,gpa,1);+return-EFAULT;+}++*hva|=gpa&~PAGE_MASK;++return0;+}++staticintsgx_inject_fault(structkvm_vcpu*vcpu,gva_tgva,inttrapnr)+{+structx86_exceptionex;++/*+*Anon-EPCM#PFindicatesabaduserspaceHVA.This*should*check+*forPFEC.SGXandnotassumeany#PFonSGX2originatedintheEPC,+*buttheerrorcodeisn't(yet)plumbedthroughtheENCLShelpers.+*/+if(trapnr==PF_VECTOR&&!boot_cpu_has(X86_FEATURE_SGX2)){+vcpu->run->exit_reason=KVM_EXIT_INTERNAL_ERROR;+vcpu->run->internal.suberror=KVM_INTERNAL_ERROR_EMULATION;+vcpu->run->internal.ndata=0;+return0;+}++/*+*Iftheguestthinksit'srunningonSGX2hardware,injectanSGX+*#PFifthefaultmatchesanEPCMfaultsignature(#GPonSGX1,+*#PFonSGX2).TheassumptionisthatEPCMfaultsaremuchmore+*likelythanabaduserspaceaddress.+*/+if((trapnr==PF_VECTOR||!boot_cpu_has(X86_FEATURE_SGX2))&&+guest_cpuid_has(vcpu,X86_FEATURE_SGX2)){+memset(&ex,0,sizeof(ex));+ex.vector=PF_VECTOR;+ex.error_code=PFERR_PRESENT_MASK|PFERR_WRITE_MASK|+PFERR_SGX_MASK;+ex.address=gva;+ex.error_code_valid=true;+ex.nested_page_fault=false;+kvm_inject_page_fault(vcpu,&ex);+}else{+kvm_inject_gp(vcpu,0);+}+return1;+}++staticint__handle_encls_ecreate(structkvm_vcpu*vcpu,+structsgx_pageinfo*pageinfo,+unsignedlongsecs_hva,+gva_tsecs_gva)+{+structsgx_secs*contents=(structsgx_secs*)pageinfo->contents;+structkvm_cpuid_entry2*sgx_12_0,*sgx_12_1;+u64attributes,xfrm,size;+u32miscselect;+u8max_size_log2;+inttrapnr;++sgx_12_0=kvm_find_cpuid_entry(vcpu,0x12,0);+sgx_12_1=kvm_find_cpuid_entry(vcpu,0x12,1);+if(!sgx_12_0||!sgx_12_1){+vcpu->run->exit_reason=KVM_EXIT_INTERNAL_ERROR;+vcpu->run->internal.suberror=KVM_INTERNAL_ERROR_EMULATION;+vcpu->run->internal.ndata=0;+return0;+}++miscselect=contents->miscselect;+attributes=contents->attributes;+xfrm=contents->xfrm;+size=contents->size;++/* Enforce restriction of access to the PROVISIONKEY. */+if(!vcpu->kvm->arch.sgx_provisioning_allowed&&+(attributes&SGX_ATTR_PROVISIONKEY)){+if(sgx_12_1->eax&SGX_ATTR_PROVISIONKEY)+pr_warn_once("KVM: SGX PROVISIONKEY advertised but not allowed\n");+kvm_inject_gp(vcpu,0);+return1;+}++/* Enforce CPUID restrictions on MISCSELECT, ATTRIBUTES and XFRM. */+if((u32)miscselect&~sgx_12_0->ebx||+(u32)attributes&~sgx_12_1->eax||+(u32)(attributes>>32)&~sgx_12_1->ebx||+(u32)xfrm&~sgx_12_1->ecx||+(u32)(xfrm>>32)&~sgx_12_1->edx){+kvm_inject_gp(vcpu,0);+return1;+}++/* Enforce CPUID restriction on max enclave size. */+max_size_log2=(attributes&SGX_ATTR_MODE64BIT)?sgx_12_0->edx>>8:+sgx_12_0->edx;+if(size>=BIT_ULL(max_size_log2))+kvm_inject_gp(vcpu,0);++if(sgx_virt_ecreate(pageinfo,(void__user*)secs_hva,&trapnr))+returnsgx_inject_fault(vcpu,secs_gva,trapnr);++returnkvm_skip_emulated_instruction(vcpu);+}++staticinthandle_encls_ecreate(structkvm_vcpu*vcpu)+{+gva_tpageinfo_gva,secs_gva;+gva_tmetadata_gva,contents_gva;+gpa_tmetadata_gpa,contents_gpa,secs_gpa;+unsignedlongmetadata_hva,contents_hva,secs_hva;+structsgx_pageinfopageinfo;+structsgx_secs*contents;+structx86_exceptionex;+intr;++if(sgx_get_encls_gva(vcpu,kvm_rbx_read(vcpu),32,32,&pageinfo_gva)||+sgx_get_encls_gva(vcpu,kvm_rcx_read(vcpu),4096,4096,&secs_gva))+return1;++/*+*CopythePAGEINFOtolocalmemory,itspointersneedtobe+*translated,i.e.weneedtodoadeepcopy/translate.+*/+r=kvm_read_guest_virt(vcpu,pageinfo_gva,&pageinfo,+sizeof(pageinfo),&ex);+if(r==X86EMUL_PROPAGATE_FAULT){+kvm_inject_emulated_page_fault(vcpu,&ex);+return1;+}elseif(r!=X86EMUL_CONTINUE){+sgx_handle_emulation_failure(vcpu,pageinfo_gva,+sizeof(pageinfo));+return0;+}++if(sgx_get_encls_gva(vcpu,pageinfo.metadata,64,64,&metadata_gva)||+sgx_get_encls_gva(vcpu,pageinfo.contents,4096,4096,+&contents_gva))+return1;++/*+*TranslatetheSECINFO,SOURCEandSECSpointersfromGVAtoGPA.+*Resumetheguestonfailuretoinjecta#PF.+*/+if(sgx_gva_to_gpa(vcpu,metadata_gva,false,&metadata_gpa)||+sgx_gva_to_gpa(vcpu,contents_gva,false,&contents_gpa)||+sgx_gva_to_gpa(vcpu,secs_gva,true,&secs_gpa))+return1;++/*+*...andthentoHVA.Theorderofaccessesisn'tarchitectural,i.e.+*KVMdoesn'thavetofullyprocessoneaddressatatime.Exitto+*userspaceifaGPAisinvalid.+*/+if(sgx_gpa_to_hva(vcpu,metadata_gpa,&metadata_hva)||+sgx_gpa_to_hva(vcpu,contents_gpa,&contents_hva)||+sgx_gpa_to_hva(vcpu,secs_gpa,&secs_hva))+return0;++/*+*CopycontentsintokernelmemorytopreventTOCTOUattack.E.g.the+*guestcoulddoECREATEw/SECS.SGX_ATTR_PROVISIONKEY=0,and+*simultaneouslysetSGX_ATTR_PROVISIONKEYtobypassthecheckto+*enforcerestrictionofaccesstothePROVISIONKEY.+*/+contents=(structsgx_secs*)__get_free_page(GFP_KERNEL);+if(!contents)+return-ENOMEM;++/* Exit to userspace if copying from a host userspace address fails. */+if(sgx_read_hva(vcpu,contents_hva,(void*)contents,PAGE_SIZE)){+free_page((unsignedlong)contents);+return0;+}++pageinfo.metadata=metadata_hva;+pageinfo.contents=(u64)contents;++r=__handle_encls_ecreate(vcpu,&pageinfo,secs_hva,secs_gva);++free_page((unsignedlong)contents);++returnr;+}+staticinlineboolencls_leaf_enabled_in_guest(structkvm_vcpu*vcpu,u32leaf){if(!enable_sgx||!guest_cpuid_has(vcpu,X86_FEATURE_SGX))
@@ -41,6 +302,8 @@ int handle_encls(struct kvm_vcpu *vcpu)}elseif(!sgx_enabled_in_guest_bios(vcpu)){kvm_inject_gp(vcpu,0);}else{+if(leaf==ECREATE)+returnhandle_encls_ecreate(vcpu);WARN(1,"KVM: unexpected exit on ENCLS[%u]",leaf);vcpu->run->exit_reason=KVM_EXIT_UNKNOWN;vcpu->run->hw.hardware_exit_reason=EXIT_REASON_ENCLS;
From: Kai Huang <hidden> Date: 2021-03-09 01:42:21
From: Sean Christopherson <redacted>
Emulate the four Launch Enclave public key hash MSRs (LE hash MSRs) that
exist on CPUs that support SGX Launch Control (LC). SGX LC modifies the
behavior of ENCLS[EINIT] to use the LE hash MSRs when verifying the key
used to sign an enclave. On CPUs without LC support, the LE hash is
hardwired into the CPU to an Intel controlled key (the Intel key is also
the reset value of the LE hash MSRs). Track the guest's desired hash so
that a future patch can stuff the hash into the hardware MSRs when
executing EINIT on behalf of the guest, when those MSRs are writable in
host.
Note, KVM allows writes to the LE hash MSRs if IA32_FEATURE_CONTROL is
unlocked. This is technically not architectural behavior, but it's
roughly equivalent to the arch behavior of the MSRs being writable prior
to activating SGX[1]. Emulating SGX activation is feasible, but adds no
tangible benefits and would just create extra work for KVM and guest
firmware.
[1] SGX related bits in IA32_FEATURE_CONTROL cannot be set until SGX
is activated, e.g. by firmware. SGX activation is triggered by
setting bit 0 in MSR 0x7a. Until SGX is activated, the LE hash
MSRs are writable, e.g. to allow firmware to lock down the LE
root key with a non-Intel value.
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Kai Huang <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kvm/vmx/sgx.c | 35 +++++++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/sgx.h | 6 ++++++
arch/x86/kvm/vmx/vmx.c | 20 ++++++++++++++++++++
arch/x86/kvm/vmx/vmx.h | 2 ++
4 files changed, 63 insertions(+)
From: Kai Huang <hidden> Date: 2021-03-09 01:42:21
From: Sean Christopherson <redacted>
Add a VM-Exit handler to trap-and-execute EINIT when SGX LC is enabled
in the host. When SGX LC is enabled, the host kernel may rewrite the
hardware values at will, e.g. to launch enclaves with different signers,
thus KVM needs to intercept EINIT to ensure it is executed with the
correct LE hash (even if the guest sees a hardwired hash).
Switching the LE hash MSRs on VM-Enter/VM-Exit is not a viable option as
writing the MSRs is prohibitively expensive, e.g. on SKL hardware each
WRMSR is ~400 cycles. And because EINIT takes tens of thousands of
cycles to execute, the ~1500 cycle overhead to trap-and-execute EINIT is
unlikely to be noticed by the guest, let alone impact its overall SGX
performance.
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kvm/vmx/sgx.c | 55 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
From: Kai Huang <hidden> Date: 2021-03-09 01:42:21
From: Sean Christopherson <redacted>
Add a capability, KVM_CAP_SGX_ATTRIBUTE, that can be used by userspace
to grant a VM access to a priveleged attribute, with args[0] holding a
file handle to a valid SGX attribute file.
The SGX subsystem restricts access to a subset of enclave attributes to
provide additional security for an uncompromised kernel, e.g. to prevent
malware from using the PROVISIONKEY to ensure its nodes are running
inside a geniune SGX enclave and/or to obtain a stable fingerprint.
To prevent userspace from circumventing such restrictions by running an
enclave in a VM, KVM restricts guest access to privileged attributes by
default.
Cc: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
Documentation/virt/kvm/api.rst | 23 +++++++++++++++++++++++
arch/x86/kvm/cpuid.c | 2 +-
arch/x86/kvm/x86.c | 21 +++++++++++++++++++++
include/uapi/linux/kvm.h | 1 +
4 files changed, 46 insertions(+), 1 deletion(-)
@@ -6227,6 +6227,29 @@ KVM_RUN_BUS_LOCK flag is used to distinguish between them. This capability can be used to check / enable 2nd DAWR feature provided by POWER10 processor.+7.24 KVM_CAP_SGX_ATTRIBUTE+----------------------++:Architectures: x86+:Target: VM+:Parameters: args[0] is a file handle of a SGX attribute file in securityfs+:Returns: 0 on success, -EINVAL if the file handle is invalid or if a requested+ attribute is not supported by KVM.++KVM_CAP_SGX_ATTRIBUTE enables a userspace VMM to grant a VM access to one or+more priveleged enclave attributes. args[0] must hold a file handle to a valid+SGX attribute file corresponding to an attribute that is supported/restricted+by KVM (currently only PROVISIONKEY).++The SGX subsystem restricts access to a subset of enclave attributes to provide+additional security for an uncompromised kernel, e.g. use of the PROVISIONKEY+is restricted to deter malware from using the PROVISIONKEY to obtain a stable+system fingerprint. To prevent userspace from circumventing such restrictions+by running an enclave in a VM, KVM prevents access to privileged attributes by+default.++See Documentation/x86/sgx/2.Kernel-internals.rst for more details.+8. Other capabilities. ======================
From: Kai Huang <hidden> Date: 2021-03-09 01:42:21
From: Sean Christopherson <redacted>
Enable SGX virtualization now that KVM has the VM-Exit handlers needed
to trap-and-execute ENCLS to ensure correctness and/or enforce the CPU
model exposed to the guest. Add a KVM module param, "sgx", to allow an
admin to disable SGX virtualization independent of the kernel.
When supported in hardware and the kernel, advertise SGX1, SGX2 and SGX
LC to userspace via CPUID and wire up the ENCLS_EXITING bitmap based on
the guest's SGX capabilities, i.e. to allow ENCLS to be executed in an
SGX-enabled guest. With the exception of the provision key, all SGX
attribute bits may be exposed to the guest. Guest access to the
provision key, which is controlled via securityfs, will be added in a
future patch.
Note, KVM does not yet support exposing ENCLS_C leafs or ENCLV leafs.
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kvm/cpuid.c | 57 +++++++++++++++++++++++++++-
arch/x86/kvm/vmx/nested.c | 26 +++++++++++--
arch/x86/kvm/vmx/nested.h | 5 +++
arch/x86/kvm/vmx/sgx.c | 80 ++++++++++++++++++++++++++++++++++++++-
arch/x86/kvm/vmx/sgx.h | 13 +++++++
arch/x86/kvm/vmx/vmcs12.c | 1 +
arch/x86/kvm/vmx/vmcs12.h | 4 +-
arch/x86/kvm/vmx/vmx.c | 35 ++++++++++++++++-
8 files changed, 212 insertions(+), 9 deletions(-)
@@ -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))
@@ -5803,9 +5821,6 @@ static bool nested_vmx_l0_wants_exit(struct kvm_vcpu *vcpu,caseEXIT_REASON_VMFUNC:/* VM functions are emulated through L2->L0 vmexits. */returntrue;-caseEXIT_REASON_ENCLS:-/* SGX is never exposed to L1 */-returntrue;default:break;}
@@ -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_mostlyenable_sgx;+bool__read_mostlyenable_sgx=1;+module_param_named(sgx,enable_sgx,bool,0444);/* Initial value of guest's virtual SGX_LEPUBKEYHASHn MSRs */staticu64sgx_pubkey_hash[4]__ro_after_init;
@@ -401,3 +403,79 @@ void vcpu_setup_sgx_lepubkeyhash(struct kvm_vcpu *vcpu)memcpy(vmx->msr_ia32_sgxlepubkeyhash,sgx_pubkey_hash,sizeof(sgx_pubkey_hash));}++/*+*ECREATEmustbeinterceptedtoenforceMISCSELECT,ATTRIBUTESandXFRM+*restrictionsiftheguest'sallowed-1settingsdivergefromhardware.+*/+staticboolsgx_intercept_encls_ecreate(structkvm_vcpu*vcpu)+{+structkvm_cpuid_entry2*guest_cpuid;+u32eax,ebx,ecx,edx;++if(!vcpu->kvm->arch.sgx_provisioning_allowed)+returntrue;++guest_cpuid=kvm_find_cpuid_entry(vcpu,0x12,0);+if(!guest_cpuid)+returntrue;++cpuid_count(0x12,0,&eax,&ebx,&ecx,&edx);+if(guest_cpuid->ebx!=ebx||guest_cpuid->edx!=edx)+returntrue;++guest_cpuid=kvm_find_cpuid_entry(vcpu,0x12,1);+if(!guest_cpuid)+returntrue;++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)+returntrue;++returnfalse;+}++voidvmx_write_encls_bitmap(structkvm_vcpu*vcpu,structvmcs12*vmcs12)+{+/*+*ThereisnosoftwareenablebitforSGXthatisvirtualizedby+*hardware,e.g.there'snoCR4.SGXE,sowhenSGXisdisabledinthe+*guest(eitherbythehostorbytheguest'sBIOS)butenabledinthe+*host,trapallENCLSleafsandinject#UD/#GPasneededtoemulate+*theexpectedsystembehaviorforENCLS.+*/+u64bitmap=-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);++/*+*TrapandexecuteEINITiflaunchcontrolisenabledinthe+*hostusingtheguest'svaluesforlaunchcontrolMSRs,even+*iftheguest'svaluesarefixedtohardwaredefaultvalues.+*TheMSRsarenotloaded/savedonVM-Enter/VM-Exitaswriting+*theMSRsisextraordinarilyexpensive.+*/+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);+}
@@ -11,11 +14,21 @@ int handle_encls(struct kvm_vcpu *vcpu);voidsetup_default_sgx_lepubkeyhash(void);voidvcpu_setup_sgx_lepubkeyhash(structkvm_vcpu*vcpu);++voidvmx_write_encls_bitmap(structkvm_vcpu*vcpu,structvmcs12*vmcs12);#else#define enable_sgx 0staticinlinevoidsetup_default_sgx_lepubkeyhash(void){}staticinlinevoidvcpu_setup_sgx_lepubkeyhash(structkvm_vcpu*vcpu){}++staticinlinevoidvmx_write_encls_bitmap(structkvm_vcpu*vcpu,+structvmcs12*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 */
@@ -2204,6 +2204,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;caseMSR_IA32_SGXLEPUBKEYHASH0...MSR_IA32_SGXLEPUBKEYHASH3:if(!msr_info->host_initiated&&
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
Unless folks have better suggestions, ofc.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
From: Kai Huang <hidden> Date: 2021-03-09 18:09:30
On Tue, 2021-03-09 at 10:30 +0100, Borislav Petkov wrote:
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
quoted
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
Unless folks have better suggestions, ofc.
Thx.
Thanks Boris!
Hi Sean, Paolo,
Could you take a look? Thanks.
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2021-03-09 18:50:42
On 09/03/21 10:30, Borislav Petkov wrote:
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
quoted
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
From: Kai Huang <hidden> Date: 2021-03-10 09:28:19
On Tue, 2021-03-09 at 10:30 +0100, Borislav Petkov wrote:
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
quoted
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
Unless folks have better suggestions, ofc.
Thx.
Hi Boris,
Sorry that we found a bug in below patch in series:
[PATCH v2 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()
that I made a mistake when copying & pasting, which results in SECS page and va_page
not being freed correctly in sgx_encl_release().
Sorry for the mistake. I will send out another version with that fixed.
From: Kai Huang <hidden> Date: 2021-03-10 09:36:04
On Tue, 9 Mar 2021 14:39:01 +1300 Kai Huang wrote:
quoted hunk
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Merge original WARN() and pr_err_once() into one single WARN(), suggested
by Sean.
---
arch/x86/kernel/cpu/sgx/encl.c | 27 ++++++++++++++++++++++++---
arch/x86/kernel/cpu/sgx/main.c | 12 ++++--------
2 files changed, 28 insertions(+), 11 deletions(-)
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-10 18:02:00
On Tue, Mar 09, 2021 at 10:30:37AM +0100, Borislav Petkov wrote:
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
quoted
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
Unless folks have better suggestions, ofc.
I'm otherwise cool with that, except patch #2.
It's based on this series:
https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
It's not reasonable to create driver specific wrapper for
sgx_free_epc_page() because there is exactly *2* call sites of the function
in the driver. The driver contains 10 call sites (11 after my NUMA patches
have been applied) of sgx_free_epc_page() in total.
Instead, it is better to add explicit EREMOVE to those call sites.
The wrapper only trashes the codebase. I'm not happy with it, given all the
trouble to make it clean and sound.
From: Kai Huang <hidden> Date: 2021-03-10 20:45:02
On Wed, 2021-03-10 at 20:01 +0200, Jarkko Sakkinen wrote:
On Tue, Mar 09, 2021 at 10:30:37AM +0100, Borislav Petkov wrote:
quoted
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
quoted
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
Unless folks have better suggestions, ofc.
I'm otherwise cool with that, except patch #2.
It's based on this series:
https://lore.kernel.org/linux-sgx/20210113233541.17669-1-jarkko@kernel.org/
It's not reasonable to create driver specific wrapper for
sgx_free_epc_page() because there is exactly *2* call sites of the function
in the driver. The driver contains 10 call sites (11 after my NUMA patches
have been applied) of sgx_free_epc_page() in total.
Instead, it is better to add explicit EREMOVE to those call sites.
The wrapper only trashes the codebase. I'm not happy with it, given all the
trouble to make it clean and sound.
However, your change has side effort: it always put page back into free pool, even
EREMOVE fails. To make your change w/o having any functional change, it has to be:
if(!sgx_reset_epc_page())
sgx_free_epc_page();
And for this, Dave raised one concern we should add a WARN() to let user know EPC
page is leaked, and reboot is requied to get them back.
However with sgx_reset_epc_page(), there's no place to add such WARN(), and
implementing original sgx_free_epc_page() as sgx_encl_free_epc_page() looks very
reasonable to me:
https://www.spinics.net/lists/linux-sgx/msg04631.html
Hi Dave,
What is your comment here?
From: Kai Huang <hidden> Date: 2021-03-11 02:02:55
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
v1->v2:
- Changed to hide both SGX1 and SGX2 from /proc/cpuinfo, since no concrete
use case, per Boris.
- Refined commit msg to explain why to hide SGX1 and SGX2 in /proc/cpuinfo.
---
arch/x86/kernel/cpu/sgx/encl.c | 27 ++++++++++++++++++++++++---
arch/x86/kernel/cpu/sgx/main.c | 12 ++++--------
2 files changed, 28 insertions(+), 11 deletions(-)
From: Kai Huang <hidden> Date: 2021-03-11 02:06:43
On Wed, 10 Mar 2021 14:29:48 +0100 Borislav Petkov wrote:
On Wed, Mar 10, 2021 at 10:27:05PM +1300, Kai Huang wrote:
quoted
Sorry for the mistake. I will send out another version with that fixed.
If patch 3 is the only one which needs to change, you can send only that
one as a reply to the original patch 3 message...
Thx.
Hi Boris,
Yes it is the only patch needs change. I have send out updated v3 patch 3.
I provided some changelog history to explain and also added Jarkko's Acked-by in
the new patch. Sorry for the trouble.
Hi Sean,
If you see this, could you take another check on whether this series is OK?
Thanks in advance.
From: Sean Christopherson <seanjc@google.com> Date: 2021-03-12 21:06:05
On Tue, Mar 09, 2021, Kai Huang wrote:
Move SGX_LC feature bit to CPUID dependency table to make clearing all
SGX feature bits easier. Also remove clear_sgx_caps() since it is just
a wrapper of setup_clear_cpu_cap(X86_FEATURE_SGX) now.
Suggested-by: Sean Christopherson <seanjc@google.com>
Acked-by: Dave Hansen <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
Reviewed-by: Sean Christopherson <seanjc@google.com>
From: Sean Christopherson <seanjc@google.com> Date: 2021-03-12 21:22:44
On Thu, Mar 11, 2021, Kai Huang wrote:
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
From: Sean Christopherson <seanjc@google.com> Date: 2021-03-12 21:34:28
On Tue, Mar 09, 2021, Kai Huang wrote:
+ if (cpu_has(c, X86_FEATURE_SGX) && IS_ENABLED(CONFIG_X86_SGX)) {
+ /*
+ * Separate out SGX driver enabling from KVM. This allows KVM
+ * guests to use SGX even if the kernel SGX driver refuses to
+ * use it. This happens if flexible Faunch Control is not
/Faunch/Launch
Clever handling of the multitude of combinations!
From: Sean Christopherson <seanjc@google.com> Date: 2021-03-12 21:45:41
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted hunk
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
From: Sean Christopherson <seanjc@google.com> Date: 2021-03-12 21:59:35
On Tue, Mar 09, 2021, Kai Huang wrote:
From: Sean Christopherson <redacted>
Expose SGX architectural structures, as KVM will use many of the
architectural constants and structs to virtualize SGX.
Name the new header file as asm/sgx.h, rather than asm/sgx_arch.h, to
have single header to provide SGX facilities to share with other kernel
componments.
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Kai Huang <redacted>
Acked-by: Dave Hansen <redacted>
diff --git a/arch/x86/kernel/cpu/sgx/arch.h b/arch/x86/include/asm/sgx.hsimilarity index 95%rename from arch/x86/kernel/cpu/sgx/arch.hrename to arch/x86/include/asm/sgx.hindex abf99bb71fdc..d4ad35f6319a 100644--- a/arch/x86/kernel/cpu/sgx/arch.h+++ b/arch/x86/include/asm/sgx.h
@@ -2,15 +2,20 @@/***Copyright(c)2016-20IntelCorporation.*-*ContainsdatastructuresdefinedbytheSGXarchitecture.Datastructures-*definedbytheLinuxsoftwarestackshouldnotbeplacedhere.+*IntelSoftwareGuardExtensions(SGX)support.*/-#ifndef _ASM_X86_SGX_ARCH_H-#define _ASM_X86_SGX_ARCH_H+#ifndef _ASM_X86_SGX_H+#define _ASM_X86_SGX_H#include<linux/bits.h>#include<linux/types.h>+/*+*ThisfilecontainsbothdatastructuresdefinedbySGXarchitectureandLinux+*definedsoftwaredatastructuresandfunctions.Thetwoshouldnotbemixed+*togetherforbetterreadibility.Thearchitecturaldefinitionscomefirst.+*/+/* The SGX specific CPUID function. */#define SGX_CPUID 0x12/* EPC enumeration. */
From: Sean Christopherson <seanjc@google.com> Date: 2021-03-12 22:04:53
On Tue, Mar 09, 2021, Paolo Bonzini wrote:
On 09/03/21 10:30, Borislav Petkov wrote:
quoted
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
quoted
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
Sounds great.
Patches 1-14 look good, just a few minor nits, nothing functional. I'll look at
the KVM patches next week.
Thanks for picking this up Kai!
From: Kai Huang <hidden> Date: 2021-03-13 04:31:13
On Fri, 2021-03-12 at 14:04 -0800, Sean Christopherson wrote:
On Tue, Mar 09, 2021, Paolo Bonzini wrote:
quoted
On 09/03/21 10:30, Borislav Petkov wrote:
quoted
On Tue, Mar 09, 2021 at 02:38:49PM +1300, Kai Huang wrote:
quoted
This series adds KVM SGX virtualization support. The first 14 patches starting
with x86/sgx or x86/cpu.. are necessary changes to x86 and SGX core/driver to
support KVM SGX virtualization, while the rest are patches to KVM subsystem.
Ok, I guess I'll queue 1-14 once Sean doesn't find anything
objectionable then give Paolo an immutable commit to base the KVM stuff
ontop.
Sounds great.
Patches 1-14 look good, just a few minor nits, nothing functional. I'll look at
the KVM patches next week.
Thanks for picking this up Kai!
Thank you Sean! I'll address your comments in next version.
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-13 10:47:08
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-13 19:06:52
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-13 19:08:49
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-14 15:26:56
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-14 15:28:33
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
/Jarkko
From: Kai Huang <hidden> Date: 2021-03-15 03:14:26
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
Hi Sean,
Please let us know if you have comments.
From: Kai Huang <hidden> Date: 2021-03-15 03:37:54
On Fri, 12 Mar 2021 13:58:40 -0800 Sean Christopherson wrote:
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
From: Sean Christopherson <redacted>
Expose SGX architectural structures, as KVM will use many of the
architectural constants and structs to virtualize SGX.
Name the new header file as asm/sgx.h, rather than asm/sgx_arch.h, to
have single header to provide SGX facilities to share with other kernel
componments.
Signed-off-by: Sean Christopherson <redacted>
Co-developed-by: Kai Huang <redacted>
Acked-by: Dave Hansen <redacted>
Same checkpatch warning. Probably doesn't matter.
Will change order to make checkpatch happy for this whole series.
Thanks for pointing out.
diff --git a/arch/x86/kernel/cpu/sgx/arch.h b/arch/x86/include/asm/sgx.hsimilarity index 95%rename from arch/x86/kernel/cpu/sgx/arch.hrename to arch/x86/include/asm/sgx.hindex abf99bb71fdc..d4ad35f6319a 100644--- a/arch/x86/kernel/cpu/sgx/arch.h+++ b/arch/x86/include/asm/sgx.h
@@ -2,15 +2,20 @@/***Copyright(c)2016-20IntelCorporation.*-*ContainsdatastructuresdefinedbytheSGXarchitecture.Datastructures-*definedbytheLinuxsoftwarestackshouldnotbeplacedhere.+*IntelSoftwareGuardExtensions(SGX)support.*/-#ifndef _ASM_X86_SGX_ARCH_H-#define _ASM_X86_SGX_ARCH_H+#ifndef _ASM_X86_SGX_H+#define _ASM_X86_SGX_H#include<linux/bits.h>#include<linux/types.h>+/*+*ThisfilecontainsbothdatastructuresdefinedbySGXarchitectureandLinux+*definedsoftwaredatastructuresandfunctions.Thetwoshouldnotbemixed+*togetherforbetterreadibility.Thearchitecturaldefinitionscomefirst.+*/+/* The SGX specific CPUID function. */#define SGX_CPUID 0x12/* EPC enumeration. */
From: Kai Huang <hidden> Date: 2021-03-15 04:09:45
On Tue, 9 Mar 2021 14:39:46 +1300 Kai Huang wrote:
quoted hunk
From: Sean Christopherson <redacted>
The host kernel must intercept ECREATE to impose policies on guests, and
intercept EINIT to be able to write guest's virtual SGX_LEPUBKEYHASH MSR
values to hardware before running guest's EINIT so it can run correctly
according to hardware behavior.
Provide wrappers around __ecreate() and __einit() to hide the ugliness
of overloading the ENCLS return value to encode multiple error formats
in a single int. KVM will trap-and-execute ECREATE and EINIT as part
of SGX virtualization, and reflect ENCLS execution result to guest by
setting up guest's GPRs, or on an exception, injecting the correct fault
based on return value of __ecreate() and __einit().
Use host userspace addresses (provided by KVM based on guest physical
address of ENCLS parameters) to execute ENCLS/EINIT when possible.
Accesses to both EPC and memory originating from ENCLS are subject to
segmentation and paging mechanisms. It's also possible to generate
kernel mappings for ENCLS parameters by resolving PFN but using
__uaccess_xx() is simpler.
Signed-off-by: Sean Christopherson <redacted>
Signed-off-by: Kai Huang <redacted>
---
v1->v2:
- Tried to address Dave's comments:
- Refined comments around @secs in sgx_virt_ecreate().
- Refined commit msg to explain why to use userspace address for ENCLS,
instead of generating kernel mapping.
- Added access_ok() on userspace addresses, and give WARN() if check fails.
---
arch/x86/include/asm/sgx.h | 7 +++
arch/x86/kernel/cpu/sgx/virt.c | 110 +++++++++++++++++++++++++++++++++
2 files changed, 117 insertions(+)
@@ -258,3 +258,113 @@ int __init sgx_vepc_init(void)returnmisc_register(&sgx_vepc_dev);}++/**+*sgx_virt_ecreate()-RunECREATEonbehalfofguest+*@pageinfo:PointertoPAGEINFOstructure+*@secs:UserspacepointertoSECSpage+*@trapnr:trapnumberinjectedtoguestincaseofECREATEerror+*+*RunECREATEonbehalfofguestafterKVMtrapsECREATEforthepurpose+*ofenforcingpoliciesofguest'senclaves,andreturnthetrapnumber+*whichshouldbeinjectedtoguestincaseofanyECREATEerror.+*+*Return:+*-0:ECREATEwassuccessful.+*--EFAULT:ECREATEreturnederror.+*/+intsgx_virt_ecreate(structsgx_pageinfo*pageinfo,void__user*secs,+int*trapnr)+{+intret;++/*+*@secsisanuntrusted,userspace-providedaddress.Itcomesfrom+*KVMandisassumedtobeavalidpointerwhichpointssomewherein+*userspace.ThiscanfaultandcallSGXorotherfaulthandlerswhen+*userspacemapping@secsdoesn'texist.+*+*AddaWARN()tomakesure@secsisalreadyvaliduserspacepointer+*fromcaller(KVM),whoshouldalreadyhavehandledinvalidpointer+*case(forinstance,madebymaliciousguest).Allotherchecks,+*suchasalignmentof@secs,aredeferredtoENCLSitself.+*/+WARN_ON_ONCE(!access_ok(PTR_ALIGN_DOWN(secs,PAGE_SIZE),PAGE_SIZE));+__uaccess_begin();+ret=__ecreate(pageinfo,(void*)secs);+__uaccess_end();++if(encls_faulted(ret)){+*trapnr=ENCLS_TRAPNR(ret);+return-EFAULT;+}++/* ECREATE doesn't return an error code, it faults or succeeds. */+WARN_ON_ONCE(ret);+return0;+}+EXPORT_SYMBOL_GPL(sgx_virt_ecreate);++staticint__sgx_virt_einit(void__user*sigstruct,void__user*token,+void__user*secs)+{+intret;++/*+*Makesurealluserspacepointersfromcaller(KVM)arevalid.+*AllotherchecksdeferredtoENCLSitself.Alsoseecomment+*for@secsinsgx_virt_ecreate().+*/+WARN_ON_ONCE(!access_ok(PTR_ALIGN_DOWN(sigstruct,PAGE_SIZE),+PAGE_SIZE)||+!access_ok(PTR_ALIGN_DOWN(token,PAGE_SIZE),PAGE_SIZE)||+!access_ok(PTR_ALIGN_DOWN(secs,PAGE_SIZE),PAGE_SIZE));
I think I was overthinking about those access_ok()s. The align down is not
needed, and size should just be the size of the structures. I'll change in next
version.
One thing is SGX driver doesn't use EINITTOKEN, so it doesn't have 'struct
sgx_einittoken {}' defined. The size of EINITTOKEN is fixed size (304 bytes),
so I will add a macro definition SGX_EINITTOKEN_SIZE and use it.
Hi Sean, please let me know if you have any comments here (and this patch).
Thanks!
+ __uaccess_begin();
+ ret = __einit((void *)sigstruct, (void *)token, (void *)secs);
+ __uaccess_end();
+
+ return ret;
+}
+
+/**
+ * sgx_virt_einit() - Run EINIT on behalf of guest
+ * @sigstruct: Userspace pointer to SIGSTRUCT structure
+ * @token: Userspace pointer to EINITTOKEN structure
+ * @secs: Userspace pointer to SECS page
+ * @lepubkeyhash: Pointer to guest's *virtual* SGX_LEPUBKEYHASH MSR
+ * values
+ * @trapnr: trap number injected to guest in case of EINIT error
+ *
+ * Run EINIT on behalf of guest after KVM traps EINIT. If SGX_LC is available
+ * in host, SGX driver may rewrite the hardware values at wish, therefore KVM
+ * needs to update hardware values to guest's virtual MSR values in order to
+ * ensure EINIT is executed with expected hardware values.
+ *
+ * Return:
+ * - 0: EINIT was successful.
+ * - -EFAULT: EINIT returned error.
+ */
+int sgx_virt_einit(void __user *sigstruct, void __user *token,
+ void __user *secs, u64 *lepubkeyhash, int *trapnr)
+{
+ int ret;
+
+ if (!boot_cpu_has(X86_FEATURE_SGX_LC)) {
+ ret = __sgx_virt_einit(sigstruct, token, secs);
+ } else {
+ preempt_disable();
+
+ sgx_update_lepubkeyhash(lepubkeyhash);
+
+ ret = __sgx_virt_einit(sigstruct, token, secs);
+ preempt_enable();
+ }
+
+ if (encls_faulted(ret)) {
+ *trapnr = ENCLS_TRAPNR(ret);
+ return -EFAULT;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sgx_virt_einit);
--
2.29.2
From: Kai Huang <hidden> Date: 2021-03-15 07:13:22
On Sat, 13 Mar 2021 12:45:53 +0200 Jarkko Sakkinen wrote:
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
quoted
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Oops, my bad. Yup, ack should be removed.
/Jarkko
Hi Jarkko,
Your reply of your concern of this patch to the cover-letter
https://lore.kernel.org/lkml/YEkJXu262YDa8ZaK@kernel.org/
reminds me to do more sanity check of whether removing EREMOVE in
sgx_free_epc_page() will impact other code path or not, and I think
sgx_encl_release() is not the only place should be changed:
- sgx_encl_shrink() needs to call sgx_encl_free_epc_page(), since when this is
called, the VA page can be already valid -- there are other failures can
trigger sgx_encl_shrink().
- sgx_encl_add_page() should call sgx_encl_free_epc_page() in "err_out_free:"
label, since the EPC page can be already valid when error happened, i.e. when
EEXTEND fails.
Other places should be OK per my check, but I'd prefer to just replacing all
sgx_free_epc_page() call sites in driver with sgx_encl_free_epc_page(), with
one exception: sgx_alloc_va_page(), which calls sgx_free_epc_page() when EPA
fails, in which case EREMOVE is not required for sure.
Your idea, please?
Btw, introducing a driver wrapper of sgx_free_epc_page() does make sense to me,
because virtualization has a counterpart in sgx/virt.c too.
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 13:06:20
On Mon, Mar 15, 2021 at 04:13:17PM +1300, Kai Huang wrote:
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
quoted
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
I don't understand what would be the bad thing here. Can you open that
up please? I'm neither capable of predicting the future...
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 13:19:43
On Mon, Mar 15, 2021 at 08:12:36PM +1300, Kai Huang wrote:
On Sat, 13 Mar 2021 12:45:53 +0200 Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
quoted
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Oops, my bad. Yup, ack should be removed.
/Jarkko
Hi Jarkko,
Your reply of your concern of this patch to the cover-letter
https://lore.kernel.org/lkml/YEkJXu262YDa8ZaK@kernel.org/
reminds me to do more sanity check of whether removing EREMOVE in
sgx_free_epc_page() will impact other code path or not, and I think
sgx_encl_release() is not the only place should be changed:
- sgx_encl_shrink() needs to call sgx_encl_free_epc_page(), since when this is
called, the VA page can be already valid -- there are other failures can
trigger sgx_encl_shrink().
You right about this, good catch.
Shrink needs to always do EREMOVE as grow has done EPA, which changes
EPC page state.
- sgx_encl_add_page() should call sgx_encl_free_epc_page() in "err_out_free:"
label, since the EPC page can be already valid when error happened, i.e. when
EEXTEND fails.
Yes, correct, good work!
Other places should be OK per my check, but I'd prefer to just replacing all
sgx_free_epc_page() call sites in driver with sgx_encl_free_epc_page(), with
one exception: sgx_alloc_va_page(), which calls sgx_free_epc_page() when EPA
fails, in which case EREMOVE is not required for sure.
I would not unless they require it.
Your idea, please?
Btw, introducing a driver wrapper of sgx_free_epc_page() does make sense to me,
because virtualization has a counterpart in sgx/virt.c too.
It does make sense to use sgx_free_epc_page() everywhere where it's
the right thing to call and here's why.
If there is some unrelated regression that causes EPC page not get
uninitialized when it actually should, doing extra EREMOVE could mask
those bugs. I.e. it can postpone a failure, which can make a bug harder
to backtrace.
Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 13:20:47
On Mon, Mar 15, 2021 at 03:18:16PM +0200, Jarkko Sakkinen wrote:
On Mon, Mar 15, 2021 at 08:12:36PM +1300, Kai Huang wrote:
quoted
On Sat, 13 Mar 2021 12:45:53 +0200 Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
quoted
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Oops, my bad. Yup, ack should be removed.
/Jarkko
Hi Jarkko,
Your reply of your concern of this patch to the cover-letter
https://lore.kernel.org/lkml/YEkJXu262YDa8ZaK@kernel.org/
reminds me to do more sanity check of whether removing EREMOVE in
sgx_free_epc_page() will impact other code path or not, and I think
sgx_encl_release() is not the only place should be changed:
- sgx_encl_shrink() needs to call sgx_encl_free_epc_page(), since when this is
called, the VA page can be already valid -- there are other failures can
trigger sgx_encl_shrink().
You right about this, good catch.
Shrink needs to always do EREMOVE as grow has done EPA, which changes
EPC page state.
quoted
- sgx_encl_add_page() should call sgx_encl_free_epc_page() in "err_out_free:"
label, since the EPC page can be already valid when error happened, i.e. when
EEXTEND fails.
Yes, correct, good work!
quoted
Other places should be OK per my check, but I'd prefer to just replacing all
sgx_free_epc_page() call sites in driver with sgx_encl_free_epc_page(), with
one exception: sgx_alloc_va_page(), which calls sgx_free_epc_page() when EPA
fails, in which case EREMOVE is not required for sure.
I would not unless they require it.
quoted
Your idea, please?
Btw, introducing a driver wrapper of sgx_free_epc_page() does make sense to me,
because virtualization has a counterpart in sgx/virt.c too.
It does make sense to use sgx_free_epc_page() everywhere where it's
the right thing to call and here's why.
If there is some unrelated regression that causes EPC page not get
uninitialized when it actually should, doing extra EREMOVE could mask
those bugs. I.e. it can postpone a failure, which can make a bug harder
to backtrace.
I.e. even though it is true that for correctly working code extra EREMOVE
is nil functionality, it could change semantics for buggy code.
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 13:52:31
On Mon, Mar 15, 2021 at 03:04:59PM +0200, Jarkko Sakkinen wrote:
On Mon, Mar 15, 2021 at 04:13:17PM +1300, Kai Huang wrote:
quoted
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
quoted
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
I don't understand what would be the bad thing here. Can you open that
up please? I'm neither capable of predicting the future...
Right, so since vepc_init() does only just device file initialization the
current function structure is fine. I totally forgot that sgx_drv_init()
does not call EPC initialization when I wrote the above :-) We refactored
during the inital cycle the driver so many times that I sometimes fix up
thing, sorry about.
To meld this into code:
ret = sgx_vepc_init();
if (ret != -ENODEV) {
pr_err("vEPC initialization failed with %d.\n", ret);
return ret;
}
ret = sgx_drv_init();
if (ret != ENODEV)
pr_info("Driver initialization failed %d.\n", ret);
This would also give more accurate information how far the initialization
went.
/Jarkko
From: Kai Huang <hidden> Date: 2021-03-15 20:30:22
On Mon, 15 Mar 2021 15:19:32 +0200 Jarkko Sakkinen wrote:
On Mon, Mar 15, 2021 at 03:18:16PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 08:12:36PM +1300, Kai Huang wrote:
quoted
On Sat, 13 Mar 2021 12:45:53 +0200 Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
quoted
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Oops, my bad. Yup, ack should be removed.
/Jarkko
Hi Jarkko,
Your reply of your concern of this patch to the cover-letter
https://lore.kernel.org/lkml/YEkJXu262YDa8ZaK@kernel.org/
reminds me to do more sanity check of whether removing EREMOVE in
sgx_free_epc_page() will impact other code path or not, and I think
sgx_encl_release() is not the only place should be changed:
- sgx_encl_shrink() needs to call sgx_encl_free_epc_page(), since when this is
called, the VA page can be already valid -- there are other failures can
trigger sgx_encl_shrink().
You right about this, good catch.
Shrink needs to always do EREMOVE as grow has done EPA, which changes
EPC page state.
quoted
- sgx_encl_add_page() should call sgx_encl_free_epc_page() in "err_out_free:"
label, since the EPC page can be already valid when error happened, i.e. when
EEXTEND fails.
Yes, correct, good work!
quoted
Other places should be OK per my check, but I'd prefer to just replacing all
sgx_free_epc_page() call sites in driver with sgx_encl_free_epc_page(), with
one exception: sgx_alloc_va_page(), which calls sgx_free_epc_page() when EPA
fails, in which case EREMOVE is not required for sure.
I would not unless they require it.
quoted
Your idea, please?
Btw, introducing a driver wrapper of sgx_free_epc_page() does make sense to me,
because virtualization has a counterpart in sgx/virt.c too.
It does make sense to use sgx_free_epc_page() everywhere where it's
the right thing to call and here's why.
If there is some unrelated regression that causes EPC page not get
uninitialized when it actually should, doing extra EREMOVE could mask
those bugs. I.e. it can postpone a failure, which can make a bug harder
to backtrace.
I.e. even though it is true that for correctly working code extra EREMOVE
is nil functionality, it could change semantics for buggy code.
Thanks for feedback. Sorry I am not sure if I understand you. So if we don't
want to bring functionality change, we need to replace sgx_free_epc_page() in
all call sites with sgx_encl_free_epc_page(). To me for this patch only, it's
better not to bring any functional change, so I intend to replace all (I now
consider even leaving sgx_alloc_va_page() out is not good idea in *this*
patch).
Or do you just want to replace sgx_free_epc_page() with
sgx_encl_free_epc_page() in sgx_encl_shrink() and sgx_encl_add_page(), as I
pointed above? In this way there will be functional change in this patch, and
we need to explicitly explain why leaving others out is OK in commit message.
To me I prefer the former.
From: Kai Huang <hidden> Date: 2021-03-15 20:50:05
On Mon, 15 Mar 2021 15:51:17 +0200 Jarkko Sakkinen wrote:
On Mon, Mar 15, 2021 at 03:04:59PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 04:13:17PM +1300, Kai Huang wrote:
quoted
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
quoted
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
I don't understand what would be the bad thing here. Can you open that
up please? I'm neither capable of predicting the future...
Conceptually they are two different functionalities, and doesn't depend on each
other. Why calling SGX driver initialization only when SGX virtualization
succeeded?
We might want to add reclaiming EPC page (VMM EPC oversubscription) from KVM
guest in the future, which may bring more initialization staff sgx_vepc_init(),
and those new staff should not impact SGX driver.
I don't see your approach is any better, both from concept and flexibility.
Like I said, we can rename to sgx_virt_init() to be more generic, but I
strongly disagree your approach.
Right, so since vepc_init() does only just device file initialization the
current function structure is fine. I totally forgot that sgx_drv_init()
does not call EPC initialization when I wrote the above :-) We refactored
during the inital cycle the driver so many times that I sometimes fix up
thing, sorry about.
To meld this into code:
ret = sgx_vepc_init();
if (ret != -ENODEV) {
pr_err("vEPC initialization failed with %d.\n", ret);
return ret;
}
ret = sgx_drv_init();
if (ret != ENODEV)
pr_info("Driver initialization failed %d.\n", ret);
Hmm.. Let's say an extreme case: misc_register() failed in sgx_vepc_init(), due
to -ENOMEM. Then OOM kill gets involved, and kills bunch of apps. And then In
this case, theoretically, misc_register() in sgx_drv_init() doesn't need to
fail.
The point is really SGX driver and SGX virt are two independent
functionalities, so don't make dependency on them, manually. Plus I don't see
any benefit of your approach, but only cons.
This would also give more accurate information how far the initialization
went.
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 23:00:50
On Tue, Mar 16, 2021 at 09:29:34AM +1300, Kai Huang wrote:
On Mon, 15 Mar 2021 15:19:32 +0200 Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 03:18:16PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 08:12:36PM +1300, Kai Huang wrote:
quoted
On Sat, 13 Mar 2021 12:45:53 +0200 Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
quoted
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Oops, my bad. Yup, ack should be removed.
/Jarkko
Hi Jarkko,
Your reply of your concern of this patch to the cover-letter
https://lore.kernel.org/lkml/YEkJXu262YDa8ZaK@kernel.org/
reminds me to do more sanity check of whether removing EREMOVE in
sgx_free_epc_page() will impact other code path or not, and I think
sgx_encl_release() is not the only place should be changed:
- sgx_encl_shrink() needs to call sgx_encl_free_epc_page(), since when this is
called, the VA page can be already valid -- there are other failures can
trigger sgx_encl_shrink().
You right about this, good catch.
Shrink needs to always do EREMOVE as grow has done EPA, which changes
EPC page state.
quoted
- sgx_encl_add_page() should call sgx_encl_free_epc_page() in "err_out_free:"
label, since the EPC page can be already valid when error happened, i.e. when
EEXTEND fails.
Yes, correct, good work!
quoted
Other places should be OK per my check, but I'd prefer to just replacing all
sgx_free_epc_page() call sites in driver with sgx_encl_free_epc_page(), with
one exception: sgx_alloc_va_page(), which calls sgx_free_epc_page() when EPA
fails, in which case EREMOVE is not required for sure.
I would not unless they require it.
quoted
Your idea, please?
Btw, introducing a driver wrapper of sgx_free_epc_page() does make sense to me,
because virtualization has a counterpart in sgx/virt.c too.
It does make sense to use sgx_free_epc_page() everywhere where it's
the right thing to call and here's why.
If there is some unrelated regression that causes EPC page not get
uninitialized when it actually should, doing extra EREMOVE could mask
those bugs. I.e. it can postpone a failure, which can make a bug harder
to backtrace.
I.e. even though it is true that for correctly working code extra EREMOVE
is nil functionality, it could change semantics for buggy code.
Thanks for feedback. Sorry I am not sure if I understand you. So if we don't
want to bring functionality change, we need to replace sgx_free_epc_page() in
all call sites with sgx_encl_free_epc_page(). To me for this patch only, it's
better not to bring any functional change, so I intend to replace all (I now
consider even leaving sgx_alloc_va_page() out is not good idea in *this*
patch).
Or do you just want to replace sgx_free_epc_page() with
sgx_encl_free_epc_page() in sgx_encl_shrink() and sgx_encl_add_page(), as I
pointed above? In this way there will be functional change in this patch, and
we need to explicitly explain why leaving others out is OK in commit message.
To me I prefer the former.
The original purpose of this patch was exactly to remove EREMOVE
sgx_free_epc_page() and call it explicitly where it is required. That's
why I introduced sgx_reset_epc_page(). So the latter was actually the goal
of this patch at least when I did it. Now this is something completely
different.
So, I don't consider myself author of this patch in any possible way,
because this is not what I intended.
To move forward, for the next patch set version, you should change the
author field as yourself, and remove all my tags, and I will review it.
So you can work out this with former approach if you wish.
I.e. my ack/nak/etc. apply to this patch because it's not my code.
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 23:06:10
On Tue, Mar 16, 2021 at 09:48:59AM +1300, Kai Huang wrote:
On Mon, 15 Mar 2021 15:51:17 +0200 Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 03:04:59PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 04:13:17PM +1300, Kai Huang wrote:
quoted
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
quoted
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
I don't understand what would be the bad thing here. Can you open that
up please? I'm neither capable of predicting the future...
Conceptually they are two different functionalities, and doesn't depend on each
other. Why calling SGX driver initialization only when SGX virtualization
succeeded?
We might want to add reclaiming EPC page (VMM EPC oversubscription) from KVM
guest in the future, which may bring more initialization staff sgx_vepc_init(),
and those new staff should not impact SGX driver.
I don't see your approach is any better, both from concept and flexibility.
Like I said, we can rename to sgx_virt_init() to be more generic, but I
strongly disagree your approach.
quoted
Right, so since vepc_init() does only just device file initialization the
current function structure is fine. I totally forgot that sgx_drv_init()
does not call EPC initialization when I wrote the above :-) We refactored
during the inital cycle the driver so many times that I sometimes fix up
thing, sorry about.
To meld this into code:
ret = sgx_vepc_init();
if (ret != -ENODEV) {
pr_err("vEPC initialization failed with %d.\n", ret);
return ret;
}
ret = sgx_drv_init();
if (ret != ENODEV)
pr_info("Driver initialization failed %d.\n", ret);
Hmm.. Let's say an extreme case: misc_register() failed in sgx_vepc_init(), due
to -ENOMEM. Then OOM kill gets involved, and kills bunch of apps. And then In
this case, theoretically, misc_register() in sgx_drv_init() doesn't need to
fail.
The point is really SGX driver and SGX virt are two independent
functionalities, so don't make dependency on them, manually. Plus I don't see
any benefit of your approach, but only cons.
The way I've understood it is that given that KVM can support SGX
without FLC, vEPC should be available even if driver cannot be
enabled.
This is also exactly what the short summary states.
"Initialize virtual EPC driver even when SGX driver is disabled"
It *does not* state:
"Initialize SGX driver even when vEPC driver is disabled"
Also, this is how I interpret the inline comment.
All this considered, the other direction is undocumented functionality.
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 23:09:55
On Tue, Mar 16, 2021 at 01:05:05AM +0200, Jarkko Sakkinen wrote:
On Tue, Mar 16, 2021 at 09:48:59AM +1300, Kai Huang wrote:
quoted
On Mon, 15 Mar 2021 15:51:17 +0200 Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 03:04:59PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 04:13:17PM +1300, Kai Huang wrote:
quoted
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
quoted
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
I don't understand what would be the bad thing here. Can you open that
up please? I'm neither capable of predicting the future...
Conceptually they are two different functionalities, and doesn't depend on each
other. Why calling SGX driver initialization only when SGX virtualization
succeeded?
We might want to add reclaiming EPC page (VMM EPC oversubscription) from KVM
guest in the future, which may bring more initialization staff sgx_vepc_init(),
and those new staff should not impact SGX driver.
I don't see your approach is any better, both from concept and flexibility.
Like I said, we can rename to sgx_virt_init() to be more generic, but I
strongly disagree your approach.
quoted
Right, so since vepc_init() does only just device file initialization the
current function structure is fine. I totally forgot that sgx_drv_init()
does not call EPC initialization when I wrote the above :-) We refactored
during the inital cycle the driver so many times that I sometimes fix up
thing, sorry about.
To meld this into code:
ret = sgx_vepc_init();
if (ret != -ENODEV) {
pr_err("vEPC initialization failed with %d.\n", ret);
return ret;
}
ret = sgx_drv_init();
if (ret != ENODEV)
pr_info("Driver initialization failed %d.\n", ret);
Hmm.. Let's say an extreme case: misc_register() failed in sgx_vepc_init(), due
to -ENOMEM. Then OOM kill gets involved, and kills bunch of apps. And then In
this case, theoretically, misc_register() in sgx_drv_init() doesn't need to
fail.
The point is really SGX driver and SGX virt are two independent
functionalities, so don't make dependency on them, manually. Plus I don't see
any benefit of your approach, but only cons.
The way I've understood it is that given that KVM can support SGX
without FLC, vEPC should be available even if driver cannot be
enabled.
This is also exactly what the short summary states.
"Initialize virtual EPC driver even when SGX driver is disabled"
It *does not* state:
"Initialize SGX driver even when vEPC driver is disabled"
Also, this is how I interpret the inline comment.
All this considered, the other direction is undocumented functionality.
Also:
1. There is *zero* good practical reasons to support the "2nd direction".
For KVM getting init'd with SGX, on the other hand, we have good
practical reasons.
2. We can get something practically useful with simpler and more verbose
code, i.e. better logging.
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-15 23:12:35
On Tue, Mar 16, 2021 at 09:29:34AM +1300, Kai Huang wrote:
On Mon, 15 Mar 2021 15:19:32 +0200 Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 03:18:16PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 08:12:36PM +1300, Kai Huang wrote:
quoted
On Sat, 13 Mar 2021 12:45:53 +0200 Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
quoted
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Oops, my bad. Yup, ack should be removed.
/Jarkko
Hi Jarkko,
Your reply of your concern of this patch to the cover-letter
https://lore.kernel.org/lkml/YEkJXu262YDa8ZaK@kernel.org/
reminds me to do more sanity check of whether removing EREMOVE in
sgx_free_epc_page() will impact other code path or not, and I think
sgx_encl_release() is not the only place should be changed:
- sgx_encl_shrink() needs to call sgx_encl_free_epc_page(), since when this is
called, the VA page can be already valid -- there are other failures can
trigger sgx_encl_shrink().
You right about this, good catch.
Shrink needs to always do EREMOVE as grow has done EPA, which changes
EPC page state.
quoted
- sgx_encl_add_page() should call sgx_encl_free_epc_page() in "err_out_free:"
label, since the EPC page can be already valid when error happened, i.e. when
EEXTEND fails.
Yes, correct, good work!
quoted
Other places should be OK per my check, but I'd prefer to just replacing all
sgx_free_epc_page() call sites in driver with sgx_encl_free_epc_page(), with
one exception: sgx_alloc_va_page(), which calls sgx_free_epc_page() when EPA
fails, in which case EREMOVE is not required for sure.
I would not unless they require it.
quoted
Your idea, please?
Btw, introducing a driver wrapper of sgx_free_epc_page() does make sense to me,
because virtualization has a counterpart in sgx/virt.c too.
It does make sense to use sgx_free_epc_page() everywhere where it's
the right thing to call and here's why.
If there is some unrelated regression that causes EPC page not get
uninitialized when it actually should, doing extra EREMOVE could mask
those bugs. I.e. it can postpone a failure, which can make a bug harder
to backtrace.
I.e. even though it is true that for correctly working code extra EREMOVE
is nil functionality, it could change semantics for buggy code.
Thanks for feedback. Sorry I am not sure if I understand you. So if we don't
want to bring functionality change, we need to replace sgx_free_epc_page() in
all call sites with sgx_encl_free_epc_page(). To me for this patch only, it's
better not to bring any functional change, so I intend to replace all (I now
consider even leaving sgx_alloc_va_page() out is not good idea in *this*
patch).
Or do you just want to replace sgx_free_epc_page() with
sgx_encl_free_epc_page() in sgx_encl_shrink() and sgx_encl_add_page(), as I
pointed above? In this way there will be functional change in this patch, and
we need to explicitly explain why leaving others out is OK in commit message.
To me I prefer the former.
But yes, I'm cool with your preference and I do get your argument, I just
need to review it, and do not consider it as my patch :-)
/Jarkko
From: Kai Huang <hidden> Date: 2021-03-15 23:50:31
On Tue, 16 Mar 2021 01:08:44 +0200 Jarkko Sakkinen wrote:
On Tue, Mar 16, 2021 at 01:05:05AM +0200, Jarkko Sakkinen wrote:
quoted
On Tue, Mar 16, 2021 at 09:48:59AM +1300, Kai Huang wrote:
quoted
On Mon, 15 Mar 2021 15:51:17 +0200 Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 03:04:59PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 04:13:17PM +1300, Kai Huang wrote:
quoted
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
quoted
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
I don't understand what would be the bad thing here. Can you open that
up please? I'm neither capable of predicting the future...
Conceptually they are two different functionalities, and doesn't depend on each
other. Why calling SGX driver initialization only when SGX virtualization
succeeded?
We might want to add reclaiming EPC page (VMM EPC oversubscription) from KVM
guest in the future, which may bring more initialization staff sgx_vepc_init(),
and those new staff should not impact SGX driver.
I don't see your approach is any better, both from concept and flexibility.
Like I said, we can rename to sgx_virt_init() to be more generic, but I
strongly disagree your approach.
quoted
Right, so since vepc_init() does only just device file initialization the
current function structure is fine. I totally forgot that sgx_drv_init()
does not call EPC initialization when I wrote the above :-) We refactored
during the inital cycle the driver so many times that I sometimes fix up
thing, sorry about.
To meld this into code:
ret = sgx_vepc_init();
if (ret != -ENODEV) {
pr_err("vEPC initialization failed with %d.\n", ret);
return ret;
}
ret = sgx_drv_init();
if (ret != ENODEV)
pr_info("Driver initialization failed %d.\n", ret);
Hmm.. Let's say an extreme case: misc_register() failed in sgx_vepc_init(), due
to -ENOMEM. Then OOM kill gets involved, and kills bunch of apps. And then In
this case, theoretically, misc_register() in sgx_drv_init() doesn't need to
fail.
The point is really SGX driver and SGX virt are two independent
functionalities, so don't make dependency on them, manually. Plus I don't see
any benefit of your approach, but only cons.
The way I've understood it is that given that KVM can support SGX
without FLC, vEPC should be available even if driver cannot be
enabled.
This is also exactly what the short summary states.
"Initialize virtual EPC driver even when SGX driver is disabled"
It *does not* state:
"Initialize SGX driver even when vEPC driver is disabled"
OK. The patch title can be improved. How about:
"Initialize SGX driver and virtual EPC driver independently"
?
quoted
Also, this is how I interpret the inline comment.
All this considered, the other direction is undocumented functionality.
OK. How about below?
/*
* Always try to initialize the native *and* KVM drivers. They are independent
* functionalities and one can be initialized even when the other is not
* supported or fails to initialize.
*/
The explicit saying of "not supported or fails to initialize" was requested by
you -- you wanted to distinguish -ENODEV with other error codes.
Also:
1. There is *zero* good practical reasons to support the "2nd direction".
For KVM getting init'd with SGX, on the other hand, we have good
practical reasons.
Why there's *zero* good practical reasons? With initializing them
independently, people don't need to worry about *internal* of
sgx_vepc_init() and sgx_drv_init(), but just need pay attention of the logic
that they are two independent functionalities. Being able to initialize them
independently is much more clear and easier to understand. And like I said, in
this way it is more flexible to extend -- for instance, we may add more staff
to support VMM EPC oversubscription. So why there is *zero* good practical
reasons?
Btw, there are customers that want to just use KVM SGX, but not SGX driver in
host, for which people may want to add separate CONFIG option, say,
CONFIG_X86_SGX_DRIVER, to be able to disable/enable SGX driver code, just like
CONFIG_X86_SGX_KVM. Make them independent logically just make things more
clear.
2. We can get something practically useful with simpler and more verbose
code, i.e. better logging.
I can add error msg in sgx_vepc_init() upon misc_register() if you want.
From: Kai Huang <hidden> Date: 2021-03-15 23:51:35
On Tue, 16 Mar 2021 00:59:31 +0200 Jarkko Sakkinen wrote:
On Tue, Mar 16, 2021 at 09:29:34AM +1300, Kai Huang wrote:
quoted
On Mon, 15 Mar 2021 15:19:32 +0200 Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 03:18:16PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 08:12:36PM +1300, Kai Huang wrote:
quoted
On Sat, 13 Mar 2021 12:45:53 +0200 Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:21:54PM -0800, Sean Christopherson wrote:
quoted
On Thu, Mar 11, 2021, Kai Huang wrote:
quoted
From: Jarkko Sakkinen <jarkko@kernel.org>
EREMOVE takes a page and removes any association between that page and
an enclave. It must be run on a page before it can be added into
another enclave. Currently, EREMOVE is run as part of pages being freed
into the SGX page allocator. It is not expected to fail.
KVM does not track how guest pages are used, which means that SGX
virtualization use of EREMOVE might fail.
Break out the EREMOVE call from the SGX page allocator. This will allow
the SGX virtualization code to use the allocator directly. (SGX/KVM
will also introduce a more permissive EREMOVE helper).
Implement original sgx_free_epc_page() as sgx_encl_free_epc_page() to be
more specific that it is used to free EPC page assigned to one enclave.
Print an error message when EREMOVE fails to explicitly call out EPC
page is leaked, and requires machine reboot to get leaked pages back.
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Co-developed-by: Kai Huang <redacted>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Kai Huang <redacted>
---
v2->v3:
- Fixed bug during copy/paste which results in SECS page and va pages are not
correctly freed in sgx_encl_release() (sorry for the mistake).
- Added Jarkko's Acked-by.
That Acked-by should either be dropped or moved above Co-developed-by to make
checkpatch happy.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Oops, my bad. Yup, ack should be removed.
/Jarkko
Hi Jarkko,
Your reply of your concern of this patch to the cover-letter
https://lore.kernel.org/lkml/YEkJXu262YDa8ZaK@kernel.org/
reminds me to do more sanity check of whether removing EREMOVE in
sgx_free_epc_page() will impact other code path or not, and I think
sgx_encl_release() is not the only place should be changed:
- sgx_encl_shrink() needs to call sgx_encl_free_epc_page(), since when this is
called, the VA page can be already valid -- there are other failures can
trigger sgx_encl_shrink().
You right about this, good catch.
Shrink needs to always do EREMOVE as grow has done EPA, which changes
EPC page state.
quoted
- sgx_encl_add_page() should call sgx_encl_free_epc_page() in "err_out_free:"
label, since the EPC page can be already valid when error happened, i.e. when
EEXTEND fails.
Yes, correct, good work!
quoted
Other places should be OK per my check, but I'd prefer to just replacing all
sgx_free_epc_page() call sites in driver with sgx_encl_free_epc_page(), with
one exception: sgx_alloc_va_page(), which calls sgx_free_epc_page() when EPA
fails, in which case EREMOVE is not required for sure.
I would not unless they require it.
quoted
Your idea, please?
Btw, introducing a driver wrapper of sgx_free_epc_page() does make sense to me,
because virtualization has a counterpart in sgx/virt.c too.
It does make sense to use sgx_free_epc_page() everywhere where it's
the right thing to call and here's why.
If there is some unrelated regression that causes EPC page not get
uninitialized when it actually should, doing extra EREMOVE could mask
those bugs. I.e. it can postpone a failure, which can make a bug harder
to backtrace.
I.e. even though it is true that for correctly working code extra EREMOVE
is nil functionality, it could change semantics for buggy code.
Thanks for feedback. Sorry I am not sure if I understand you. So if we don't
want to bring functionality change, we need to replace sgx_free_epc_page() in
all call sites with sgx_encl_free_epc_page(). To me for this patch only, it's
better not to bring any functional change, so I intend to replace all (I now
consider even leaving sgx_alloc_va_page() out is not good idea in *this*
patch).
Or do you just want to replace sgx_free_epc_page() with
sgx_encl_free_epc_page() in sgx_encl_shrink() and sgx_encl_add_page(), as I
pointed above? In this way there will be functional change in this patch, and
we need to explicitly explain why leaving others out is OK in commit message.
To me I prefer the former.
The original purpose of this patch was exactly to remove EREMOVE
sgx_free_epc_page() and call it explicitly where it is required. That's
why I introduced sgx_reset_epc_page(). So the latter was actually the goal
of this patch at least when I did it. Now this is something completely
different.
So, I don't consider myself author of this patch in any possible way,
because this is not what I intended.
To move forward, for the next patch set version, you should change the
author field as yourself, and remove all my tags, and I will review it.
So you can work out this with former approach if you wish.
I.e. my ack/nak/etc. apply to this patch because it's not my code.
From: Sean Christopherson <seanjc@google.com> Date: 2021-03-16 01:14:56
On Tue, Mar 16, 2021, Jarkko Sakkinen wrote:
On Tue, Mar 16, 2021 at 01:05:05AM +0200, Jarkko Sakkinen wrote:
quoted
The way I've understood it is that given that KVM can support SGX
without FLC, vEPC should be available even if driver cannot be
enabled.
This is also exactly what the short summary states.
"Initialize virtual EPC driver even when SGX driver is disabled"
It *does not* state:
"Initialize SGX driver even when vEPC driver is disabled"
Also, this is how I interpret the inline comment.
All this considered, the other direction is undocumented functionality.
Also:
1. There is *zero* good practical reasons to support the "2nd direction".
Uh, yes there is. CONFIG_KVM_INTEL=n and X86_FEATURE_VMX=n, either of which
will cause vEPC initialization to fail. The former is obvious, the latter is
possible via BIOS configuration.
For KVM getting init'd with SGX, on the other hand, we have good
practical reasons.
2. We can get something practically useful with simpler and more verbose
code, i.e. better logging.
/Jarkko
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-16 12:45:28
On Tue, Mar 16, 2021 at 12:49:33PM +1300, Kai Huang wrote:
On Tue, 16 Mar 2021 01:08:44 +0200 Jarkko Sakkinen wrote:
quoted
On Tue, Mar 16, 2021 at 01:05:05AM +0200, Jarkko Sakkinen wrote:
quoted
On Tue, Mar 16, 2021 at 09:48:59AM +1300, Kai Huang wrote:
quoted
On Mon, 15 Mar 2021 15:51:17 +0200 Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 03:04:59PM +0200, Jarkko Sakkinen wrote:
quoted
On Mon, Mar 15, 2021 at 04:13:17PM +1300, Kai Huang wrote:
quoted
On Sun, 14 Mar 2021 17:27:18 +0200 Jarkko Sakkinen wrote:
quoted
On Sun, Mar 14, 2021 at 05:25:26PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:07:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Sat, Mar 13, 2021 at 09:05:36PM +0200, Jarkko Sakkinen wrote:
quoted
On Fri, Mar 12, 2021 at 01:44:58PM -0800, Sean Christopherson wrote:
quoted
On Tue, Mar 09, 2021, Kai Huang wrote:
quoted
Modify sgx_init() to always try to initialize the virtual EPC driver,
even if the SGX driver is disabled. The SGX driver might be disabled
if SGX Launch Control is in locked mode, or not supported in the
hardware at all. This allows (non-Linux) guests that support non-LC
configurations to use SGX.
Acked-by: Dave Hansen <redacted>
Signed-off-by: Kai Huang <redacted>
---
arch/x86/kernel/cpu/sgx/main.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
@@ -712,7 +712,15 @@ static int __init sgx_init(void)gotoerr_page_cache;}-ret=sgx_drv_init();+/*+*Alwaystrytoinitializethenative*and*KVMdrivers.+*TheKVMdriverislesspickythanthenativeoneand+*canfunctionifthenativeoneisnotsupportedonthe+*currentsystemorfailstoinitialize.+*+*Erroroutonlyifbothfailtoinitialize.+*/+ret=!!sgx_drv_init()&!!sgx_vepc_init();
I love this code.
Reviewed-by: Sean Christopherson <seanjc@google.com>
I'm still wondering why this code let's go through when sgx_drv_init()
succeeds and sgx_vepc_init() fails.
The inline comment explains only the mirrored case (which does make
sense).
I.e. if sgx_drv_init() succeeds, I'd expect that sgx_vepc_init() must
succeed. Why expect legitly anything else?
Apologies coming with these ideas at this point, but here is what this
led me.
I think that the all this complexity comes from a bad code structure.
So, what is essentially happening here:
- We essentially want to make EPC always work.
- Driver optionally.
So what this sums to is something like:
ret = sgx_epc_init();
if (ret) {
pr_err("EPC initialization failed.\n");
return ret;
}
ret = sgx_drv_init();
if (ret)
pr_info("Driver could not be initialized.\n");
/* continue */
I.e. I think there should be a single EPC init, which does both EPC
bootstrapping and vepc, and driver initialization comes after that.
In other words, from SGX point of view, the thing that KVM needs is
to cut out EPC and driver part into different islands. How this is now
implemented in the current patch set is half-way there but not yet what
it should be.
Well conceptually, SGX virtualization and SGX driver are two independently
functionalities can be enabled separately, although they both requires some
come functionalities, such as /dev/sgx_provision, which we have moved to
sgx/main.c exactly for this purpose. THerefore, conceptually, it is bad to make
assumption that, if SGX virtualization initialization succeeded, SGX driver
must succeed -- we can potentially add more staff in SGX virtualization in the
future..
If the name sgx_vepc_init() confuses you, I can rename it to sgx_virt_init().
I don't understand what would be the bad thing here. Can you open that
up please? I'm neither capable of predicting the future...
Conceptually they are two different functionalities, and doesn't depend on each
other. Why calling SGX driver initialization only when SGX virtualization
succeeded?
We might want to add reclaiming EPC page (VMM EPC oversubscription) from KVM
guest in the future, which may bring more initialization staff sgx_vepc_init(),
and those new staff should not impact SGX driver.
I don't see your approach is any better, both from concept and flexibility.
Like I said, we can rename to sgx_virt_init() to be more generic, but I
strongly disagree your approach.
quoted
Right, so since vepc_init() does only just device file initialization the
current function structure is fine. I totally forgot that sgx_drv_init()
does not call EPC initialization when I wrote the above :-) We refactored
during the inital cycle the driver so many times that I sometimes fix up
thing, sorry about.
To meld this into code:
ret = sgx_vepc_init();
if (ret != -ENODEV) {
pr_err("vEPC initialization failed with %d.\n", ret);
return ret;
}
ret = sgx_drv_init();
if (ret != ENODEV)
pr_info("Driver initialization failed %d.\n", ret);
Hmm.. Let's say an extreme case: misc_register() failed in sgx_vepc_init(), due
to -ENOMEM. Then OOM kill gets involved, and kills bunch of apps. And then In
this case, theoretically, misc_register() in sgx_drv_init() doesn't need to
fail.
The point is really SGX driver and SGX virt are two independent
functionalities, so don't make dependency on them, manually. Plus I don't see
any benefit of your approach, but only cons.
The way I've understood it is that given that KVM can support SGX
without FLC, vEPC should be available even if driver cannot be
enabled.
This is also exactly what the short summary states.
"Initialize virtual EPC driver even when SGX driver is disabled"
It *does not* state:
"Initialize SGX driver even when vEPC driver is disabled"
OK. The patch title can be improved. How about:
"Initialize SGX driver and virtual EPC driver independently"
?
quoted
quoted
Also, this is how I interpret the inline comment.
All this considered, the other direction is undocumented functionality.
OK. How about below?
/*
* Always try to initialize the native *and* KVM drivers. They are independent
* functionalities and one can be initialized even when the other is not
* supported or fails to initialize.
*/
The explicit saying of "not supported or fails to initialize" was requested by
you -- you wanted to distinguish -ENODEV with other error codes.
quoted
Also:
1. There is *zero* good practical reasons to support the "2nd direction".
For KVM getting init'd with SGX, on the other hand, we have good
practical reasons.
Why there's *zero* good practical reasons? With initializing them
independently, people don't need to worry about *internal* of
sgx_vepc_init() and sgx_drv_init(), but just need pay attention of the logic
that they are two independent functionalities. Being able to initialize them
independently is much more clear and easier to understand. And like I said, in
this way it is more flexible to extend -- for instance, we may add more staff
to support VMM EPC oversubscription. So why there is *zero* good practical
reasons?
Then things would be reconsidered.
Btw, there are customers that want to just use KVM SGX, but not SGX driver in
host, for which people may want to add separate CONFIG option, say,
CONFIG_X86_SGX_DRIVER, to be able to disable/enable SGX driver code, just like
CONFIG_X86_SGX_KVM. Make them independent logically just make things more
clear.
Why?
quoted
2. We can get something practically useful with simpler and more verbose
code, i.e. better logging.
I can add error msg in sgx_vepc_init() upon misc_register() if you want.
From: Jarkko Sakkinen <jarkko@kernel.org> Date: 2021-03-16 12:48:14
On Mon, Mar 15, 2021 at 06:13:53PM -0700, Sean Christopherson wrote:
On Tue, Mar 16, 2021, Jarkko Sakkinen wrote:
quoted
On Tue, Mar 16, 2021 at 01:05:05AM +0200, Jarkko Sakkinen wrote:
quoted
The way I've understood it is that given that KVM can support SGX
without FLC, vEPC should be available even if driver cannot be
enabled.
This is also exactly what the short summary states.
"Initialize virtual EPC driver even when SGX driver is disabled"
It *does not* state:
"Initialize SGX driver even when vEPC driver is disabled"
Also, this is how I interpret the inline comment.
All this considered, the other direction is undocumented functionality.
Also:
1. There is *zero* good practical reasons to support the "2nd direction".
Uh, yes there is. CONFIG_KVM_INTEL=n and X86_FEATURE_VMX=n, either of which
will cause vEPC initialization to fail. The former is obvious, the latter is
possible via BIOS configuration.
Hmm... So you make the checks as if ret != -ENODEV? That's the sane way to
deal with that situation IMHO.
/Jarkko
From: Kai Huang <hidden> Date: 2021-03-18 00:05:50
On Tue, 16 Mar 2021 14:46:05 +0200 Jarkko Sakkinen wrote:
On Mon, Mar 15, 2021 at 06:13:53PM -0700, Sean Christopherson wrote:
quoted
On Tue, Mar 16, 2021, Jarkko Sakkinen wrote:
quoted
On Tue, Mar 16, 2021 at 01:05:05AM +0200, Jarkko Sakkinen wrote:
quoted
The way I've understood it is that given that KVM can support SGX
without FLC, vEPC should be available even if driver cannot be
enabled.
This is also exactly what the short summary states.
"Initialize virtual EPC driver even when SGX driver is disabled"
It *does not* state:
"Initialize SGX driver even when vEPC driver is disabled"
Also, this is how I interpret the inline comment.
All this considered, the other direction is undocumented functionality.
Also:
1. There is *zero* good practical reasons to support the "2nd direction".
Uh, yes there is. CONFIG_KVM_INTEL=n and X86_FEATURE_VMX=n, either of which
will cause vEPC initialization to fail. The former is obvious, the latter is
possible via BIOS configuration.
Hmm... So you make the checks as if ret != -ENODEV? That's the sane way to
deal with that situation IMHO.
/Jarkko
OK. I actually wrote the code to show your idea:
- ret = sgx_drv_init();
- if (ret)
+ /*
+ * Only continue to initialize SGX driver when SGX virtualization
+ * initialization is successful, or is not supported (-ENODEV), since
+ * any other initialization failure means SGX driver is unlikely to be
+ * initialized successfully.
+ */
+ ret = sgx_vepc_init();
+ if (ret && ret != -ENODEV)
goto err_kthread;
+ if (sgx_drv_init()) {
+ /*
+ * Cleanup when *both* SGX virtualization and SGX driver are
+ * not enabled, due to either not supported (-ENODEV), or
+ * somehow fail to initialize.
+ */
+ if (ret && ret != -ENODEV)
+ goto err_kthread;
+ }
+
return 0;
err_kthread:
Is this the code you want?