x86: Fix exceptions for fxsave/fxrstor

This patch corrects the following aspects of exception generation in
fxsave/fxrstor:

* Generate #GP if the operand is not aligned to a 16 byte boundary
* Generate #UD if the LOCK prefix is used
* For CR0.EM = 1 #NM is generated, not #UD

Signed-off-by: Kevin Wolf <mail@kevin-wolf.de>
---
 target-i386/op_helper.c |   10 ++++++++++
 target-i386/translate.c |    8 ++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

On Fri, Oct 02, 2009 at 10:28:57PM +0200, Kevin Wolf wrote:
> This patch corrects the following aspects of exception generation in
> fxsave/fxrstor:
> 
> * Generate #GP if the operand is not aligned to a 16 byte boundary

Agreed.

> * Generate #UD if the LOCK prefix is used

Agreed.

> * For CR0.EM = 1 #NM is generated, not #UD

This does not match the Intel manual:

| #NM If CR0.TS[bit 3] = 1.

| #UD If CR0.EM[bit 2] = 1.
|     If CPUID.01H:EDX.FXSR[bit 24] = 0.
|     If the LOCK prefix is used.

 
> Signed-off-by: Kevin Wolf <mail@kevin-wolf.de>
> ---
>  target-i386/op_helper.c |   10 ++++++++++
>  target-i386/translate.c |    8 ++++----
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
> index 33d44b0..3f05532 100644
> --- a/target-i386/op_helper.c
> +++ b/target-i386/op_helper.c
> @@ -4338,6 +4338,11 @@ void helper_fxsave(target_ulong ptr, int data64)
>      CPU86_LDouble tmp;
>      target_ulong addr;
>  
> +    /* The operand must be 16 byte aligned */
> +    if (ptr & 0xf) {
> +        raise_exception(EXCP0D_GPF);
> +    }
> +
>      fpus = (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11;
>      fptag = 0;
>      for(i = 0; i < 8; i++) {
> @@ -4394,6 +4399,11 @@ void helper_fxrstor(target_ulong ptr, int data64)
>      CPU86_LDouble tmp;
>      target_ulong addr;
>  
> +    /* The operand must be 16 byte aligned */
> +    if (ptr & 0xf) {
> +        raise_exception(EXCP0D_GPF);
> +    }
> +
>      env->fpuc = lduw(ptr);
>      fpus = lduw(ptr + 2);
>      fptag = lduw(ptr + 4);
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 5b11d7f..9af2eed 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7436,9 +7436,9 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
>          switch(op) {
>          case 0: /* fxsave */
>              if (mod == 3 || !(s->cpuid_features & CPUID_FXSR) ||
> -                (s->flags & HF_EM_MASK))
> +                (s->prefix & PREFIX_LOCK))
>                  goto illegal_op;
> -            if (s->flags & HF_TS_MASK) {
> +            if ((s->flags & HF_EM_MASK) || (s->flags & HF_TS_MASK)) {
>                  gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
>                  break;
>              }
> @@ -7450,9 +7450,9 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
>              break;
>          case 1: /* fxrstor */
>              if (mod == 3 || !(s->cpuid_features & CPUID_FXSR) ||
> -                (s->flags & HF_EM_MASK))
> +                (s->prefix & PREFIX_LOCK))
>                  goto illegal_op;
> -            if (s->flags & HF_TS_MASK) {
> +            if ((s->flags & HF_EM_MASK) || (s->flags & HF_TS_MASK)) {
>                  gen_exception(s, EXCP07_PREX, pc_start - s->cs_base);
>                  break;
>              }
> -- 
> 1.6.0.2
> 
> 
> 
>

Am Sonntag, 4. Oktober 2009 12:05 schrieb Aurelien Jarno:
> On Fri, Oct 02, 2009 at 10:28:57PM +0200, Kevin Wolf wrote:
> > This patch corrects the following aspects of exception generation in
> > fxsave/fxrstor:
> >
> > * Generate #GP if the operand is not aligned to a 16 byte boundary
>
> Agreed.
>
> > * Generate #UD if the LOCK prefix is used
>
> Agreed.
>
> > * For CR0.EM = 1 #NM is generated, not #UD
>
> This does not match the Intel manual:
> | #NM If CR0.TS[bit 3] = 1.
> |
> | #UD If CR0.EM[bit 2] = 1.
> |     If CPUID.01H:EDX.FXSR[bit 24] = 0.
> |     If the LOCK prefix is used.
> |

Hm, you seem to have a different Intel manual. In my copy the CR0.EM part 
still belongs to #NM. Also, I ran my test code in KVM for comparision and it 
did generate an #NM (on two different machines, one Intel, one AMD), so I'm 
quite sure this is right (well, at least not completely wrong).

On the other hand, I just had a look at the AMD documentation and it seems to 
support your version... So while my hardware suggests that #NM is right, I'm 
not going to insist on it. Maybe there is some hardware that actually does 
generate #UD.

If you don't like to commit this part of the fix despite my explanation, just 
let me know and I'll resend the patch without it.

Kevin

On Sun, Oct 04, 2009 at 10:43:54PM +0200, Kevin Wolf wrote:
> Am Sonntag, 4. Oktober 2009 12:05 schrieb Aurelien Jarno:
> > On Fri, Oct 02, 2009 at 10:28:57PM +0200, Kevin Wolf wrote:
> > > This patch corrects the following aspects of exception generation in
> > > fxsave/fxrstor:
> > >
> > > * Generate #GP if the operand is not aligned to a 16 byte boundary
> >
> > Agreed.
> >
> > > * Generate #UD if the LOCK prefix is used
> >
> > Agreed.
> >
> > > * For CR0.EM = 1 #NM is generated, not #UD
> >
> > This does not match the Intel manual:
> > | #NM If CR0.TS[bit 3] = 1.
> > |
> > | #UD If CR0.EM[bit 2] = 1.
> > |     If CPUID.01H:EDX.FXSR[bit 24] = 0.
> > |     If the LOCK prefix is used.
> > |
> 
> Hm, you seem to have a different Intel manual. In my copy the CR0.EM part 
> still belongs to #NM. Also, I ran my test code in KVM for comparision and it 
> did generate an #NM (on two different machines, one Intel, one AMD), so I'm 
> quite sure this is right (well, at least not completely wrong).

My copy of the Intel Manual was quite outdated (May 2007). The new
version from September 2009 matches your patch, so I have applied it.
Sorry.