Re: [PATCH 13/13] arm64: KVM: VHE: Early interrupt handling

[Christoffer Dall <christoffer.dall@linaro.org>]

On Wed, Jul 08, 2015 at 05:19:16PM +0100, Marc Zyngier wrote:
> With VHE enabled, it is possible to let the kernel handle an interrupt
> without saving the full guest context, and without restoring the full
> host context either. This reduces the latency of handling an interrupt.
> 
> When an interrupt fires we can:
> - save the guest's general purpose registers, shared system registers
>   and timer state
> - switch to a host context (setting vectors, deactivating traps and
>   stage-2 translation)
> - restore the host's shared sysregs
> 
> At that stage, we're able to jump to the interrupt handler.
> 
> On return from the handler, we can finish the switch (save/restore
> whatever is missing from the above).

This feels like a specific optimization, and again it looks to me like
we're just going to see more of this.

For example, sending a virtual IPI shoud ideally be done without having
to do all the crazy save/restore stuff.

Maybe I'm missing something overall here, but I feel the whole design of
this solution requires some justification in the cover letter.

Thanks,
-Christoffer

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/arm.c              |  3 +++
>  arch/arm64/kvm/hyp.S            | 56 ++++++++++++++++++++++++++++++++++++++---
>  arch/arm64/kvm/vgic-v2-switch.S | 19 +++++++++++---
>  arch/arm64/kvm/vgic-v3-switch.S | 33 +++++++++++++-----------
>  4 files changed, 90 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index ca3c662..ab9333f 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -573,6 +573,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		 * disabled.  Enabling the interrupts now will have
>  		 * the effect of taking the interrupt again, in SVC
>  		 * mode this time.
> +		 *
> +		 * With VHE, the interrupt is likely to have been
> +		 * already taken already.
>  		 */
>  		local_irq_enable();
>  
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 3cbd2c4..ac95ba3 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -714,8 +714,10 @@ skip_el1_restore:
>  .endm
>  
>  .macro skip_32bit_state tmp, target
> -	// Skip 32bit state if not needed
> -	mrs	\tmp, hcr_el2
> +	// Skip 32bit state if not needed.
> +	// With VHE, we may have cleared the RW bit in HCR_EL2 early,
> +	// and have to rely on the memory version instead.
> +ifnvhe "mrs	\tmp, hcr_el2",			_S_(ldr	\tmp, [x0, #VCPU_HCR_EL2])
>  	tbnz	\tmp, #HCR_RW_SHIFT, \target
>  .endm
>  
> @@ -1053,11 +1055,18 @@ __kvm_vcpu_return:
>  	save_guest_32bit_state
>  
>  	save_timer_state
> +ifnvhe "b	1f",		nop
> +	alternative_insn	"bl __vgic_v2_disable", \
> +				"bl __vgic_v3_disable", \
> +				ARM64_HAS_SYSREG_GIC_CPUIF
> +1:
>  	save_vgic_state
>  
>  	deactivate_traps
>  	deactivate_vm
>  
> +__kvm_vcpu_return_irq:
> +
>  	// Host context
>  	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
>  	kern_hyp_va x2
> @@ -1355,8 +1364,47 @@ el1_irq:
>  	push	x0, x1
>  	push	x2, x3
>  	mrs	x0, tpidr_el2
> -	mov	x1, #ARM_EXCEPTION_IRQ
> -	b	__kvm_vcpu_return
> +ifnvhe _S_(mov	x1, #ARM_EXCEPTION_IRQ),	nop
> +ifnvhe "b	__kvm_vcpu_return", 		nop
> +
> +	// Fallthrough for VHE
> +	add	x2, x0, #VCPU_CONTEXT
> +
> +	save_guest_regs
> +	bl	__save_shared_sysregs
> +	save_timer_state
> +	alternative_insn	"bl __vgic_v2_disable", \
> +				"bl __vgic_v3_disable", \
> +				ARM64_HAS_SYSREG_GIC_CPUIF
> +	deactivate_traps
> +	deactivate_vm
> +
> +	isb	// Needed to ensure TGE is on and vectors have been switched
> +
> +	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
> +	restore_shared_sysregs
> +
> +	mov	x0, x2
> +	adrp	x1, handle_arch_irq
> +	add	x1, x1, #:lo12:handle_arch_irq
> +	ldr	x1, [x1]
> +	blr	x1
> +
> +	// Back from the interrupt handler, finalize the world switch
> +	mrs	x0, tpidr_el2
> +	add	x2, x0, #VCPU_CONTEXT
> +
> +	bl	__save_fpsimd
> +	bl	__save_sysregs	// We've already saved the shared regs
> +	save_guest_32bit_state
> +
> +	skip_debug_state x3, 1f
> +	bl	__save_debug
> +1:
> +	save_vgic_state
> +
> +	mov	x1,	#ARM_EXCEPTION_IRQ
> +	b	__kvm_vcpu_return_irq
>  
>  	.ltorg
>  
> diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
> index 3f00071..c8864b4 100644
> --- a/arch/arm64/kvm/vgic-v2-switch.S
> +++ b/arch/arm64/kvm/vgic-v2-switch.S
> @@ -26,16 +26,30 @@
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_mmu.h>
>  
> +#include "vhe-macros.h"
> +
>  	.text
>  	.pushsection	.hyp.text, "ax"
>  
> +ENTRY(__vgic_v2_disable)
> +	/* Get VGIC VCTRL base into x2 */
> +	ldr	x2, [x0, #VCPU_KVM]
> +	kern_hyp_va	x2
> +	ldr	x2, [x2, #KVM_VGIC_VCTRL]
> +	kern_hyp_va	x2
> +	cbz	x2, 1f		// disabled
> +
> +	/* Clear GICH_HCR */
> +	str	wzr, [x2, #GICH_HCR]
> +1:	ret
> +ENDPROC(__vgic_v2_disable)
> +
>  /*
>   * Save the VGIC CPU state into memory
>   * x0: Register pointing to VCPU struct
>   * Do not corrupt x1!!!
>   */
>  ENTRY(__save_vgic_v2_state)
> -__save_vgic_v2_state:
>  	/* Get VGIC VCTRL base into x2 */
>  	ldr	x2, [x0, #VCPU_KVM]
>  	kern_hyp_va	x2
> @@ -75,7 +89,7 @@ CPU_BE(	str	w10, [x3, #VGIC_V2_CPU_ELRSR] )
>  	str	w11, [x3, #VGIC_V2_CPU_APR]
>  
>  	/* Clear GICH_HCR */
> -	str	wzr, [x2, #GICH_HCR]
> +ifnvhe	_S_(str	wzr, [x2, #GICH_HCR]),		nop
>  
>  	/* Save list registers */
>  	add	x2, x2, #GICH_LR0
> @@ -95,7 +109,6 @@ ENDPROC(__save_vgic_v2_state)
>   * x0: Register pointing to VCPU struct
>   */
>  ENTRY(__restore_vgic_v2_state)
> -__restore_vgic_v2_state:
>  	/* Get VGIC VCTRL base into x2 */
>  	ldr	x2, [x0, #VCPU_KVM]
>  	kern_hyp_va	x2
> diff --git a/arch/arm64/kvm/vgic-v3-switch.S b/arch/arm64/kvm/vgic-v3-switch.S
> index 3c20730..b9b45e3 100644
> --- a/arch/arm64/kvm/vgic-v3-switch.S
> +++ b/arch/arm64/kvm/vgic-v3-switch.S
> @@ -25,6 +25,8 @@
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_arm.h>
>  
> +#include "vhe-macros.h"
> +
>  	.text
>  	.pushsection	.hyp.text, "ax"
>  
> @@ -35,11 +37,21 @@
>  #define LR_OFFSET(n)	(VGIC_V3_CPU_LR + (15 - n) * 8)
>  
>  /*
> + * Disable the vcpu interface, preventing interrupts from
> + * being delivered.
> + */
> +ENTRY(__vgic_v3_disable)
> +	// Nuke the HCR register.
> +	msr_s	ICH_HCR_EL2, xzr
> +	ret
> +ENDPROC(__vgic_v3_disable)
> +
> +/*
>   * Save the VGIC CPU state into memory
>   * x0: Register pointing to VCPU struct
>   * Do not corrupt x1!!!
>   */
> -.macro	save_vgic_v3_state
> +ENTRY(__save_vgic_v3_state)
>  	// Compute the address of struct vgic_cpu
>  	add	x3, x0, #VCPU_VGIC_CPU
>  
> @@ -58,7 +70,7 @@
>  	str	w7, [x3, #VGIC_V3_CPU_EISR]
>  	str	w8, [x3, #VGIC_V3_CPU_ELRSR]
>  
> -	msr_s	ICH_HCR_EL2, xzr
> +ifnvhe	_S_(msr_s	ICH_HCR_EL2, xzr),	nop
>  
>  	mrs_s	x21, ICH_VTR_EL2
>  	mvn	w22, w21
> @@ -137,15 +149,17 @@
>  	orr	x5, x5, #ICC_SRE_EL2_ENABLE
>  	msr_s	ICC_SRE_EL2, x5
>  	isb
> -	mov	x5, #1
> +	// If using VHE, we don't care about EL1 and can early out.
> +ifnvhe "mov	x5, #1",			ret
>  	msr_s	ICC_SRE_EL1, x5
> -.endm
> +	ret
> +ENDPROC(__save_vgic_v3_state)
>  
>  /*
>   * Restore the VGIC CPU state from memory
>   * x0: Register pointing to VCPU struct
>   */
> -.macro	restore_vgic_v3_state
> +ENTRY(__restore_vgic_v3_state)
>  	// Compute the address of struct vgic_cpu
>  	add	x3, x0, #VCPU_VGIC_CPU
>  
> @@ -249,15 +263,6 @@
>  	and	x5, x5, #~ICC_SRE_EL2_ENABLE
>  	msr_s	ICC_SRE_EL2, x5
>  1:
> -.endm
> -
> -ENTRY(__save_vgic_v3_state)
> -	save_vgic_v3_state
> -	ret
> -ENDPROC(__save_vgic_v3_state)
> -
> -ENTRY(__restore_vgic_v3_state)
> -	restore_vgic_v3_state
>  	ret
>  ENDPROC(__restore_vgic_v3_state)
>  
> -- 
> 2.1.4
>