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

[Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>]

Hi Segher,

Thanks for the quick review!

> On Jan 15, 2018, at 10:38 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> 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 :-)

Looks like we are now looking at -m[no-]speculate-indirect-jumps with
default to true.  LLVM folks are in agreement too.
> 
>> 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.

Yeah, that's the plan.  Everything that uses bctr[l].  I used too loose of language.
> 
>> 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?

An indirect jump is expected to be of the form (set (pc) (...other stuff));
otherwise it might get missed and blocks that are only reachable from
an indirect jump can get deleted.  I found this out when I tried to do
something involving a parallel that was not very bright; that doesn't
actually prevent anything if you are doing things right.  And the
clobber solution you suggest should be just fine.

tldr:  Ignore my lunatic ravings. ;-)

Thanks,
Bill
> 
>> (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
>