Matthew Malcomson <matthew.malcom...@arm.com> writes:
> @@ -14466,6 +14466,81 @@ aarch64_validate_mcpu (const char *str, const struct
> processor **res,
> return false;
> }
>
> +
Should just be one blank line here.
> +/* Straight line speculation indicators. */
> +enum aarch64_sls_hardening_type
> +{
> + SLS_NONE = 0,
> + SLS_RETBR = 1,
> + SLS_BLR = 2,
> + SLS_ALL = 3,
Just indent by two spaces rather than four.
> +};
> +static enum aarch64_sls_hardening_type aarch64_sls_hardening;
Maybe easier to read with a line break here.
> +/* Return whether we should mitigatate Straight Line Speculation for the RET
> + and BR instructions. */
> +bool
> +aarch64_harden_sls_retbr_p (void)
> +{
> + return aarch64_sls_hardening & SLS_RETBR;
> +}
…and here.
> +/* Return whether we should mitigatate Straight Line Speculation for the RET
> + and BR instructions. */
> +bool
> +aarch64_harden_sls_blr_p (void)
> +{
> + return aarch64_sls_hardening & SLS_BLR;
> +}
Pasto: returns true for BLR speculation instead of RET + BR.
> +
> +/* As of yet we only allow setting these options globally, in the future we
> may
> + allow setting them per function. */
> +static void
> +aarch64_validate_sls_mitigation (const char *const_str)
> +{
> + char *str_root = xstrdup (const_str);
> + char *token_save = NULL;
> + char *str = NULL;
> + int temp = SLS_NONE;
> +
> + aarch64_sls_hardening = SLS_NONE;
> + if (strcmp (str_root, "none") == 0)
> + goto finish;
In Clang I think this would override any previous option, so should
we set aarch64_sls_hardening to 0?
> + if (strcmp (str_root, "all") == 0)
> + {
> + aarch64_sls_hardening = SLS_ALL;
> + goto finish;
> + }
> +
> + str = strtok_r (str_root, ",", &token_save);
> + if (!str)
> + {
> + error ("invalid argument given to %<-mharden-sls=%>");
> + goto finish;
> + }
I'm not particularly anti-goto, but in this case it looks simpler
to do the full-string comparisons on const_str and only duplicate
the string before the strtok_r.
> + while (str)
> + {
> + if (strcmp (str, "blr") == 0)
> + temp |= SLS_BLR;
> + else if (strcmp (str, "retbr") == 0)
> + temp |= SLS_RETBR;
> + else if (strcmp (str, "none") == 0 || strcmp (str, "all") == 0)
> + {
> + error ("%<%s%> must be by itself for %<-mharden-sls=%>", str);
> + break;
> + }
> + else
> + {
> + error ("invalid argument %<%s%> for %<-mharden-sls=%>", str);
> + break;
> + }
> + str = strtok_r (NULL, ",", &token_save);
> + }
> + aarch64_sls_hardening = (aarch64_sls_hardening_type) temp;
> +finish:
> + free (str_root);
> + return;
> +}
Think it's more usual in gcc not to have explicit end-of-function void
returns.
> /* Parses CONST_STR for branch protection features specified in
> aarch64_branch_protect_types, and set any global variables required.
> Returns
> the parsing result and assigns LAST_STR to the last processed token from
> @@ -14710,6 +14785,9 @@ aarch64_override_options (void)
> selected_arch = NULL;
> selected_tune = NULL;
>
> + if (aarch64_harden_sls_string)
> + aarch64_validate_sls_mitigation (aarch64_harden_sls_string);
Last line is indented two spaces too many.
> +
> if (aarch64_branch_protection_string)
> aarch64_validate_mbranch_protection (aarch64_branch_protection_string);
>
> diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt
> index
> d99d14c137d8774d3c8dab860d475f68c01a2817..5170361fd5e5721e044d1664e522b2718f654b8e
> 100644
> --- a/gcc/config/aarch64/aarch64.opt
> +++ b/gcc/config/aarch64/aarch64.opt
> @@ -71,6 +71,10 @@ mgeneral-regs-only
> Target Report RejectNegative Mask(GENERAL_REGS_ONLY) Save
> Generate code which uses only the general registers.
>
> +mharden-sls=
> +Target RejectNegative Joined Var(aarch64_harden_sls_string)
> +Generate code to mitigate against straight line speculation.
> +
> mfix-cortex-a53-835769
> Target Report Var(aarch64_fix_a53_err835769) Init(2) Save
> Workaround for ARM Cortex-A53 Erratum number 835769.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index
> 35e8242af5fa4c52744fd2c3e2cfee0a617e22bb..8a3fab2964c9bb06c820766d284768751d63ac9a
> 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -696,6 +696,7 @@ Objective-C and Objective-C++ Dialects}.
> -msign-return-address=@var{scope} @gol
> -mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf}
> +@var{b-key}]|@var{bti} @gol
> +-mharden-sls=@var{none}|@var{all}|@var{retbr}|@var{blr} @gol
> -march=@var{name} -mcpu=@var{name} -mtune=@var{name} @gol
> -moverride=@var{string} -mverbose-cost-dump @gol
> -mstack-protector-guard=@var{guard} -mstack-protector-guard-reg=@var{sysreg}
> @gol
> @@ -17045,6 +17046,15 @@ functions. The optional argument @samp{b-key} can
> be used to sign the functions
> with the B-key instead of the A-key.
> @samp{bti} turns on branch target identification mechanism.
>
> +@item -mharden-sls=@var{none}|@var{all}|@var{retbr}|@var{blr}
> +@opindex mharden-sls
> +Enable compiler hardening against straight line speculation (SLS).
> +There are two options for hardening against straight line speculation.
> +@samp{retbr} allows inserting speculation barriers after every
> +@samp{br} and @samp{ret} instruction. While @samp{blr} enables replacing
> +@samp{blr} instructions with a @samp{bl} to a function stub.
> +@samp{all} enables all SLS hardening, while @samp{none} does not enable any.
OK, so this is even more picky, sorry, but the syntax and description
imply to me that you can choose only one of the four options. I think
it would be more accurate to say something like:
@item -mharden-sls=@var{opts}
@opindex mharden-sls
Enable compiler hardening against straight line speculation (SLS).
@var{opts} is a comma-separated list of the following options:
@table @samp
@item retbr
…
@item blr
…
@end table
In addition, @samp{-mharden-sls=all} enables all SLS hardening
while @samp{-mharden-sls=none} disables all SLS hardening.
(assuming the above behaviour change for “none”)
Thanks,
Richard
