* [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
@ 2017-06-20 15:01 Thomas Preudhomme
2017-06-28 15:12 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
2017-06-28 15:57 ` [PATCH, GCC/ARM] " Richard Earnshaw (lists)
0 siblings, 2 replies; 14+ messages in thread
From: Thomas Preudhomme @ 2017-06-20 15:01 UTC (permalink / raw)
To: Kyrill Tkachov, Ramana Radhakrishnan, Richard Earnshaw, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
Hi,
Function cmse_nonsecure_entry_clear_before_return has code to deal with
high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
not support more than 16 double VFP registers (D0-D15). This makes this
security-sensitive code harder to read for not much benefit since
libcall for cmse_nonsecure_call functions do not deal with those high
VFP registers anyway.
This commit gets rid of this code for simplicity and fixes 2 issues in
the same function:
- stop the first loop when reaching maxregno to avoid dealing with VFP
registers if targetting Thumb-1 or using -mfloat-abi=soft
- include maxregno in that loop
ChangeLog entry is as follows:
*** gcc/ChangeLog ***
2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
* config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
Extensions with more than 16 double VFP registers.
(cmse_nonsecure_entry_clear_before_return): Remove second entry of
to_clear_mask and all code related to it and make the remaining
entry a 64-bit scalar integer variable and adapt code accordingly.
Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
ARMv8-M Mainline.
Is this ok for trunk?
Best regards,
Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 4595 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..60a4d1f46765d285de469f51fbb5a0ad76d56d9b 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3620,6 +3620,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -24996,15 +25001,15 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ uint64_t to_clear_mask;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
int regno, maxregno = IP_REGNUM;
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ to_clear_mask = (1ULL << (NUM_ARG_REGS)) - 1;
+ to_clear_mask |= (1ULL << IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
@@ -25015,23 +25020,22 @@ cmse_nonsecure_entry_clear_before_return (void)
maxregno = LAST_VFP_REGNUM;
float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
-
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ to_clear_mask |= float_mask;
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ to_clear_mask &= ~(1ULL << IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ to_clear_mask &= ~(1ULL << 4);
}
+ gcc_assert ((unsigned) maxregno <= sizeof (to_clear_mask) * __CHAR_BIT__);
+
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25041,7 +25045,7 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ to_clear_mask |= (1ULL << regno);
}
/* Make sure we do not clear the registers used to return the result in. */
@@ -25052,7 +25056,7 @@ cmse_nonsecure_entry_clear_before_return (void)
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
+ to_clear_mask
&= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
padding_bits_to_clear_ptr);
}
@@ -25063,7 +25067,7 @@ cmse_nonsecure_entry_clear_before_return (void)
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ gcc_assert ((to_clear_mask & 0xe) == 0xe);
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25085,7 +25089,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!(to_clear_mask & (1ULL << regno)))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25094,7 +25098,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && to_clear_mask & (1ULL << (regno + 1)))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31
2017-06-20 15:01 [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31 Thomas Preudhomme
@ 2017-06-28 15:12 ` Thomas Preudhomme
2017-06-28 15:57 ` [PATCH, GCC/ARM] " Richard Earnshaw (lists)
1 sibling, 0 replies; 14+ messages in thread
From: Thomas Preudhomme @ 2017-06-28 15:12 UTC (permalink / raw)
To: Kyrill Tkachov, Ramana Radhakrishnan, Richard Earnshaw, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]
Ping?
*** gcc/ChangeLog ***
2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
* config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
Extensions with more than 16 double VFP registers.
(cmse_nonsecure_entry_clear_before_return): Remove second entry of
to_clear_mask and all code related to it and make the remaining
entry a 64-bit scalar integer variable and adapt code accordingly.
Best regards,
Thomas
On 20/06/17 16:01, Thomas Preudhomme wrote:
> Hi,
>
> Function cmse_nonsecure_entry_clear_before_return has code to deal with
> high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
> not support more than 16 double VFP registers (D0-D15). This makes this
> security-sensitive code harder to read for not much benefit since
> libcall for cmse_nonsecure_call functions do not deal with those high
> VFP registers anyway.
>
> This commit gets rid of this code for simplicity and fixes 2 issues in
> the same function:
>
> - stop the first loop when reaching maxregno to avoid dealing with VFP
> registers if targetting Thumb-1 or using -mfloat-abi=soft
> - include maxregno in that loop
>
> ChangeLog entry is as follows:
>
> *** gcc/ChangeLog ***
>
> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
> Extensions with more than 16 double VFP registers.
> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
> to_clear_mask and all code related to it and make the remaining
> entry a 64-bit scalar integer variable and adapt code accordingly.
>
> Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
> ARMv8-M Mainline.
>
> Is this ok for trunk?
>
> Best regards,
>
> Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 4595 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..60a4d1f46765d285de469f51fbb5a0ad76d56d9b 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3620,6 +3620,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -24996,15 +25001,15 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ uint64_t to_clear_mask;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
int regno, maxregno = IP_REGNUM;
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ to_clear_mask = (1ULL << (NUM_ARG_REGS)) - 1;
+ to_clear_mask |= (1ULL << IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
@@ -25015,23 +25020,22 @@ cmse_nonsecure_entry_clear_before_return (void)
maxregno = LAST_VFP_REGNUM;
float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
-
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ to_clear_mask |= float_mask;
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ to_clear_mask &= ~(1ULL << IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ to_clear_mask &= ~(1ULL << 4);
}
+ gcc_assert ((unsigned) maxregno <= sizeof (to_clear_mask) * __CHAR_BIT__);
+
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25041,7 +25045,7 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ to_clear_mask |= (1ULL << regno);
}
/* Make sure we do not clear the registers used to return the result in. */
@@ -25052,7 +25056,7 @@ cmse_nonsecure_entry_clear_before_return (void)
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
+ to_clear_mask
&= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
padding_bits_to_clear_ptr);
}
@@ -25063,7 +25067,7 @@ cmse_nonsecure_entry_clear_before_return (void)
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ gcc_assert ((to_clear_mask & 0xe) == 0xe);
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25085,7 +25089,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!(to_clear_mask & (1ULL << regno)))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25094,7 +25098,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && to_clear_mask & (1ULL << (regno + 1)))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
2017-06-20 15:01 [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31 Thomas Preudhomme
2017-06-28 15:12 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
@ 2017-06-28 15:57 ` Richard Earnshaw (lists)
2017-06-29 8:44 ` Thomas Preudhomme
2017-07-06 12:36 ` Thomas Preudhomme
1 sibling, 2 replies; 14+ messages in thread
From: Richard Earnshaw (lists) @ 2017-06-28 15:57 UTC (permalink / raw)
To: Thomas Preudhomme, Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
On 20/06/17 16:01, Thomas Preudhomme wrote:
> Hi,
>
> Function cmse_nonsecure_entry_clear_before_return has code to deal with
> high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
> not support more than 16 double VFP registers (D0-D15). This makes this
> security-sensitive code harder to read for not much benefit since
> libcall for cmse_nonsecure_call functions do not deal with those high
> VFP registers anyway.
>
> This commit gets rid of this code for simplicity and fixes 2 issues in
> the same function:
>
> - stop the first loop when reaching maxregno to avoid dealing with VFP
> registers if targetting Thumb-1 or using -mfloat-abi=soft
> - include maxregno in that loop
>
This is silently baking in dangerous assumptions about GCC's internal
numbering of the registers. That's not a good idea from a long-term
portability perspective.
At the very least you need to assert that all the interesting registers
are numbered in the range 0..63; but ideally the code should just handle
pretty much any assignment of internal register numbers.
Did you consider using sbitmaps rather than doing all the multi-word
stuff by steam?
R.
> ChangeLog entry is as follows:
>
> *** gcc/ChangeLog ***
>
> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
> Extensions with more than 16 double VFP registers.
> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
> to_clear_mask and all code related to it and make the remaining
> entry a 64-bit scalar integer variable and adapt code accordingly.
>
> Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
> ARMv8-M Mainline.
>
> Is this ok for trunk?
>
> Best regards,
>
> Thomas
>
> remove_d16-d31_armv8m_clearing_code.patch
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..60a4d1f46765d285de469f51fbb5a0ad76d56d9b 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -3620,6 +3620,11 @@ arm_option_override (void)
> if (use_cmse && !arm_arch_cmse)
> error ("target CPU does not support ARMv8-M Security Extensions");
>
> + /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
> + and ARMv8-M Baseline and Mainline do not allow such configuration. */
> + if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
> + error ("ARMv8-M Security Extensions incompatible with selected FPU");
> +
> /* Disable scheduling fusion by default if it's not armv7 processor
> or doesn't prefer ldrd/strd. */
> if (flag_schedule_fusion == 2
> @@ -24996,15 +25001,15 @@ thumb1_expand_prologue (void)
> void
> cmse_nonsecure_entry_clear_before_return (void)
> {
> - uint64_t to_clear_mask[2];
> + uint64_t to_clear_mask;
> uint32_t padding_bits_to_clear = 0;
> uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
> int regno, maxregno = IP_REGNUM;
> tree result_type;
> rtx result_rtl;
>
> - to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
> - to_clear_mask[0] |= (1ULL << IP_REGNUM);
> + to_clear_mask = (1ULL << (NUM_ARG_REGS)) - 1;
> + to_clear_mask |= (1ULL << IP_REGNUM);
>
> /* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
> registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
> @@ -25015,23 +25020,22 @@ cmse_nonsecure_entry_clear_before_return (void)
> maxregno = LAST_VFP_REGNUM;
>
> float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
> - to_clear_mask[0] |= float_mask;
> -
> - float_mask = (1ULL << (maxregno - 63)) - 1;
> - to_clear_mask[1] = float_mask;
> + to_clear_mask |= float_mask;
>
> /* Make sure we don't clear the two scratch registers used to clear the
> relevant FPSCR bits in output_return_instruction. */
> emit_use (gen_rtx_REG (SImode, IP_REGNUM));
> - to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
> + to_clear_mask &= ~(1ULL << IP_REGNUM);
> emit_use (gen_rtx_REG (SImode, 4));
> - to_clear_mask[0] &= ~(1ULL << 4);
> + to_clear_mask &= ~(1ULL << 4);
> }
>
> + gcc_assert ((unsigned) maxregno <= sizeof (to_clear_mask) * __CHAR_BIT__);
> +
> /* If the user has defined registers to be caller saved, these are no longer
> restored by the function before returning and must thus be cleared for
> security purposes. */
> - for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
> + for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
> {
> /* We do not touch registers that can be used to pass arguments as per
> the AAPCS, since these should never be made callee-saved by user
> @@ -25041,7 +25045,7 @@ cmse_nonsecure_entry_clear_before_return (void)
> if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
> continue;
> if (call_used_regs[regno])
> - to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
> + to_clear_mask |= (1ULL << regno);
> }
>
> /* Make sure we do not clear the registers used to return the result in. */
> @@ -25052,7 +25056,7 @@ cmse_nonsecure_entry_clear_before_return (void)
>
> /* No need to check that we return in registers, because we don't
> support returning on stack yet. */
> - to_clear_mask[0]
> + to_clear_mask
> &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
> padding_bits_to_clear_ptr);
> }
> @@ -25063,7 +25067,7 @@ cmse_nonsecure_entry_clear_before_return (void)
> /* Padding bits to clear is not 0 so we know we are dealing with
> returning a composite type, which only uses r0. Let's make sure that
> r1-r3 is cleared too, we will use r1 as a scratch register. */
> - gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
> + gcc_assert ((to_clear_mask & 0xe) == 0xe);
>
> reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
>
> @@ -25085,7 +25089,7 @@ cmse_nonsecure_entry_clear_before_return (void)
>
> for (regno = R0_REGNUM; regno <= maxregno; regno++)
> {
> - if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
> + if (!(to_clear_mask & (1ULL << regno)))
> continue;
>
> if (IS_VFP_REGNUM (regno))
> @@ -25094,7 +25098,7 @@ cmse_nonsecure_entry_clear_before_return (void)
> be cleared, use vmov. */
> if (TARGET_VFP_DOUBLE
> && VFP_REGNO_OK_FOR_DOUBLE (regno)
> - && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
> + && to_clear_mask & (1ULL << (regno + 1)))
> {
> emit_move_insn (gen_rtx_REG (DFmode, regno),
> CONST1_RTX (DFmode));
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
2017-06-28 15:57 ` [PATCH, GCC/ARM] " Richard Earnshaw (lists)
@ 2017-06-29 8:44 ` Thomas Preudhomme
2017-07-06 12:36 ` Thomas Preudhomme
1 sibling, 0 replies; 14+ messages in thread
From: Thomas Preudhomme @ 2017-06-29 8:44 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
Hi Richard,
On 28/06/17 16:56, Richard Earnshaw (lists) wrote:
> On 20/06/17 16:01, Thomas Preudhomme wrote:
>> Hi,
>>
>> Function cmse_nonsecure_entry_clear_before_return has code to deal with
>> high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
>> not support more than 16 double VFP registers (D0-D15). This makes this
>> security-sensitive code harder to read for not much benefit since
>> libcall for cmse_nonsecure_call functions do not deal with those high
>> VFP registers anyway.
>>
>> This commit gets rid of this code for simplicity and fixes 2 issues in
>> the same function:
>>
>> - stop the first loop when reaching maxregno to avoid dealing with VFP
>> registers if targetting Thumb-1 or using -mfloat-abi=soft
>> - include maxregno in that loop
>>
>
> This is silently baking in dangerous assumptions about GCC's internal
> numbering of the registers. That's not a good idea from a long-term
> portability perspective.
>
> At the very least you need to assert that all the interesting registers
> are numbered in the range 0..63; but ideally the code should just handle
> pretty much any assignment of internal register numbers.
Well there is already this:
gcc_assert ((unsigned) maxregno <= sizeof (to_clear_mask) * __CHAR_BIT__);
>
> Did you consider using sbitmaps rather than doing all the multi-word
> stuff by steam?
No but am happy to. I'll respin the patch.
Best regards,
Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
2017-06-28 15:57 ` [PATCH, GCC/ARM] " Richard Earnshaw (lists)
2017-06-29 8:44 ` Thomas Preudhomme
@ 2017-07-06 12:36 ` Thomas Preudhomme
2017-07-07 14:19 ` Richard Earnshaw (lists)
1 sibling, 1 reply; 14+ messages in thread
From: Thomas Preudhomme @ 2017-07-06 12:36 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 863 bytes --]
Hi Richard,
On 28/06/17 16:56, Richard Earnshaw (lists) wrote:
>>
>
> This is silently baking in dangerous assumptions about GCC's internal
> numbering of the registers. That's not a good idea from a long-term
> portability perspective.
>
> At the very least you need to assert that all the interesting registers
> are numbered in the range 0..63; but ideally the code should just handle
> pretty much any assignment of internal register numbers.
There is already such an assert in my patch. :-)
>
> Did you consider using sbitmaps rather than doing all the multi-word
> stuff by steam?
I did now, most of it is trivial but interaction with compute_not_to_clear_mask
is now more verbose because it returns a bitfield and one assert got quite ugly
and expensive.
Please find an updated patch in attachment and judge by yourself.
Best regards,
Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 5968 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..93e152b1f38d3675e4ada1de7a34c2c209d8db1f 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3620,6 +3620,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -24996,42 +25001,41 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ sbitmap to_clear_bitmap;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
int regno, maxregno = IP_REGNUM;
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ to_clear_bitmap = sbitmap_alloc (maxregno + 1);
+ bitmap_clear (to_clear_bitmap);
+ bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+ bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
to make sure the instructions used to clear them are present. */
if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
{
- uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
+ int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
maxregno = LAST_VFP_REGNUM;
+ to_clear_bitmap = sbitmap_resize (to_clear_bitmap, maxregno, 0);
- float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
-
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ bitmap_clear_bit (to_clear_bitmap, 4);
}
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25041,29 +25045,50 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ bitmap_set_bit (to_clear_bitmap, regno);
}
/* Make sure we do not clear the registers used to return the result in. */
result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
if (!VOID_TYPE_P (result_type))
{
+ unsigned count;
+ uint64_t to_clear_return_mask;
result_rtl = arm_function_value (result_type, current_function_decl, 0);
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
- &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
- padding_bits_to_clear_ptr);
+ gcc_assert (REG_P (result_rtl));
+ to_clear_return_mask
+ = compute_not_to_clear_mask (result_type, result_rtl, 0,
+ padding_bits_to_clear_ptr);
+ if (to_clear_return_mask)
+ {
+ gcc_assert (maxregno < sizeof (long long) * __CHAR_BIT__);
+ for (regno = R0_REGNUM; regno <= maxregno; regno++)
+ {
+ if (to_clear_return_mask & (1ULL << regno))
+ bitmap_clear_bit (to_clear_bitmap, regno);
+ }
+ }
}
if (padding_bits_to_clear != 0)
{
rtx reg_rtx;
+ sbitmap to_clear_arg_regs_bitmap, and_bitmap;
+
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ to_clear_arg_regs_bitmap = sbitmap_alloc (maxregno + 1);
+ bitmap_clear (to_clear_arg_regs_bitmap);
+ and_bitmap = sbitmap_alloc (maxregno + 1);
+ bitmap_clear (and_bitmap);
+ bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+ NUM_ARG_REGS - 1);
+ bitmap_and (and_bitmap, to_clear_arg_regs_bitmap, to_clear_bitmap);
+ gcc_assert (bitmap_equal_p (and_bitmap, to_clear_arg_regs_bitmap));
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25085,7 +25110,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!bitmap_bit_p (to_clear_bitmap, regno))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25094,7 +25119,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && bitmap_bit_p (to_clear_bitmap, regno + 1))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
2017-07-06 12:36 ` Thomas Preudhomme
@ 2017-07-07 14:19 ` Richard Earnshaw (lists)
2017-07-12 8:59 ` Thomas Preudhomme
0 siblings, 1 reply; 14+ messages in thread
From: Richard Earnshaw (lists) @ 2017-07-07 14:19 UTC (permalink / raw)
To: Thomas Preudhomme, Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
On 06/07/17 13:36, Thomas Preudhomme wrote:
> Hi Richard,
>
> On 28/06/17 16:56, Richard Earnshaw (lists) wrote:
>
>>>
>>
>> This is silently baking in dangerous assumptions about GCC's internal
>> numbering of the registers. That's not a good idea from a long-term
>> portability perspective.
>>
>> At the very least you need to assert that all the interesting registers
>> are numbered in the range 0..63; but ideally the code should just handle
>> pretty much any assignment of internal register numbers.
>
> There is already such an assert in my patch. :-)
>
>>
>> Did you consider using sbitmaps rather than doing all the multi-word
>> stuff by steam?
>
> I did now, most of it is trivial but interaction with
> compute_not_to_clear_mask is now more verbose because it returns a
> bitfield and one assert got quite ugly and expensive.
>
> Please find an updated patch in attachment and judge by yourself.
>
Hmm, I think that's because really this is a partial conversion. It
looks like doing this properly would involve moving that existing code
to use sbitmaps as well. I think doing that would be better for
long-term maintenance perspectives, but I'm not going to insist that you
do it now.
As a result I'll let you take the call as to whether you keep this
version or go back to your earlier patch. If you do decide to keep this
version, then see the comment below.
> Best regards,
>
> Thomas
>
> remove_d16-d31_armv8m_clearing_code.patch
>
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..93e152b1f38d3675e4ada1de7a34c2c209d8db1f 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -3620,6 +3620,11 @@ arm_option_override (void)
> if (use_cmse && !arm_arch_cmse)
> error ("target CPU does not support ARMv8-M Security Extensions");
>
> + /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
> + and ARMv8-M Baseline and Mainline do not allow such configuration. */
> + if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
> + error ("ARMv8-M Security Extensions incompatible with selected FPU");
> +
> /* Disable scheduling fusion by default if it's not armv7 processor
> or doesn't prefer ldrd/strd. */
> if (flag_schedule_fusion == 2
> @@ -24996,42 +25001,41 @@ thumb1_expand_prologue (void)
> void
> cmse_nonsecure_entry_clear_before_return (void)
> {
> - uint64_t to_clear_mask[2];
> + sbitmap to_clear_bitmap;
I see the bitmap_alloc, but not the bitmap_free once its dead. I think
you should use an auto_sbitmap here so that it will clean up
automatically when it goes out of scope.
> uint32_t padding_bits_to_clear = 0;
> uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
> int regno, maxregno = IP_REGNUM;
> tree result_type;
> rtx result_rtl;
>
> - to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
> - to_clear_mask[0] |= (1ULL << IP_REGNUM);
> + to_clear_bitmap = sbitmap_alloc (maxregno + 1);
> + bitmap_clear (to_clear_bitmap);
> + bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
> + bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
>
> /* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
> registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
> to make sure the instructions used to clear them are present. */
> if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
> {
> - uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
> + int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
> maxregno = LAST_VFP_REGNUM;
> + to_clear_bitmap = sbitmap_resize (to_clear_bitmap, maxregno, 0);
>
> - float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
> - to_clear_mask[0] |= float_mask;
> -
> - float_mask = (1ULL << (maxregno - 63)) - 1;
> - to_clear_mask[1] = float_mask;
> + bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
>
> /* Make sure we don't clear the two scratch registers used to clear the
> relevant FPSCR bits in output_return_instruction. */
> emit_use (gen_rtx_REG (SImode, IP_REGNUM));
> - to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
> + bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
> emit_use (gen_rtx_REG (SImode, 4));
> - to_clear_mask[0] &= ~(1ULL << 4);
> + bitmap_clear_bit (to_clear_bitmap, 4);
> }
>
> /* If the user has defined registers to be caller saved, these are no longer
> restored by the function before returning and must thus be cleared for
> security purposes. */
> - for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
> + for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
> {
> /* We do not touch registers that can be used to pass arguments as per
> the AAPCS, since these should never be made callee-saved by user
> @@ -25041,29 +25045,50 @@ cmse_nonsecure_entry_clear_before_return (void)
> if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
> continue;
> if (call_used_regs[regno])
> - to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
> + bitmap_set_bit (to_clear_bitmap, regno);
> }
>
> /* Make sure we do not clear the registers used to return the result in. */
> result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
> if (!VOID_TYPE_P (result_type))
> {
> + unsigned count;
> + uint64_t to_clear_return_mask;
> result_rtl = arm_function_value (result_type, current_function_decl, 0);
>
> /* No need to check that we return in registers, because we don't
> support returning on stack yet. */
> - to_clear_mask[0]
> - &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
> - padding_bits_to_clear_ptr);
> + gcc_assert (REG_P (result_rtl));
> + to_clear_return_mask
> + = compute_not_to_clear_mask (result_type, result_rtl, 0,
> + padding_bits_to_clear_ptr);
> + if (to_clear_return_mask)
> + {
> + gcc_assert (maxregno < sizeof (long long) * __CHAR_BIT__);
> + for (regno = R0_REGNUM; regno <= maxregno; regno++)
> + {
> + if (to_clear_return_mask & (1ULL << regno))
> + bitmap_clear_bit (to_clear_bitmap, regno);
> + }
> + }
> }
>
> if (padding_bits_to_clear != 0)
> {
> rtx reg_rtx;
> + sbitmap to_clear_arg_regs_bitmap, and_bitmap;
> +
> /* Padding bits to clear is not 0 so we know we are dealing with
> returning a composite type, which only uses r0. Let's make sure that
> r1-r3 is cleared too, we will use r1 as a scratch register. */
> - gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
> + to_clear_arg_regs_bitmap = sbitmap_alloc (maxregno + 1);
> + bitmap_clear (to_clear_arg_regs_bitmap);
> + and_bitmap = sbitmap_alloc (maxregno + 1);
> + bitmap_clear (and_bitmap);
> + bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
> + NUM_ARG_REGS - 1);
> + bitmap_and (and_bitmap, to_clear_arg_regs_bitmap, to_clear_bitmap);
> + gcc_assert (bitmap_equal_p (and_bitmap, to_clear_arg_regs_bitmap));
>
> reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
>
> @@ -25085,7 +25110,7 @@ cmse_nonsecure_entry_clear_before_return (void)
>
> for (regno = R0_REGNUM; regno <= maxregno; regno++)
> {
> - if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
> + if (!bitmap_bit_p (to_clear_bitmap, regno))
> continue;
>
> if (IS_VFP_REGNUM (regno))
> @@ -25094,7 +25119,7 @@ cmse_nonsecure_entry_clear_before_return (void)
> be cleared, use vmov. */
> if (TARGET_VFP_DOUBLE
> && VFP_REGNO_OK_FOR_DOUBLE (regno)
> - && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
> + && bitmap_bit_p (to_clear_bitmap, regno + 1))
> {
> emit_move_insn (gen_rtx_REG (DFmode, regno),
> CONST1_RTX (DFmode));
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
2017-07-07 14:19 ` Richard Earnshaw (lists)
@ 2017-07-12 8:59 ` Thomas Preudhomme
2017-07-17 8:52 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Preudhomme @ 2017-07-12 8:59 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]
Hi Richard,
On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>
> Hmm, I think that's because really this is a partial conversion. It
> looks like doing this properly would involve moving that existing code
> to use sbitmaps as well. I think doing that would be better for
> long-term maintenance perspectives, but I'm not going to insist that you
> do it now.
There's also the assert later but I've found a way to improve it slightly. While
switching to auto_sbitmap I also changed the code slightly to allocate directly
bitmaps to the right size. Since the change is probably bigger than what you had
in mind I'd appreciate if you can give me an OK again. See updated patch in
attachment. ChangeLog entry is unchanged:
2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
* config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
Extensions with more than 16 double VFP registers.
(cmse_nonsecure_entry_clear_before_return): Remove second entry of
to_clear_mask and all code related to it. Replace the remaining
entry by a sbitmap and adapt code accordingly.
>
> As a result I'll let you take the call as to whether you keep this
> version or go back to your earlier patch. If you do decide to keep this
> version, then see the comment below.
Given the changes I'm more happy with how the patch looks now and making it go
in can be a nice incentive to change other ARMv8-M Security Extension related
code later on.
Best regards,
Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 5816 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..4d09e891c288c7bf9b519ad7cd62bd38df661a02 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3620,6 +3620,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -24996,42 +25001,39 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ bool clear_vfpregs = TARGET_HARD_FLOAT && !TARGET_THUMB1;
+ int regno, maxregno = clear_vfpregs ? LAST_VFP_REGNUM : IP_REGNUM;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
- int regno, maxregno = IP_REGNUM;
+ auto_sbitmap to_clear_bitmap (LAST_VFP_REGNUM);
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ bitmap_clear (to_clear_bitmap);
+ bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+ bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
to make sure the instructions used to clear them are present. */
- if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
+ if (clear_vfpregs)
{
- uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
- maxregno = LAST_VFP_REGNUM;
-
- float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
+ int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ bitmap_clear_bit (to_clear_bitmap, 4);
}
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25041,29 +25043,45 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ bitmap_set_bit (to_clear_bitmap, regno);
}
/* Make sure we do not clear the registers used to return the result in. */
result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
if (!VOID_TYPE_P (result_type))
{
+ uint64_t to_clear_return_mask;
result_rtl = arm_function_value (result_type, current_function_decl, 0);
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
- &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
- padding_bits_to_clear_ptr);
+ gcc_assert (REG_P (result_rtl));
+ to_clear_return_mask
+ = compute_not_to_clear_mask (result_type, result_rtl, 0,
+ padding_bits_to_clear_ptr);
+ if (to_clear_return_mask)
+ {
+ gcc_assert ((unsigned) maxregno < sizeof (long long) * __CHAR_BIT__);
+ for (regno = R0_REGNUM; regno <= maxregno; regno++)
+ {
+ if (to_clear_return_mask & (1ULL << regno))
+ bitmap_clear_bit (to_clear_bitmap, regno);
+ }
+ }
}
if (padding_bits_to_clear != 0)
{
rtx reg_rtx;
+ auto_sbitmap to_clear_arg_regs_bitmap (R0_REGNUM + NUM_ARG_REGS - 1);
+
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ bitmap_clear (to_clear_arg_regs_bitmap);
+ bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+ NUM_ARG_REGS - 1);
+ gcc_assert (bitmap_subset_p (to_clear_arg_regs_bitmap, to_clear_bitmap));
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25085,7 +25103,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!bitmap_bit_p (to_clear_bitmap, regno))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25094,7 +25112,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && bitmap_bit_p (to_clear_bitmap, regno + 1))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31
2017-07-12 8:59 ` Thomas Preudhomme
@ 2017-07-17 8:52 ` Thomas Preudhomme
2017-07-17 16:25 ` Thomas Preudhomme
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Preudhomme @ 2017-07-17 8:52 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]
Ping?
Best regards,
Thomas
On 12/07/17 09:59, Thomas Preudhomme wrote:
> Hi Richard,
>
> On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>>
>> Hmm, I think that's because really this is a partial conversion. It
>> looks like doing this properly would involve moving that existing code
>> to use sbitmaps as well. I think doing that would be better for
>> long-term maintenance perspectives, but I'm not going to insist that you
>> do it now.
>
> There's also the assert later but I've found a way to improve it slightly. While
> switching to auto_sbitmap I also changed the code slightly to allocate directly
> bitmaps to the right size. Since the change is probably bigger than what you had
> in mind I'd appreciate if you can give me an OK again. See updated patch in
> attachment. ChangeLog entry is unchanged:
>
> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
> Extensions with more than 16 double VFP registers.
> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
> to_clear_mask and all code related to it. Replace the remaining
> entry by a sbitmap and adapt code accordingly.
>
>>
>> As a result I'll let you take the call as to whether you keep this
>> version or go back to your earlier patch. If you do decide to keep this
>> version, then see the comment below.
>
> Given the changes I'm more happy with how the patch looks now and making it go
> in can be a nice incentive to change other ARMv8-M Security Extension related
> code later on.
>
> Best regards,
>
> Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 5816 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..4d09e891c288c7bf9b519ad7cd62bd38df661a02 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3620,6 +3620,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -24996,42 +25001,39 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ bool clear_vfpregs = TARGET_HARD_FLOAT && !TARGET_THUMB1;
+ int regno, maxregno = clear_vfpregs ? LAST_VFP_REGNUM : IP_REGNUM;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
- int regno, maxregno = IP_REGNUM;
+ auto_sbitmap to_clear_bitmap (LAST_VFP_REGNUM);
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ bitmap_clear (to_clear_bitmap);
+ bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+ bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
to make sure the instructions used to clear them are present. */
- if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
+ if (clear_vfpregs)
{
- uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
- maxregno = LAST_VFP_REGNUM;
-
- float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
+ int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ bitmap_clear_bit (to_clear_bitmap, 4);
}
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25041,29 +25043,45 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ bitmap_set_bit (to_clear_bitmap, regno);
}
/* Make sure we do not clear the registers used to return the result in. */
result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
if (!VOID_TYPE_P (result_type))
{
+ uint64_t to_clear_return_mask;
result_rtl = arm_function_value (result_type, current_function_decl, 0);
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
- &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
- padding_bits_to_clear_ptr);
+ gcc_assert (REG_P (result_rtl));
+ to_clear_return_mask
+ = compute_not_to_clear_mask (result_type, result_rtl, 0,
+ padding_bits_to_clear_ptr);
+ if (to_clear_return_mask)
+ {
+ gcc_assert ((unsigned) maxregno < sizeof (long long) * __CHAR_BIT__);
+ for (regno = R0_REGNUM; regno <= maxregno; regno++)
+ {
+ if (to_clear_return_mask & (1ULL << regno))
+ bitmap_clear_bit (to_clear_bitmap, regno);
+ }
+ }
}
if (padding_bits_to_clear != 0)
{
rtx reg_rtx;
+ auto_sbitmap to_clear_arg_regs_bitmap (R0_REGNUM + NUM_ARG_REGS - 1);
+
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ bitmap_clear (to_clear_arg_regs_bitmap);
+ bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+ NUM_ARG_REGS - 1);
+ gcc_assert (bitmap_subset_p (to_clear_arg_regs_bitmap, to_clear_bitmap));
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25085,7 +25103,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!bitmap_bit_p (to_clear_bitmap, regno))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25094,7 +25112,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && bitmap_bit_p (to_clear_bitmap, regno + 1))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31
2017-07-17 8:52 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
@ 2017-07-17 16:25 ` Thomas Preudhomme
2017-08-23 11:01 ` Thomas Preudhomme
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Preudhomme @ 2017-07-17 16:25 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]
My bad, found an off-by-one error in the sizing of bitmaps. Please find fixed
patch in attachment.
ChangeLog entry is unchanged:
*** gcc/ChangeLog ***
2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
* config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
Extensions with more than 16 double VFP registers.
(cmse_nonsecure_entry_clear_before_return): Remove second entry of
to_clear_mask and all code related to it. Replace the remaining
entry by a sbitmap and adapt code accordingly.
Best regards,
Thomas
On 17/07/17 09:52, Thomas Preudhomme wrote:
> Ping?
>
> Best regards,
>
> Thomas
>
> On 12/07/17 09:59, Thomas Preudhomme wrote:
>> Hi Richard,
>>
>> On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>>>
>>> Hmm, I think that's because really this is a partial conversion. It
>>> looks like doing this properly would involve moving that existing code
>>> to use sbitmaps as well. I think doing that would be better for
>>> long-term maintenance perspectives, but I'm not going to insist that you
>>> do it now.
>>
>> There's also the assert later but I've found a way to improve it slightly.
>> While switching to auto_sbitmap I also changed the code slightly to allocate
>> directly bitmaps to the right size. Since the change is probably bigger than
>> what you had in mind I'd appreciate if you can give me an OK again. See
>> updated patch in attachment. ChangeLog entry is unchanged:
>>
>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>
>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>> Extensions with more than 16 double VFP registers.
>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>> to_clear_mask and all code related to it. Replace the remaining
>> entry by a sbitmap and adapt code accordingly.
>>
>>>
>>> As a result I'll let you take the call as to whether you keep this
>>> version or go back to your earlier patch. If you do decide to keep this
>>> version, then see the comment below.
>>
>> Given the changes I'm more happy with how the patch looks now and making it go
>> in can be a nice incentive to change other ARMv8-M Security Extension related
>> code later on.
>>
>> Best regards,
>>
>> Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 5816 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..b79f0e701713d1958da3874da3a34f9c8f78bb5c 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3620,6 +3620,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -24996,42 +25001,39 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ bool clear_vfpregs = TARGET_HARD_FLOAT && !TARGET_THUMB1;
+ int regno, maxregno = clear_vfpregs ? LAST_VFP_REGNUM : IP_REGNUM;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
- int regno, maxregno = IP_REGNUM;
+ auto_sbitmap to_clear_bitmap (LAST_VFP_REGNUM + 1);
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ bitmap_clear (to_clear_bitmap);
+ bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+ bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
to make sure the instructions used to clear them are present. */
- if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
+ if (clear_vfpregs)
{
- uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
- maxregno = LAST_VFP_REGNUM;
-
- float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
+ int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ bitmap_clear_bit (to_clear_bitmap, 4);
}
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25041,29 +25043,45 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ bitmap_set_bit (to_clear_bitmap, regno);
}
/* Make sure we do not clear the registers used to return the result in. */
result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
if (!VOID_TYPE_P (result_type))
{
+ uint64_t to_clear_return_mask;
result_rtl = arm_function_value (result_type, current_function_decl, 0);
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
- &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
- padding_bits_to_clear_ptr);
+ gcc_assert (REG_P (result_rtl));
+ to_clear_return_mask
+ = compute_not_to_clear_mask (result_type, result_rtl, 0,
+ padding_bits_to_clear_ptr);
+ if (to_clear_return_mask)
+ {
+ gcc_assert ((unsigned) maxregno < sizeof (long long) * __CHAR_BIT__);
+ for (regno = R0_REGNUM; regno <= maxregno; regno++)
+ {
+ if (to_clear_return_mask & (1ULL << regno))
+ bitmap_clear_bit (to_clear_bitmap, regno);
+ }
+ }
}
if (padding_bits_to_clear != 0)
{
rtx reg_rtx;
+ auto_sbitmap to_clear_arg_regs_bitmap (R0_REGNUM + NUM_ARG_REGS);
+
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ bitmap_clear (to_clear_arg_regs_bitmap);
+ bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+ NUM_ARG_REGS - 1);
+ gcc_assert (bitmap_subset_p (to_clear_arg_regs_bitmap, to_clear_bitmap));
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25085,7 +25103,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!bitmap_bit_p (to_clear_bitmap, regno))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25094,7 +25112,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && bitmap_bit_p (to_clear_bitmap, regno + 1))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31
2017-07-17 16:25 ` Thomas Preudhomme
@ 2017-08-23 11:01 ` Thomas Preudhomme
2017-08-25 13:23 ` [PATCH, GCC/ARM] " Thomas Preudhomme
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Preudhomme @ 2017-08-23 11:01 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]
Ping?
Best regards,
Thomas
On 17/07/17 17:25, Thomas Preudhomme wrote:
> My bad, found an off-by-one error in the sizing of bitmaps. Please find fixed
> patch in attachment.
>
> ChangeLog entry is unchanged:
>
> *** gcc/ChangeLog ***
>
> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
> Extensions with more than 16 double VFP registers.
> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
> to_clear_mask and all code related to it. Replace the remaining
> entry by a sbitmap and adapt code accordingly.
>
> Best regards,
>
> Thomas
>
> On 17/07/17 09:52, Thomas Preudhomme wrote:
>> Ping?
>>
>> Best regards,
>>
>> Thomas
>>
>> On 12/07/17 09:59, Thomas Preudhomme wrote:
>>> Hi Richard,
>>>
>>> On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>>>>
>>>> Hmm, I think that's because really this is a partial conversion. It
>>>> looks like doing this properly would involve moving that existing code
>>>> to use sbitmaps as well. I think doing that would be better for
>>>> long-term maintenance perspectives, but I'm not going to insist that you
>>>> do it now.
>>>
>>> There's also the assert later but I've found a way to improve it slightly.
>>> While switching to auto_sbitmap I also changed the code slightly to allocate
>>> directly bitmaps to the right size. Since the change is probably bigger than
>>> what you had in mind I'd appreciate if you can give me an OK again. See
>>> updated patch in attachment. ChangeLog entry is unchanged:
>>>
>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>
>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>> Extensions with more than 16 double VFP registers.
>>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>>> to_clear_mask and all code related to it. Replace the remaining
>>> entry by a sbitmap and adapt code accordingly.
>>>
>>>>
>>>> As a result I'll let you take the call as to whether you keep this
>>>> version or go back to your earlier patch. If you do decide to keep this
>>>> version, then see the comment below.
>>>
>>> Given the changes I'm more happy with how the patch looks now and making it
>>> go in can be a nice incentive to change other ARMv8-M Security Extension
>>> related code later on.
>>>
>>> Best regards,
>>>
>>> Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 5816 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 259597d8890ee84c5bd92b12b6f9f6521c8dcd2e..b79f0e701713d1958da3874da3a34f9c8f78bb5c 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3620,6 +3620,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -24996,42 +25001,39 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ bool clear_vfpregs = TARGET_HARD_FLOAT && !TARGET_THUMB1;
+ int regno, maxregno = clear_vfpregs ? LAST_VFP_REGNUM : IP_REGNUM;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
- int regno, maxregno = IP_REGNUM;
+ auto_sbitmap to_clear_bitmap (LAST_VFP_REGNUM + 1);
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ bitmap_clear (to_clear_bitmap);
+ bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+ bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
to make sure the instructions used to clear them are present. */
- if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
+ if (clear_vfpregs)
{
- uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
- maxregno = LAST_VFP_REGNUM;
-
- float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
+ int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ bitmap_clear_bit (to_clear_bitmap, 4);
}
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25041,29 +25043,45 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ bitmap_set_bit (to_clear_bitmap, regno);
}
/* Make sure we do not clear the registers used to return the result in. */
result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
if (!VOID_TYPE_P (result_type))
{
+ uint64_t to_clear_return_mask;
result_rtl = arm_function_value (result_type, current_function_decl, 0);
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
- &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
- padding_bits_to_clear_ptr);
+ gcc_assert (REG_P (result_rtl));
+ to_clear_return_mask
+ = compute_not_to_clear_mask (result_type, result_rtl, 0,
+ padding_bits_to_clear_ptr);
+ if (to_clear_return_mask)
+ {
+ gcc_assert ((unsigned) maxregno < sizeof (long long) * __CHAR_BIT__);
+ for (regno = R0_REGNUM; regno <= maxregno; regno++)
+ {
+ if (to_clear_return_mask & (1ULL << regno))
+ bitmap_clear_bit (to_clear_bitmap, regno);
+ }
+ }
}
if (padding_bits_to_clear != 0)
{
rtx reg_rtx;
+ auto_sbitmap to_clear_arg_regs_bitmap (R0_REGNUM + NUM_ARG_REGS);
+
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ bitmap_clear (to_clear_arg_regs_bitmap);
+ bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+ NUM_ARG_REGS - 1);
+ gcc_assert (bitmap_subset_p (to_clear_arg_regs_bitmap, to_clear_bitmap));
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25085,7 +25103,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!bitmap_bit_p (to_clear_bitmap, regno))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25094,7 +25112,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && bitmap_bit_p (to_clear_bitmap, regno + 1))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
2017-08-23 11:01 ` Thomas Preudhomme
@ 2017-08-25 13:23 ` Thomas Preudhomme
2017-09-05 9:04 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Preudhomme @ 2017-08-25 13:23 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]
Hi,
I've now also added a couple more changes:
* size to_clear_bitmap according to maxregno to be consistent with its use
* use directly TARGET_HARD_FLOAT instead of clear_vfpregs
Original message below (ChangeLog unchanged):
Function cmse_nonsecure_entry_clear_before_return has code to deal with
high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
not support more than 16 double VFP registers (D0-D15). This makes this
security-sensitive code harder to read for not much benefit since
libcall for cmse_nonsecure_call functions do not deal with those high
VFP registers anyway.
This commit gets rid of this code for simplicity and fixes 2 issues in
the same function:
- stop the first loop when reaching maxregno to avoid dealing with VFP
registers if targetting Thumb-1 or using -mfloat-abi=soft
- include maxregno in that loop
ChangeLog entry is as follows:
*** gcc/ChangeLog ***
2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
* config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
Extensions with more than 16 double VFP registers.
(cmse_nonsecure_entry_clear_before_return): Remove second entry of
to_clear_mask and all code related to it. Replace the remaining
entry by a sbitmap and adapt code accordingly.
Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
ARMv8-M Mainline.
Is this ok for trunk?
Best regards,
Thomas
On 23/08/17 11:56, Thomas Preudhomme wrote:
> Ping?
>
> Best regards,
>
> Thomas
>
> On 17/07/17 17:25, Thomas Preudhomme wrote:
>> My bad, found an off-by-one error in the sizing of bitmaps. Please find fixed
>> patch in attachment.
>>
>> ChangeLog entry is unchanged:
>>
>> *** gcc/ChangeLog ***
>>
>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>
>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>> Extensions with more than 16 double VFP registers.
>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>> to_clear_mask and all code related to it. Replace the remaining
>> entry by a sbitmap and adapt code accordingly.
>>
>> Best regards,
>>
>> Thomas
>>
>> On 17/07/17 09:52, Thomas Preudhomme wrote:
>>> Ping?
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>> On 12/07/17 09:59, Thomas Preudhomme wrote:
>>>> Hi Richard,
>>>>
>>>> On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>>>>>
>>>>> Hmm, I think that's because really this is a partial conversion. It
>>>>> looks like doing this properly would involve moving that existing code
>>>>> to use sbitmaps as well. I think doing that would be better for
>>>>> long-term maintenance perspectives, but I'm not going to insist that you
>>>>> do it now.
>>>>
>>>> There's also the assert later but I've found a way to improve it slightly.
>>>> While switching to auto_sbitmap I also changed the code slightly to allocate
>>>> directly bitmaps to the right size. Since the change is probably bigger than
>>>> what you had in mind I'd appreciate if you can give me an OK again. See
>>>> updated patch in attachment. ChangeLog entry is unchanged:
>>>>
>>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>>
>>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>>> Extensions with more than 16 double VFP registers.
>>>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>>>> to_clear_mask and all code related to it. Replace the remaining
>>>> entry by a sbitmap and adapt code accordingly.
>>>>
>>>>>
>>>>> As a result I'll let you take the call as to whether you keep this
>>>>> version or go back to your earlier patch. If you do decide to keep this
>>>>> version, then see the comment below.
>>>>
>>>> Given the changes I'm more happy with how the patch looks now and making it
>>>> go in can be a nice incentive to change other ARMv8-M Security Extension
>>>> related code later on.
>>>>
>>>> Best regards,
>>>>
>>>> Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 5777 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 3d15a8185a74164743961d7d666cef4d60b8b11e..680a3c564bdad4ae7cacd57b61f099bdf42d3e73 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3658,6 +3658,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -25038,42 +25043,37 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ int regno, maxregno = TARGET_HARD_FLOAT ? LAST_VFP_REGNUM : IP_REGNUM;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
- int regno, maxregno = IP_REGNUM;
+ auto_sbitmap to_clear_bitmap (maxregno + 1);
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ bitmap_clear (to_clear_bitmap);
+ bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+ bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
- registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
- to make sure the instructions used to clear them are present. */
- if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
+ registers. */
+ if (TARGET_HARD_FLOAT)
{
- uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
- maxregno = LAST_VFP_REGNUM;
+ int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
- float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
-
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ bitmap_clear_bit (to_clear_bitmap, 4);
}
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25083,29 +25083,45 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ bitmap_set_bit (to_clear_bitmap, regno);
}
/* Make sure we do not clear the registers used to return the result in. */
result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
if (!VOID_TYPE_P (result_type))
{
+ uint64_t to_clear_return_mask;
result_rtl = arm_function_value (result_type, current_function_decl, 0);
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
- &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
- padding_bits_to_clear_ptr);
+ gcc_assert (REG_P (result_rtl));
+ to_clear_return_mask
+ = compute_not_to_clear_mask (result_type, result_rtl, 0,
+ padding_bits_to_clear_ptr);
+ if (to_clear_return_mask)
+ {
+ gcc_assert ((unsigned) maxregno < sizeof (long long) * __CHAR_BIT__);
+ for (regno = R0_REGNUM; regno <= maxregno; regno++)
+ {
+ if (to_clear_return_mask & (1ULL << regno))
+ bitmap_clear_bit (to_clear_bitmap, regno);
+ }
+ }
}
if (padding_bits_to_clear != 0)
{
rtx reg_rtx;
+ auto_sbitmap to_clear_arg_regs_bitmap (R0_REGNUM + NUM_ARG_REGS);
+
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ bitmap_clear (to_clear_arg_regs_bitmap);
+ bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+ NUM_ARG_REGS - 1);
+ gcc_assert (bitmap_subset_p (to_clear_arg_regs_bitmap, to_clear_bitmap));
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25127,7 +25143,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!bitmap_bit_p (to_clear_bitmap, regno))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25136,7 +25152,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && bitmap_bit_p (to_clear_bitmap, regno + 1))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31
2017-08-25 13:23 ` [PATCH, GCC/ARM] " Thomas Preudhomme
@ 2017-09-05 9:04 ` Thomas Preudhomme
2017-09-06 8:12 ` Kyrill Tkachov
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Preudhomme @ 2017-09-05 9:04 UTC (permalink / raw)
To: Richard Earnshaw (lists),
Kyrill Tkachov, Ramana Radhakrishnan, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 4219 bytes --]
Ping?
Best regards,
Thomas
On 25/08/17 12:18, Thomas Preudhomme wrote:
> Hi,
>
> I've now also added a couple more changes:
>
> * size to_clear_bitmap according to maxregno to be consistent with its use
> * use directly TARGET_HARD_FLOAT instead of clear_vfpregs
>
>
> Original message below (ChangeLog unchanged):
>
> Function cmse_nonsecure_entry_clear_before_return has code to deal with
> high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
> not support more than 16 double VFP registers (D0-D15). This makes this
> security-sensitive code harder to read for not much benefit since
> libcall for cmse_nonsecure_call functions do not deal with those high
> VFP registers anyway.
>
> This commit gets rid of this code for simplicity and fixes 2 issues in
> the same function:
>
> - stop the first loop when reaching maxregno to avoid dealing with VFP
> registers if targetting Thumb-1 or using -mfloat-abi=soft
> - include maxregno in that loop
>
> ChangeLog entry is as follows:
>
> *** gcc/ChangeLog ***
>
> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
> Extensions with more than 16 double VFP registers.
> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
> to_clear_mask and all code related to it. Replace the remaining
> entry by a sbitmap and adapt code accordingly.
>
> Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
> ARMv8-M Mainline.
>
> Is this ok for trunk?
>
> Best regards,
>
> Thomas
>
> On 23/08/17 11:56, Thomas Preudhomme wrote:
>> Ping?
>>
>> Best regards,
>>
>> Thomas
>>
>> On 17/07/17 17:25, Thomas Preudhomme wrote:
>>> My bad, found an off-by-one error in the sizing of bitmaps. Please find fixed
>>> patch in attachment.
>>>
>>> ChangeLog entry is unchanged:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>
>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>> Extensions with more than 16 double VFP registers.
>>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>>> to_clear_mask and all code related to it. Replace the remaining
>>> entry by a sbitmap and adapt code accordingly.
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>> On 17/07/17 09:52, Thomas Preudhomme wrote:
>>>> Ping?
>>>>
>>>> Best regards,
>>>>
>>>> Thomas
>>>>
>>>> On 12/07/17 09:59, Thomas Preudhomme wrote:
>>>>> Hi Richard,
>>>>>
>>>>> On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>>>>>>
>>>>>> Hmm, I think that's because really this is a partial conversion. It
>>>>>> looks like doing this properly would involve moving that existing code
>>>>>> to use sbitmaps as well. I think doing that would be better for
>>>>>> long-term maintenance perspectives, but I'm not going to insist that you
>>>>>> do it now.
>>>>>
>>>>> There's also the assert later but I've found a way to improve it slightly.
>>>>> While switching to auto_sbitmap I also changed the code slightly to
>>>>> allocate directly bitmaps to the right size. Since the change is probably
>>>>> bigger than what you had in mind I'd appreciate if you can give me an OK
>>>>> again. See updated patch in attachment. ChangeLog entry is unchanged:
>>>>>
>>>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>>>
>>>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>>>> Extensions with more than 16 double VFP registers.
>>>>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>>>>> to_clear_mask and all code related to it. Replace the remaining
>>>>> entry by a sbitmap and adapt code accordingly.
>>>>>
>>>>>>
>>>>>> As a result I'll let you take the call as to whether you keep this
>>>>>> version or go back to your earlier patch. If you do decide to keep this
>>>>>> version, then see the comment below.
>>>>>
>>>>> Given the changes I'm more happy with how the patch looks now and making it
>>>>> go in can be a nice incentive to change other ARMv8-M Security Extension
>>>>> related code later on.
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Thomas
[-- Attachment #2: remove_d16-d31_armv8m_clearing_code.patch --]
[-- Type: text/x-patch, Size: 5777 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 3d15a8185a74164743961d7d666cef4d60b8b11e..680a3c564bdad4ae7cacd57b61f099bdf42d3e73 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -3658,6 +3658,11 @@ arm_option_override (void)
if (use_cmse && !arm_arch_cmse)
error ("target CPU does not support ARMv8-M Security Extensions");
+ /* We don't clear D16-D31 VFP registers for cmse_nonsecure_call functions
+ and ARMv8-M Baseline and Mainline do not allow such configuration. */
+ if (use_cmse && LAST_VFP_REGNUM > LAST_LO_VFP_REGNUM)
+ error ("ARMv8-M Security Extensions incompatible with selected FPU");
+
/* Disable scheduling fusion by default if it's not armv7 processor
or doesn't prefer ldrd/strd. */
if (flag_schedule_fusion == 2
@@ -25038,42 +25043,37 @@ thumb1_expand_prologue (void)
void
cmse_nonsecure_entry_clear_before_return (void)
{
- uint64_t to_clear_mask[2];
+ int regno, maxregno = TARGET_HARD_FLOAT ? LAST_VFP_REGNUM : IP_REGNUM;
uint32_t padding_bits_to_clear = 0;
uint32_t * padding_bits_to_clear_ptr = &padding_bits_to_clear;
- int regno, maxregno = IP_REGNUM;
+ auto_sbitmap to_clear_bitmap (maxregno + 1);
tree result_type;
rtx result_rtl;
- to_clear_mask[0] = (1ULL << (NUM_ARG_REGS)) - 1;
- to_clear_mask[0] |= (1ULL << IP_REGNUM);
+ bitmap_clear (to_clear_bitmap);
+ bitmap_set_range (to_clear_bitmap, R0_REGNUM, NUM_ARG_REGS);
+ bitmap_set_bit (to_clear_bitmap, IP_REGNUM);
/* If we are not dealing with -mfloat-abi=soft we will need to clear VFP
- registers. We also check that TARGET_HARD_FLOAT and !TARGET_THUMB1 hold
- to make sure the instructions used to clear them are present. */
- if (TARGET_HARD_FLOAT && !TARGET_THUMB1)
+ registers. */
+ if (TARGET_HARD_FLOAT)
{
- uint64_t float_mask = (1ULL << (D7_VFP_REGNUM + 1)) - 1;
- maxregno = LAST_VFP_REGNUM;
+ int float_bits = D7_VFP_REGNUM - FIRST_VFP_REGNUM + 1;
- float_mask &= ~((1ULL << FIRST_VFP_REGNUM) - 1);
- to_clear_mask[0] |= float_mask;
-
- float_mask = (1ULL << (maxregno - 63)) - 1;
- to_clear_mask[1] = float_mask;
+ bitmap_set_range (to_clear_bitmap, FIRST_VFP_REGNUM, float_bits);
/* Make sure we don't clear the two scratch registers used to clear the
relevant FPSCR bits in output_return_instruction. */
emit_use (gen_rtx_REG (SImode, IP_REGNUM));
- to_clear_mask[0] &= ~(1ULL << IP_REGNUM);
+ bitmap_clear_bit (to_clear_bitmap, IP_REGNUM);
emit_use (gen_rtx_REG (SImode, 4));
- to_clear_mask[0] &= ~(1ULL << 4);
+ bitmap_clear_bit (to_clear_bitmap, 4);
}
/* If the user has defined registers to be caller saved, these are no longer
restored by the function before returning and must thus be cleared for
security purposes. */
- for (regno = NUM_ARG_REGS; regno < LAST_VFP_REGNUM; regno++)
+ for (regno = NUM_ARG_REGS; regno <= maxregno; regno++)
{
/* We do not touch registers that can be used to pass arguments as per
the AAPCS, since these should never be made callee-saved by user
@@ -25083,29 +25083,45 @@ cmse_nonsecure_entry_clear_before_return (void)
if (IN_RANGE (regno, IP_REGNUM, PC_REGNUM))
continue;
if (call_used_regs[regno])
- to_clear_mask[regno / 64] |= (1ULL << (regno % 64));
+ bitmap_set_bit (to_clear_bitmap, regno);
}
/* Make sure we do not clear the registers used to return the result in. */
result_type = TREE_TYPE (DECL_RESULT (current_function_decl));
if (!VOID_TYPE_P (result_type))
{
+ uint64_t to_clear_return_mask;
result_rtl = arm_function_value (result_type, current_function_decl, 0);
/* No need to check that we return in registers, because we don't
support returning on stack yet. */
- to_clear_mask[0]
- &= ~compute_not_to_clear_mask (result_type, result_rtl, 0,
- padding_bits_to_clear_ptr);
+ gcc_assert (REG_P (result_rtl));
+ to_clear_return_mask
+ = compute_not_to_clear_mask (result_type, result_rtl, 0,
+ padding_bits_to_clear_ptr);
+ if (to_clear_return_mask)
+ {
+ gcc_assert ((unsigned) maxregno < sizeof (long long) * __CHAR_BIT__);
+ for (regno = R0_REGNUM; regno <= maxregno; regno++)
+ {
+ if (to_clear_return_mask & (1ULL << regno))
+ bitmap_clear_bit (to_clear_bitmap, regno);
+ }
+ }
}
if (padding_bits_to_clear != 0)
{
rtx reg_rtx;
+ auto_sbitmap to_clear_arg_regs_bitmap (R0_REGNUM + NUM_ARG_REGS);
+
/* Padding bits to clear is not 0 so we know we are dealing with
returning a composite type, which only uses r0. Let's make sure that
r1-r3 is cleared too, we will use r1 as a scratch register. */
- gcc_assert ((to_clear_mask[0] & 0xe) == 0xe);
+ bitmap_clear (to_clear_arg_regs_bitmap);
+ bitmap_set_range (to_clear_arg_regs_bitmap, R0_REGNUM + 1,
+ NUM_ARG_REGS - 1);
+ gcc_assert (bitmap_subset_p (to_clear_arg_regs_bitmap, to_clear_bitmap));
reg_rtx = gen_rtx_REG (SImode, R1_REGNUM);
@@ -25127,7 +25143,7 @@ cmse_nonsecure_entry_clear_before_return (void)
for (regno = R0_REGNUM; regno <= maxregno; regno++)
{
- if (!(to_clear_mask[regno / 64] & (1ULL << (regno % 64))))
+ if (!bitmap_bit_p (to_clear_bitmap, regno))
continue;
if (IS_VFP_REGNUM (regno))
@@ -25136,7 +25152,7 @@ cmse_nonsecure_entry_clear_before_return (void)
be cleared, use vmov. */
if (TARGET_VFP_DOUBLE
&& VFP_REGNO_OK_FOR_DOUBLE (regno)
- && to_clear_mask[regno / 64] & (1ULL << ((regno % 64) + 1)))
+ && bitmap_bit_p (to_clear_bitmap, regno + 1))
{
emit_move_insn (gen_rtx_REG (DFmode, regno),
CONST1_RTX (DFmode));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31
2017-09-05 9:04 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
@ 2017-09-06 8:12 ` Kyrill Tkachov
2017-09-28 16:13 ` Thomas Preudhomme
0 siblings, 1 reply; 14+ messages in thread
From: Kyrill Tkachov @ 2017-09-06 8:12 UTC (permalink / raw)
To: Thomas Preudhomme, Richard Earnshaw (lists),
Ramana Radhakrishnan, gcc-patches
Hi Thomas,
On 05/09/17 10:04, Thomas Preudhomme wrote:
> Ping?
>
This is ok if a bootstrap and test run on arm-none-linux-gnueabihf shows
no problems.
Thanks,
Kyrill
> Best regards,
>
> Thomas
>
> On 25/08/17 12:18, Thomas Preudhomme wrote:
>> Hi,
>>
>> I've now also added a couple more changes:
>>
>> * size to_clear_bitmap according to maxregno to be consistent with
>> its use
>> * use directly TARGET_HARD_FLOAT instead of clear_vfpregs
>>
>>
>> Original message below (ChangeLog unchanged):
>>
>> Function cmse_nonsecure_entry_clear_before_return has code to deal with
>> high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
>> not support more than 16 double VFP registers (D0-D15). This makes this
>> security-sensitive code harder to read for not much benefit since
>> libcall for cmse_nonsecure_call functions do not deal with those high
>> VFP registers anyway.
>>
>> This commit gets rid of this code for simplicity and fixes 2 issues in
>> the same function:
>>
>> - stop the first loop when reaching maxregno to avoid dealing with VFP
>> registers if targetting Thumb-1 or using -mfloat-abi=soft
>> - include maxregno in that loop
>>
>> ChangeLog entry is as follows:
>>
>> *** gcc/ChangeLog ***
>>
>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>
>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>> Extensions with more than 16 double VFP registers.
>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>> to_clear_mask and all code related to it. Replace the remaining
>> entry by a sbitmap and adapt code accordingly.
>>
>> Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
>> ARMv8-M Mainline.
>>
>> Is this ok for trunk?
>>
>> Best regards,
>>
>> Thomas
>>
>> On 23/08/17 11:56, Thomas Preudhomme wrote:
>>> Ping?
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>> On 17/07/17 17:25, Thomas Preudhomme wrote:
>>>> My bad, found an off-by-one error in the sizing of bitmaps. Please
>>>> find fixed patch in attachment.
>>>>
>>>> ChangeLog entry is unchanged:
>>>>
>>>> *** gcc/ChangeLog ***
>>>>
>>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>>
>>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>>> Extensions with more than 16 double VFP registers.
>>>> (cmse_nonsecure_entry_clear_before_return): Remove second
>>>> entry of
>>>> to_clear_mask and all code related to it. Replace the remaining
>>>> entry by a sbitmap and adapt code accordingly.
>>>>
>>>> Best regards,
>>>>
>>>> Thomas
>>>>
>>>> On 17/07/17 09:52, Thomas Preudhomme wrote:
>>>>> Ping?
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Thomas
>>>>>
>>>>> On 12/07/17 09:59, Thomas Preudhomme wrote:
>>>>>> Hi Richard,
>>>>>>
>>>>>> On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>>>>>>>
>>>>>>> Hmm, I think that's because really this is a partial
>>>>>>> conversion. It
>>>>>>> looks like doing this properly would involve moving that
>>>>>>> existing code
>>>>>>> to use sbitmaps as well. I think doing that would be better for
>>>>>>> long-term maintenance perspectives, but I'm not going to insist
>>>>>>> that you
>>>>>>> do it now.
>>>>>>
>>>>>> There's also the assert later but I've found a way to improve it
>>>>>> slightly. While switching to auto_sbitmap I also changed the code
>>>>>> slightly to allocate directly bitmaps to the right size. Since
>>>>>> the change is probably bigger than what you had in mind I'd
>>>>>> appreciate if you can give me an OK again. See updated patch in
>>>>>> attachment. ChangeLog entry is unchanged:
>>>>>>
>>>>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>>>>
>>>>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M
>>>>>> Security
>>>>>> Extensions with more than 16 double VFP registers.
>>>>>> (cmse_nonsecure_entry_clear_before_return): Remove second
>>>>>> entry of
>>>>>> to_clear_mask and all code related to it. Replace the
>>>>>> remaining
>>>>>> entry by a sbitmap and adapt code accordingly.
>>>>>>
>>>>>>>
>>>>>>> As a result I'll let you take the call as to whether you keep this
>>>>>>> version or go back to your earlier patch. If you do decide to
>>>>>>> keep this
>>>>>>> version, then see the comment below.
>>>>>>
>>>>>> Given the changes I'm more happy with how the patch looks now and
>>>>>> making it go in can be a nice incentive to change other ARMv8-M
>>>>>> Security Extension related code later on.
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31
2017-09-06 8:12 ` Kyrill Tkachov
@ 2017-09-28 16:13 ` Thomas Preudhomme
0 siblings, 0 replies; 14+ messages in thread
From: Thomas Preudhomme @ 2017-09-28 16:13 UTC (permalink / raw)
To: Kyrill Tkachov, Richard Earnshaw (lists),
Ramana Radhakrishnan, gcc-patches
Committed (sorry for delay).
Best regards,
Thomas
On 06/09/17 09:12, Kyrill Tkachov wrote:
> Hi Thomas,
>
> On 05/09/17 10:04, Thomas Preudhomme wrote:
>> Ping?
>>
>
> This is ok if a bootstrap and test run on arm-none-linux-gnueabihf shows no
> problems.
> Thanks,
> Kyrill
>
>> Best regards,
>>
>> Thomas
>>
>> On 25/08/17 12:18, Thomas Preudhomme wrote:
>>> Hi,
>>>
>>> I've now also added a couple more changes:
>>>
>>> * size to_clear_bitmap according to maxregno to be consistent with its use
>>> * use directly TARGET_HARD_FLOAT instead of clear_vfpregs
>>>
>>>
>>> Original message below (ChangeLog unchanged):
>>>
>>> Function cmse_nonsecure_entry_clear_before_return has code to deal with
>>> high VFP register (D16-D31) while ARMv8-M Baseline and Mainline both do
>>> not support more than 16 double VFP registers (D0-D15). This makes this
>>> security-sensitive code harder to read for not much benefit since
>>> libcall for cmse_nonsecure_call functions do not deal with those high
>>> VFP registers anyway.
>>>
>>> This commit gets rid of this code for simplicity and fixes 2 issues in
>>> the same function:
>>>
>>> - stop the first loop when reaching maxregno to avoid dealing with VFP
>>> registers if targetting Thumb-1 or using -mfloat-abi=soft
>>> - include maxregno in that loop
>>>
>>> ChangeLog entry is as follows:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>
>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>> Extensions with more than 16 double VFP registers.
>>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>>> to_clear_mask and all code related to it. Replace the remaining
>>> entry by a sbitmap and adapt code accordingly.
>>>
>>> Testing: Testsuite shows no regression when run for ARMv8-M Baseline and
>>> ARMv8-M Mainline.
>>>
>>> Is this ok for trunk?
>>>
>>> Best regards,
>>>
>>> Thomas
>>>
>>> On 23/08/17 11:56, Thomas Preudhomme wrote:
>>>> Ping?
>>>>
>>>> Best regards,
>>>>
>>>> Thomas
>>>>
>>>> On 17/07/17 17:25, Thomas Preudhomme wrote:
>>>>> My bad, found an off-by-one error in the sizing of bitmaps. Please find
>>>>> fixed patch in attachment.
>>>>>
>>>>> ChangeLog entry is unchanged:
>>>>>
>>>>> *** gcc/ChangeLog ***
>>>>>
>>>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>>>
>>>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>>>> Extensions with more than 16 double VFP registers.
>>>>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>>>>> to_clear_mask and all code related to it. Replace the remaining
>>>>> entry by a sbitmap and adapt code accordingly.
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Thomas
>>>>>
>>>>> On 17/07/17 09:52, Thomas Preudhomme wrote:
>>>>>> Ping?
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Thomas
>>>>>>
>>>>>> On 12/07/17 09:59, Thomas Preudhomme wrote:
>>>>>>> Hi Richard,
>>>>>>>
>>>>>>> On 07/07/17 15:19, Richard Earnshaw (lists) wrote:
>>>>>>>>
>>>>>>>> Hmm, I think that's because really this is a partial conversion. It
>>>>>>>> looks like doing this properly would involve moving that existing code
>>>>>>>> to use sbitmaps as well. I think doing that would be better for
>>>>>>>> long-term maintenance perspectives, but I'm not going to insist that you
>>>>>>>> do it now.
>>>>>>>
>>>>>>> There's also the assert later but I've found a way to improve it
>>>>>>> slightly. While switching to auto_sbitmap I also changed the code
>>>>>>> slightly to allocate directly bitmaps to the right size. Since the change
>>>>>>> is probably bigger than what you had in mind I'd appreciate if you can
>>>>>>> give me an OK again. See updated patch in attachment. ChangeLog entry is
>>>>>>> unchanged:
>>>>>>>
>>>>>>> 2017-06-13 Thomas Preud'homme <thomas.preudhomme@arm.com>
>>>>>>>
>>>>>>> * config/arm/arm.c (arm_option_override): Forbid ARMv8-M Security
>>>>>>> Extensions with more than 16 double VFP registers.
>>>>>>> (cmse_nonsecure_entry_clear_before_return): Remove second entry of
>>>>>>> to_clear_mask and all code related to it. Replace the remaining
>>>>>>> entry by a sbitmap and adapt code accordingly.
>>>>>>>
>>>>>>>>
>>>>>>>> As a result I'll let you take the call as to whether you keep this
>>>>>>>> version or go back to your earlier patch. If you do decide to keep this
>>>>>>>> version, then see the comment below.
>>>>>>>
>>>>>>> Given the changes I'm more happy with how the patch looks now and making
>>>>>>> it go in can be a nice incentive to change other ARMv8-M Security
>>>>>>> Extension related code later on.
>>>>>>>
>>>>>>> Best regards,
>>>>>>>
>>>>>>> Thomas
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-09-28 16:13 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-20 15:01 [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31 Thomas Preudhomme
2017-06-28 15:12 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
2017-06-28 15:57 ` [PATCH, GCC/ARM] " Richard Earnshaw (lists)
2017-06-29 8:44 ` Thomas Preudhomme
2017-07-06 12:36 ` Thomas Preudhomme
2017-07-07 14:19 ` Richard Earnshaw (lists)
2017-07-12 8:59 ` Thomas Preudhomme
2017-07-17 8:52 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
2017-07-17 16:25 ` Thomas Preudhomme
2017-08-23 11:01 ` Thomas Preudhomme
2017-08-25 13:23 ` [PATCH, GCC/ARM] " Thomas Preudhomme
2017-09-05 9:04 ` [PATCH, GCC/ARM, ping] " Thomas Preudhomme
2017-09-06 8:12 ` Kyrill Tkachov
2017-09-28 16:13 ` Thomas Preudhomme
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).