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

[Andrew Scull <ascull@google.com>]

On Sat, Jul 18, 2020 at 10:00:30AM +0100, Marc Zyngier wrote:
> 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?

It's used in the next patch to keep host SErrors pending and left for
the host to handle when reentering the host vcpu. IIUC, this matches the
previous behaviour since hyp would mask SErrors.

We wanted to avoid the need to convert host SErrors into virtual ones
and I opted for this approach to keep as much of the logic/policy as
possible in C.

Let me know if you'd prefer a different split of the patches or there
should be different design goals.

> > 
> > 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.

Fine... ;)

> > +
> >  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"?

The earlier reply should cover most of this. I claim it's easier to make
choices as the predicate isn't stuck in assembly. Maybe others feel
differently and I should use less provocative language.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm