Re: [PATCH 07/37] KVM: arm64: Separate SError detection from VAXorcism

From: Marc Zyngier <maz@kernel.org>
To: Andrew Scull <ascull@google.com>
Cc: kernel-team@android.com, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 07/37] KVM: arm64: Separate SError detection from VAXorcism
Date: Sat, 18 Jul 2020 10:00:30 +0100	[thread overview]
Message-ID: <87a6zxxknl.wl-maz@kernel.org> (raw)
In-Reply-To: <20200715184438.1390996-8-ascull@google.com>

Hi Andrew,

On Wed, 15 Jul 2020 19:44:08 +0100,
Andrew Scull <ascull@google.com> wrote:
> 
> When exiting a guest, just check whether there is an SError pending and
> set the bit in the exit code. The fixup then initiates the ceremony
> should it be required.
> 
> The separation allows for easier choices to be made as to whether the
> demonic consultation should proceed.

Such as?

> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  arch/arm64/include/asm/kvm_hyp.h        |  2 ++
>  arch/arm64/kvm/hyp/entry.S              | 27 +++++++++++++++++--------
>  arch/arm64/kvm/hyp/hyp-entry.S          |  1 -
>  arch/arm64/kvm/hyp/include/hyp/switch.h |  4 ++++
>  4 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 07745d9c49fc..50a774812761 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -91,6 +91,8 @@ void deactivate_traps_vhe_put(void);
>  
>  u64 __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host_ctxt);
>  
> +void __vaxorcize_serror(void);

I think a VAXorsist reference in the comments is plenty. The code can
definitely stay architectural. Something like "__handle_SEI()" should
be good. I'm not *that* fun.

> +
>  void __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt);
>  #ifdef __KVM_NVHE_HYPERVISOR__
>  void __noreturn __hyp_do_panic(unsigned long, ...);
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index 6a641fcff4f7..dc4e3e7e7407 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -174,18 +174,31 @@ alternative_if ARM64_HAS_RAS_EXTN
>  	mrs_s	x2, SYS_DISR_EL1
>  	str	x2, [x1, #(VCPU_FAULT_DISR - VCPU_CONTEXT)]
>  	cbz	x2, 1f
> -	msr_s	SYS_DISR_EL1, xzr
>  	orr	x0, x0, #(1<<ARM_EXIT_WITH_SERROR_BIT)
> -1:	ret
> +	nop
> +1:
>  alternative_else
>  	dsb	sy		// Synchronize against in-flight ld/st
>  	isb			// Prevent an early read of side-effect free ISR
>  	mrs	x2, isr_el1
> -	tbnz	x2, #8, 2f	// ISR_EL1.A
> -	ret
> -	nop
> +	tbz	x2, #8, 2f	// ISR_EL1.A
> +	orr	x0, x0, #(1<<ARM_EXIT_WITH_SERROR_BIT)
>  2:
>  alternative_endif
> +
> +	ret
> +SYM_FUNC_END(__guest_enter)
> +
> +/*
> + * void __vaxorcize_serror(void);
> + */
> +SYM_FUNC_START(__vaxorcize_serror)
> +
> +alternative_if ARM64_HAS_RAS_EXTN
> +	msr_s	SYS_DISR_EL1, xzr
> +	ret
> +alternative_else_nop_endif
> +
>  	// We know we have a pending asynchronous abort, now is the
>  	// time to flush it out. From your VAXorcist book, page 666:
>  	// "Threaten me not, oh Evil one!  For I speak with
> @@ -193,7 +206,6 @@ alternative_endif
>  	mrs	x2, elr_el2
>  	mrs	x3, esr_el2
>  	mrs	x4, spsr_el2
> -	mov	x5, x0
>  
>  	msr	daifclr, #4	// Unmask aborts
>  
> @@ -217,6 +229,5 @@ abort_guest_exit_end:
>  	msr	elr_el2, x2
>  	msr	esr_el2, x3
>  	msr	spsr_el2, x4
> -	orr	x0, x0, x5
>  1:	ret
> -SYM_FUNC_END(__guest_enter)
> +SYM_FUNC_END(__vaxorcize_serror)
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index e727bee8e110..c441aabb8ab0 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -177,7 +177,6 @@ el2_error:
>  	adr	x1, abort_guest_exit_end
>  	ccmp	x0, x1, #4, ne
>  	b.ne	__hyp_panic
> -	mov	x0, #(1 << ARM_EXIT_WITH_SERROR_BIT)
>  	eret
>  	sb
>  
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 0511af14dc81..14a774d1a35a 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -405,6 +405,10 @@ static inline bool __hyp_handle_ptrauth(struct kvm_vcpu *vcpu)
>   */
>  static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
>  {
> +	/* Flush guest SErrors. */
> +	if (ARM_SERROR_PENDING(*exit_code))
> +		__vaxorcize_serror();
> +
>  	if (ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ)
>  		vcpu->arch.fault.esr_el2 = read_sysreg_el2(SYS_ESR);
>  
> -- 
> 2.27.0.389.gc38d7665816-goog
> 
> 

I'm not against this patch, but I wonder whether it actually helps
with anything. It spreads the handling across multiple paths, making
it harder to read. Could you explain the rational beyond "it's
easier"?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm