| Message ID | 1498040401-16361-12-git-send-email-yang.zhong@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/06/2017 12:19, Yang Zhong wrote: > Split the cpu_set_mxcsr()/cpu_set_fpuc() with specific tcg code. > tcg_update_mxcsr()/tcg_set_fpuc() need be implemented in tcg-stub.c > file if tcg is disabled. > > Signed-off-by: Yang Zhong <yang.zhong@intel.com> > --- > accel/stubs/tcg-stub.c | 8 ++++++++ > target/i386/cpu.h | 15 +++++++++++++-- > target/i386/fpu_helper.c | 8 +++----- > 3 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c > index dafb1d0..91625a8 100644 > --- a/accel/stubs/tcg-stub.c > +++ b/accel/stubs/tcg-stub.c > @@ -75,6 +75,14 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf) > { > } > > +void tcg_update_mxcsr(CPUX86State *env) > +{ > +} > + > +void tcg_set_fpuc(CPUX86State *env) > +{ > +} > + > void cpu_loop_exit(CPUState *cpu) > { > abort(); > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 8b3b535..229b216 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1643,8 +1643,19 @@ static inline int32_t x86_get_a20_mask(CPUX86State *env) > } > > /* fpu_helper.c */ > -void cpu_set_mxcsr(CPUX86State *env, uint32_t val); > -void cpu_set_fpuc(CPUX86State *env, uint16_t val); > +void tcg_update_mxcsr(CPUX86State *env); > +void tcg_set_fpuc(CPUX86State *env); > +static inline void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) > +{ > + env->mxcsr = mxcsr; > + tcg_update_mxcsr(env); Instead of having to add stubs, please guard the call with "if (tcg_enabled())". Same below. Paolo > +} > + > +static inline void cpu_set_fpuc(CPUX86State *env, uint16_t fpuc) > +{ > + env->fpuc = fpuc; > + tcg_set_fpuc(env); > +} > > /* mem_helper.c */ > void helper_lock_init(void); > diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c > index 34fb5fc..a7550c9 100644 > --- a/target/i386/fpu_helper.c > +++ b/target/i386/fpu_helper.c > @@ -1550,12 +1550,11 @@ void helper_xsetbv(CPUX86State *env, uint32_t ecx, uint64_t mask) > #define SSE_RC_CHOP 0x6000 > #define SSE_FZ 0x8000 > > -void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) > +void tcg_update_mxcsr(CPUX86State *env) > { > + uint32_t mxcsr = env->mxcsr; > int rnd_type; > > - env->mxcsr = mxcsr; > - > /* set rounding mode */ > switch (mxcsr & SSE_RC_MASK) { > default: > @@ -1581,9 +1580,8 @@ void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) > set_flush_to_zero((mxcsr & SSE_FZ) ? 1 : 0, &env->fp_status); > } > > -void cpu_set_fpuc(CPUX86State *env, uint16_t val) > +void tcg_set_fpuc(CPUX86State *env) > { > - env->fpuc = val; > update_fp_status(env); > } > >
On 06/21/2017 03:19 AM, Yang Zhong wrote: > Split the cpu_set_mxcsr()/cpu_set_fpuc() with specific tcg code. > tcg_update_mxcsr()/tcg_set_fpuc() need be implemented in tcg-stub.c > file if tcg is disabled. > > Signed-off-by: Yang Zhong <yang.zhong@intel.com> > --- > accel/stubs/tcg-stub.c | 8 ++++++++ > target/i386/cpu.h | 15 +++++++++++++-- > target/i386/fpu_helper.c | 8 +++----- > 3 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c > index dafb1d0..91625a8 100644 > --- a/accel/stubs/tcg-stub.c > +++ b/accel/stubs/tcg-stub.c > @@ -75,6 +75,14 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf) > { > } > > +void tcg_update_mxcsr(CPUX86State *env) > +{ > +} > + > +void tcg_set_fpuc(CPUX86State *env) > +{ > +} Do not put x86-specific functions in what is clearly a target-independent file. Paolo gave you one way of doing that for this case. r~
On Wed, Jun 21, 2017 at 03:15:25PM +0200, Paolo Bonzini wrote: > > > On 21/06/2017 12:19, Yang Zhong wrote: > > Split the cpu_set_mxcsr()/cpu_set_fpuc() with specific tcg code. > > tcg_update_mxcsr()/tcg_set_fpuc() need be implemented in tcg-stub.c > > file if tcg is disabled. > > > > Signed-off-by: Yang Zhong <yang.zhong@intel.com> > > --- > > accel/stubs/tcg-stub.c | 8 ++++++++ > > target/i386/cpu.h | 15 +++++++++++++-- > > target/i386/fpu_helper.c | 8 +++----- > > 3 files changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c > > index dafb1d0..91625a8 100644 > > --- a/accel/stubs/tcg-stub.c > > +++ b/accel/stubs/tcg-stub.c > > @@ -75,6 +75,14 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf) > > { > > } > > > > +void tcg_update_mxcsr(CPUX86State *env) > > +{ > > +} > > + > > +void tcg_set_fpuc(CPUX86State *env) > > +{ > > +} > > + > > void cpu_loop_exit(CPUState *cpu) > > { > > abort(); > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index 8b3b535..229b216 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -1643,8 +1643,19 @@ static inline int32_t x86_get_a20_mask(CPUX86State *env) > > } > > > > /* fpu_helper.c */ > > -void cpu_set_mxcsr(CPUX86State *env, uint32_t val); > > -void cpu_set_fpuc(CPUX86State *env, uint16_t val); > > +void tcg_update_mxcsr(CPUX86State *env); > > +void tcg_set_fpuc(CPUX86State *env); > > +static inline void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) > > +{ > > + env->mxcsr = mxcsr; > > + tcg_update_mxcsr(env); > > Instead of having to add stubs, please guard the call with "if > (tcg_enabled())". Same below. > > Paolo > Hello Paolo, Got it! thanks! From your comments + Richard Henderson's comments, i got below guideline (1) accel/stubs/tcg-stub.c This file is target-independent file, do not add x86-specific stubs functions into this file. (2) there are three kind of methods to mask TCG relative function a) if(tcg_enabled()) b) if CONFIG_TCG c) stub function Paolo, which one is your prefer? a) ? please also make sure your prefer sequence, a) > b) > c) ? Yang > > +} > > + > > +static inline void cpu_set_fpuc(CPUX86State *env, uint16_t fpuc) > > +{ > > + env->fpuc = fpuc; > > + tcg_set_fpuc(env); > > +} > > > > /* mem_helper.c */ > > void helper_lock_init(void); > > diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c > > index 34fb5fc..a7550c9 100644 > > --- a/target/i386/fpu_helper.c > > +++ b/target/i386/fpu_helper.c > > @@ -1550,12 +1550,11 @@ void helper_xsetbv(CPUX86State *env, uint32_t ecx, uint64_t mask) > > #define SSE_RC_CHOP 0x6000 > > #define SSE_FZ 0x8000 > > > > -void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) > > +void tcg_update_mxcsr(CPUX86State *env) > > { > > + uint32_t mxcsr = env->mxcsr; > > int rnd_type; > > > > - env->mxcsr = mxcsr; > > - > > /* set rounding mode */ > > switch (mxcsr & SSE_RC_MASK) { > > default: > > @@ -1581,9 +1580,8 @@ void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) > > set_flush_to_zero((mxcsr & SSE_FZ) ? 1 : 0, &env->fp_status); > > } > > > > -void cpu_set_fpuc(CPUX86State *env, uint16_t val) > > +void tcg_set_fpuc(CPUX86State *env) > > { > > - env->fpuc = val; > > update_fp_status(env); > > } > > > >
----- Original Message ----- > From: "Zhong Yang" <yang.zhong@intel.com> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: qemu-devel@nongnu.org, "anthony xu" <anthony.xu@intel.com>, "a rigo" <a.rigo@virtualopensystems.com>, "Richard > Henderson" <rth@twiddle.net>, "Thomas Huth" <thuth@redhat.com> > Sent: Thursday, June 22, 2017 10:03:25 AM > Subject: Re: [PATCH 11/15] tcg: split cpu_set_mxcsr()/cpu_set_fpuc() > > On Wed, Jun 21, 2017 at 03:15:25PM +0200, Paolo Bonzini wrote: > > > > > > On 21/06/2017 12:19, Yang Zhong wrote: > > > Split the cpu_set_mxcsr()/cpu_set_fpuc() with specific tcg code. > > > tcg_update_mxcsr()/tcg_set_fpuc() need be implemented in tcg-stub.c > > > file if tcg is disabled. > > > > > > Signed-off-by: Yang Zhong <yang.zhong@intel.com> > > > --- > > > accel/stubs/tcg-stub.c | 8 ++++++++ > > > target/i386/cpu.h | 15 +++++++++++++-- > > > target/i386/fpu_helper.c | 8 +++----- > > > 3 files changed, 24 insertions(+), 7 deletions(-) > > > > > > diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c > > > index dafb1d0..91625a8 100644 > > > --- a/accel/stubs/tcg-stub.c > > > +++ b/accel/stubs/tcg-stub.c > > > @@ -75,6 +75,14 @@ void dump_opcount_info(FILE *f, fprintf_function > > > cpu_fprintf) > > > { > > > } > > > > > > +void tcg_update_mxcsr(CPUX86State *env) > > > +{ > > > +} > > > + > > > +void tcg_set_fpuc(CPUX86State *env) > > > +{ > > > +} > > > + > > > void cpu_loop_exit(CPUState *cpu) > > > { > > > abort(); > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > > index 8b3b535..229b216 100644 > > > --- a/target/i386/cpu.h > > > +++ b/target/i386/cpu.h > > > @@ -1643,8 +1643,19 @@ static inline int32_t x86_get_a20_mask(CPUX86State > > > *env) > > > } > > > > > > /* fpu_helper.c */ > > > -void cpu_set_mxcsr(CPUX86State *env, uint32_t val); > > > -void cpu_set_fpuc(CPUX86State *env, uint16_t val); > > > +void tcg_update_mxcsr(CPUX86State *env); > > > +void tcg_set_fpuc(CPUX86State *env); > > > +static inline void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) > > > +{ > > > + env->mxcsr = mxcsr; > > > + tcg_update_mxcsr(env); > > > > Instead of having to add stubs, please guard the call with "if > > (tcg_enabled())". Same below. > > > > Paolo > > > Hello Paolo, > > Got it! thanks! > > From your comments + Richard Henderson's comments, i got below guideline > (1) accel/stubs/tcg-stub.c > This file is target-independent file, do not add x86-specific stubs > functions into this file. > > (2) there are three kind of methods to mask TCG relative function > a) if(tcg_enabled()) > b) if CONFIG_TCG > c) stub function > > Paolo, which one is your prefer? a) ? please also make sure your prefer > sequence, a) > b) > c) ? There is also d) method in AccelClass It seldom makes sense, but when it does it's the best. Apart from this one, a) > c) > b). For target-independent code, "c" is more often the solution. For target-dependent code, "a" is more common. Paolo
diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c index dafb1d0..91625a8 100644 --- a/accel/stubs/tcg-stub.c +++ b/accel/stubs/tcg-stub.c @@ -75,6 +75,14 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf) { } +void tcg_update_mxcsr(CPUX86State *env) +{ +} + +void tcg_set_fpuc(CPUX86State *env) +{ +} + void cpu_loop_exit(CPUState *cpu) { abort(); diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 8b3b535..229b216 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1643,8 +1643,19 @@ static inline int32_t x86_get_a20_mask(CPUX86State *env) } /* fpu_helper.c */ -void cpu_set_mxcsr(CPUX86State *env, uint32_t val); -void cpu_set_fpuc(CPUX86State *env, uint16_t val); +void tcg_update_mxcsr(CPUX86State *env); +void tcg_set_fpuc(CPUX86State *env); +static inline void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) +{ + env->mxcsr = mxcsr; + tcg_update_mxcsr(env); +} + +static inline void cpu_set_fpuc(CPUX86State *env, uint16_t fpuc) +{ + env->fpuc = fpuc; + tcg_set_fpuc(env); +} /* mem_helper.c */ void helper_lock_init(void); diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c index 34fb5fc..a7550c9 100644 --- a/target/i386/fpu_helper.c +++ b/target/i386/fpu_helper.c @@ -1550,12 +1550,11 @@ void helper_xsetbv(CPUX86State *env, uint32_t ecx, uint64_t mask) #define SSE_RC_CHOP 0x6000 #define SSE_FZ 0x8000 -void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) +void tcg_update_mxcsr(CPUX86State *env) { + uint32_t mxcsr = env->mxcsr; int rnd_type; - env->mxcsr = mxcsr; - /* set rounding mode */ switch (mxcsr & SSE_RC_MASK) { default: @@ -1581,9 +1580,8 @@ void cpu_set_mxcsr(CPUX86State *env, uint32_t mxcsr) set_flush_to_zero((mxcsr & SSE_FZ) ? 1 : 0, &env->fp_status); } -void cpu_set_fpuc(CPUX86State *env, uint16_t val) +void tcg_set_fpuc(CPUX86State *env) { - env->fpuc = val; update_fp_status(env); }
Split the cpu_set_mxcsr()/cpu_set_fpuc() with specific tcg code. tcg_update_mxcsr()/tcg_set_fpuc() need be implemented in tcg-stub.c file if tcg is disabled. Signed-off-by: Yang Zhong <yang.zhong@intel.com> --- accel/stubs/tcg-stub.c | 8 ++++++++ target/i386/cpu.h | 15 +++++++++++++-- target/i386/fpu_helper.c | 8 +++----- 3 files changed, 24 insertions(+), 7 deletions(-)