| Message ID | 1451166900-3711-2-git-send-email-m.smarduch@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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" this looks dodgy... can you move vfpinstr.h instead? > > 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 */ the comment is misleading, you're not enabling guest access to the fp/simd unit, you're just setting the enabled bit to ensure guest accesses trap. > +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 */ I would probably get rid of the "Called from" stuff and just describe what these functions do locally. Comments like this are likely to be out of date soon'ish. > +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; > +} > + > +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} > +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) > +{ > + return false; > +} > +#endif this kind of feels like it belongs in its own C-file instead of a header file, perhaps arch/arm/kvm/vfp.C. Marc, what do you think? > + > #endif /* __ARM_KVM_EMULATE_H__ */ > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index f9f2779..d3ef58a 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -105,6 +105,12 @@ struct kvm_vcpu_arch { > /* HYP trapping configuration */ > u32 hcr; > > + /* HYP Co-processor fp/simd and trace trapping configuration */ > + u32 hcptr; > + > + /* Save host FPEXC register to later restore on vcpu put */ > + u32 host_fpexc; > + > /* Interrupt related fields */ > u32 irq_lines; /* IRQ and FIQ levels */ > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 3066328..ffe8ccf 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -299,4 +299,12 @@ 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 void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} > + > +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) > +{ > + return false; > +} > + > #endif /* __ARM64_KVM_EMULATE_H__ */ > -- > 1.9.1 > Thanks, -Christoffer
On 1/5/2016 7:00 AM, Christoffer Dall wrote: > 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" > > this looks dodgy... > > can you move vfpinstr.h instead? Sure I'll fix it up, it's in couple other places in kernel and kvm - copied it. > >> >> 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 */ > > the comment is misleading, you're not enabling guest access to the > fp/simd unit, you're just setting the enabled bit to ensure guest > accesses trap. That's more accurate. > >> +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 */ > > I would probably get rid of the "Called from" stuff and just describe > what these functions do locally. Comments like this are likely to be > out of date soon'ish. Yeah true, will do. > >> +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; >> +} >> + >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) >> +{ >> + return false; >> +} >> +#endif > > this kind of feels like it belongs in its own C-file instead of a header > file, perhaps arch/arm/kvm/vfp.C. > > Marc, what do you think? > That would be starting from vcpu_trap_vfp_enable()? The file is getting little overloaded. I'm also thinking that 3rd patch should have one function call for vcpu_put like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic. When you have a chance to review that patch please keep that in mind. >> + >> #endif /* __ARM_KVM_EMULATE_H__ */ >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index f9f2779..d3ef58a 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -105,6 +105,12 @@ struct kvm_vcpu_arch { >> /* HYP trapping configuration */ >> u32 hcr; >> >> + /* HYP Co-processor fp/simd and trace trapping configuration */ >> + u32 hcptr; >> + >> + /* Save host FPEXC register to later restore on vcpu put */ >> + u32 host_fpexc; >> + >> /* Interrupt related fields */ >> u32 irq_lines; /* IRQ and FIQ levels */ >> >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h >> index 3066328..ffe8ccf 100644 >> --- a/arch/arm64/include/asm/kvm_emulate.h >> +++ b/arch/arm64/include/asm/kvm_emulate.h >> @@ -299,4 +299,12 @@ 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 void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} >> + >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) >> +{ >> + return false; >> +} >> + >> #endif /* __ARM64_KVM_EMULATE_H__ */ >> -- >> 1.9.1 >> > > Thanks, > -Christoffer >
On Tue, Jan 05, 2016 at 11:28:18AM -0800, Mario Smarduch wrote: > > > On 1/5/2016 7:00 AM, Christoffer Dall wrote: > > 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" > > > > this looks dodgy... > > > > can you move vfpinstr.h instead? > Sure I'll fix it up, it's in couple other places in kernel and kvm > - copied it. > > > >> > >> 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 */ > > > > the comment is misleading, you're not enabling guest access to the > > fp/simd unit, you're just setting the enabled bit to ensure guest > > accesses trap. > > That's more accurate. > > > >> +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 */ > > > > I would probably get rid of the "Called from" stuff and just describe > > what these functions do locally. Comments like this are likely to be > > out of date soon'ish. > > Yeah true, will do. > > > >> +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; > >> +} > >> + > >> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} > >> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) > >> +{ > >> + return false; > >> +} > >> +#endif > > > > this kind of feels like it belongs in its own C-file instead of a header > > file, perhaps arch/arm/kvm/vfp.C. > > > > Marc, what do you think? > > > > That would be starting from vcpu_trap_vfp_enable()? The file is getting > little overloaded. yes, starting from vcpu_trap_vfp_enable. Which file is getting overloaded? I'm thinking the assembly file you add in the next patch could be inline assembly in a C-file combined with this code, perhaps. > > I'm also thinking that 3rd patch should have one function call for vcpu_put > like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic. > When you have a chance to review that patch please keep that in mind. > ok, will try ;) -Christoffer
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? Thanks, -Christoffer
On 1/10/2016 8:32 AM, Christoffer Dall wrote: > On Tue, Jan 05, 2016 at 11:28:18AM -0800, Mario Smarduch wrote: >> >> >> On 1/5/2016 7:00 AM, Christoffer Dall wrote: >>> 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" >>> >>> this looks dodgy... >>> >>> can you move vfpinstr.h instead? >> Sure I'll fix it up, it's in couple other places in kernel and kvm >> - copied it. >>> >>>> >>>> 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 */ >>> >>> the comment is misleading, you're not enabling guest access to the >>> fp/simd unit, you're just setting the enabled bit to ensure guest >>> accesses trap. >> >> That's more accurate. >>> >>>> +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 */ >>> >>> I would probably get rid of the "Called from" stuff and just describe >>> what these functions do locally. Comments like this are likely to be >>> out of date soon'ish. >> >> Yeah true, will do. >>> >>>> +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; >>>> +} >>>> + >>>> +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} >>>> +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) >>>> +{ >>>> + return false; >>>> +} >>>> +#endif >>> >>> this kind of feels like it belongs in its own C-file instead of a header >>> file, perhaps arch/arm/kvm/vfp.C. >>> >>> Marc, what do you think? >>> >> >> That would be starting from vcpu_trap_vfp_enable()? The file is getting >> little overloaded. > > yes, starting from vcpu_trap_vfp_enable. > > Which file is getting overloaded? > > I'm thinking the assembly file you add in the next patch could be > inline assembly in a C-file combined with this code, perhaps. Yes that makes sense, I'll convert it. > >> >> I'm also thinking that 3rd patch should have one function call for vcpu_put >> like vcpu_load does instead of exposing arm32/arm64, arm32 only relevant logic. >> When you have a chance to review that patch please keep that in mind. >> > > ok, will try ;) > > -Christoffer >
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. > > Thanks, > -Christoffer >
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? -Christoffer
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? Hi Christoffer, - on host I disabled VFP/VFPv3 and got a lot of Illegal instructions and many other problems. Perhaps disabling for armv7 this option may need re-evaluation. - Enabling VFPv3 on host and running guest with no vfpv3 appears to work with few glitches. - and vpfv3 host/guest works just fine. Appears disabling vfpv3 on armv7 requires another investigation (atleast on my end). BTW this is on fast models. - Mario > > -Christoffer >
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? - Mario > > Hi Christoffer, > - on host I disabled VFP/VFPv3 and got a lot of Illegal instructions > and many other problems. Perhaps disabling for armv7 this option may need > re-evaluation. > - Enabling VFPv3 on host and running guest with no vfpv3 appears > to work with few glitches. > - and vpfv3 host/guest works just fine. > > Appears disabling vfpv3 on armv7 requires another investigation (atleast > on my end). > > BTW this is on fast models. > > - Mario > >> >> -Christoffer >> > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >
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. I think it's fair to assume VFP is enabled for a KVM host, but I don't have enough familiarity with this to be sure. Marc, any thoughts? -Christoffer
On 14/01/16 13:27, 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. I think > it's fair to assume VFP is enabled for a KVM host, but I don't have > enough familiarity with this to be sure. > > Marc, any thoughts? I'm not so sure about the host kernel configuration - after all, you could have a very specialized host that doesn't use VFP at all (softfloat), and yet offers VFP support to guests. But what I'm worried about is the case where someone has built an ARMv7+VE without VFP. We may end-up exploding in that case. What we could do is to probe for the VFP HW, and not enable KVM if absent. This would give us the freedom to remove the #ifdefery. What do you think? M.
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. I could make this change but have no way to bring the host up to test it. - Mario > I think > it's fair to assume VFP is enabled for a KVM host, but I don't have > enough familiarity with this to be sure. > > Marc, any thoughts? > > -Christoffer >
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. Thanks, M.
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. >
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. I investigated VFP and virtualiztion. Came up with few hw/sw configurations, some of it tested other not so it may be full of holes. 1. CONFIG_VFP set and VFP not implemented on all CPUs vfp_init() - discovers VFP is not supported via vfp_vector and bails out. A guest should be able to boot successfully going through the same check. And obviously there is no need to lazy switch vfp registers, vcpu_put/get should know about this. This would be softfloat/softfloat. 2. CONFIG_VFP set and platform calls vfp_disable() which I believe the platform is saying not all cpus have a VFP. Some guests may detect a VFP others not. Doesn't appear like a workable configuration with virtualization enabled. 3. CONFIG_VFP not set and VFP impl/not impl host using softfloat. To get this to work code from vfp_init is needed. - Enable coproc for cp10, 11 on cpus - Test for vfp support allow platform to set vfp_disable. in vput_load() - if all CPUs have a VFP set FPEXC.EN, set hcptr vfp trapping - Here guest gets an undefined instruction on fmrx(FPSID), but if vfp traps are not set the guest boots just fine, don't know what the problem is. 4. CONFIG_VFP set VFP implemented on all CPUs - This works fine, Guest VFP no exception on FPSIMD access like in 3. Appears like full host VFP initialization does something to make the guest with hcptr traps work. Unfortunately my priorities have been changed and need to move to other work. I could complete 4 per Christoffers last comments. - Mario > > 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. > > Thanks, > > M. >
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; +} + +static inline void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) +{ + return false; +} +#endif + #endif /* __ARM_KVM_EMULATE_H__ */ diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index f9f2779..d3ef58a 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -105,6 +105,12 @@ struct kvm_vcpu_arch { /* HYP trapping configuration */ u32 hcr; + /* HYP Co-processor fp/simd and trace trapping configuration */ + u32 hcptr; + + /* Save host FPEXC register to later restore on vcpu put */ + u32 host_fpexc; + /* Interrupt related fields */ u32 irq_lines; /* IRQ and FIQ levels */ diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 3066328..ffe8ccf 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -299,4 +299,12 @@ 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 void vcpu_restore_host_fpexc(struct kvm_vcpu *vcpu) {} + +static inline bool vcpu_vfp_isdirty(struct kvm_vcpu *vcpu) +{ + return false; +} + #endif /* __ARM64_KVM_EMULATE_H__ */
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(+)