Re: [PATCH v2 00/25] KVM SGX virtualization support
From: Kai Huang <hidden>
Date: 2021-03-10 20:45:02
Also in:
lkml
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?
quoted
Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette/Jarkko