Hi Robin,
On 26/01/18 15:50, Robin Murphy wrote:
> On 26/01/18 14:28, Marc Zyngier wrote:
>> Add the detection and runtime code for ARM_SMCCC_ARCH_WORKAROUND_1.
>> It is lovely. Really.
>>
>> Signed-off-by: Marc Zyngier <marc.zyng...@arm.com>
>> ---
>> arch/arm64/kernel/bpi.S | 20 ++++++++++++
>> arch/arm64/kernel/cpu_errata.c | 71
>> +++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 90 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/bpi.S b/arch/arm64/kernel/bpi.S
>> index 76225c2611ea..add7e08a018d 100644
>> --- a/arch/arm64/kernel/bpi.S
>> +++ b/arch/arm64/kernel/bpi.S
>> @@ -17,6 +17,7 @@
>> */
>>
>> #include <linux/linkage.h>
>> +#include <linux/arm-smccc.h>
>>
>> .macro ventry target
>> .rept 31
>> @@ -85,3 +86,22 @@ ENTRY(__qcom_hyp_sanitize_link_stack_start)
>> .endr
>> ldp x29, x30, [sp], #16
>> ENTRY(__qcom_hyp_sanitize_link_stack_end)
>> +
>> +.macro smccc_workaround_1 inst
>> + sub sp, sp, #(8 * 4)
>> + stp x2, x3, [sp, #(16 * 0)]
>
> This seems unnecessarily confusing - using either units of registers, or
> of register pairs, is fine, but mixing both in the same sequence just
> hurts more than it needs to.
Point taken.
>
>> + stp x0, x1, [sp, #(16 * 1)]
>> + orr w0, wzr, #ARM_SMCCC_ARCH_WORKAROUND_1
>
> Writing this as a MOV like a sane person would make things 0.37% more
> lovely, I promise ;)
But I swear that's what the assembler actually generates! Do I still get
the additional loveliness? ;-) I'll MOV it up.
>
>> + \inst #0
>> + ldp x2, x3, [sp, #(16 * 0)]
>> + ldp x0, x1, [sp, #(16 * 1)]
>> + add sp, sp, #(8 * 4)
>> +.endm
>> +
>> +ENTRY(__smccc_workaround_1_smc_start)
>> + smccc_workaround_1 smc
>> +ENTRY(__smccc_workaround_1_smc_end)
>> +
>> +ENTRY(__smccc_workaround_1_hvc_start)
>> + smccc_workaround_1 hvc
>> +ENTRY(__smccc_workaround_1_hvc_end)
>
> That said, should we not be implementing this lot in smccc-call.S...
Wouldn't work. We *copy* that code in the KVM vectors, see
__install_bp_hardening_cb and __copy_hyp_vect_bpi...
Yes, I know.
>
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index ed6881882231..f1501873f2e4 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -70,6 +70,10 @@ DEFINE_PER_CPU_READ_MOSTLY(struct bp_hardening_data,
>> bp_hardening_data);
>> extern char __psci_hyp_bp_inval_start[], __psci_hyp_bp_inval_end[];
>> extern char __qcom_hyp_sanitize_link_stack_start[];
>> extern char __qcom_hyp_sanitize_link_stack_end[];
>> +extern char __smccc_workaround_1_smc_start[];
>> +extern char __smccc_workaround_1_smc_end[];
>> +extern char __smccc_workaround_1_hvc_start[];
>> +extern char __smccc_workaround_1_hvc_end[];
>>
>> static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start,
>> const char *hyp_vecs_end)
>> @@ -116,6 +120,10 @@ static void __install_bp_hardening_cb(bp_hardening_cb_t
>> fn,
>> #define __psci_hyp_bp_inval_end NULL
>> #define __qcom_hyp_sanitize_link_stack_start NULL
>> #define __qcom_hyp_sanitize_link_stack_end NULL
>> +#define __smccc_workaround_1_smc_start NULL
>> +#define __smccc_workaround_1_smc_end NULL
>> +#define __smccc_workaround_1_hvc_start NULL
>> +#define __smccc_workaround_1_hvc_end NULL
>>
>> static void __install_bp_hardening_cb(bp_hardening_cb_t fn,
>> const char *hyp_vecs_start,
>> @@ -142,17 +150,78 @@ static void install_bp_hardening_cb(const struct
>> arm64_cpu_capabilities *entry,
>> __install_bp_hardening_cb(fn, hyp_vecs_start, hyp_vecs_end);
>> }
>>
>> +#include <uapi/linux/psci.h>
>> +#include <linux/arm-smccc.h>
>> #include <linux/psci.h>
>>
>> +static void call_smc_arch_workaround_1(void)
>> +{
>> + register int w0 asm("w0") = ARM_SMCCC_ARCH_WORKAROUND_1;
>> + asm volatile("smc #0\n"
>> + : "+r" (w0));
>> +}
>> +
>> +static void call_hvc_arch_workaround_1(void)
>> +{
>> + register int w0 asm("w0") = ARM_SMCCC_ARCH_WORKAROUND_1;
>> + asm volatile("hvc #0\n"
>> + : "+r" (w0));
>> +}
>
> ...such that these could simply be something like:
>
> static void call_{smc,hvc}_arch_workaround_1(void)
> {
> arm_smccc_v1_1_{smc,hvc}(ARM_SMCCC_ARCH_WORKAROUND_1);
> }
>
> ?
That we could do. And maybe define them inline in arm-smccc.h so that we
don't get any extra call (we'd just need a way to declare x0-x3 as being
clobbered).
I'll have a go at it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
