Thank you for your comments, how about this patch? Enums are not part of the intrinsic ABI, they are just meaningful names for constants, taken from reference doc.
gcc/
* common/config/i386/i386-common.c
(OPTION_MASK_ISA_SGX_UNSET, OPTION_MASK_ISA_SGX_SET): New.
(ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* config/i386/cpuid.h (bit_SGX): New.
* config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
* config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
* config/i386/i386.c
(ix86_target_string): Add -msgx.
(PTA_SGX): New.
(ix86_option_override_internal): Handle new options.
(ix86_valid_target_attribute_inner_p): Add sgx.
* config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
* config/i386/i386.opt: Add msgx.
* config/i386/sgxintrin.h: New file.
* config/i386/x86intrin.h: Add sgxintrin.h.
* testsuite/gcc.target/i386/sgx.c New test
libgcc/
config/i386/cpuinfo.c (get_available_features): Handle FEATURE_SGX.
config/i386/cpuinfo.h (FEATURE_SGX): New.
BR,
Julia
-----Original Message-----
From: Jakub Jelinek [mailto:ja...@redhat.com]
Sent: Friday, December 30, 2016 10:19 AM
To: Uros Bizjak <ubiz...@gmail.com>
Cc: Koval, Julia <julia.ko...@intel.com>; gcc-patches@gcc.gnu.org;
vaalfr...@gmail.com; Senkevich, Andrew <andrew.senkev...@intel.com>
Subject: Re: [PATCH] Enable SGX intrinsics
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.ko...@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
0001-Enable-SGX.PATCH
Description: 0001-Enable-SGX.PATCH
