Adds an UNMAP primitive to make use of vcpu_unmap_xsave_area() when
linked into xen. unmap is a no-op during tests.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2:
  * Added comments highlighting fastpath on `current`
---
 xen/arch/x86/x86_emulate/blk.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
index 08a05f8453f7..76fd497ed8a3 100644
--- a/xen/arch/x86/x86_emulate/blk.c
+++ b/xen/arch/x86/x86_emulate/blk.c
@@ -11,9 +11,12 @@
     !defined(X86EMUL_NO_SIMD)
 # ifdef __XEN__
 #  include <asm/xstate.h>
-#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
+/* has a fastpath for `current`, so there's no actual map */
+#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
+#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(currt ent, x)
 # else
 #  define FXSAVE_AREA get_fpu_save_area()
+#  define UNMAP_FXSAVE_AREA(x) ((void)x)
 # endif
 #endif
 
@@ -292,6 +295,9 @@ int x86_emul_blk(
         }
         else
             asm volatile ( "fxrstor %0" :: "m" (*fxsr) );
+
+        UNMAP_FXSAVE_AREA(fxsr);
+
         break;
     }
 
@@ -320,6 +326,9 @@ int x86_emul_blk(
 
         if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
             memcpy(ptr, fxsr, s->op_bytes);
+
+        UNMAP_FXSAVE_AREA(fxsr);
+
         break;
     }
 
-- 
2.47.0

On 05.11.2024 15:33, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/x86_emulate/blk.c
> +++ b/xen/arch/x86/x86_emulate/blk.c
> @@ -11,9 +11,12 @@
>      !defined(X86EMUL_NO_SIMD)
>  # ifdef __XEN__
>  #  include <asm/xstate.h>
> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
> +/* has a fastpath for `current`, so there's no actual map */
> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(currt ent, x)

The typo of the first argument strongly suggests that the macro should
already now evaluate its parameters, also pleasing Misra.

>  # else
>  #  define FXSAVE_AREA get_fpu_save_area()
> +#  define UNMAP_FXSAVE_AREA(x) ((void)x)

If only for consistency and to avoid setting bad precedents - parentheses
please around x.

> @@ -292,6 +295,9 @@ int x86_emul_blk(
>          }
>          else
>              asm volatile ( "fxrstor %0" :: "m" (*fxsr) );
> +
> +        UNMAP_FXSAVE_AREA(fxsr);
> +
>          break;
>      }
>  
> @@ -320,6 +326,9 @@ int x86_emul_blk(
>  
>          if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
>              memcpy(ptr, fxsr, s->op_bytes);
> +
> +        UNMAP_FXSAVE_AREA(fxsr);
> +
>          break;
>      }

So for now the emulator only supports FXSAVE / FXRSTOR. That'll need to change
sooner or later. Is it really appropriate in that light to name the new macro
after FXSAVE, when the underlying machinery uses all XSAVE?

Jan

On Mon Dec 9, 2024 at 4:26 PM GMT, Jan Beulich wrote:
> On 05.11.2024 15:33, Alejandro Vallejo wrote:
> > --- a/xen/arch/x86/x86_emulate/blk.c
> > +++ b/xen/arch/x86/x86_emulate/blk.c
> > @@ -11,9 +11,12 @@
> >      !defined(X86EMUL_NO_SIMD)
> >  # ifdef __XEN__
> >  #  include <asm/xstate.h>
> > -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
> > +/* has a fastpath for `current`, so there's no actual map */
> > +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
> > +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(currt ent, x)
>
> The typo of the first argument strongly suggests that the macro should
> already now evaluate its parameters, also pleasing Misra.

Not an unreasonable takeaway. I can expand as follows instead:

#define VCPU_UNMAP_XSAVE_AREA(v, x) ({ (void)(v); x; })

Thoughts?

>
> >  # else
> >  #  define FXSAVE_AREA get_fpu_save_area()
> > +#  define UNMAP_FXSAVE_AREA(x) ((void)x)
>
> If only for consistency and to avoid setting bad precedents - parentheses
> please around x.

Ack.

>
> > @@ -292,6 +295,9 @@ int x86_emul_blk(
> >          }
> >          else
> >              asm volatile ( "fxrstor %0" :: "m" (*fxsr) );
> > +
> > +        UNMAP_FXSAVE_AREA(fxsr);
> > +
> >          break;
> >      }
> >  
> > @@ -320,6 +326,9 @@ int x86_emul_blk(
> >  
> >          if ( fxsr != ptr ) /* i.e. s->op_bytes < sizeof(*fxsr) */
> >              memcpy(ptr, fxsr, s->op_bytes);
> > +
> > +        UNMAP_FXSAVE_AREA(fxsr);
> > +
> >          break;
> >      }
>
> So for now the emulator only supports FXSAVE / FXRSTOR. That'll need to change
> sooner or later. Is it really appropriate in that light to name the new macro
> after FXSAVE, when the underlying machinery uses all XSAVE?
>
> Jan

I have no strong feeling one way or the other, except that it should mirror the
other macro's name. I'd say it makes more sense to rename _both_ after the
emulator is XSAVE-aware. That the internal machinery is actually XSAVE-based is
an implementation detail largely irrelevant at the call sites.

Cheers,
Alejandro

On 16.12.2024 12:58, Alejandro Vallejo wrote:
> On Mon Dec 9, 2024 at 4:26 PM GMT, Jan Beulich wrote:
>> On 05.11.2024 15:33, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/x86_emulate/blk.c
>>> +++ b/xen/arch/x86/x86_emulate/blk.c
>>> @@ -11,9 +11,12 @@
>>>      !defined(X86EMUL_NO_SIMD)
>>>  # ifdef __XEN__
>>>  #  include <asm/xstate.h>
>>> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
>>> +/* has a fastpath for `current`, so there's no actual map */
>>> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
>>> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(currt ent, x)
>>
>> The typo of the first argument strongly suggests that the macro should
>> already now evaluate its parameters, also pleasing Misra.
> 
> Not an unreasonable takeaway. I can expand as follows instead:
> 
> #define VCPU_UNMAP_XSAVE_AREA(v, x) ({ (void)(v); x; })
> 
> Thoughts?

Why would x not also be cast to void?

Jan

On Mon Dec 16, 2024 at 12:01 PM GMT, Jan Beulich wrote:
> On 16.12.2024 12:58, Alejandro Vallejo wrote:
> > On Mon Dec 9, 2024 at 4:26 PM GMT, Jan Beulich wrote:
> >> On 05.11.2024 15:33, Alejandro Vallejo wrote:
> >>> --- a/xen/arch/x86/x86_emulate/blk.c
> >>> +++ b/xen/arch/x86/x86_emulate/blk.c
> >>> @@ -11,9 +11,12 @@
> >>>      !defined(X86EMUL_NO_SIMD)
> >>>  # ifdef __XEN__
> >>>  #  include <asm/xstate.h>
> >>> -#  define FXSAVE_AREA ((void *)&current->arch.xsave_area->fpu_sse)
> >>> +/* has a fastpath for `current`, so there's no actual map */
> >>> +#  define FXSAVE_AREA ((void *)VCPU_MAP_XSAVE_AREA(current))
> >>> +#  define UNMAP_FXSAVE_AREA(x) VCPU_UNMAP_XSAVE_AREA(currt ent, x)
> >>
> >> The typo of the first argument strongly suggests that the macro should
> >> already now evaluate its parameters, also pleasing Misra.
> > 
> > Not an unreasonable takeaway. I can expand as follows instead:
> > 
> > #define VCPU_UNMAP_XSAVE_AREA(v, x) ({ (void)(v); x; })
> > 
> > Thoughts?
>
> Why would x not also be cast to void?
>
> Jan

it should have the "x = NULL", in fact.

  #define VCPU_UNMAP_XSAVE_AREA(v, x) do { (void)(v); (x) = NULL; } while(0)

Regardless, the important bit was adding a separate statement for (v) casted to
void. Leaving the rest as-is.

Cheers,
Alejandro