Re: [PATCH v3 08/12] x86/emulator: Refactor FXSAVE_AREA to use wrappers

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

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@xxxxxxxxx>
>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

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