Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP

2021-05-27 Thread Eduardo Habkost
On Tue, May 18, 2021 at 11:06:56AM +0800, Ziqiao Kong wrote:
[...]
> > > +/*
> > > + * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> > > + * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> > > + * FCS and FDS; it saves each as H.
> > > + */
> > > +if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> > > +&& (env->cr[0] & CR0_PE_MASK)) {
> > > +fpcs = env->fpcs;
> > > +fpds = env->fpds;
> >
> >
> > If you want to start supporting this feature flag, I suggest
> > moving this to a separate patch.  The description of this patch
> > seems to imply it is just a bug fix, not the implementation of a
> > new feature flag.
> >
> > When adding support for a new feature flag in TCG code, you need
> > to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
> > feature flag will never be enabled by the CPU configuration code.
> 
> Yes, currently this feature flag is never enabled for all CPU types.
> How about removing this
> feature flag in this patch and leaving a TODO in the comment? Thus I
> can issue another patch
> to complete the feature later.

Sounds better to me.  Otherwise you'll be just adding dead code
(because the flag will is impossible to be enabled if it's not
present in TCG_*_FEATURES).

-- 
Eduardo




Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP

2021-05-24 Thread Ziqiao Kong
Ping?

On Tue, May 18, 2021 at 11:06 AM Ziqiao Kong  wrote:
>
> On Tue, May 18, 2021 at 4:29 AM Eduardo Habkost  wrote:
> >
> > Hi,
> >
> > Thanks for the patch, and apologies for not reviewing earlier
> > versions.
> >
>
> Nevermind, the earlier version is also hard to review without a proper split.
>
> > On Fri, May 07, 2021 at 04:00:58PM +0800, Ziqiao Kong wrote:
> > > Changes since v3:
> > >  - Split the long patches to series to make review easier.
> > >  - Fix the coding style problems in v3.
> > >
> > > Changes since v2:
> > >  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
> > >  - Use stl instead of stw in do_fstenv.
> > >  - Move variables to floats instruction case block.
> > >  - Move last accessed memory operand to a temp variable to avoid another 
> > > load.
> > >  - Move segment selectors instead of segment base to fpcs and fpds.
> > >  - Fix some code stype problems for the original code in floats case 
> > > block.
> >
> > On the next versions, please include the changelog after a "---"
> > line, so it won't be included in the final commit.
> >
> > In addition to the changelog, the actual commit message (the text
> > above "---") needs to include an explanation for the change.  If
> > you are fixing a bug, please explain what's the bug you are
> > fixing.
>
> The bug is tracked in this thread
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00307.html.
>
> >
> >
> > >
> > > Signed-off-by: Ziqiao Kong 
> > > ---
> > >  target/i386/cpu.h|  4 +++
> > >  target/i386/tcg/fpu_helper.c | 48 ++--
> > >  target/i386/tcg/translate.c  | 45 -
> > >  3 files changed, 77 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 570f916878..241945320b 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
> > >  #define CPUID_7_0_EBX_INVPCID   (1U << 10)
> > >  /* Restricted Transactional Memory */
> > >  #define CPUID_7_0_EBX_RTM   (1U << 11)
> > > +/* Deprecates FPU CS and FPU DS values */
> > > +#define CPUID_7_0_EBX_FCS_FDS   (1U << 13)
> > >  /* Memory Protection Extension */
> > >  #define CPUID_7_0_EBX_MPX   (1U << 14)
> > >  /* AVX-512 Foundation */
> > > @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
> > >  FPReg fpregs[8];
> > >  /* KVM-only so far */
> > >  uint16_t fpop;
> > > +uint16_t fpcs;
> > > +uint16_t fpds;
> > >  uint64_t fpip;
> > >  uint64_t fpdp;
> > >
> > > diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> > > index 60ed93520a..f1a8717ed8 100644
> > > --- a/target/i386/tcg/fpu_helper.c
> > > +++ b/target/i386/tcg/fpu_helper.c
> > > @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
> > >  {
> > >  env->fpus = 0;
> > >  env->fpstt = 0;
> > > +env->fpcs = 0;
> > > +env->fpip = 0;
> > > +env->fpds = 0;
> > > +env->fpdp = 0;
> > >  cpu_set_fpuc(env, 0x37f);
> > >  env->fptags[0] = 1;
> > >  env->fptags[1] = 1;
> > > @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, 
> > > target_ulong ptr, int data32,
> > >  {
> > >  int fpus, fptag, exp, i;
> > >  uint64_t mant;
> > > +uint16_t fpcs, fpds;
> > >  CPU_LDoubleU tmp;
> > >
> > >  fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> > > @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, 
> > > target_ulong ptr, int data32,
> > >  }
> > >  }
> > >  }
> > > +
> > > +/*
> > > + * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> > > + * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> > > + * FCS and FDS; it saves each as H.
> > > + */
> > > +if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> > > +&& (env->cr[0] & CR0_PE_MASK)) {
> > > +fpcs = env->fpcs;
> > > +fpds = env->fpds;
> >
> >
> > If you want to start supporting this feature flag, I suggest
> > moving this to a separate patch.  The description of this patch
> > seems to imply it is just a bug fix, not the implementation of a
> > new feature flag.
> >
> > When adding support for a new feature flag in TCG code, you need
> > to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
> > feature flag will never be enabled by the CPU configuration code.
>
> Yes, currently this feature flag is never enabled for all CPU types.
> How about removing this
> feature flag in this patch and leaving a TODO in the comment? Thus I
> can issue another patch
> to complete the feature later.
>
> >
> >
> > > +} else {
> > > +fpcs = 0;
> > > +fpds = 0;
> > > +}
> > > +
> > >  if (data32) {
> > >  /* 32 bit */
> > >  cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
> > >  cpu_stl_data_ra(env, ptr + 4, 

Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP

2021-05-17 Thread Ziqiao Kong
On Tue, May 18, 2021 at 4:29 AM Eduardo Habkost  wrote:
>
> Hi,
>
> Thanks for the patch, and apologies for not reviewing earlier
> versions.
>

Nevermind, the earlier version is also hard to review without a proper split.

> On Fri, May 07, 2021 at 04:00:58PM +0800, Ziqiao Kong wrote:
> > Changes since v3:
> >  - Split the long patches to series to make review easier.
> >  - Fix the coding style problems in v3.
> >
> > Changes since v2:
> >  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
> >  - Use stl instead of stw in do_fstenv.
> >  - Move variables to floats instruction case block.
> >  - Move last accessed memory operand to a temp variable to avoid another 
> > load.
> >  - Move segment selectors instead of segment base to fpcs and fpds.
> >  - Fix some code stype problems for the original code in floats case block.
>
> On the next versions, please include the changelog after a "---"
> line, so it won't be included in the final commit.
>
> In addition to the changelog, the actual commit message (the text
> above "---") needs to include an explanation for the change.  If
> you are fixing a bug, please explain what's the bug you are
> fixing.

The bug is tracked in this thread
https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg00307.html.

>
>
> >
> > Signed-off-by: Ziqiao Kong 
> > ---
> >  target/i386/cpu.h|  4 +++
> >  target/i386/tcg/fpu_helper.c | 48 ++--
> >  target/i386/tcg/translate.c  | 45 -
> >  3 files changed, 77 insertions(+), 20 deletions(-)
> >
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 570f916878..241945320b 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
> >  #define CPUID_7_0_EBX_INVPCID   (1U << 10)
> >  /* Restricted Transactional Memory */
> >  #define CPUID_7_0_EBX_RTM   (1U << 11)
> > +/* Deprecates FPU CS and FPU DS values */
> > +#define CPUID_7_0_EBX_FCS_FDS   (1U << 13)
> >  /* Memory Protection Extension */
> >  #define CPUID_7_0_EBX_MPX   (1U << 14)
> >  /* AVX-512 Foundation */
> > @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
> >  FPReg fpregs[8];
> >  /* KVM-only so far */
> >  uint16_t fpop;
> > +uint16_t fpcs;
> > +uint16_t fpds;
> >  uint64_t fpip;
> >  uint64_t fpdp;
> >
> > diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> > index 60ed93520a..f1a8717ed8 100644
> > --- a/target/i386/tcg/fpu_helper.c
> > +++ b/target/i386/tcg/fpu_helper.c
> > @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
> >  {
> >  env->fpus = 0;
> >  env->fpstt = 0;
> > +env->fpcs = 0;
> > +env->fpip = 0;
> > +env->fpds = 0;
> > +env->fpdp = 0;
> >  cpu_set_fpuc(env, 0x37f);
> >  env->fptags[0] = 1;
> >  env->fptags[1] = 1;
> > @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong 
> > ptr, int data32,
> >  {
> >  int fpus, fptag, exp, i;
> >  uint64_t mant;
> > +uint16_t fpcs, fpds;
> >  CPU_LDoubleU tmp;
> >
> >  fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> > @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, 
> > target_ulong ptr, int data32,
> >  }
> >  }
> >  }
> > +
> > +/*
> > + * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> > + * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> > + * FCS and FDS; it saves each as H.
> > + */
> > +if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> > +&& (env->cr[0] & CR0_PE_MASK)) {
> > +fpcs = env->fpcs;
> > +fpds = env->fpds;
>
>
> If you want to start supporting this feature flag, I suggest
> moving this to a separate patch.  The description of this patch
> seems to imply it is just a bug fix, not the implementation of a
> new feature flag.
>
> When adding support for a new feature flag in TCG code, you need
> to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
> feature flag will never be enabled by the CPU configuration code.

Yes, currently this feature flag is never enabled for all CPU types.
How about removing this
feature flag in this patch and leaving a TODO in the comment? Thus I
can issue another patch
to complete the feature later.

>
>
> > +} else {
> > +fpcs = 0;
> > +fpds = 0;
> > +}
> > +
> >  if (data32) {
> >  /* 32 bit */
> >  cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
> >  cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
> >  cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> > -cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> > -cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> > -cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> > -cpu_stl_data_ra(env, ptr + 24, 0, 

Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP

2021-05-17 Thread Eduardo Habkost
Hi,

Thanks for the patch, and apologies for not reviewing earlier
versions.

On Fri, May 07, 2021 at 04:00:58PM +0800, Ziqiao Kong wrote:
> Changes since v3:
>  - Split the long patches to series to make review easier.
>  - Fix the coding style problems in v3.
> 
> Changes since v2:
>  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
>  - Use stl instead of stw in do_fstenv.
>  - Move variables to floats instruction case block.
>  - Move last accessed memory operand to a temp variable to avoid another load.
>  - Move segment selectors instead of segment base to fpcs and fpds.
>  - Fix some code stype problems for the original code in floats case block.

On the next versions, please include the changelog after a "---"
line, so it won't be included in the final commit.

In addition to the changelog, the actual commit message (the text
above "---") needs to include an explanation for the change.  If
you are fixing a bug, please explain what's the bug you are
fixing.


> 
> Signed-off-by: Ziqiao Kong 
> ---
>  target/i386/cpu.h|  4 +++
>  target/i386/tcg/fpu_helper.c | 48 ++--
>  target/i386/tcg/translate.c  | 45 -
>  3 files changed, 77 insertions(+), 20 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 570f916878..241945320b 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EBX_INVPCID   (1U << 10)
>  /* Restricted Transactional Memory */
>  #define CPUID_7_0_EBX_RTM   (1U << 11)
> +/* Deprecates FPU CS and FPU DS values */
> +#define CPUID_7_0_EBX_FCS_FDS   (1U << 13)
>  /* Memory Protection Extension */
>  #define CPUID_7_0_EBX_MPX   (1U << 14)
>  /* AVX-512 Foundation */
> @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
>  FPReg fpregs[8];
>  /* KVM-only so far */
>  uint16_t fpop;
> +uint16_t fpcs;
> +uint16_t fpds;
>  uint64_t fpip;
>  uint64_t fpdp;
>  
> diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> index 60ed93520a..f1a8717ed8 100644
> --- a/target/i386/tcg/fpu_helper.c
> +++ b/target/i386/tcg/fpu_helper.c
> @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
>  {
>  env->fpus = 0;
>  env->fpstt = 0;
> +env->fpcs = 0;
> +env->fpip = 0;
> +env->fpds = 0;
> +env->fpdp = 0;
>  cpu_set_fpuc(env, 0x37f);
>  env->fptags[0] = 1;
>  env->fptags[1] = 1;
> @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong 
> ptr, int data32,
>  {
>  int fpus, fptag, exp, i;
>  uint64_t mant;
> +uint16_t fpcs, fpds;
>  CPU_LDoubleU tmp;
>  
>  fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, target_ulong 
> ptr, int data32,
>  }
>  }
>  }
> +
> +/*
> + * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> + * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> + * FCS and FDS; it saves each as H.
> + */
> +if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> +&& (env->cr[0] & CR0_PE_MASK)) {
> +fpcs = env->fpcs;
> +fpds = env->fpds;


If you want to start supporting this feature flag, I suggest
moving this to a separate patch.  The description of this patch
seems to imply it is just a bug fix, not the implementation of a
new feature flag.

When adding support for a new feature flag in TCG code, you need
to extend TCG_*_FEATURES in target/i386/cpu.c, otherwise the
feature flag will never be enabled by the CPU configuration code.


> +} else {
> +fpcs = 0;
> +fpds = 0;
> +}
> +
>  if (data32) {
>  /* 32 bit */
>  cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
>  cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
>  cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> -cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> -cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> -cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> -cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
> +cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */
> +cpu_stl_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
> +cpu_stl_data_ra(env, ptr + 20, env->fpdp, retaddr); /* fpdp */
> +cpu_stl_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
>  } else {
>  /* 16 bit */
>  cpu_stw_data_ra(env, ptr, env->fpuc, retaddr);
>  cpu_stw_data_ra(env, ptr + 2, fpus, retaddr);
>  cpu_stw_data_ra(env, ptr + 4, fptag, retaddr);
> -cpu_stw_data_ra(env, ptr + 6, 0, retaddr);
> -cpu_stw_data_ra(env, ptr + 8, 0, retaddr);
> -cpu_stw_data_ra(env, ptr + 10, 0, retaddr);
> -

Re: [PATCH v4 2/2] target/i386: Correct implementation for FCS, FIP, FDS and FDP

2021-05-13 Thread Ziqiao Kong
Ping.

On 5/7/21, Ziqiao Kong  wrote:
> Changes since v3:
>  - Split the long patches to series to make review easier.
>  - Fix the coding style problems in v3.
>
> Changes since v2:
>  - Change the sequence of fpcs, fpds, fpip and fpdp in CPUX86State.
>  - Use stl instead of stw in do_fstenv.
>  - Move variables to floats instruction case block.
>  - Move last accessed memory operand to a temp variable to avoid another
> load.
>  - Move segment selectors instead of segment base to fpcs and fpds.
>  - Fix some code stype problems for the original code in floats case block.
>
> Signed-off-by: Ziqiao Kong 
> ---
>  target/i386/cpu.h|  4 +++
>  target/i386/tcg/fpu_helper.c | 48 ++--
>  target/i386/tcg/translate.c  | 45 -
>  3 files changed, 77 insertions(+), 20 deletions(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 570f916878..241945320b 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -705,6 +705,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EBX_INVPCID   (1U << 10)
>  /* Restricted Transactional Memory */
>  #define CPUID_7_0_EBX_RTM   (1U << 11)
> +/* Deprecates FPU CS and FPU DS values */
> +#define CPUID_7_0_EBX_FCS_FDS   (1U << 13)
>  /* Memory Protection Extension */
>  #define CPUID_7_0_EBX_MPX   (1U << 14)
>  /* AVX-512 Foundation */
> @@ -1440,6 +1442,8 @@ typedef struct CPUX86State {
>  FPReg fpregs[8];
>  /* KVM-only so far */
>  uint16_t fpop;
> +uint16_t fpcs;
> +uint16_t fpds;
>  uint64_t fpip;
>  uint64_t fpdp;
>
> diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
> index 60ed93520a..f1a8717ed8 100644
> --- a/target/i386/tcg/fpu_helper.c
> +++ b/target/i386/tcg/fpu_helper.c
> @@ -766,6 +766,10 @@ void helper_fninit(CPUX86State *env)
>  {
>  env->fpus = 0;
>  env->fpstt = 0;
> +env->fpcs = 0;
> +env->fpip = 0;
> +env->fpds = 0;
> +env->fpdp = 0;
>  cpu_set_fpuc(env, 0x37f);
>  env->fptags[0] = 1;
>  env->fptags[1] = 1;
> @@ -2368,6 +2372,7 @@ static void do_fstenv(CPUX86State *env, target_ulong
> ptr, int data32,
>  {
>  int fpus, fptag, exp, i;
>  uint64_t mant;
> +uint16_t fpcs, fpds;
>  CPU_LDoubleU tmp;
>
>  fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
> @@ -2390,24 +2395,39 @@ static void do_fstenv(CPUX86State *env, target_ulong
> ptr, int data32,
>  }
>  }
>  }
> +
> +/*
> + * If CR0.PE = 1, each instruction saves FCS and FDS into memory. If
> + * CPUID.(EAX=07H,ECX=0H):EBX[bit 13] = 1, the processor deprecates
> + * FCS and FDS; it saves each as H.
> + */
> +if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_FCS_FDS)
> +&& (env->cr[0] & CR0_PE_MASK)) {
> +fpcs = env->fpcs;
> +fpds = env->fpds;
> +} else {
> +fpcs = 0;
> +fpds = 0;
> +}
> +
>  if (data32) {
>  /* 32 bit */
>  cpu_stl_data_ra(env, ptr, env->fpuc, retaddr);
>  cpu_stl_data_ra(env, ptr + 4, fpus, retaddr);
>  cpu_stl_data_ra(env, ptr + 8, fptag, retaddr);
> -cpu_stl_data_ra(env, ptr + 12, 0, retaddr); /* fpip */
> -cpu_stl_data_ra(env, ptr + 16, 0, retaddr); /* fpcs */
> -cpu_stl_data_ra(env, ptr + 20, 0, retaddr); /* fpoo */
> -cpu_stl_data_ra(env, ptr + 24, 0, retaddr); /* fpos */
> +cpu_stl_data_ra(env, ptr + 12, env->fpip, retaddr); /* fpip */
> +cpu_stl_data_ra(env, ptr + 16, fpcs, retaddr); /* fpcs */
> +cpu_stl_data_ra(env, ptr + 20, env->fpdp, retaddr); /* fpdp */
> +cpu_stl_data_ra(env, ptr + 24, fpds, retaddr); /* fpds */
>  } else {
>  /* 16 bit */
>  cpu_stw_data_ra(env, ptr, env->fpuc, retaddr);
>  cpu_stw_data_ra(env, ptr + 2, fpus, retaddr);
>  cpu_stw_data_ra(env, ptr + 4, fptag, retaddr);
> -cpu_stw_data_ra(env, ptr + 6, 0, retaddr);
> -cpu_stw_data_ra(env, ptr + 8, 0, retaddr);
> -cpu_stw_data_ra(env, ptr + 10, 0, retaddr);
> -cpu_stw_data_ra(env, ptr + 12, 0, retaddr);
> +cpu_stw_data_ra(env, ptr + 6, env->fpip, retaddr);
> +cpu_stw_data_ra(env, ptr + 8, fpcs, retaddr);
> +cpu_stw_data_ra(env, ptr + 10, env->fpdp, retaddr);
> +cpu_stw_data_ra(env, ptr + 12, fpds, retaddr);
>  }
>  }
>
> @@ -2473,17 +2493,7 @@ void helper_fsave(CPUX86State *env, target_ulong ptr,
> int data32)
>  }
>
>  /* fninit */
> -env->fpus = 0;
> -env->fpstt = 0;
> -cpu_set_fpuc(env, 0x37f);
> -env->fptags[0] = 1;
> -env->fptags[1] = 1;
> -env->fptags[2] = 1;
> -env->fptags[3] = 1;
> -env->fptags[4] = 1;
> -env->fptags[5] = 1;
> -env->fptags[6] = 1;
> -env->fptags[7] = 1;
> +helper_fninit(env);
>  }
>
>  void helper_frstor(CPUX86State *env, target_ulong ptr, int data32)
> diff --git