Re: [PATCH 06/37] KVM: arm64: Only check pending interrupts if it would trap

[Marc Zyngier <maz@kernel.org>]

Hi Andrew,

On Wed, 15 Jul 2020 19:44:07 +0100,
Andrew Scull <ascull@google.com> wrote:
> 
> Allow entry to a vcpu that can handle interrupts if there is an
> interrupts pending. Entry will still be aborted if the vcpu cannot
> handle interrupts.

This is pretty confusing. All vcpus can handle interrupts, it's just
that there are multiple classes of interrupts (physical or virtual).
Instead, this should outline *where* physical interrupt are taken.

Something like:

  When entering a vcpu for which physical interrupts are not taken to
  EL2, don't bother evaluating ISR_EL1 to work out whether we should
  go back to EL2 early. Instead, just enter the guest without any
  further ado. This is done by checking HCR_EL2.IMO bit.

> 
> This allows use of __guest_enter to enter into the host.
> 
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  arch/arm64/kvm/hyp/entry.S | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> index ee32a7743389..6a641fcff4f7 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -73,13 +73,17 @@ SYM_FUNC_START(__guest_enter)
>  	save_sp_el0	x1, x2
>  
>  	// Now the host state is stored if we have a pending RAS SError it must
> -	// affect the host. If any asynchronous exception is pending we defer
> -	// the guest entry. The DSB isn't necessary before v8.2 as any SError
> -	// would be fatal.
> +	// affect the host. If physical IRQ interrupts are going to be trapped
> +	// and there are already asynchronous exceptions pending then we defer
> +	// the entry. The DSB isn't necessary before v8.2 as any SError would
> +	// be fatal.
>  alternative_if ARM64_HAS_RAS_EXTN
>  	dsb	nshst
>  	isb
>  alternative_else_nop_endif
> +	mrs	x1, hcr_el2
> +	and	x1, x1, #HCR_IMO
> +	cbz	x1, 1f

Do we really want to take the overhead of the above DSB/ISB when on
the host? We're not even evaluating ISR_EL1, so what is the gain?

This also assumes that IMO/FMO/AMO are all set together, which
deserves to be documented.

Another thing is that you are also restoring registers that the host
vcpu expects to be corrupted (the caller-saved registers, X0-17).
You probably should just zero them instead if leaking data from EL2 is
your concern. Yes, this is a departure from SMCCC 1.1, but I think
this is a valid one, as EL2 isn't a fully independent piece of SW.
Same goes on the __guest_exit() path. PtrAuth is another concern (I'm
pretty sure this doesn't do what we want, but I haven't tried on a model).

I've hacked the following patch together, which allowed me to claw
back about 10% of the performance loss. I'm pretty sure there are
similar places where you have introduced extra overhead, and we should
hunt them down.

Thanks,

	M.

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 6c3a6b27a96c..2d1a71bd7baa 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -33,6 +33,10 @@ SYM_FUNC_START(__guest_enter)
 	// Save the hyp's sp_el0
 	save_sp_el0	x1, x2
 
+	mrs	x1, hcr_el2
+	and	x1, x1, #HCR_IMO
+	cbz	x1, 2f
+
 	// Now the hyp state is stored if we have a pending RAS SError it must
 	// affect the hyp. If physical IRQ interrupts are going to be trapped
 	// and there are already asynchronous exceptions pending then we defer
@@ -42,9 +46,6 @@ alternative_if ARM64_HAS_RAS_EXTN
 	dsb	nshst
 	isb
 alternative_else_nop_endif
-	mrs	x1, hcr_el2
-	and	x1, x1, #HCR_IMO
-	cbz	x1, 1f
 	mrs	x1, isr_el1
 	cbz	x1,  1f
 	mov	x0, #ARM_EXCEPTION_IRQ
@@ -81,6 +82,31 @@ alternative_else_nop_endif
 	eret
 	sb
 
+2:
+	add	x29, x0, #VCPU_CONTEXT
+
+	// Macro ptrauth_switch_to_guest format:
+	// 	ptrauth_switch_to_guest(guest cxt, tmp1, tmp2, tmp3)
+	// The below macro to restore guest keys is not implemented in C code
+	// as it may cause Pointer Authentication key signing mismatch errors
+	// when this feature is enabled for kernel code.
+	ptrauth_switch_to_guest x29, x0, x1, x2
+
+	// Restore the guest's sp_el0
+	restore_sp_el0 x29, x0
+
+	.irp n,4,5,6,7,8,9,10,11,12,13,14,15,16,17
+	mov	x\n, xzr
+	.endr
+
+	ldp	x0, x1,   [x29, #CPU_XREG_OFFSET(0)]
+	ldp	x2, x3,   [x29, #CPU_XREG_OFFSET(2)]
+
+	// Restore guest regs x18-x29, lr
+	restore_callee_saved_regs x29
+	eret
+	sb
+
 SYM_INNER_LABEL(__guest_exit, SYM_L_GLOBAL)
 	// x0: return code
 	// x1: vcpu
@@ -99,6 +125,11 @@ SYM_INNER_LABEL(__guest_exit, SYM_L_GLOBAL)
 
 	// Store the guest regs x0-x1 and x4-x17
 	stp	x2, x3,   [x1, #CPU_XREG_OFFSET(0)]
+
+	mrs	x2, hcr_el2
+	and	x2, x2, #HCR_IMO
+	cbz	x2, 1f
+
 	stp	x4, x5,   [x1, #CPU_XREG_OFFSET(4)]
 	stp	x6, x7,   [x1, #CPU_XREG_OFFSET(6)]
 	stp	x8, x9,   [x1, #CPU_XREG_OFFSET(8)]
@@ -107,6 +138,7 @@ SYM_INNER_LABEL(__guest_exit, SYM_L_GLOBAL)
 	stp	x14, x15, [x1, #CPU_XREG_OFFSET(14)]
 	stp	x16, x17, [x1, #CPU_XREG_OFFSET(16)]
 
+1:
 	// Store the guest regs x18-x29, lr
 	save_callee_saved_regs x1
 

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