From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57200 invoked by alias); 28 Jun 2017 15:57:09 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 57189 invoked by uid 89); 28 Jun 2017 15:57:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=steam X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 28 Jun 2017 15:57:06 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5E0B780D; Wed, 28 Jun 2017 08:57:03 -0700 (PDT) Received: from e105689-lin.cambridge.arm.com (e105689-lin.cambridge.arm.com [10.2.207.32]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 79BB13F41F; Wed, 28 Jun 2017 08:57:02 -0700 (PDT) Subject: Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31 To: Thomas Preudhomme , Kyrill Tkachov , Ramana Radhakrishnan , "gcc-patches@gcc.gnu.org" References: From: "Richard Earnshaw (lists)" Message-ID: Date: Wed, 28 Jun 2017 15:57:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-SW-Source: 2017-06/txt/msg02187.txt.bz2 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 > > * 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)); >