KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
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 06/37] KVM: arm64: Only check pending interrupts if it would trap
Date: Fri, 17 Jul 2020 17:21:42 +0100
Message-ID: <87blkexgbt.wl-maz@kernel.org> (raw)
In-Reply-To: <20200715184438.1390996-7-ascull@google.com>

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

  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-15 18:44 [PATCH 00/37] Transform the host into a vCPU Andrew Scull
2020-07-15 18:44 ` [PATCH 01/37] smccc: Make constants available to assembly Andrew Scull
2020-07-15 18:44 ` [PATCH 02/37] KVM: arm64: Move clearing of vcpu debug dirty bit Andrew Scull
2020-07-15 18:44 ` [PATCH 03/37] KVM: arm64: Track running vCPU outside of the CPU context Andrew Scull
2020-07-15 18:44 ` [PATCH 04/37] KVM: arm64: nVHE: Pass pointers consistently to hyp-init Andrew Scull
2020-07-15 18:44 ` [PATCH 05/37] KVM: arm64: nVHE: Break out of the hyp-init idmap Andrew Scull
2020-07-15 18:44 ` [PATCH 06/37] KVM: arm64: Only check pending interrupts if it would trap Andrew Scull
2020-07-17 16:21   ` Marc Zyngier [this message]
2020-07-15 18:44 ` [PATCH 07/37] KVM: arm64: Separate SError detection from VAXorcism Andrew Scull
2020-07-18  9:00   ` Marc Zyngier
2020-07-20 14:13     ` Andrew Scull
2020-07-20 14:56       ` Marc Zyngier
2020-07-23  0:59         ` FW: " Renters Cancellation Requests
2020-07-20 15:40   ` Andrew Scull
2020-07-20 15:57     ` Marc Zyngier
2020-07-15 18:44 ` [PATCH 08/37] KVM: arm64: nVHE: Introduce a hyp run loop for the host Andrew Scull
2020-07-15 18:44 ` [PATCH 09/37] smccc: Cast arguments to unsigned long Andrew Scull
2020-07-15 18:44 ` [PATCH 10/37] KVM: arm64: nVHE: Migrate hyp interface to SMCCC Andrew Scull
2020-07-15 18:44 ` [PATCH 11/37] KVM: arm64: nVHE: Migrate hyp-init " Andrew Scull
2020-07-15 18:44 ` [PATCH 12/37] KVM: arm64: nVHE: Fix pointers during SMCCC convertion Andrew Scull
2020-07-15 18:44 ` [PATCH 13/37] KVM: arm64: Rename workaround 2 helpers Andrew Scull
2020-07-15 18:44 ` [PATCH 14/37] KVM: arm64: nVHE: Use __kvm_vcpu_run for the host vcpu Andrew Scull
2020-07-15 18:44 ` [PATCH 15/37] KVM: arm64: Share some context save and restore macros Andrew Scull
2020-07-15 18:44 ` [PATCH 16/37] KVM: arm64: nVHE: Handle stub HVCs in the host loop Andrew Scull
2020-07-15 18:44 ` [PATCH 17/37] KVM: arm64: nVHE: Store host sysregs in host vcpu Andrew Scull
2020-07-15 18:44 ` [PATCH 18/37] KVM: arm64: nVHE: Access pmu_events directly in kvm_host_data Andrew Scull
2020-07-15 18:44 ` [PATCH 19/37] KVM: arm64: nVHE: Drop host_ctxt argument for context switching Andrew Scull
2020-07-15 18:44 ` [PATCH 20/37] KVM: arm64: nVHE: Use host vcpu context for host debug state Andrew Scull
2020-07-15 18:44 ` [PATCH 21/37] KVM: arm64: Move host debug state from vcpu to percpu Andrew Scull
2020-07-15 18:44 ` [PATCH 22/37] KVM: arm64: nVHE: Store host's mdcr_el2 and hcr_el2 in its vcpu Andrew Scull
2020-07-15 18:44 ` [PATCH 23/37] KVM: arm64: Skip __hyp_panic and go direct to hyp_panic Andrew Scull
2020-07-15 18:44 ` [PATCH 24/37] KVM: arm64: Break apart kvm_host_data Andrew Scull
2020-07-15 18:44 ` [PATCH 25/37] KVM: arm64: nVHE: Unify sysreg state saving paths Andrew Scull
2020-07-15 18:44 ` [PATCH 26/37] KVM: arm64: nVHE: Unify 32-bit sysreg " Andrew Scull
2020-07-15 18:44 ` [PATCH 27/37] KVM: arm64: nVHE: Unify vgic save and restore Andrew Scull
2020-07-15 18:44 ` [PATCH 28/37] KVM: arm64: nVHE: Unify fpexc32 saving paths Andrew Scull
2020-07-15 18:44 ` [PATCH 29/37] KVM: arm64: nVHE: Separate the save and restore of debug state Andrew Scull
2020-07-15 18:44 ` [PATCH 30/37] KVM: arm64: nVHE: Remove MMU assumption in speculative AT workaround Andrew Scull
2020-07-15 18:44 ` [PATCH 31/37] KVM: arm64: Move speculative AT ISBs into context Andrew Scull
2020-07-15 18:44 ` [PATCH 32/37] KVM: arm64: nVHE: Unify sysreg state restoration paths Andrew Scull
2020-07-15 18:44 ` [PATCH 33/37] KVM: arm64: Remove __activate_vm wrapper Andrew Scull
2020-07-15 18:44 ` [PATCH 34/37] KVM: arm64: nVHE: Unify timer restore paths Andrew Scull
2020-07-15 18:44 ` [PATCH 35/37] KVM: arm64: nVHE: Unify PMU event restoration paths Andrew Scull
2020-07-15 18:44 ` [PATCH 36/37] KVM: arm64: nVHE: Unify GIC PMR " Andrew Scull
2020-07-15 18:44 ` [PATCH 37/37] KVM: arm64: Separate save and restore of vcpu trap state Andrew Scull

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87blkexgbt.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=ascull@google.com \
    --cc=kernel-team@android.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git