Re: [PATCH v2, rs6000] Add -msafe-indirect-jumps option and implement safe bctr / bctrl

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi!

On Sat, Jan 13, 2018 at 10:53:57PM -0600, Bill Schmidt wrote:
> This patch adds a new option for the compiler to produce only "safe" indirect
> jumps, in the sense that these jumps are deliberately mispredicted to inhibit
> speculative execution.  For now, this option is undocumented; this may change
> at some future date.  It is intended eventually for the linker to also honor
> this flag when creating PLT stubs, for example.

I think we settled on calling the option -mmispredict-indirect-jumps;
please let me know if you still agree with that.  Or have thought of a
better name :-)

> In addition to the new option, I've included changes to indirect calls for
> the ELFv2 ABI when the option is specified.  In place of bctrl, we generate
> a "crset eq" followed by a beqctrl-.  Using the CR0.eq bit is safe since CR0
> is volatile over the call.

And CR0 is unused by the call; compare to CR1 (on older ABIs) for example.

> I've also added code to replace uses of bctr when the new option is specified,
> with the sequence
> 
> 	crset 4x[CRb]+2
> 	beqctr- CRb
> 	b .
> 
> where CRb is an available condition register field.  This applies to all
> subtargets, and in particular is not restricted to ELFv2.  The use cases
> covered here are computed gotos and switch statements.
> 
> NOT yet covered by this patch: indirect calls for ELFv1.  That will come later.

Would be nice to have it for all ABIs, even.

> Please let me know if there is a better way to represent the crset without
> an unspec.

See the various patterns using cr%q.  I'm not sure they can generate creqv
(i.e. crset) currently, but that could be added (like crnot is there already,
for example).  If you don't use unspec (or maybe unspec_volatile) it can be
optimised away though.

Maybe it is best not to put the crset into its own insn, just make it part
of the bigger pattern, with an appropriate clobber?

> For the indirect jump, I don't see a way around it due to the
> expected form of indirect jumps in cfganal.c.

I'm not sure what you are getting at here, could you explain a bit?

>  (define_expand "indirect_jump"
> -  [(set (pc) (match_operand 0 "register_operand"))])
> +  [(set (pc) (match_operand 0 "register_operand"))]
> + ""
> +{
> +  /* We need to reserve a CR when forcing a mispredicted jump.  */
> +  if (rs6000_safe_indirect_jumps) {
> +    rtx ccreg = gen_reg_rtx (CCmode);
> +    emit_insn (gen_rtx_SET (ccreg,
> +			    gen_rtx_UNSPEC (CCmode,
> +					    gen_rtvec (1, const0_rtx),
> +					    UNSPEC_CRSET_EQ)));
> +    rtvec v = rtvec_alloc (2);
> +    RTVEC_ELT (v, 0) = operands[0];
> +    RTVEC_ELT (v, 1) = ccreg;
> +    emit_jump_insn (gen_rtx_SET (pc_rtx,
> +				 gen_rtx_UNSPEC (Pmode, v,
> +						 UNSPEC_COMP_GOTO_CR)));
> +    DONE;
> +  }
> +})
>  
>  (define_insn "*indirect_jump<mode>"
>    [(set (pc)
>  	(match_operand:P 0 "register_operand" "c,*l"))]
> -  ""
> +  "!rs6000_safe_indirect_jumps"
>    "b%T0"
>    [(set_attr "type" "jmpreg")])
>  
> +(define_insn "*indirect_jump<mode>_safe"
> +  [(set (pc)
> +	(unspec:P [(match_operand:P 0 "register_operand" "c,*l")
> +		   (match_operand:CC 1 "cc_reg_operand" "y,y")]
> +		   UNSPEC_COMP_GOTO_CR))]
> +  "rs6000_safe_indirect_jumps"
> +  "beq%T0- %1\;b ."
> +  [(set_attr "type" "jmpreg")
> +   (set_attr "length" "8")])
> +
> +(define_insn "*set_cr_eq"
> +  [(set (match_operand:CC 0 "cc_reg_operand" "=y")
> +	(unspec:CC [(const_int 0)] UNSPEC_CRSET_EQ))]
> +  "rs6000_safe_indirect_jumps"
> +  "crset %E0"
> +  [(set_attr "type" "cr_logical")])

So merge this latter insn into the previous, making the CC a clobber?
Like (not tested):

+(define_insn "indirect_jump<mode>_mispredict"
+  [(set (pc)
+	(match_operand:P 0 "register_operand" "c,*l")
+   (clobber (match_operand:CC 1 "cc_reg_operand" "y,y"))]
+  "rs6000_safe_indirect_jumps"
+  "crset %E1\;beq%T0- %1\;b ."
+  [(set_attr "type" "jmpreg")
+   (set_attr "length" "12")])

and then change the indirect_jump pattern to simply select between the normal
and mispredict patterns?

> +(define_expand "tablejumpsi_safe"

And then similar for tablejump.


Segher