| Message ID | 20180608171216.26521-9-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/08/2018 10:09 AM, Jarkko Sakkinen wrote: > This commit adds wrappers for Intel(R) SGX ENCLS opcode functionality. What's ENCLS? I know what an opcode is, but I don't know what "opcode functionality" is. Could you give us more than a single, cryptic sentence, please? > +enum sgx_commands { > + ECREATE = 0x0, > + EADD = 0x1, > + EINIT = 0x2, > + EREMOVE = 0x3, > + EDGBRD = 0x4, > + EDGBWR = 0x5, > + EEXTEND = 0x6, > + ELDU = 0x8, > + EBLOCK = 0x9, > + EPA = 0xA, > + EWB = 0xB, > + ETRACK = 0xC, > + EAUG = 0xD, > + EMODPR = 0xE, > + EMODT = 0xF, > +}; Again, please differentiate hardware-defined values from software-defines ones. Also, would it hurt to expand the acronyms a bit, like: + ELDU = 0x8, /* LoaD Underpants */ > +#define SGX_FN(name, params...) \ > +{ \ > + void *epc; \ > + int ret; \ > + epc = sgx_get_page(epc_page); \ > + ret = __##name(params); \ > + sgx_put_page(epc); \ > + return ret; \ > +} Have I seen sgx_*_page() yet in this series? This seems out of order. > +#define BUILD_SGX_FN(fn, name) \ > +static inline int fn(struct sgx_epc_page *epc_page) \ > + SGX_FN(name, epc) > +BUILD_SGX_FN(sgx_eremove, eremove) > +BUILD_SGX_FN(sgx_eblock, eblock) > +BUILD_SGX_FN(sgx_etrack, etrack) > +BUILD_SGX_FN(sgx_epa, epa) > + > +static inline int sgx_emodpr(struct sgx_secinfo *secinfo, > + struct sgx_epc_page *epc_page) > + SGX_FN(emodpr, secinfo, epc) > +static inline int sgx_emodt(struct sgx_secinfo *secinfo, > + struct sgx_epc_page *epc_page) > + SGX_FN(emodt, secinfo, epc) Wow, that's hideous. Can't you just do: BUILD_SGX_FN(__sgx_emopt, foo) static inline int sgx_emodt(struct sgx_secinfo *secinfo, struct sgx_epc_page *epc_page) { return __sgx_emopt(secinfo, page); } Also, this entire patch seems rather comment-free. Was that intentional?
On Fri, Jun 08, 2018 at 10:43:50AM -0700, Dave Hansen wrote: > On 06/08/2018 10:09 AM, Jarkko Sakkinen wrote: > > This commit adds wrappers for Intel(R) SGX ENCLS opcode functionality. > > What's ENCLS? I know what an opcode is, but I don't know what "opcode > functionality" is. Could you give us more than a single, cryptic > sentence, please? Sure. > > +enum sgx_commands { > > + ECREATE = 0x0, > > + EADD = 0x1, > > + EINIT = 0x2, > > + EREMOVE = 0x3, > > + EDGBRD = 0x4, > > + EDGBWR = 0x5, > > + EEXTEND = 0x6, > > + ELDU = 0x8, > > + EBLOCK = 0x9, > > + EPA = 0xA, > > + EWB = 0xB, > > + ETRACK = 0xC, > > + EAUG = 0xD, > > + EMODPR = 0xE, > > + EMODT = 0xF, > > +}; > > Again, please differentiate hardware-defined values from > software-defines ones. Also, would it hurt to expand the acronyms a > bit, like: > > + ELDU = 0x8, /* LoaD Underpants */ Not a bad idea at all. > > +#define SGX_FN(name, params...) \ > > +{ \ > > + void *epc; \ > > + int ret; \ > > + epc = sgx_get_page(epc_page); \ > > + ret = __##name(params); \ > > + sgx_put_page(epc); \ > > + return ret; \ > > +} > > Have I seen sgx_*_page() yet in this series? This seems out of order. Oops, thanks for spotting this out. > > +#define BUILD_SGX_FN(fn, name) \ > > +static inline int fn(struct sgx_epc_page *epc_page) \ > > + SGX_FN(name, epc) > > +BUILD_SGX_FN(sgx_eremove, eremove) > > +BUILD_SGX_FN(sgx_eblock, eblock) > > +BUILD_SGX_FN(sgx_etrack, etrack) > > +BUILD_SGX_FN(sgx_epa, epa) > > + > > +static inline int sgx_emodpr(struct sgx_secinfo *secinfo, > > + struct sgx_epc_page *epc_page) > > + SGX_FN(emodpr, secinfo, epc) > > +static inline int sgx_emodt(struct sgx_secinfo *secinfo, > > + struct sgx_epc_page *epc_page) > > + SGX_FN(emodt, secinfo, epc) > > Wow, that's hideous. > > Can't you just do: > > BUILD_SGX_FN(__sgx_emopt, foo) > > static inline int sgx_emodt(struct sgx_secinfo *secinfo, > struct sgx_epc_page *epc_page) > { > return __sgx_emopt(secinfo, page); > } > > Also, this entire patch seems rather comment-free. Was that intentional? Something that I've ignored (big series) but I'll add comments to the next version. /Jarkko
On Fri, 2018-06-08 at 19:09 +0200, Jarkko Sakkinen wrote: > This commit adds wrappers for Intel(R) SGX ENCLS opcode functionality. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/include/asm/sgx.h | 198 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 198 insertions(+) > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index fa3e6e0eb8af..a2f727f85b91 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -10,6 +10,10 @@ > #ifndef _ASM_X86_SGX_H > #define _ASM_X86_SGX_H > > +#include <asm/sgx_arch.h> > +#include <asm/asm.h> > +#include <linux/bitops.h> > +#include <linux/err.h> > #include <linux/types.h> > > #define SGX_CPUID 0x12 > @@ -20,6 +24,200 @@ enum sgx_cpuid { > SGX_CPUID_EPC_BANKS = 2, > }; > > +enum sgx_commands { This should be something like "sgx_encls_leafs" and probably moved to sgx_arch.h (as Dave alluded to, these are architectural values). "sgx_commands" is not accurate (they're only the cpl0 "commands") and will have collision issues in the future, e.g. with the ENCLV instruction and its leafs. > + ECREATE = 0x0, > + EADD = 0x1, > + EINIT = 0x2, > + EREMOVE = 0x3, > + EDGBRD = 0x4, > + EDGBWR = 0x5, > + EEXTEND = 0x6, Even though it's not used in the code (yet...), I think ELDB, leaf 0x7, should be defined here for completeness. > + ELDU = 0x8, > + EBLOCK = 0x9, > + EPA = 0xA, > + EWB = 0xB, > + ETRACK = 0xC, > + EAUG = 0xD, > + EMODPR = 0xE, > + EMODT = 0xF, > +};
On Wed, 2018-06-20 at 06:12 -0700, Sean Christopherson wrote: > On Fri, 2018-06-08 at 19:09 +0200, Jarkko Sakkinen wrote: > > This commit adds wrappers for Intel(R) SGX ENCLS opcode functionality. > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > arch/x86/include/asm/sgx.h | 198 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 198 insertions(+) > > > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > index fa3e6e0eb8af..a2f727f85b91 100644 > > --- a/arch/x86/include/asm/sgx.h > > +++ b/arch/x86/include/asm/sgx.h > > @@ -10,6 +10,10 @@ > > #ifndef _ASM_X86_SGX_H > > #define _ASM_X86_SGX_H > > > > +#include <asm/sgx_arch.h> > > +#include <asm/asm.h> > > +#include <linux/bitops.h> > > +#include <linux/err.h> > > #include <linux/types.h> > > > > #define SGX_CPUID 0x12 > > @@ -20,6 +24,200 @@ enum sgx_cpuid { > > SGX_CPUID_EPC_BANKS = 2, > > }; > > > > +enum sgx_commands { > > This should be something like "sgx_encls_leafs" and probably moved > to sgx_arch.h (as Dave alluded to, these are architectural values). > "sgx_commands" is not accurate (they're only the cpl0 "commands") > and will have collision issues in the future, e.g. with the ENCLV > instruction and its leafs. > > > + ECREATE = 0x0, > > + EADD = 0x1, > > + EINIT = 0x2, > > + EREMOVE = 0x3, > > + EDGBRD = 0x4, > > + EDGBWR = 0x5, > > + EEXTEND = 0x6, > > Even though it's not used in the code (yet...), I think ELDB, > leaf 0x7, should be defined here for completeness. Sure. /Jarkko
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index fa3e6e0eb8af..a2f727f85b91 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -10,6 +10,10 @@ #ifndef _ASM_X86_SGX_H #define _ASM_X86_SGX_H +#include <asm/sgx_arch.h> +#include <asm/asm.h> +#include <linux/bitops.h> +#include <linux/err.h> #include <linux/types.h> #define SGX_CPUID 0x12 @@ -20,6 +24,200 @@ enum sgx_cpuid { SGX_CPUID_EPC_BANKS = 2, }; +enum sgx_commands { + ECREATE = 0x0, + EADD = 0x1, + EINIT = 0x2, + EREMOVE = 0x3, + EDGBRD = 0x4, + EDGBWR = 0x5, + EEXTEND = 0x6, + ELDU = 0x8, + EBLOCK = 0x9, + EPA = 0xA, + EWB = 0xB, + ETRACK = 0xC, + EAUG = 0xD, + EMODPR = 0xE, + EMODT = 0xF, +}; + +#define IS_ENCLS_FAULT(r) ((r) & 0xffff0000) +#define ENCLS_FAULT_VECTOR(r) ((r) >> 16) + +#define ENCLS_TO_ERR(r) (IS_ENCLS_FAULT(r) ? -EFAULT : \ + (r) == SGX_UNMASKED_EVENT ? -EINTR : \ + (r) == SGX_MAC_COMPARE_FAIL ? -EIO : \ + (r) == SGX_ENTRYEPOCH_LOCKED ? -EBUSY : -EPERM) + +#define __encls_ret_N(rax, inputs...) \ + ({ \ + int ret; \ + asm volatile( \ + "1: .byte 0x0f, 0x01, 0xcf;\n\t" \ + "2:\n" \ + ".section .fixup,\"ax\"\n" \ + "3: shll $16,%%eax\n" \ + " jmp 2b\n" \ + ".previous\n" \ + _ASM_EXTABLE_FAULT(1b, 3b) \ + : "=a"(ret) \ + : "a"(rax), inputs \ + : "memory"); \ + ret; \ + }) + +#define __encls_ret_1(rax, rcx) \ + ({ \ + __encls_ret_N(rax, "c"(rcx)); \ + }) + +#define __encls_ret_2(rax, rbx, rcx) \ + ({ \ + __encls_ret_N(rax, "b"(rbx), "c"(rcx)); \ + }) + +#define __encls_ret_3(rax, rbx, rcx, rdx) \ + ({ \ + __encls_ret_N(rax, "b"(rbx), "c"(rcx), "d"(rdx)); \ + }) + +#define __encls_N(rax, rbx_out, inputs...) \ + ({ \ + int ret; \ + asm volatile( \ + "1: .byte 0x0f, 0x01, 0xcf;\n\t" \ + " xor %%eax,%%eax;\n" \ + "2:\n" \ + ".section .fixup,\"ax\"\n" \ + "3: shll $16,%%eax\n" \ + " jmp 2b\n" \ + ".previous\n" \ + _ASM_EXTABLE_FAULT(1b, 3b) \ + : "=a"(ret), "=b"(rbx_out) \ + : "a"(rax), inputs \ + : "memory"); \ + ret; \ + }) + +#define __encls_2(rax, rbx, rcx) \ + ({ \ + unsigned long ign_rbx_out; \ + __encls_N(rax, ign_rbx_out, "b"(rbx), "c"(rcx)); \ + }) + +#define __encls_1_1(rax, data, rcx) \ + ({ \ + unsigned long rbx_out; \ + int ret = __encls_N(rax, rbx_out, "c"(rcx)); \ + if (!ret) \ + data = rbx_out; \ + ret; \ + }) + +static inline int __ecreate(struct sgx_pageinfo *pginfo, void *secs) +{ + return __encls_2(ECREATE, pginfo, secs); +} + +static inline int __eextend(void *secs, void *epc) +{ + return __encls_2(EEXTEND, secs, epc); +} + +static inline int __eadd(struct sgx_pageinfo *pginfo, void *epc) +{ + return __encls_2(EADD, pginfo, epc); +} + +static inline int __einit(void *sigstruct, struct sgx_einittoken *einittoken, + void *secs) +{ + return __encls_ret_3(EINIT, sigstruct, secs, einittoken); +} + +static inline int __eremove(void *epc) +{ + return __encls_ret_1(EREMOVE, epc); +} + +static inline int __edbgwr(unsigned long addr, unsigned long *data) +{ + return __encls_2(EDGBWR, *data, addr); +} + +static inline int __edbgrd(unsigned long addr, unsigned long *data) +{ + return __encls_1_1(EDGBRD, *data, addr); +} + +static inline int __etrack(void *epc) +{ + return __encls_ret_1(ETRACK, epc); +} + +static inline int __eldu(struct sgx_pageinfo *pginfo, void *epc, void *va) +{ + return __encls_ret_3(ELDU, pginfo, epc, va); +} + +static inline int __eblock(void *epc) +{ + return __encls_ret_1(EBLOCK, epc); +} + +static inline int __epa(void *epc) +{ + unsigned long rbx = SGX_PAGE_TYPE_VA; + + return __encls_2(EPA, rbx, epc); +} + +static inline int __ewb(struct sgx_pageinfo *pginfo, void *epc, void *va) +{ + return __encls_ret_3(EWB, pginfo, epc, va); +} + +static inline int __eaug(struct sgx_pageinfo *pginfo, void *epc) +{ + return __encls_2(EAUG, pginfo, epc); +} + +static inline int __emodpr(struct sgx_secinfo *secinfo, void *epc) +{ + return __encls_ret_2(EMODPR, secinfo, epc); +} + +static inline int __emodt(struct sgx_secinfo *secinfo, void *epc) +{ + return __encls_ret_2(EMODT, secinfo, epc); +} + extern bool sgx_enabled; +#define SGX_FN(name, params...) \ +{ \ + void *epc; \ + int ret; \ + epc = sgx_get_page(epc_page); \ + ret = __##name(params); \ + sgx_put_page(epc); \ + return ret; \ +} + +#define BUILD_SGX_FN(fn, name) \ +static inline int fn(struct sgx_epc_page *epc_page) \ + SGX_FN(name, epc) +BUILD_SGX_FN(sgx_eremove, eremove) +BUILD_SGX_FN(sgx_eblock, eblock) +BUILD_SGX_FN(sgx_etrack, etrack) +BUILD_SGX_FN(sgx_epa, epa) + +static inline int sgx_emodpr(struct sgx_secinfo *secinfo, + struct sgx_epc_page *epc_page) + SGX_FN(emodpr, secinfo, epc) +static inline int sgx_emodt(struct sgx_secinfo *secinfo, + struct sgx_epc_page *epc_page) + SGX_FN(emodt, secinfo, epc) + #endif /* _ASM_X86_SGX_H */
This commit adds wrappers for Intel(R) SGX ENCLS opcode functionality. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/include/asm/sgx.h | 198 +++++++++++++++++++++++++++++++++++++ 1 file changed, 198 insertions(+)