Re: [PATCH v3 1/2] arm64: KVM: Optimize arm64 skip 30-50% vfp/simd save/restore on exits

[Mario Smarduch <m.smarduch@samsung.com>]

On 07/03/2015 04:53 AM, Christoffer Dall wrote:
> On Thu, Jul 02, 2015 at 02:51:57PM -0700, Mario Smarduch wrote:
>> On 07/01/2015 06:46 AM, Christoffer Dall wrote:
>>> On Wed, Jun 24, 2015 at 05:04:11PM -0700, Mario Smarduch wrote:
>>>> This patch only saves and restores FP/SIMD registers on Guest access. To do
>>>> this cptr_el2 FP/SIMD trap is set on Guest entry and later checked on exit.
>>>> lmbench, hackbench show significant improvements, for 30-50% exits FP/SIMD
>>>> context is not saved/restored
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_arm.h |    5 ++++-
>>>>  arch/arm64/kvm/hyp.S             |   46 +++++++++++++++++++++++++++++++++++---
>>>>  2 files changed, 47 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>>> index ac6fafb..7605e09 100644
>>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>>> @@ -171,10 +171,13 @@
>>>>  #define HSTR_EL2_TTEE	(1 << 16)
>>>>  #define HSTR_EL2_T(x)	(1 << x)
>>>>  
>>>> +/* Hyp Coproccessor Trap Register Shifts */
>>>> +#define CPTR_EL2_TFP_SHIFT 10
>>>> +
>>>>  /* Hyp Coprocessor Trap Register */
>>>>  #define CPTR_EL2_TCPAC	(1 << 31)
>>>>  #define CPTR_EL2_TTA	(1 << 20)
>>>> -#define CPTR_EL2_TFP	(1 << 10)
>>>> +#define CPTR_EL2_TFP	(1 << CPTR_EL2_TFP_SHIFT)
>>>>  
>>>>  /* Hyp Debug Configuration Register bits */
>>>>  #define MDCR_EL2_TDRA		(1 << 11)
>>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>>>> index 5befd01..de0788f 100644
>>>> --- a/arch/arm64/kvm/hyp.S
>>>> +++ b/arch/arm64/kvm/hyp.S
>>>> @@ -673,6 +673,15 @@
>>>>  	tbz	\tmp, #KVM_ARM64_DEBUG_DIRTY_SHIFT, \target
>>>>  .endm
>>>>  
>>>> +/*
>>>> + * Check cptr VFP/SIMD accessed bit, if set VFP/SIMD not accessed by guest.
>>>
>>> This comment doesn't really help me understand the function, may I
>>> suggest:
>>>
>>> Branch to target if CPTR_EL2.TFP bit is set (VFP/SIMD trapping enabled)
>>
>> Yes actually describes what it does.
>>
>>>
>>>> + */
>>>> +.macro skip_fpsimd_state tmp, target
>>>> +	mrs	\tmp, cptr_el2
>>>> +	tbnz	\tmp, #CPTR_EL2_TFP_SHIFT, \target
>>>> +.endm
>>>> +
>>>> +
>>>>  .macro compute_debug_state target
>>>>  	// Compute debug state: If any of KDE, MDE or KVM_ARM64_DEBUG_DIRTY
>>>>  	// is set, we do a full save/restore cycle and disable trapping.
>>>> @@ -763,6 +772,7 @@
>>>>  	ldr     x2, [x0, #VCPU_HCR_EL2]
>>>>  	msr     hcr_el2, x2
>>>>  	mov	x2, #CPTR_EL2_TTA
>>>> +	orr     x2, x2, #CPTR_EL2_TFP
>>>>  	msr	cptr_el2, x2
>>>>  
>>>>  	mov	x2, #(1 << 15)	// Trap CP15 Cr=15
>>>> @@ -785,7 +795,6 @@
>>>>  .macro deactivate_traps
>>>>  	mov	x2, #HCR_RW
>>>>  	msr	hcr_el2, x2
>>>> -	msr	cptr_el2, xzr
>>>>  	msr	hstr_el2, xzr
>>>>  
>>>>  	mrs	x2, mdcr_el2
>>>> @@ -912,6 +921,28 @@ __restore_fpsimd:
>>>>  	restore_fpsimd
>>>>  	ret
>>>>  
>>>> +switch_to_guest_fpsimd:
>>>> +	push	x4, lr
>>>> +
>>>> +	mrs	x2, cptr_el2
>>>> +	bic	x2, x2, #CPTR_EL2_TFP
>>>> +	msr	cptr_el2, x2
>>>> +
>>>> +	mrs	x0, tpidr_el2
>>>> +
>>>> +	ldr	x2, [x0, #VCPU_HOST_CONTEXT]
>>>> +	kern_hyp_va x2
>>>> +	bl __save_fpsimd
>>>> +
>>>> +	add	x2, x0, #VCPU_CONTEXT
>>>> +	bl __restore_fpsimd
>>>> +
>>>> +	pop	x4, lr
>>>> +	pop	x2, x3
>>>> +	pop	x0, x1
>>>> +
>>>> +	eret
>>>> +
>>>>  /*
>>>>   * u64 __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>>>>   *
>>>> @@ -932,7 +963,6 @@ ENTRY(__kvm_vcpu_run)
>>>>  	kern_hyp_va x2
>>>>  
>>>>  	save_host_regs
>>>> -	bl __save_fpsimd
>>>>  	bl __save_sysregs
>>>>  
>>>>  	compute_debug_state 1f
>>>> @@ -948,7 +978,6 @@ ENTRY(__kvm_vcpu_run)
>>>>  	add	x2, x0, #VCPU_CONTEXT
>>>>  
>>>>  	bl __restore_sysregs
>>>> -	bl __restore_fpsimd
>>>>  
>>>>  	skip_debug_state x3, 1f
>>>>  	bl	__restore_debug
>>>> @@ -967,7 +996,9 @@ __kvm_vcpu_return:
>>>>  	add	x2, x0, #VCPU_CONTEXT
>>>>  
>>>>  	save_guest_regs
>>>> +	skip_fpsimd_state x3, 1f
>>>>  	bl __save_fpsimd
>>>> +1:
>>>>  	bl __save_sysregs
>>>>  
>>>>  	skip_debug_state x3, 1f
>>>> @@ -986,7 +1017,11 @@ __kvm_vcpu_return:
>>>>  	kern_hyp_va x2
>>>>  
>>>>  	bl __restore_sysregs
>>>> +	skip_fpsimd_state x3, 1f
>>>>  	bl __restore_fpsimd
>>>> +1:
>>>> +	/* Clear FPSIMD and Trace trapping */
>>>> +	msr     cptr_el2, xzr
>>>
>>> why not simply move the deactivate_traps down here instead?
>>
>> Putting deactivate_traps there  trashes x2, setup earlier
>> to restore debug, host registers from host context
>>
>> Do we want deactivate_traps to use another register and
>> move the macro there? Or leave as is?
>>
> There was some clean symmetry in the code by using deactivate traps, but
> given this, I don't care strongly which way we end up doing it.

I agree. Also it probably makes more sense to keep the trap disable
code together to match the trap enables, without objection from
Marc I'll rework the patch, repost it after Monday.

Thanks,
- Mario
> 
> Thanks,
> -Christoffer
>