Subject: [PATCH v6 5/6] arm/arm64: KVM: Introduce armv8 fp/simd vcpu fields and helpers

From: [email-masked] (Mario Smarduch)
Date: Mon, 11 Jan 2016 16:31:03 -0800

On 1/10/2016 8:32 AM, Christoffer Dall wrote:
> On Sat, Dec 26, 2015 at 01:56:29PM -0800, Mario Smarduch wrote:
>> Similar to armv7 add helper functions to enable access to fp/smid registers on 
>> guest entry. Save guest fpexc32_el2 on vcpu_put, check if guest is 32 bit.
>> Save guest and restore host registers from host kernel, and check 
>> if fp/simd registers are dirty, lastly add cptr_el2 vcpu field.
>>
>> Signed-off-by: Mario Smarduch <[email-masked]>
>> ---
>>  arch/arm/include/asm/kvm_emulate.h   | 12 ++++++++++++
>>  arch/arm64/include/asm/kvm_asm.h     |  5 +++++
>>  arch/arm64/include/asm/kvm_emulate.h | 26 ++++++++++++++++++++++++--
>>  arch/arm64/include/asm/kvm_host.h    | 12 +++++++++++-
>>  arch/arm64/kvm/hyp/hyp-entry.S       | 26 ++++++++++++++++++++++++++
>>  5 files changed, 78 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>> index d4d9da1..a434dc5 100644
>> --- a/arch/arm/include/asm/kvm_emulate.h
>> +++ b/arch/arm/include/asm/kvm_emulate.h
>> @@ -284,6 +284,12 @@ static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>  {
>>  	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>  }
>> +
>> +static inline bool vcpu_guest_is_32bit(struct kvm_vcpu *vcpu)
>> +{
>> +	return true;
>> +}
>> +static inline void vcpu_save_fpexc(struct kvm_vcpu *vcpu) {}
>>  #else
>>  static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>  {
>> @@ -295,6 +301,12 @@ static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>  {
>>  	return false;
>>  }
>> +
>> +static inline bool vcpu_guest_is_32bit(struct kvm_vcpu *vcpu)
>> +{
>> +	return true;
>> +}
>> +static inline void vcpu_save_fpexc(struct kvm_vcpu *vcpu) {}
>>  #endif
>>  
>>  #endif /* __ARM_KVM_EMULATE_H__ */
>> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
>> index 52b777b..ddae814 100644
>> --- a/arch/arm64/include/asm/kvm_asm.h
>> +++ b/arch/arm64/include/asm/kvm_asm.h
>> @@ -48,6 +48,11 @@ extern u64 __vgic_v3_get_ich_vtr_el2(void);
>>  
>>  extern u32 __kvm_get_mdcr_el2(void);
>>  
>> +extern void __fpsimd_prepare_fpexc32(void);
>> +extern void __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu);
>> +extern void __fpsimd_save_state(struct user_fpsimd_state *);
>> +extern void __fpsimd_restore_state(struct user_fpsimd_state *);
>> +
>>  #endif
>>  
>>  #endif /* __ARM_KVM_ASM_H__ */
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index ffe8ccf..f8203c7 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -299,12 +299,34 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>  	return data;		/* Leave LE untouched */
>>  }
>>  
>> -static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu) {}
>> +static inline bool vcpu_guest_is_32bit(struct kvm_vcpu *vcpu)
>> +{
>> +	return !(vcpu->arch.hcr_el2 & HCR_RW);
>> +}
>> +
>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>> +{
>> +	/* For 32 bit guest enable access to fp/simd registers */
>> +	if (vcpu_guest_is_32bit(vcpu))
>> +		vcpu_prepare_fpexc();
>> +
>> +	vcpu->arch.cptr_el2 = CPTR_EL2_TTA | CPTR_EL2_TFP;
>> +}
>> +
>>  static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {}
>>  
>>  static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>  {
>> -	return false;
>> +	return !(vcpu->arch.cptr_el2 & CPTR_EL2_TFP);
>> +}
>> +
>> +static inline void vcpu_restore_host_vfp_state(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
>> +	struct kvm_cpu_context *guest_ctxt = &vcpu->arch.ctxt;
>> +
>> +	__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
>> +	__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
>>  }
>>  
>>  #endif /* __ARM64_KVM_EMULATE_H__ */
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index bfe4d4e..5d0c256 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -26,6 +26,7 @@
>>  #include <linux/kvm_types.h>
>>  #include <asm/kvm.h>
>>  #include <asm/kvm_mmio.h>
>> +#include <asm/kvm_asm.h>
>>  
>>  #define __KVM_HAVE_ARCH_INTC_INITIALIZED
>>  
>> @@ -180,6 +181,7 @@ struct kvm_vcpu_arch {
>>  	/* HYP configuration */
>>  	u64 hcr_el2;
>>  	u32 mdcr_el2;
>> +	u32 cptr_el2;
>>  
>>  	/* Exception Information */
>>  	struct kvm_vcpu_fault_info fault;
>> @@ -338,7 +340,15 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>>  static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {}
>>  static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {}
>>  
>> -static inline void vcpu_restore_host_vfp_state(struct kvm_vcpu *vcpu) {}
>> +static inline void vcpu_prepare_fpexc(void)
>> +{
>> +	kvm_call_hyp(__fpsimd_prepare_fpexc32);
>> +}
>> +
>> +static inline void vcpu_save_fpexc(struct kvm_vcpu *vcpu)
>> +{
>> +	kvm_call_hyp(__fpsimd_save_fpexc32, vcpu);
>> +}
>>  
>>  void kvm_arm_init_debug(void);
>>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
>> index 93e8d983..a9235e2 100644
>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
>> @@ -164,6 +164,32 @@ ENTRY(__hyp_do_panic)
>>  	eret
>>  ENDPROC(__hyp_do_panic)
>>  
>> +/**
>> +  * void __fpsimd_prepare_fpexc32(void) -
>> +  *	We may be entering the guest and set CPTR_EL2.TFP to trap all floating
>> +  *	point register accesses to EL2, however, the ARM manual clearly states
>> +  *	that traps are only taken to EL2 if the operation would not otherwise
>> +  *	trap to EL1.  Therefore, always make sure that for 32-bit guests,
>> +  *	we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
>> +  */
>> +ENTRY(__fpsimd_prepare_fpexc32)
>> +	mov	x2, #(1 << 30)
>> +	msr	fpexc32_el2, x2
>> +	ret
>> +ENDPROC(__fpsimd_prepare_fpexc32)
> 
> Why is this code in assembly?
> 
> It feels like you should be able to stick this in switch.c or something?
Yeah you're right just reusing code and not integrating into the new structure.
> 
> Also, it's strange to review this patch as duplicating an entire comment
> from elsewhere and expecting it to get removed in the next patch, but
> ok...

Not sure what to do here, the comment is required. Maybe this is a
one off you could overlook :)
> 
>> +
>> +/**
>> + * void __fpsimd_save_fpexc32(void) -
>> + *	This function restores guest FPEXC to its vcpu context, we call this
>> + *	function from vcpu_put.
>> + */
>> +ENTRY(__fpsimd_save_fpexc32)
>> +	kern_hyp_va x0
> 
> the C prototype for this function indicates you don't take any
> parameters, but you seem to rely on the vcpu pointer being in x0 for
> this?

Yes right again should have spotted it.
> 
>> +	mrs	x2, fpexc32_el2
>> +	str	x2, [x0, #VCPU_FPEXC32_EL2]
>> +	ret
>> +ENDPROC(__fpsimd_save_fpexc32)
>> +
>>  .macro invalid_vector	label, target = __hyp_panic
>>  	.align	2
>>  \label:
>> -- 
>> 1.9.1
>>
> 
> Thanks,
> -Christoffer
>