Re: [Xen-devel] [PATCH 1/3] tests/x86emul: Helpers to save and restore FPU state

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/03/18 13:38, Jan Beulich wrote:
>>>> On 09.03.18 at 13:37, <JBeulich@xxxxxxxx> wrote:
>>>>> On 06.03.18 at 21:24, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +void emul_save_fpu_state(void)
>>> +{
>>> +    if ( use_xsave )
>>> +        asm volatile ( "xsave" __OS " %[ptr]"
>>> +                       : [ptr] "=m" (fpu_save_area)
>>> +                       : "a" (~0ull), "d" (~0ull) );
>> Wait, this doesn't build as 32-bit binary. Needs to be ~0ul, and
>> __OS also can't be used here.
>>
>>> +    else
>>> +        asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );
>> Whereas if you want something like __OS above, you'd want the
>> same here.
> Here's the full incremental diff I've used for now, so that things
> would build everywhere I've tried:
>
> --- unstable.orig/tools/tests/x86_emulator/x86-emulate.c
> +++ unstable/tools/tests/x86_emulator/x86-emulate.c
> @@ -32,20 +32,22 @@ static bool use_xsave;
>  void emul_save_fpu_state(void)
>  {
>      if ( use_xsave )
> -        asm volatile ( "xsave" __OS " %[ptr]"
> +        asm volatile ( "xsave %[ptr]"
>                         : [ptr] "=m" (fpu_save_area)
> -                       : "a" (~0ull), "d" (~0ull) );
> +                       : "a" (~0ul), "d" (~0ul) );
>      else
>          asm volatile ( "fxsave %0" : "=m" (fpu_save_area) );
>  }
>  
>  void emul_restore_fpu_state(void)
>  {
> +    /* Older gcc can't deal with "m" array inputs; make them outputs 
> instead. */
>      if ( use_xsave )
> -        asm volatile ( "xrstor" __OS " %[ptr]"
> -                       :: [ptr] "m" (fpu_save_area), "a" (~0ull), "d" 
> (~0ull) );
> +        asm volatile ( "xrstor %[ptr]"
> +                       : [ptr] "+m" (fpu_save_area)
> +                       : "a" (~0ul), "d" (~0ul) );
>      else
> -        asm volatile ( "fxrstor %0" :: "m" (fpu_save_area) );
> +        asm volatile ( "fxrstor %0" : "+m" (fpu_save_area) );
>  }
>  
>  bool emul_test_init(void)

Ok - I'll merge this in.

My worry with the __OS was to make sure that we matched how the kernel
would save and restore context, so the exception pointers don't get
lost.  However, that only matters at the point that we attempt to
memcmp(), and thinking about it, we'd need a better algorithm anyway.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel