Hi Segher,
Thanks for the quick review!
> On Jan 15, 2018, at 10:38 AM, Segher Boessenkool <seg...@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
>
