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
