[RFC PATCH v2 2/3] KVM: arm64: Convert lazy FPSIMD context switch trap to C

[Marc Zyngier]

On 09/04/18 10:44, Christoffer Dall wrote:
> On Fri, Apr 06, 2018 at 04:51:53PM +0100, Dave Martin wrote:
>> On Fri, Apr 06, 2018 at 04:25:57PM +0100, Marc Zyngier wrote:
>>> Hi Dave,
>>>
>>> On 06/04/18 16:01, Dave Martin wrote:
>>>> To make the lazy FPSIMD context switch trap code easier to hack on,
>>>> this patch converts it to C.
>>>>
>>>> This is not amazingly efficient, but the trap should typically only
>>>> be taken once per host context switch.
>>>>
>>>> Signed-off-by: Dave Martin <Dave.Martin at arm.com>
>>>>
>>>> ---
>>>>
>>>> Since RFCv1:
>>>>
>>>>  * Fix indentation to be consistent with the rest of the file.
>>>>  * Add missing ! to write back to sp with attempting to push regs.
>>>> ---
>>>>  arch/arm64/kvm/hyp/entry.S  | 57 +++++++++++++++++----------------------------
>>>>  arch/arm64/kvm/hyp/switch.c | 24 +++++++++++++++++++
>>>>  2 files changed, 46 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
>>>> index fdd1068..47c6a78 100644
>>>> --- a/arch/arm64/kvm/hyp/entry.S
>>>> +++ b/arch/arm64/kvm/hyp/entry.S
>>>> @@ -176,41 +176,28 @@ ENTRY(__fpsimd_guest_restore)
>>>>  	// x1: vcpu
>>>>  	// x2-x29,lr: vcpu regs
>>>>  	// vcpu x0-x1 on the stack
>>>> -	stp	x2, x3, [sp, #-16]!
>>>> -	stp	x4, lr, [sp, #-16]!
>>>> -
>>>> -alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
>>>> -	mrs	x2, cptr_el2
>>>> -	bic	x2, x2, #CPTR_EL2_TFP
>>>> -	msr	cptr_el2, x2
>>>> -alternative_else
>>>> -	mrs	x2, cpacr_el1
>>>> -	orr	x2, x2, #CPACR_EL1_FPEN
>>>> -	msr	cpacr_el1, x2
>>>> -alternative_endif
>>>> -	isb
>>>> -
>>>> -	mov	x3, x1
>>>> -
>>>> -	ldr	x0, [x3, #VCPU_HOST_CONTEXT]
>>>> -	kern_hyp_va x0
>>>> -	add	x0, x0, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
>>>> -	bl	__fpsimd_save_state
>>>> -
>>>> -	add	x2, x3, #VCPU_CONTEXT
>>>> -	add	x0, x2, #CPU_GP_REG_OFFSET(CPU_FP_REGS)
>>>> -	bl	__fpsimd_restore_state
>>>> -
>>>> -	// Skip restoring fpexc32 for AArch64 guests
>>>> -	mrs	x1, hcr_el2
>>>> -	tbnz	x1, #HCR_RW_SHIFT, 1f
>>>> -	ldr	x4, [x3, #VCPU_FPEXC32_EL2]
>>>> -	msr	fpexc32_el2, x4
>>>> -1:
>>>> -	ldp	x4, lr, [sp], #16
>>>> -	ldp	x2, x3, [sp], #16
>>>> -	ldp	x0, x1, [sp], #16
>>>> -
>>>> +	stp	x2, x3, [sp, #-144]!
>>>> +	stp	x4, x5, [sp, #16]
>>>> +	stp	x6, x7, [sp, #32]
>>>> +	stp	x8, x9, [sp, #48]
>>>> +	stp	x10, x11, [sp, #64]
>>>> +	stp	x12, x13, [sp, #80]
>>>> +	stp	x14, x15, [sp, #96]
>>>> +	stp	x16, x17, [sp, #112]
>>>> +	stp	x18, lr, [sp, #128]
>>>> +
>>>> +	bl	__hyp_switch_fpsimd
>>>> +
>>>> +	ldp	x4, x5, [sp, #16]
>>>> +	ldp	x6, x7, [sp, #32]
>>>> +	ldp	x8, x9, [sp, #48]
>>>> +	ldp	x10, x11, [sp, #64]
>>>> +	ldp	x12, x13, [sp, #80]
>>>> +	ldp	x14, x15, [sp, #96]
>>>> +	ldp	x16, x17, [sp, #112]
>>>> +	ldp	x18, lr, [sp, #128]
>>>> +	ldp	x0, x1, [sp, #144]
>>>> +	ldp	x2, x3, [sp], #160
>>>
>>> I can't say I'm overly thrilled with adding another save/restore 
>>> sequence. How about treating it like a real guest exit instead? Granted, 
>>> there is a bit more overhead to it, but as you pointed out above, this 
>>> should be pretty rare...
>>
>> I have no objection to handling this after exiting back to
>> __kvm_vcpu_run(), provided the performance is deemed acceptable.
>>
> 
> My guess is that it's going to be visible on non-VHE systems, and given
> that we're doing all of this for performance in the first place, I'm not
> exceited about that approach either.

My rational is that, as we don't disable FP access across most
exit/entry sequences, we still significantly benefit from the optimization.

> I thought it was acceptable to do another save/restore, because it was
> only the GPRs (and equivalent to what the compiler would generate for a
> function call?) and thus not susceptible to the complexities of sysreg
> save/restores.

Sysreg? That's not what I'm proposing. What I'm proposing here is that
we treat FP exception as a shallow exit that immediately returns to the
guest without touching them. The overhead is an extra save/restore of
the host's x19-x30, if I got my maths right. I agree that this is
significant, but I'd like to measure this overhead before we go one way
or the other.

> Another alternative would be to go back to Dave's original approach of
> implementing the fpsimd state update to the host's structure in assembly
> directly, but I was having a hard time understanding that.  Perhaps I
> just need to try harder.
I'd rather stick to the current C approach, no matter how we perform the
save/restore. It feels a lot more readable and maintainable in the long run.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...