Add unratified Zicfilp extension #355
Conversation
ved-rivos
force-pushed
the
zicfilp
branch
2 times, most recently
from
c85f8cb
to
cb61fcd
Compare
ved-rivos
force-pushed
the
zicfilp
branch
6 times, most recently
from
3f9b362
to
f4f28a3
Compare
ved-rivos
force-pushed
the
zicfilp
branch
4 times, most recently
from
bbac057
to
fc0b599
Compare
|
Can you please do your development of the feature elsewhere and file the PR when it's actually ready? 11 pushes to the branch used for this PR in the space of the past 24h is way too high, shows the PR as submitted was not at all ready and makes it hard to know when it's actually appropriate to review the PR (nobody wants to waste their time reviewing something that's known to be broken/incomplete or is going to be rewritten...). Not to mention I know have 15 GitHub notifications in my inbox for this PR since it was opened a week ago, all of them from you, which isn't particularly welcome. |
| x1 = x1 // to avoid hook being optimized out | ||
| x1 = x1; // to avoid hook being optimized out | ||
| /* Zicfilp register state */ | ||
| init_zicfilp_regs(); |
There was a problem hiding this comment.
This is the first thing I saw opening the PR. Please read the style guide:
Do not use the ext* types and hooks for standard extensions unless providing a stub implementation; these are reserved for use by out-of-tree extensions that provide their own non-stub implementations
| RISCV_LUI => off, | ||
| RISCV_AUIPC => get_arch_pc() + off | ||
| /* Zicfilp : AUIPC with rd=x0 is a landing pad */ | ||
| if haveZicfilp() & rd == 0b00000 & op == RISCV_AUIPC then { |
There was a problem hiding this comment.
Match it as a separate instruction that takes precedence over AUIPC
|
|
||
| /* extensions */ | ||
| E_Extension(e) => ext_exc_type_to_str(e) | ||
| } | ||
|
|
||
| overload to_str = {exceptionType_to_str} | ||
|
|
||
| /* SW check exception codes */ | ||
| enum SWCheckCodes = {LANDING_PAD_FAULT, SHADOW_STACK_FAULT} |
There was a problem hiding this comment.
Shadow stack exceptions should be added when adding shadow stack support, not as part of landing pad support
| function handle_trap_extension(p : Privilege, pc : xlenbits, u : option(unit)) -> unit = () | ||
| /* Zicfilp : save elp on trap */ | ||
| val zicfilp_trap_extension : (Privilege) -> unit | ||
| function handle_trap_extension(del_priv : Privilege, pc : xlenbits, u : option(unit)) -> unit = { |
There was a problem hiding this comment.
This is one of those ext* functions, just not named quite uniformly like the others
|
Also:
|
Sorry about the inconvenience. Indeed I realized on further testing that more changes were needed and the PR was created a bit prematurely. Thanks for the feedback on the ext functions. I will close this and send a new PR. |
Requesting feedback on the PR.
Zicfilp extension introduces labeled landing pads for forward-edge control flow integrity.
Specification: