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->v3:
  * Fixed typo in first parameter of UNMAP_FXSAVE_AREA.
  * Added Parenthesis around "x" in #else's UNMAP_FXSAVE_AREA(x)

v1->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..11b16aeaec39 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(current, 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.1

On 10.01.2025 14:28, Alejandro Vallejo wrote:
> 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>

Acked-by: Jan Beulich <jbeulich@suse.com>
despite ...

> --- 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(current, x)

... the comment here kind of suggesting that ...

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

... use of this new construct is solely decoration, and could hence as
well be omitted.

Jan

On Mon Jan 27, 2025 at 10:52 AM GMT, Jan Beulich wrote:
> On 10.01.2025 14:28, Alejandro Vallejo wrote:
> > 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>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,

> despite ...
>
> > --- 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(current, x)
>
> ... the comment here kind of suggesting that ...
>
> >  # else
> >  #  define FXSAVE_AREA get_fpu_save_area()
> > +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
>
> ... use of this new construct is solely decoration, and could hence as
> well be omitted.
>
> Jan

It seems like a dangerous proposition to abuse knowledge of an implementation
in order to skip parts of its interface. The fact that no such map is required
at this point in x86_emulate does not mean it never will be. Predicting the
future is hard, but being consistent today is less so (imo).

Cheers,
Alejandro

On 27.01.2025 16:42, Alejandro Vallejo wrote:
> On Mon Jan 27, 2025 at 10:52 AM GMT, Jan Beulich wrote:
>> On 10.01.2025 14:28, Alejandro Vallejo wrote:
>>> 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>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks,
> 
>> despite ...
>>
>>> --- 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(current, x)
>>
>> ... the comment here kind of suggesting that ...
>>
>>>  # else
>>>  #  define FXSAVE_AREA get_fpu_save_area()
>>> +#  define UNMAP_FXSAVE_AREA(x) ((void)(x))
>>
>> ... use of this new construct is solely decoration, and could hence as
>> well be omitted.
> 
> It seems like a dangerous proposition to abuse knowledge of an implementation
> in order to skip parts of its interface. The fact that no such map is required
> at this point in x86_emulate does not mean it never will be. Predicting the
> future is hard, but being consistent today is less so (imo).

Entirely true. How likely do you consider it though that with a future
change altering that property, the comment above would be left in place
(and hence then be stale)? My take: Very likely, as long as the two
"current" uses wouldn't need altering.

Yet FTAOD: I'm not asking for any adjustment here, I'm merely mentioning
an observation.

Jan