Re: [PATCH v6 1/6] arm/arm64: KVM: Introduce armv7 fp/simd vcpu fields and helpers

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

On 1/15/2016 1:03 AM, Marc Zyngier wrote:
> On 15/01/16 02:02, Mario Smarduch wrote:
>>
>>
>> On 1/14/2016 5:27 AM, Christoffer Dall wrote:
>>> On Wed, Jan 13, 2016 at 07:03:04PM -0800, Mario Smarduch wrote:
>>>>
>>>>
>>>> On 1/12/2016 4:57 PM, Mario Smarduch wrote:
>>>>>
>>>>>
>>>>> On 1/12/2016 6:12 AM, Christoffer Dall wrote:
>>>>>> On Mon, Jan 11, 2016 at 03:39:21PM -0800, Mario Smarduch wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 1/10/2016 8:32 AM, Christoffer Dall wrote:
>>>>>>>> Hi Mario,
>>>>>>>>
>>>>>>>> I spotted one more potential issue...
>>>>>>>>
>>>>>>>> On Sat, Dec 26, 2015 at 01:54:55PM -0800, Mario Smarduch wrote:
>>>>>>>>> Add helper functions to enable access to fp/smid on guest entry and save host
>>>>>>>>> fpexc on vcpu put, check if fp/simd registers are dirty and add new vcpu
>>>>>>>>> fields.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>>>>>>> ---
>>>>>>>>>  arch/arm/include/asm/kvm_emulate.h   | 42 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>  arch/arm/include/asm/kvm_host.h      |  6 ++++++
>>>>>>>>>  arch/arm64/include/asm/kvm_emulate.h |  8 +++++++
>>>>>>>>>  3 files changed, 56 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> index 3095df0..d4d9da1 100644
>>>>>>>>> --- a/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> +++ b/arch/arm/include/asm/kvm_emulate.h
>>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>>  #include <asm/kvm_mmio.h>
>>>>>>>>>  #include <asm/kvm_arm.h>
>>>>>>>>>  #include <asm/cputype.h>
>>>>>>>>> +#include <asm/vfp.h>
>>>>>>>>> +#include "../vfp/vfpinstr.h"
>>>>>>>>>  
>>>>>>>>>  unsigned long *vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num);
>>>>>>>>>  unsigned long *vcpu_spsr(struct kvm_vcpu *vcpu);
>>>>>>>>> @@ -255,4 +257,44 @@ static inline unsigned long vcpu_data_host_to_guest(struct kvm_vcpu *vcpu,
>>>>>>>>>  	}
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +#ifdef CONFIG_VFPv3
>>>>>>>>> +/* Called from vcpu_load - save fpexc and enable guest access to fp/simd unit */
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	u32 fpexc;
>>>>>>>>> +
>>>>>>>>> +	/* Save host fpexc, and enable guest access to fp unit */
>>>>>>>>> +	fpexc = fmrx(FPEXC);
>>>>>>>>> +	vcpu->arch.host_fpexc = fpexc;
>>>>>>>>> +	fpexc |= FPEXC_EN;
>>>>>>>>> +	fmxr(FPEXC, fpexc);
>>>>>>>>> +
>>>>>>>>> +	/* Configure HCPTR to trap on tracing and fp/simd access */
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA | HCPTR_TCP(10)  | HCPTR_TCP(11);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* Called from vcpu_put - restore host fpexc */
>>>>>>>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	fmxr(FPEXC, vcpu->arch.host_fpexc);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* If trap bits are reset then fp/simd registers are dirty */
>>>>>>>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	return !(vcpu->arch.hcptr & (HCPTR_TCP(10) | HCPTR_TCP(11)));
>>>>>>>>> +}
>>>>>>>>> +#else
>>>>>>>>> +static inline void vcpu_trap_vfp_enable(struct kvm_vcpu *vcpu)
>>>>>>>>> +{
>>>>>>>>> +	vcpu->arch.hcptr = HCPTR_TTA;
>>>>>>>>
>>>>>>>> Is it correct not to trap VFP registers when the host kernel does not
>>>>>>>> have CONFIG_VFPv3?  I think this is a change in functionality compared
>>>>>>>> to the current kernels is it not?
>>>>>>>
>>>>>>> With CPU_V7 VFPv3 gets selected, without it fp should be emulated,
>>>>>>> with exceptions taken in guest kernel. I don't see a reason why
>>>>>>> fp hcptr access should be enabled in that case.
>>>>>>>
>>>>>>
>>>>>> If you have to guests with CONFIG_VFPV3 but your host doesn't have
>>>>>> CONFIG_VFPV3, you will never context-switch the VFP registers between
>>>>>> the two VMs, and mayhem will ensue.
>>>>>>
>>>>>> Unless I'm missing something very obvious?
>>>>
>>>> Did more testing on this enabling OABI_COMPAT and selecting
>>>> NWFPE/FastFPE breaks the boot. So far can't find a way to boot host
>>>> without VFP/VFPv3 enabled on ARMv7. CPU_V7 defaults to VFPv3
>>>> selection. I'm wondering if !VFPv3 path should be removed from
>>>> the patches?
>>>>
>>> I think this is related to your particular choice of userspace. 
>>
>> It appears like there are two soft float implementations.
>>
>> FastFPE - but that's missing arch/arm/fastfpe directory, the option
>> can still be selected but nothing is built.
>>
>> And the Netwidner FPE arch/arm/nwfpe, that doesn't appear to be
>> hooked into the kernel. The hook nwfpe_enter is not referenced
>> anywhere.
> 
> It is:
> 
> arch/arm/nwfpe/entry.S: .globl  nwfpe_enter
> arch/arm/nwfpe/entry.S:nwfpe_enter:
> arch/arm/nwfpe/fpmodule.c:extern void nwfpe_enter(void);
> arch/arm/nwfpe/fpmodule.c:      kern_fp_enter = nwfpe_enter;
> 
>> I could make this change but have no way to bring the host up to
>> test it.
> 
> None of these are relevant - they are emulation for the FPA (Floating
> Point Accelerator). Most of the time, nobody uses this but instead a
> userspace softfloat implementation, which saves the trap to kernel space
> for emulation.
> 
> You can try Debian armel (as opposed to armhf, which mandates VFP) for
> example, which is a softfloat-based distribution.

I've been using armel debian but it appears some binaries have hard fp
instructions (-mfloat-abi=hard,softfp). I have a simple rootfs now and
it comes up cleanly on the host. I'll work on qemu now.

Thanks for pointing out FPA.

- Mario


> 
> Thanks,
> 
> 	M.
>