Re: [PATCH v1 2/6] KVM: arm64: Consume pending SError as early as possible

[Julien Thierry <julien.thierry@arm.com>]

Hi James,

On 04/06/2019 15:45, James Morse wrote:
> On systems with v8.2 we switch the 'vaxorcism' of guest SError with an
> alternative sequence that uses the ESB-instruction, then reads DISR_EL1.
> This saves the unmasking and re-masking of asynchronous exceptions.
> 
> We do this after we've saved the guest registers and restored the
> host's. Any SError that becomes pending due to this will be accounted
> to the guest, when it actually occurred during host-execution.
> 
> Move the ESB-instruction as early as possible. Any guest SError
> will become pending due to this ESB-instruction and then consumed to
> DISR_EL1 before the host touches anything.
> 

Since you're moving the ESB from a HAS_RAS alternative location to a
normal location, it might be worth noting in the commit message that the
ESB is a NOP when RAS is not implemented, to clarify that we are not
uselessly adding a barrier (or potentially undefined instruction).

> This lets us account for host/guest SError precisely on the guest
> exit exception boundary.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> N.B. ESB-instruction is a nop on CPUs that don't support it.
> 
>  arch/arm64/include/asm/kvm_asm.h | 2 +-
>  arch/arm64/kvm/hyp/entry.S       | 5 ++---
>  arch/arm64/kvm/hyp/hyp-entry.S   | 2 ++
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 9170c43b332f..5c9548ae8fa7 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -45,7 +45,7 @@
>   * Size of the HYP vectors preamble. kvm_patch_vector_branch() generates code
>   * that jumps over this.
>   */
> -#define KVM_VECTOR_PREAMBLE	4
> +#define KVM_VECTOR_PREAMBLE	8
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 93ba3d7ef027..7863ec5266e2 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -138,8 +138,8 @@ ENTRY(__guest_exit)
>  
>  alternative_if ARM64_HAS_RAS_EXTN
>  	// If we have the RAS extensions we can consume a pending error
> -	// without an unmask-SError and isb.
> -	esb
> +	// without an unmask-SError and isb. The ESB-instruction consumed any
> +	// pending guest error when we took the exception from the guest.
>  	mrs_s	x2, SYS_DISR_EL1
>  	str	x2, [x1, #(VCPU_FAULT_DISR - VCPU_CONTEXT)]
>  	cbz	x2, 1f
> @@ -157,7 +157,6 @@ alternative_else
>  	mov	x5, x0
>  
>  	dsb	sy		// Synchronize against in-flight ld/st
> -	nop
>  	msr	daifclr, #4	// Unmask aborts
>  alternative_endif
>  
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index 914036e6b6d7..b8d37a987b34 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -230,6 +230,7 @@ ENDPROC(\label)
>  .macro valid_vect target
>  	.align 7
>  661:
> +	esb

Having said the above, if the kernel is built without RAS support (you
have to disable some of options enabled by default to get to that) but
runs on a CPU that does have the RAS extention, should we execute a nop
instead of an esb (so have an alternative here)?

Also, when we have the smccc workaround installed we do two esb, is that
intentional/necessary?

Could we have only one esb at the start of hyp_ventry (and "only" 26
nops after it) for KVM_INDIRECT_VECTORS? Or does this not affect
performance that much to be of interest?

>  	stp	x0, x1, [sp, #-16]!
>  662:
>  	b	\target
> @@ -320,6 +321,7 @@ ENTRY(__bp_harden_hyp_vecs_end)
>  	.popsection
>  
>  ENTRY(__smccc_workaround_1_smc_start)
> +	esb
>  	sub	sp, sp, #(8 * 4)
>  	stp	x2, x3, [sp, #(8 * 0)]
>  	stp	x0, x1, [sp, #(8 * 2)]
> 

Thanks,

-- 
Julien Thierry
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm