Re: [PATCH 13/14] arm64: KVM: Handle guest's ARCH_WORKAROUND_2 requests

[Mark Rutland]

On Tue, May 22, 2018 at 04:06:47PM +0100, Marc Zyngier wrote:
> In order to forward the guest's ARCH_WORKAROUND_2 calls to EL3,
> add a small(-ish) sequence to handle it at EL2. Special care must
> be taken to track the state of the guest itself by updating the
> workaround flags. We also rely on patching to enable calls into
> the firmware.
> 
> Note that since we need to execute branches, this always executes
> after the Spectre-v2 mitigation has been applied.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
>  arch/arm64/kernel/asm-offsets.c |  1 +
>  arch/arm64/kvm/hyp/hyp-entry.S  | 38 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 5bdda651bd05..323aeb5f2fe6 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -136,6 +136,7 @@ int main(void)
>  #ifdef CONFIG_KVM_ARM_HOST
>    DEFINE(VCPU_CONTEXT,		offsetof(struct kvm_vcpu, arch.ctxt));
>    DEFINE(VCPU_FAULT_DISR,	offsetof(struct kvm_vcpu, arch.fault.disr_el1));
> +  DEFINE(VCPU_WORKAROUND_FLAGS,	offsetof(struct kvm_vcpu, arch.workaround_flags));
>    DEFINE(CPU_GP_REGS,		offsetof(struct kvm_cpu_context, gp_regs));
>    DEFINE(CPU_USER_PT_REGS,	offsetof(struct kvm_regs, regs));
>    DEFINE(CPU_FP_REGS,		offsetof(struct kvm_regs, fp_regs));
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index bffece27b5c1..5b1fa37ca1f4 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -106,8 +106,44 @@ el1_hvc_guest:
>  	 */
>  	ldr	x1, [sp]				// Guest's x0
>  	eor	w1, w1, #ARM_SMCCC_ARCH_WORKAROUND_1
> +	cbz	w1, wa_epilogue
> +
> +	/* ARM_SMCCC_ARCH_WORKAROUND_2 handling */
> +	eor	w1, w1, #(ARM_SMCCC_ARCH_WORKAROUND_1 ^ \
> +			  ARM_SMCCC_ARCH_WORKAROUND_2)

... that took me a second. Lovely. :)

>  	cbnz	w1, el1_trap
> -	mov	x0, x1
> +
> +#ifdef CONFIG_ARM64_SSBD
> +alternative_cb	arm64_enable_wa2_handling
> +	b	wa2_end
> +alternative_cb_end
> +	get_vcpu_ptr	x2, x0
> +	ldr	x0, [x2, #VCPU_WORKAROUND_FLAGS]
> +
> +	/* Sanitize the argument and update the guest flags*/

Nit: space before the trailing '*/'. Either that or use a '//' comment.

Otherwise, this looks fine, so with that fixed:

Reviewed-by: Mark Rutland <mark.rutland@xxxxxxx>

Mark.

> +	ldr	x1, [sp, #8]			// Guest's x1
> +	clz	w1, w1				// Murphy's device:
> +	lsr	w1, w1, #5			// w1 = !!w1 without using
> +	eor	w1, w1, #1			// the flags...
> +	bfi	x0, x1, #VCPU_WORKAROUND_2_FLAG_SHIFT, #1
> +	str	x0, [x2, #VCPU_WORKAROUND_FLAGS]
> +
> +	/* Check that we actually need to perform the call */
> +	hyp_ldr_this_cpu x0, arm64_ssbd_callback_required, x2
> +	cbz	x0, wa2_end
> +
> +	mov	w0, #ARM_SMCCC_ARCH_WORKAROUND_2
> +	smc	#0
> +
> +	/* Don't leak data from the SMC call */
> +	mov	x3, xzr
> +wa2_end:
> +	mov	x2, xzr
> +	mov	x1, xzr
> +#endif
> +
> +wa_epilogue:
> +	mov	x0, xzr
>  	add	sp, sp, #16
>  	eret
>  
> -- 
> 2.14.2
>