From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73859 invoked by alias); 5 Sep 2017 09:04:50 -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 73791 invoked by uid 89); 5 Sep 2017 09:04:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: foss.arm.com Received: from usa-sjc-mx-foss1.foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 05 Sep 2017 09:04:42 +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 95C1D13D5; Tue, 5 Sep 2017 02:04:41 -0700 (PDT) Received: from [10.2.206.52] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9F2943F578; Tue, 5 Sep 2017 02:04:40 -0700 (PDT) Subject: Re: [PATCH, GCC/ARM, ping] Remove ARMv8-M code for D17-D31 From: Thomas Preudhomme To: "Richard Earnshaw (lists)" , Kyrill Tkachov , Ramana Radhakrishnan , "gcc-patches@gcc.gnu.org" References: <5da9957f-06bc-11dd-a63a-6bf341b84486@foss.arm.com> <8248378c-d652-1f75-369f-904706f4b022@arm.com> <08072c0e-614d-0855-8040-3ccd9e417c05@foss.arm.com> <5a7866d8-e0fb-00d4-7f46-ae90f298fb07@foss.arm.com> <6753e9df-975d-b97f-b419-9283ba5aa8e9@foss.arm.com> <2b4031d6-cc6c-67e4-c372-319694df65be@foss.arm.com> Message-ID: Date: Tue, 05 Sep 2017 09:04:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <2b4031d6-cc6c-67e4-c372-319694df65be@foss.arm.com> Content-Type: multipart/mixed; boundary="------------222D875F2F3918FD261159D8" X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00228.txt.bz2 This is a multi-part message in MIME format. --------------222D875F2F3918FD261159D8 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-length: 4219 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 > > * 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 >>> >>> * 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 >>>>> >>>>> * 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 --------------222D875F2F3918FD261159D8 Content-Type: text/x-patch; name="remove_d16-d31_armv8m_clearing_code.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="remove_d16-d31_armv8m_clearing_code.patch" Content-length: 5777 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)); --------------222D875F2F3918FD261159D8--