Page MenuHomePhabricator

[ARM] CMSE code generation
ClosedPublic

Authored by chill on Mar 20 2020, 11:10 AM.

Details

Summary

This patch implements the final bits of CMSE code generation:

  • emit special linker symbols
  • restrict parameter passing to no use memory
  • emit BXNS and BLXNS instructions for returns from non-secure entry functions, and non-secure function calls, respectively
  • emit code to save/restore secure floating-point state around calls to non-secure functions
  • emit code to save/restore non-secure floating-pointy state upon entry to non-secure entry function, and return to non-secure state
  • emit code to clobber registers not used for arguments and returns when switching to no-secure state

Patch by Momchil Velikov, Bradley Smith, Javed Absar, David Green, possibly others.

Diff Detail

Event Timeline

chill created this revision.Mar 20 2020, 11:10 AM

I'm a little concerned that mid-level optimizations won't understand the semantics of the cmse attributes, and unintentionally break something. Maybe the optimizer can't see both sides of any of the edges that would matter, though?

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
569 ↗(On Diff #251706)

Do we really need to check for both CPSR and APSR, separately?

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1002

Why does the immediate here depend on the subtarget?

llvm/lib/Target/ARM/ARMISelLowering.cpp
2854

What's the semantic issue here? Or is this just complicated enough that you don't want to implement it in the first iteration?

chill marked 3 inline comments as done.Mar 25 2020, 8:24 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
569 ↗(On Diff #251706)

APSR is a required operand to CLRM and CLRM clears it. We need to have *some* PSR operand present, or else the compiler won't notice modification to condition flags. We can't use register ARM::CPSR as it conflicts with r0 (just like APSR used to clash with r1 (cf. https://reviews.llvm.org/D65873).

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1002

0xc00 clears also the GE[3:0] bits in APSR, and this encoding of the instruction is supported only if the DSP extension is implemented.

ostannard added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
2854

The issue here is that normally, the caller would pass a pointer to some memory in it's own stack space which it wants to receive the result in. However, a malicious non-secure caller could pass a pointer to some secure memory, tricking the secure code into overwriting something it shouldn't have access to.

This could be fixed by using the TT instruction to check the permissions on the memory, and abort if it's not non-secure memory, but we haven't implemented that yet.

efriedma added inline comments.Mar 25 2020, 11:07 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
569 ↗(On Diff #251706)

I'm more generally concerned that we aren't representing the subregister relationship between APSR and CPSR in the ARM backend, so we need a special case in every location that's concerned about liveness.

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1002

You need to clear the bits whether or not the current translation unit has the DSP extension enabled, no?

chill marked 3 inline comments as done.Apr 1 2020, 3:44 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1002

Ideally, yes, for the off chance there is another translation unit, compiled for secure state, which uses DSP.
There does not seem to be a way, though.

chill marked 3 inline comments as done.Apr 1 2020, 11:04 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
569 ↗(On Diff #251706)

APSR is a required operand to CLRM and CLRM clears it.

Ugh, that's the second time I confuse CLRM/APSR with VSCCLRM/VPR: no, APSR is not a required operand.

Alternatively, we could add CPSR as an implicit def of CLRM. The "drawback" is that this is too
conservative, since operands might not include APSR. On the other hand, in the only place, where we
generate CLRM, we do add APSR to operands, so it makes no difference in practice.

dmgreen added inline comments.Apr 1 2020, 11:28 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
569 ↗(On Diff #251706)

You can use CLRM to clear multiple registers, even if it's not in the context of a CMSE. It would be an interesting optimisation if more that 2 MOV #0's are in the same block to generate a CLRM instead, a bit like generating LDM's. (That's providing they are implemented efficiently, I've not actually tried it yet).

I'd prefer to leave it open as an option if possible though.

chill updated this revision to Diff 254612.Apr 2 2020, 2:13 PM
chill marked an inline comment as done.
chill marked 5 inline comments as done.Apr 2 2020, 2:34 PM
chill added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
569 ↗(On Diff #251706)

Fair enough.

This last version of the patch adds CPSR as an implicit def only on the specific CLRM instance, the the compiler generates.

Probably not ideal, but I tried the (one?) sub-registers approach and pretty much hit the wall.

chill marked an inline comment as done.Apr 9 2020, 2:02 AM

Ping?

I don't feel like I'm familiar enough with cmse to approve this, but I don't see any other issues.

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1002

I guess that's okay, then.

dmgreen added inline comments.Apr 15 2020, 1:33 AM
llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
990

Formatting

1083

Can you add a comment saying // 20 == CONTROL (?)

Is this jumping needed for correctness on clearing fp contexts? If not, should it be skipped on minsize, just executing the clearing anyway even if it's slower? That doesn't seem like something that needs to be done here in either case. Perhaps one for a later patch.

llvm/lib/Target/ARM/ARMFrameLowering.cpp
449–450

Formatting.

llvm/test/CodeGen/ARM/cmse-clear-float-hard2.ll
17

Some of these files look like they have check lines left over from an earlier set of RUN lines.

chill updated this revision to Diff 257971.Apr 15 2020, 11:59 PM
chill marked 5 inline comments as done.
dmgreen accepted this revision.Apr 26 2020, 5:03 AM

I don't feel like I'm familiar enough with cmse to approve this, but I don't see any other issues.

Thanks for taking a look. The more eyes on this one the better.

I've looked through this again and think it looks OK, from all I know of CMSE. LGTM

This revision is now accepted and ready to land.Apr 26 2020, 5:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2020, 5:03 AM
chill added a comment.Apr 27 2020, 1:47 AM

@efriedma , @dmgreen , thanks, much appreciated!

chill updated this revision to Diff 260990.Apr 29 2020, 12:28 PM

Sorry, noticed a potential issue, as I was preparing the merge. We're replacing a call insn, I think we should move the call site info to the new insn.

chill marked an inline comment as done.May 1 2020, 1:36 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1985

This added line is the last change.

dmgreen added inline comments.May 1 2020, 1:46 AM
llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1985

Do we need to call if (MI.isCandidateForCallSiteEntry())?

chill marked 2 inline comments as done.May 1 2020, 2:02 AM
chill added inline comments.
llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1985

I don't think so, we know exactly that we're replacing a tBXNS_CALL, for which isCandidateForCallSiteEntry ought to return true.

dmgreen accepted this revision.May 1 2020, 3:38 AM

LGTM still, whichever way you choose to go with the call site info.

llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
1985

And do we know that will always remain true in the future? All other uses of moveCallSiteInfo I could find seemed to be guarded by isCandidateForCallSiteEntry, including the two in this file. You could argue that it should be moveCallSiteInfo's responsibility to check that, but what do you think of adding it here just for the sake of consistency?

chill updated this revision to Diff 261467.May 1 2020, 9:05 AM
chill marked an inline comment as done.
chill marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
chill reopened this revision.May 5 2020, 11:06 AM
This revision is now accepted and ready to land.May 5 2020, 11:06 AM
chill planned changes to this revision.May 5 2020, 11:06 AM
chill updated this revision to Diff 262628.May 7 2020, 6:02 AM

The last version of the patch broke compilation with expensive checks - the machine verifier
reported a score of lifetime errors.
In this update, we are more precise with the RegState flags when creating instructions
to save/restore/clear registers around non-secure calls and before return to non-secure state.

This revision is now accepted and ready to land.May 7 2020, 6:02 AM
chill requested review of this revision.May 7 2020, 6:02 AM
dmgreen accepted this revision.May 11 2020, 10:58 PM

Changes LGTM still.

This revision is now accepted and ready to land.May 11 2020, 10:58 PM
This revision was automatically updated to reflect the committed changes.