Messages in this thread | | | Subject | Re: [PATCH v11 08/13] x86, sgx: added ENCLS wrappers | From | Dave Hansen <> | Date | Fri, 8 Jun 2018 10:43:50 -0700 |
| |
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?
| |