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.vall...@cloud.com> >> >> Acked-by: Jan Beulich <jbeul...@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 *)¤t->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
