Re: [PATCH] Enable SGX intrinsics
On Wed, Jan 11, 2017 at 8:59 PM, Koval, Julia wrote:
> Hi, I rebased the patch onto latest trunk version and changed specification
> according to ICC:
> _enclu_u32 (const int __L, size_t *__D) --> _enclu_u32 (const int __L,
> size_t __D[])
I have committed the patch with additional testsuite changes and
removal of libgcc part. The later is wrong, since it changes order of
bits (please see the comment above the enum), and is generally not
needed, since it doesn't implement ABI addition, like SSE, AVX, or
similar.
2017-01-11 Julia Koval
* common/config/i386/i386-common.c (OPTION_MASK_ISA_SGX_UNSET): New.
(OPTION_MASK_ISA_SGX_SET): New.
(ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* 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/ChangeLog:
2017-01-11 Julia Koval
Uros Bizjak
* gcc.target/i386/sgx.c New test.
* gcc.target/i386/sse-12.c: Add -msgx.
* gcc.target/i386/sse-13.c: Ditto.
* gcc.target/i386/sse-14.c: Ditto.
* gcc.target/i386/sse-22.c: Ditto.
* gcc.target/i386/sse-23.c: Ditto.
* g++.dg/other/i386-2.C: Ditto.
* g++.dg/other/i386-3.C: Ditto.
Uros.
Index: common/config/i386/i386-common.c
===
--- common/config/i386/i386-common.c(revision 244335)
+++ common/config/i386/i386-common.c(working copy)
@@ -116,6 +116,7 @@ along with GCC; see the file COPYING3. If not see
#define OPTION_MASK_ISA_ABM_SET \
(OPTION_MASK_ISA_ABM | OPTION_MASK_ISA_POPCNT)
+#define OPTION_MASK_ISA_SGX_SET OPTION_MASK_ISA_SGX
#define OPTION_MASK_ISA_BMI_SET OPTION_MASK_ISA_BMI
#define OPTION_MASK_ISA_BMI2_SET OPTION_MASK_ISA_BMI2
#define OPTION_MASK_ISA_LZCNT_SET OPTION_MASK_ISA_LZCNT
@@ -214,6 +215,7 @@ along with GCC; see the file COPYING3. If not see
#define OPTION_MASK_ISA_SHA_UNSET OPTION_MASK_ISA_SHA
#define OPTION_MASK_ISA_PCLMUL_UNSET OPTION_MASK_ISA_PCLMUL
#define OPTION_MASK_ISA_ABM_UNSET OPTION_MASK_ISA_ABM
+#define OPTION_MASK_ISA_SGX_UNSET OPTION_MASK_ISA_SGX
#define OPTION_MASK_ISA_BMI_UNSET OPTION_MASK_ISA_BMI
#define OPTION_MASK_ISA_BMI2_UNSET OPTION_MASK_ISA_BMI2
#define OPTION_MASK_ISA_LZCNT_UNSET OPTION_MASK_ISA_LZCNT
@@ -500,6 +502,19 @@ ix86_handle_option (struct gcc_options *opts,
}
return true;
+case OPT_msgx:
+ if (value)
+ {
+ opts->x_ix86_isa_flags2 |= OPTION_MASK_ISA_SGX_SET;
+ opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_SGX_SET;
+ }
+ else
+ {
+ opts->x_ix86_isa_flags2 &= ~OPTION_MASK_ISA_SGX_UNSET;
+ opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_SGX_UNSET;
+ }
+ return true;
+
case OPT_mavx512dq:
if (value)
{
Index: config/i386/cpuid.h
===
--- config/i386/cpuid.h (revision 244335)
+++ config/i386/cpuid.h (working copy)
@@ -74,6 +74,7 @@
/* Extended Features (%eax == 7) */
/* %ebx */
#define bit_FSGSBASE (1 << 0)
+#define bit_SGX (1 << 2)
#define bit_BMI(1 << 3)
#define bit_HLE(1 << 4)
#define bit_AVX2 (1 << 5)
Index: config/i386/driver-i386.c
===
--- config/i386/driver-i386.c (revision 244335)
+++ config/i386/driver-i386.c (working copy)
@@ -404,7 +404,7 @@ const char *host_detect_local_cpu (int argc, const
unsigned int has_pclmul = 0, has_abm = 0, has_lwp = 0;
unsigned int has_fma = 0, has_fma4 = 0, has_xop = 0;
unsigned int has_bmi = 0, has_bmi2 = 0, has_tbm = 0, has_lzcnt = 0;
- unsigned int has_hle = 0, has_rtm = 0;
+ unsigned int has_hle = 0, has_rtm = 0, has_sgx = 0;
unsigned int has_rdrnd = 0, has_f16c = 0, has_fsgsbase = 0;
unsigned int has_rdseed = 0, has_prfchw = 0, has_adx = 0;
unsigned int has_osxsave = 0, has_fxsr = 0, has_xsave = 0, has_xsaveopt = 0;
@@ -480,6 +480,7 @@ const char *host_detect_local_cpu (int argc, const
__cpuid_count (7, 0, eax, ebx, ecx, edx);
has_bmi = ebx & bit_BMI;
+ has_sgx = ebx & bit_SGX;
has_hle = ebx & bit_HLE;
has_rtm = ebx & bit_RTM;
has_avx2 = ebx & bit_AVX2;
@@ -993,6 +994,7 @@ const char *host_detect_local_cpu (int argc, const
const char *fma4 = has_fma4 ? " -mfma4" : " -mno-fma4";
const char *xop = has_xop ? " -mxop" : " -mno-xop";
const char *bmi = has_bmi ? " -mbmi" : " -mno-bmi";
+ const char *sgx = has_sgx ? " -msgx" : " -mno-sgx";
RE: [PATCH] Enable SGX intrinsics
Hi, I rebased the patch onto latest trunk version and changed specification according to ICC: _enclu_u32 (const int __L, size_t *__D) --> _enclu_u32 (const int __L, size_t __D[]) The Changelogs remained the same: 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. gcc/testsuite/ * 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. Thanks, Julia -Original Message- From: Koval, Julia Sent: Wednesday, January 11, 2017 1:08 PM To: 'Uros Bizjak' Cc: Andrew Senkevich ; GCC Patches ; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub Jelinek Subject: RE: [PATCH] Enable SGX intrinsics Here is it. 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. Thanks, Julia -Original Message- From: Uros Bizjak [mailto:ubiz...@gmail.com] Sent: Wednesday, January 11, 2017 1:02 PM To: Koval, Julia Cc: Andrew Senkevich ; GCC Patches ; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub Jelinek Subject: Re: [PATCH] Enable SGX intrinsics On Wed, Jan 11, 2017 at 12:40 PM, Koval, Julia wrote: > Ok, fixed it. Can you please commit it for me, cause I don't have rights to > commit? OK, but please send me updated ChangeLogs. Uros. > Thanks, > Julia > > -Original Message- > From: Uros Bizjak [mailto:ubiz...@gmail.com] > Sent: Wednesday, January 11, 2017 12:11 PM > To: Koval, Julia > Cc: Andrew Senkevich ; GCC Patches > ; vaalfr...@gmail.com; kirill.yuk...@gmail.com; > Jakub Jelinek > Subject: Re: [PATCH] Enable SGX intrinsics > > On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia wrote: >> Ok. I fixed the enum formatting and the enums remain internal. > > @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee) >bool ret = false; >tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller); >tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee); > - >/* If callee has no option attributes, then it is ok to inline. */ >if (!callee_tree) > ret = true; > > > No need for the above whitespace change. > > OK for mainline with the above part reverted. > > Thanks, > Uros. 0001-SGX-intrinsiks.patch Description: 0001-SGX-intrinsiks.patch
RE: [PATCH] Enable SGX intrinsics
Here is it. 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. Thanks, Julia -Original Message- From: Uros Bizjak [mailto:ubiz...@gmail.com] Sent: Wednesday, January 11, 2017 1:02 PM To: Koval, Julia Cc: Andrew Senkevich ; GCC Patches ; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub Jelinek Subject: Re: [PATCH] Enable SGX intrinsics On Wed, Jan 11, 2017 at 12:40 PM, Koval, Julia wrote: > Ok, fixed it. Can you please commit it for me, cause I don't have rights to > commit? OK, but please send me updated ChangeLogs. Uros. > Thanks, > Julia > > -Original Message- > From: Uros Bizjak [mailto:ubiz...@gmail.com] > Sent: Wednesday, January 11, 2017 12:11 PM > To: Koval, Julia > Cc: Andrew Senkevich ; GCC Patches > ; vaalfr...@gmail.com; kirill.yuk...@gmail.com; > Jakub Jelinek > Subject: Re: [PATCH] Enable SGX intrinsics > > On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia wrote: >> Ok. I fixed the enum formatting and the enums remain internal. > > @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee) >bool ret = false; >tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller); >tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee); > - >/* If callee has no option attributes, then it is ok to inline. */ >if (!callee_tree) > ret = true; > > > No need for the above whitespace change. > > OK for mainline with the above part reverted. > > Thanks, > Uros.
Re: [PATCH] Enable SGX intrinsics
On Wed, Jan 11, 2017 at 12:40 PM, Koval, Julia wrote: > Ok, fixed it. Can you please commit it for me, cause I don't have rights to > commit? OK, but please send me updated ChangeLogs. Uros. > Thanks, > Julia > > -Original Message- > From: Uros Bizjak [mailto:ubiz...@gmail.com] > Sent: Wednesday, January 11, 2017 12:11 PM > To: Koval, Julia > Cc: Andrew Senkevich ; GCC Patches > ; vaalfr...@gmail.com; kirill.yuk...@gmail.com; > Jakub Jelinek > Subject: Re: [PATCH] Enable SGX intrinsics > > On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia wrote: >> Ok. I fixed the enum formatting and the enums remain internal. > > @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee) >bool ret = false; >tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller); >tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee); > - >/* If callee has no option attributes, then it is ok to inline. */ >if (!callee_tree) > ret = true; > > > No need for the above whitespace change. > > OK for mainline with the above part reverted. > > Thanks, > Uros.
RE: [PATCH] Enable SGX intrinsics
Ok, fixed it. Can you please commit it for me, cause I don't have rights to commit? Thanks, Julia -Original Message- From: Uros Bizjak [mailto:ubiz...@gmail.com] Sent: Wednesday, January 11, 2017 12:11 PM To: Koval, Julia Cc: Andrew Senkevich ; GCC Patches ; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub Jelinek Subject: Re: [PATCH] Enable SGX intrinsics On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia wrote: > Ok. I fixed the enum formatting and the enums remain internal. @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee) bool ret = false; tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller); tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee); - /* If callee has no option attributes, then it is ok to inline. */ if (!callee_tree) ret = true; No need for the above whitespace change. OK for mainline with the above part reverted. Thanks, Uros. 0001-Enable-SGX.PATCH Description: 0001-Enable-SGX.PATCH
Re: [PATCH] Enable SGX intrinsics
On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia wrote: > Ok. I fixed the enum formatting and the enums remain internal. @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee) bool ret = false; tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller); tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee); - /* If callee has no option attributes, then it is ok to inline. */ if (!callee_tree) ret = true; No need for the above whitespace change. OK for mainline with the above part reverted. Thanks, Uros.
Re: [PATCH] Enable SGX intrinsics
On Wed, Jan 11, 2017 at 10:31:33AM +, Koval, Julia wrote: > Ok. I fixed the enum formatting and the enums remain internal. No further objections from me, if Uros acks it, check it in. > > Sure. Plus it depends on if users of the APIs should just write the > > operands on their own as numbers, or as __SGX_E*, or as E*. > > In the first case the patch sans formatting is reasonable, in the second > > case the enums should be moved to file scope, in the last case we have to > > live with the namespace pollution. > > The pdf you've referenced in the thread doesn't list the _encls_u32 and > > _enclu_u32 intrinsics, so I think it depends on what ICC does (if it has > > been shipped with such a support already, or on coordination with ICC if > > not). > > Jakub, it is in accordance with ICC. > So the first case will be used. Jakub
RE: [PATCH] Enable SGX intrinsics
Ok. I fixed the enum formatting and the enums remain internal.
-Julia
-Original Message-
From: Andrew Senkevich [mailto:andrew.n.senkev...@gmail.com]
Sent: Tuesday, January 10, 2017 5:48 PM
To: Uros Bizjak
Cc: Koval, Julia ; GCC Patches
; vaalfr...@gmail.com
Subject: Re: [PATCH] Enable SGX intrinsics
On Fri, Dec 30, 2016 at 03:37:14PM +0100, Uros Bizjak wrote:
>> As suggested in [1], you should write multi-line enums like:
>>
>> enum foo
>> {
>> a = ...
>> b = ...
>> }
>
> Sure. Plus it depends on if users of the APIs should just write the operands
> on their own as numbers, or as __SGX_E*, or as E*.
> In the first case the patch sans formatting is reasonable, in the second case
> the enums should be moved to file scope, in the last case we have to live
> with the namespace pollution.
> The pdf you've referenced in the thread doesn't list the _encls_u32 and
> _enclu_u32 intrinsics, so I think it depends on what ICC does (if it has been
> shipped with such a support already, or on coordination with ICC if not).
Jakub, it is in accordance with ICC.
So the first case will be used.
--
WBR,
Andrew
0001-Enable-SGX.PATCH
Description: 0001-Enable-SGX.PATCH
Re: [PATCH] Enable SGX intrinsics
On Fri, Dec 30, 2016 at 03:37:14PM +0100, Uros Bizjak wrote:
>> As suggested in [1], you should write multi-line enums like:
>>
>> enum foo
>> {
>> a = ...
>> b = ...
>> }
>
> Sure. Plus it depends on if users of the APIs should just write the operands
> on their own as numbers, or as __SGX_E*, or as E*.
> In the first case the patch sans formatting is reasonable, in the second case
> the enums should be moved to file scope, in the last case we have to live
> with the namespace pollution.
> The pdf you've referenced in the thread doesn't list the _encls_u32 and
> _enclu_u32 intrinsics, so I think it depends on what ICC does (if it has been
> shipped with such a support already, or on coordination with ICC if not).
Jakub, it is in accordance with ICC.
So the first case will be used.
--
WBR,
Andrew
Re: [PATCH] Enable SGX intrinsics
On Fri, Dec 30, 2016 at 03:37:14PM +0100, Uros Bizjak wrote:
> As suggested in [1], you should write multi-line enums like:
>
> enum foo
> {
> a = ...
> b = ...
> }
Sure. Plus it depends on if users of the APIs should just write the
operands on their own as numbers, or as __SGX_E*, or as E*.
In the first case the patch sans formatting is reasonable, in the second
case the enums should be moved to file scope, in the last case we have to
live with the namespace pollution.
The pdf you've referenced in the thread doesn't list the _encls_u32 and
_enclu_u32 intrinsics, so I think it depends on what ICC does (if it has
been shipped with such a support already, or on coordination with ICC if not).
Jakub
Re: [PATCH] Enable SGX intrinsics
On Fri, Dec 30, 2016 at 3:17 PM, Koval, Julia wrote:
> 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.
As suggested in [1], you should write multi-line enums like:
enum foo
{
a = ...
b = ...
}
OK with the above change(s), but please wait for Jakub, if he has some
more comments.
[1] https://www.gnu.org/prep/standards/standards.html
Thanks,
Uros.
RE: [PATCH] Enable SGX intrinsics
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
Cc: Koval, Julia ; gcc-patches@gcc.gnu.org;
vaalfr...@gmail.com; Senkevich, Andrew
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 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
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
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 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
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
Re: [PATCH] Enable SGX intrinsics
On Thu, Dec 29, 2016 at 10:50 AM, Koval, Julia 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.
Uros.
Re: [PATCH] Enable SGX intrinsics
Sorry, didn't include changelog. Here is it: 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. On Thu, Dec 29, 2016 at 10:50 AM, Koval, Julia 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? > > Thanks, > Julia
