From: Thomas Preudhomme <thomas.preudhomme@foss.arm.com>
To: "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>,
Kyrill Tkachov <kyrylo.tkachov@arm.com>,
Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH, GCC/ARM] Remove ARMv8-M code for D17-D31
Date: Fri, 25 Aug 2017 13:23:00 -0000 [thread overview]
Message-ID: <2b4031d6-cc6c-67e4-c372-319694df65be@foss.arm.com> (raw)
In-Reply-To: <f793f27c-413d-6360-48f7-daf82c54c57d@foss.arm.com>
[-- 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));
next prev parent reply other threads:[~2017-08-25 11:18 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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)
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 ` Thomas Preudhomme [this message]
2017-09-05 9:04 ` Thomas Preudhomme
2017-09-06 8:12 ` Kyrill Tkachov
2017-09-28 16:13 ` Thomas Preudhomme
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2b4031d6-cc6c-67e4-c372-319694df65be@foss.arm.com \
--to=thomas.preudhomme@foss.arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@arm.com \
--cc=ramana.radhakrishnan@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).