From: "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>
To: Thomas Preudhomme <thomas.preudhomme@foss.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: Wed, 28 Jun 2017 15:57:00 -0000 [thread overview]
Message-ID: <faba24a3-e07d-403d-b3ba-cc725fcdc742@arm.com> (raw)
In-Reply-To: <a83e0a14-3c4d-1df8-8210-22fbe1dd3fb8@foss.arm.com>
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));
>
next prev parent reply other threads:[~2017-06-28 15:57 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 ` Richard Earnshaw (lists) [this message]
2017-06-29 8:44 ` [PATCH, GCC/ARM] " 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
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=faba24a3-e07d-403d-b3ba-cc725fcdc742@arm.com \
--to=richard.earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@arm.com \
--cc=ramana.radhakrishnan@arm.com \
--cc=thomas.preudhomme@foss.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).