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?
