| Message ID | cover.1615250634.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM SGX virtualization support | expand |
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.
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: > > 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.
On 09/03/21 10:30, Borislav Petkov wrote: > 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. Sounds great. Paolo
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: > > 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.
On Wed, Mar 10, 2021 at 10:27:05PM +1300, Kai Huang wrote:
> 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.
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: > > 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. > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette /Jarkko
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: > > 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. > > 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? > > > Thx. > > > > -- > > Regards/Gruss, > > Boris. > > > > https://people.kernel.org/tglx/notes-about-netiquette > > > /Jarkko
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: > > 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.
On Tue, Mar 09, 2021, Paolo Bonzini wrote: > On 09/03/21 10:30, Borislav Petkov wrote: > > 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. > > 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!
On Fri, 2021-03-12 at 14:04 -0800, Sean Christopherson wrote: > On Tue, Mar 09, 2021, Paolo Bonzini wrote: > > On 09/03/21 10:30, Borislav Petkov wrote: > > > 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. > > > > 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.
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