Message ID | 20250110132823.24348-9-alejandro.vallejo@cloud.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | x86: Address Space Isolation FPU preparations | expand |
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 *)¤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. 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 *)¤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. > > 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 *)¤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
On 10.01.2025 14:28, 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 *)¤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) > # else > # define FXSAVE_AREA get_fpu_save_area() > +# define UNMAP_FXSAVE_AREA(x) ((void)(x)) > # endif > #endif While preparing to commit this I felt a little uneasy. The mapping aspect is ... > @@ -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; > } > ... is entirely invisible at the use sites. This imo calls for making mistakes, and hence the existing macro better would be adjusted to become MAP_FXSAVE_AREA(). Jan
On Wed Mar 5, 2025 at 3:29 PM GMT, Jan Beulich wrote: > On 10.01.2025 14:28, 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 *)¤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) > > # else > > # define FXSAVE_AREA get_fpu_save_area() > > +# define UNMAP_FXSAVE_AREA(x) ((void)(x)) > > # endif > > #endif > > While preparing to commit this I felt a little uneasy. The mapping aspect > is ... Thanks for coming back to it :) > > > @@ -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; > > } > > > > ... is entirely invisible at the use sites. This imo calls for making > mistakes, and hence the existing macro better would be adjusted to become > MAP_FXSAVE_AREA(). > > Jan I prefer it that way too; I was simply trying to minimize the diff. Would you like me to resend it with that adjustment? Cheers, Alejandro
On 05.03.2025 17:16, Alejandro Vallejo wrote: > On Wed Mar 5, 2025 at 3:29 PM GMT, Jan Beulich wrote: >> On 10.01.2025 14:28, 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 *)¤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) >>> # else >>> # define FXSAVE_AREA get_fpu_save_area() >>> +# define UNMAP_FXSAVE_AREA(x) ((void)(x)) >>> # endif >>> #endif >> >> While preparing to commit this I felt a little uneasy. The mapping aspect >> is ... > > Thanks for coming back to it :) > >> >>> @@ -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; >>> } >>> >> >> ... is entirely invisible at the use sites. This imo calls for making >> mistakes, and hence the existing macro better would be adjusted to become >> MAP_FXSAVE_AREA(). > > I prefer it that way too; I was simply trying to minimize the diff. Would you > like me to resend it with that adjustment? Yes please. Plus whatever else adjustments are needed in the subsequent patches (sorry, it has been a while, so I don't recall the state of those later ones). Jan
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 *)¤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) # 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; }
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(-)