Re: [PATCH] Enable SGX intrinsics

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Dec 30, 2016 at 09:25:49AM +0100, Uros Bizjak wrote:
> On Thu, Dec 29, 2016 at 10:50 AM, Koval, Julia <julia.koval@intel.com> wrote:
> > Hi,
> >
> > This patch enables Intel SGX instructions (Reference: https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf page 4478 in pdf and 3D 41-1 in page numbers) Ok for trunk?
> 
> I don't like asm macros, but since we can tolerate similar
> implementation of cpuid, we can also tolerate encls/enclu.
> 
> One genreal remark:
> 
> +#define macro_encls_bc(leaf, b, c, retval)             \
> +  __asm__ __volatile__ ("encls\n\t"                 \
> +       : "=a" (retval)                     \
> +       : "a" (leaf), "b" (b), "c" (c))
> 
> These internal macros are user-visible, so please uglify them with a
> double underscore, like
> 
> __encls_bc
> 
> to put them into internal namespace. IMO, there is no need to use
> "macro" prefix.

I see other issues:

+extern __inline int
+__attribute__((__gnu_inline__, __always_inline__, __artificial__))
+_encls_u32 (const int __leaf, size_t *data)
+{
+  enum encls_type type = (enum encls_type)__leaf;
+  int retval = 0;
+  if (!__builtin_constant_p (type))
+  {
+    macro_encls_generic (__leaf, data[0], data[1], data[2], retval);
+  }
+  else

1) some parameters and variable names not uglified in inline functions,
data, type, retval in this case; using __data etc. or __L, __D, __T, __R
would be better; look at other intrin files for what identifiers they are
using
2) the indentation is wrong, { should after if/else should be indented
2 more columns to the right, so column 4 in this case, and the body of the
block 2 further positions.
3) For a single line body there should be no {}s around, so put the macro
without the {}s.

Is:
enum encls_type {ECREATE, EADD, EINIT, EREMOVE, EDBGRD, EDBGWR, EEXTEND,
                 ELDB, ELDU, EBLOCK, EPA, EWB, ETRACK, EAUG, EMODPR, EMODT};

enum enclu_type {EREPORT, EGETKEY, EENTER, ERESUME, EEXIT, EACCEPT, EMODPE,
                 EACCEPTCOPY};
really part of the official sgxintrin.h ABI?
4) the enum names are not uglified
5) the enumerators use namespace reserved for errno.h extensions, see
   http://pubs.opengroup.org/onlinepubs/007904975/functions/xsh_chap02_02.html
Header		Prefix
<errno.h>	E[0-9], E[A-Z]
Especially because they aren't used for errors in this case, it is a
particularly bad choice; and, because the header is unconditionally
included in x86intrin.h, this won't affect just users that want SGX, but all
users of x86intrin.h, which means huge amount of code in the wild.

	Jakub