Re: [PATCH, GCC/ARM, 3/10] Save/restore FPCXTNS in nsentry functions

Tue, 17 Dec 2019 02:25:00 -0800 

Hi Mihail,

On 12/16/19 6:29 PM, Mihail Ionescu wrote:


Hi Kyrill,

On 11/06/2019 04:12 PM, Kyrill Tkachov wrote:

Hi Mihail,

On 10/23/19 10:26 AM, Mihail Ionescu wrote:

[PATCH, GCC/ARM, 3/10] Save/restore FPCXTNS in nsentry functions

Hi,

=== Context ===

This patch is part of a patch series to add support for Armv8.1-M
Mainline Security Extensions architecture. Its purpose is to enable
saving/restoring of nonsecure FP context in function with the
cmse_nonsecure_entry attribute.

=== Motivation ===


In Armv8-M Baseline and Mainline, the FP context is cleared on 
return from

nonsecure entry functions. This means the FP context might change when
calling a nonsecure entry function. This patch uses the new VLDR and

VSTR instructions available in Armv8.1-M Mainline to save/restore 
the FP

context when calling a nonsecure entry functionfrom nonsecure code.

=== Patch description ===

This patch consists mainly of creating 2 new instruction patterns to
push and pop special FP registers via vldm and vstr and using them in
prologue and epilogue. The patterns are defined as push/pop with an
unspecified operation on the memory accessed, with an unspecified
constant indicating what special FP register is being saved/restored.

Other aspects of the patch include:

  * defining the set of special registers that can be saved/restored 
and

    their name
  * reserving space in the stack frames for these push/pop
  * preventing return via pop
  * guarding the clearing of FPSCR to target architecture not having
    Armv8.1-M Mainline instructions.

ChangeLog entry is as follows:

*** gcc/ChangeLog ***

2019-10-23  Mihail-Calin Ionescu <mihail.ione...@arm.com>
2019-10-23  Thomas Preud'homme <thomas.preudho...@arm.com>

        * config/arm/arm.c (fp_sysreg_names): Declare and define.
        (use_return_insn): Also return false for Armv8.1-M Mainline.
        (output_return_instruction): Skip FPSCR clearing if Armv8.1-M
        Mainline instructions are available.
        (arm_compute_frame_layout): Allocate space in frame for FPCXTNS
        when targeting Armv8.1-M Mainline Security Extensions.
        (arm_expand_prologue): Save FPCXTNS if this is an Armv8.1-M
        Mainline entry function.
        (cmse_nonsecure_entry_clear_before_return): Clear IP and r4 if
        targeting Armv8.1-M Mainline or successor.
        (arm_expand_epilogue): Fix indentation of caller-saved register
        clearing.  Restore FPCXTNS if this is an Armv8.1-M Mainline
        entry function.
        * config/arm/arm.h (TARGET_HAVE_FP_CMSE): New macro.
        (FP_SYSREGS): Likewise.
        (enum vfp_sysregs_encoding): Define enum.
        (fp_sysreg_names): Declare.

        * config/arm/unspecs.md (VUNSPEC_VSTR_VLDR): New volatile 
unspec.

        * config/arm/vfp.md (push_fpsysreg_insn): New define_insn.
        (pop_fpsysreg_insn): Likewise.

*** gcc/testsuite/Changelog ***

2019-10-23  Mihail-Calin Ionescu <mihail.ione...@arm.com>
2019-10-23  Thomas Preud'homme <thomas.preudho...@arm.com>


        * gcc.target/arm/cmse/bitfield-1.c: add checks for VSTR and 
VLDR.

        * gcc.target/arm/cmse/bitfield-2.c: Likewise.
        * gcc.target/arm/cmse/bitfield-3.c: Likewise.
        * gcc.target/arm/cmse/cmse-1.c: Likewise.
        * gcc.target/arm/cmse/struct-1.c: Likewise.

        * gcc.target/arm/cmse/cmse.exp: Run existing Armv8-M 
Mainline tests
        from mainline/8m subdirectory and new Armv8.1-M Mainline 
tests from

        mainline/8_1m subdirectory.
        * gcc.target/arm/cmse/mainline/bitfield-4.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/bitfield-4.c: This.
        * gcc.target/arm/cmse/mainline/bitfield-5.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/bitfield-5.c: This.
        * gcc.target/arm/cmse/mainline/bitfield-6.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/bitfield-6.c: This.
        * gcc.target/arm/cmse/mainline/bitfield-7.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/bitfield-7.c: This.
        * gcc.target/arm/cmse/mainline/bitfield-8.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/bitfield-8.c: This.
        * gcc.target/arm/cmse/mainline/bitfield-9.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/bitfield-9.c: This.

        * gcc.target/arm/cmse/mainline/bitfield-and-union-1.c: Move 
and rename

        into ...
        * gcc.target/arm/cmse/mainline/8m/bitfield-and-union.c: This.
        * gcc.target/arm/cmse/mainline/hard-sp/cmse-13.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard-sp/cmse-13.c: This. 
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/hard-sp/cmse-5.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard-sp/cmse-5.c: This. 
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/hard-sp/cmse-7.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard-sp/cmse-7.c: This. 
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/hard-sp/cmse-8.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard-sp/cmse-8.c: This. 
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/hard/cmse-13.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard/cmse-13.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/hard/cmse-5.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard/cmse-5.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/hard/cmse-7.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard/cmse-7.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/hard/cmse-8.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/hard/cmse-8.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/soft/cmse-13.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/soft/cmse-13.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/soft/cmse-5.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/soft/cmse-5.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/soft/cmse-7.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/soft/cmse-7.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/soft/cmse-8.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/soft/cmse-8.c: This.  
Clean up

        dg-skip-if directive for float ABI.

        * gcc.target/arm/cmse/mainline/softfp-sp/cmse-5.c: Move into 
...
        * gcc.target/arm/cmse/mainline/8m/softfp-sp/cmse-5.c: This. 
Clean up

        dg-skip-if directive for float ABI.

        * gcc.target/arm/cmse/mainline/softfp-sp/cmse-7.c: Move into 
...
        * gcc.target/arm/cmse/mainline/8m/softfp-sp/cmse-7.c: This. 
Clean up

        dg-skip-if directive for float ABI.

        * gcc.target/arm/cmse/mainline/softfp-sp/cmse-8.c: Move into 
...
        * gcc.target/arm/cmse/mainline/8m/softfp-sp/cmse-8.c: This. 
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/softfp/cmse-13.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/softfp/cmse-13.c: This. 
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/softfp/cmse-5.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/softfp/cmse-5.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/softfp/cmse-7.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/softfp/cmse-7.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/softfp/cmse-8.c: Move into ...

        * gcc.target/arm/cmse/mainline/8m/softfp/cmse-8.c: This.  
Clean up

        dg-skip-if directive for float ABI.
        * gcc.target/arm/cmse/mainline/union-1.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/union-1.c: This.
        * gcc.target/arm/cmse/mainline/union-2.c: Move into ...
        * gcc.target/arm/cmse/mainline/8m/union-2.c: This.
        * gcc.target/arm/cmse/mainline/8_1m/bitfield-4.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/bitfield-5.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/bitfield-6.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/bitfield-7.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/bitfield-8.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/bitfield-9.c: New file.

        * gcc.target/arm/cmse/mainline/8_1m/bitfield-and-union.c: 
New file.
        * gcc.target/arm/cmse/mainline/8_1m/hard-sp/cmse-13.c: New 
file.

        * gcc.target/arm/cmse/mainline/8_1m/hard-sp/cmse-5.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/hard-sp/cmse-7.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/hard-sp/cmse-8.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/hard/cmse-13.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/hard/cmse-5.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/hard/cmse-7.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/hard/cmse-8.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/soft/cmse-13.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/soft/cmse-5.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/soft/cmse-7.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/soft/cmse-8.c: New file.

        * gcc.target/arm/cmse/mainline/8_1m/softfp-sp/cmse-5.c: New 
file.
        * gcc.target/arm/cmse/mainline/8_1m/softfp-sp/cmse-7.c: New 
file.
        * gcc.target/arm/cmse/mainline/8_1m/softfp-sp/cmse-8.c: New 
file.

        * gcc.target/arm/cmse/mainline/8_1m/softfp/cmse-13.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/softfp/cmse-5.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/softfp/cmse-7.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/softfp/cmse-8.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/union-1.c: New file.
        * gcc.target/arm/cmse/mainline/8_1m/union-2.c: New file.

        * lib/target-supports.exp 
(check_effective_target_arm_cmse_clear_ok):

        New procedure.


Testing: bootstrapped on arm-linux-gnueabihf and arm-none-eabi; 
testsuite shows no

regression.

Is this ok for trunk?


<snip>

+(define_insn "push_fpsysreg_insn"
+  [(set (match_operand:SI 0 "s_register_operand" "+&rk")
+    (plus:SI (match_dup 0) (const_int -4)))
+   (unspec_volatile [(match_operand:SI 1 "const_int_operand" "n")
+             (mem:SI (plus:SI (match_dup 0) (const_int -4)))]
+            VUNSPEC_VSTR_VLDR)]
+  "TARGET_HAVE_FPCXT_CMSE && use_cmse"
+  {
+    static char buf[32];
+    int fp_sysreg_enum = INTVAL (operands[1]);
+
+    gcc_assert (IN_RANGE (fp_sysreg_enum, 0, NB_FP_SYSREGS - 1));
+
+    snprintf (buf, sizeof (buf), \"vstr%%?\\t%s, [%%0, #-4]!\",
+          fp_sysreg_names[fp_sysreg_enum]);
+    return buf;
+  }
+  [(set_attr "predicable" "yes")
+   (set_attr "type" "store_4")]
+)


I'm a bit concerned by the RTL representation here. This is a memory 
store instruction but the MEM operand is just part of an UNSPEC.



Wouldn't this be more conveniently represented as a SET of a MEM to 
an unspec_volatile value where the address of the MEM uses a POST_INC 
addressing mode?



+
+(define_insn "pop_fpsysreg_insn"
+  [(set (match_operand:SI 0 "s_register_operand" "+&rk")
+    (plus:SI (match_dup 0) (const_int 4)))
+   (unspec_volatile:SI [(match_operand:SI 1 "const_int_operand" "n")
+            (mem:SI (match_dup 0))]
+               VUNSPEC_VSTR_VLDR)]
+  "TARGET_HAVE_FPCXT_CMSE && use_cmse"
+  {
+    static char buf[32];
+    int fp_sysreg_enum = INTVAL (operands[1]);
+
+    gcc_assert (IN_RANGE (fp_sysreg_enum, 0, NB_FP_SYSREGS - 1));
+
+    snprintf (buf, sizeof (buf), \"vldr%%?\\t%s, [%%0], #4\",
+          fp_sysreg_names[fp_sysreg_enum]);
+    return buf;
+  }
+  [(set_attr "predicable" "yes")
+   (set_attr "type" "load_4")]
+)



Similarly here, can't we use one of the addressing modes that 
describe the address update?





I've updated the two unspec patterns to represent the operation more 
accurately.



Ok.

Thanks,

Kyrill




Regards,
Mihail


Thanks,

Kyrill



Best regards,

Mihail






















					Reply via email to