On 02/12/2018 07:16 AM, Tsimbalist, Igor V wrote:
>> -----Original Message-----
>> From: Sandra Loosemore [mailto:san...@codesourcery.com]
>> Sent: Friday, February 9, 2018 7:42 PM
>> To: Tsimbalist, Igor V <igor.v.tsimbal...@intel.com>; gcc-
>> patc...@gcc.gnu.org
>> Cc: Uros Bizjak <ubiz...@gmail.com>
>> Subject: Re: PR84239, Reimplement CET intrinsics for rdssp/incssp insn
>>
>> On 02/09/2018 05:50 AM, Tsimbalist, Igor V wrote:
>>> Introduce a couple of new CET intrinsics for reading and updating a
>> shadow stack
>>> pointer (_get_ssp and _inc_ssp), which are more user friendly. They replace
>> the existing
>>> _rdssp[d|q] and _incssp[d|q] instrinsics. The _get_ssp intrinsic has more
>> deterministic
>>> semantic: it returns a value of the shadow stack pointer if HW is CET
>> capable and
>>> 0 otherwise.
>>>
>>> Ok for trunk?
>> Just reviewing the documentation part:
>>
>>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>>> index cb9df97..9f25dd9 100644
>>> --- a/gcc/doc/extend.texi
>>> +++ b/gcc/doc/extend.texi
>>> @@ -12461,6 +12461,7 @@ instructions, but allow the compiler to
>> schedule those calls.
>>> * TILEPro Built-in Functions::
>>> * x86 Built-in Functions::
>>> * x86 transactional memory intrinsics::
>>> +* x86 control-flow protection intrinsics::
>>> @end menu
>>>
>>> @node AArch64 Built-in Functions
>>> @@ -21772,13 +21773,17 @@ void __builtin_ia32_wrpkru (unsigned int)
>>> unsigned int __builtin_ia32_rdpkru ()
>>> @end smallexample
>>>
>>> -The following built-in functions are available when @option{-mcet} is
>> used.
>>> -They are used to support Intel Control-flow Enforcment Technology (CET).
>>> -Each built-in function generates the machine instruction that is part of
>> the
>>> -function's name.
>>> +The following built-in functions are available when @option{-mcet} or
>>> +@option{-mshstk} option is used. They support shadow stack
>>> +machine instructions from Intel Control-flow Enforcment Technology
>> (CET).
>>
>> s/Enforcment/Enforcement/
>>
>>> +Each built-in function generates the machine instruction that is part
>>> +of the function's name. These are the internal low level functions.
>> s/low level/low-level/
>>
>>> +Normally the functions in @ref{x86 control-flow protection intrinsics}
>>> +should be used instead.
>>> +
>>> @smallexample
>>> -unsigned int __builtin_ia32_rdsspd (unsigned int)
>>> -unsigned long long __builtin_ia32_rdsspq (unsigned long long)
>>> +unsigned int __builtin_ia32_rdsspd (void)
>>> +unsigned long long __builtin_ia32_rdsspq (void)
>>> void __builtin_ia32_incsspd (unsigned int)
>>> void __builtin_ia32_incsspq (unsigned long long)
>>> void __builtin_ia32_saveprevssp(void);
>>> @@ -21885,6 +21890,51 @@ else
>>> Note that, in most cases, the transactional and non-transactional code
>>> must synchronize together to ensure consistency.
>>>
>>> +@node x86 control-flow protection intrinsics
>>> +@subsection x86 Control-Flow Protection Intrinsics
>>> +
>>> +@deftypefn {CET Function} {ret_type} _get_ssp (void)
>>> +The @code{ret_type} is @code{unsigned long long} for x86-64 platform
>>> +and @code{unsigned int} for x86 pltform.
>> I'd prefer the sentence about the return type be placed after the
>> description of what the function does. And please fix typos:
>> s/x86-64 platform/64-bit targets/
>> s/x86 pltform/32-bit targets/
>>
>>> +Get the current value of shadow stack pointer if shadow stack support
>>> +from Intel CET is enabled in the HW or @code{0} otherwise.
>> s/HW/hardware,/
>>
>>> +@end deftypefn
>>> +
>>> +@deftypefn {CET Function} void _inc_ssp (unsigned int)
>>> +Increment the current shadow stack pointer by the size specified by the
>>> +function argument. For security reason only unsigned byte value is used
>>> +from the argument. Therefore for the size greater than @code{255} the
>>> +function should be called several times.
>> How about rephrasing the last two sentences:
>>
>> The argument is masked to a byte value for security reasons, so to
>> increment by more than 255 bytes you must call the function multiple times.
>>
>>> +@end deftypefn
>>> +
>>> +The shadow stack unwind code looks like:
>>> +
>>> +@smallexample
>>> +#include <immintrin.h>
>>> +
>>> +/* Unwind the shadow stack for EH. */
>>> +#define _Unwind_Frames_Extra(x) \
>>> + do \
>>> + @{ \
>>> + _Unwind_Word ssp = _get_ssp (); \
>>> + if (ssp != 0) \
>>> + @{ \
>>> + _Unwind_Word tmp = (x); \
>>> + while (tmp > 255) \
>>> + @{ \
>>> + _inc_ssp (tmp); \
>>> + tmp -= 255; \
>>> + @} \
>>> + _inc_ssp (tmp); \
>>> + @} \
>>> + @} \
>>> + while (0)
>>> +@end smallexample
>> Tabs in Texinfo input don't work well. Please use spaces to format code
>> environments.
>>
>>> +
>>> +@noindent
>>> +This code runs unconditionally on all x86-64 processors and all x86
>>> +processors that support multi-byte NOP instructions.
>> s/x86-64 and all x86/32-bit and 64-bit/
>>
>>> +
>>> @node Target Format Checks
>>> @section Format Checks Specific to Particular Target Machines
>>>
> All comments are fixed. The updated patch is attached.
>
> Igor
>
>> -Sandra
>
> 0001-Reimplement-CET-intrinsics-for-rdssp-incssp-insn.patch
>
>
> From f9453d2f1eec40c04812ba4059c329fbe6fa9309 Mon Sep 17 00:00:00 2001
> From: Igor Tsimbalist <igor.v.tsimbal...@intel.com>
> Date: Wed, 7 Feb 2018 19:31:32 +0300
> Subject: [PATCH] Reimplement CET intrinsics for rdssp/incssp insn
>
> PR target/84239
> ---
> gcc/ChangeLog | 16 +++++++
> gcc/config/i386/cetintrin.h | 31 ++++++--------
> gcc/config/i386/i386-builtin-types.def | 1 +
> gcc/config/i386/i386-builtin.def | 4 +-
> gcc/config/i386/i386.c | 3 +-
> gcc/config/i386/i386.md | 16 ++++---
> gcc/doc/extend.texi | 62
> +++++++++++++++++++++++++---
> gcc/testsuite/ChangeLog | 9 ++++
> gcc/testsuite/gcc.target/i386/cet-intrin-3.c | 10 ++---
> gcc/testsuite/gcc.target/i386/cet-intrin-4.c | 25 +----------
> gcc/testsuite/gcc.target/i386/cet-rdssp-1.c | 8 ++--
> libgcc/ChangeLog | 6 +++
> libgcc/config/i386/shadow-stack-unwind.h | 17 +++-----
> 13 files changed, 126 insertions(+), 82 deletions(-)
[ ... ]
OK. Thanks,
Jeff
