Message ID | a83e0a14-3c4d-1df8-8210-22fbe1dd3fb8@foss.arm.com |
---|---|
State | New |
Headers | show |
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)); >
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
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));