xstate_alloc_save_area() configures FCW and MXCSR to #RESET values but misses
FTW. Fixing this means that the backing memory always has an architecturally
correct value.
Adjust the comment to state that it's the #RESET values which we care about.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
I don't understand what the rest of the comment is trying to say, so have left
it alone. There's still a lot of cleanup to be done to merge i387 and xstate.
---
xen/arch/x86/xstate.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index e990abc9d18c..747df0b2e9a9 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -550,11 +550,12 @@ int xstate_alloc_save_area(struct vcpu *v)
return -ENOMEM;
/*
- * Set the memory image to default values, but don't force the context
+ * Set the memory image to #RESET values, but don't force the context
* to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv
* clear).
*/
save_area->fpu_sse.fcw = FCW_DEFAULT;
+ save_area->fpu_sse.ftw = FXSAVE_FTW_RESET;
save_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
v->arch.xsave_area = save_area;
--
2.39.5
On 26.03.2026 20:04, Andrew Cooper wrote: > xstate_alloc_save_area() configures FCW and MXCSR to #RESET values but misses > FTW. Fixing this means that the backing memory always has an architecturally > correct value. > > Adjust the comment to state that it's the #RESET values which we care about. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> The title using "as well as" reads to me as if both are being fixed, when really you bring FTW in line with FCW. Preferably with this adjusted to be unambiguous: Reviewed-by: Jan Beulich <jbeulich@suse.com> > I don't understand what the rest of the comment is trying to say, so have left > it alone. There's still a lot of cleanup to be done to merge i387 and xstate. I think it tries to say that the values put in memory aren't actually going to be used by a subsequent XRSTOR, by it putting the respective registers into init-state without reading memory. Jan
On 3/26/26 7:04 PM, Andrew Cooper wrote: > xstate_alloc_save_area() configures FCW and MXCSR to #RESET values but misses > FTW. Fixing this means that the backing memory always has an architecturally > correct value. > > Adjust the comment to state that it's the #RESET values which we care about. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Ross Lagerwall <ross.lagerwall@citrix.com> > > I don't understand what the rest of the comment is trying to say, so have left > it alone. There's still a lot of cleanup to be done to merge i387 and xstate. > --- > xen/arch/x86/xstate.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c > index e990abc9d18c..747df0b2e9a9 100644 > --- a/xen/arch/x86/xstate.c > +++ b/xen/arch/x86/xstate.c > @@ -550,11 +550,12 @@ int xstate_alloc_save_area(struct vcpu *v) > return -ENOMEM; > > /* > - * Set the memory image to default values, but don't force the context > + * Set the memory image to #RESET values, but don't force the context > * to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv > * clear). > */ > save_area->fpu_sse.fcw = FCW_DEFAULT; > + save_area->fpu_sse.ftw = FXSAVE_FTW_RESET; > save_area->fpu_sse.mxcsr = MXCSR_DEFAULT; > > v->arch.xsave_area = save_area; Is this comment correct given that it is initializing FCW to FCW_DEFAULT which is different from FCW_RESET? As they seem to be mostly doing the same thing, could we call vcpu_reset_fpu() here instead? Ross
On 27.03.2026 11:04, Ross Lagerwall wrote: > On 3/26/26 7:04 PM, Andrew Cooper wrote: >> xstate_alloc_save_area() configures FCW and MXCSR to #RESET values but misses >> FTW. Fixing this means that the backing memory always has an architecturally >> correct value. >> >> Adjust the comment to state that it's the #RESET values which we care about. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Ross Lagerwall <ross.lagerwall@citrix.com> >> >> I don't understand what the rest of the comment is trying to say, so have left >> it alone. There's still a lot of cleanup to be done to merge i387 and xstate. >> --- >> xen/arch/x86/xstate.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c >> index e990abc9d18c..747df0b2e9a9 100644 >> --- a/xen/arch/x86/xstate.c >> +++ b/xen/arch/x86/xstate.c >> @@ -550,11 +550,12 @@ int xstate_alloc_save_area(struct vcpu *v) >> return -ENOMEM; >> >> /* >> - * Set the memory image to default values, but don't force the context >> + * Set the memory image to #RESET values, but don't force the context >> * to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv >> * clear). >> */ >> save_area->fpu_sse.fcw = FCW_DEFAULT; >> + save_area->fpu_sse.ftw = FXSAVE_FTW_RESET; >> save_area->fpu_sse.mxcsr = MXCSR_DEFAULT; >> >> v->arch.xsave_area = save_area; > > Is this comment correct given that it is initializing FCW to FCW_DEFAULT > which is different from FCW_RESET? Is the goal here to represent XSAVE init-state in memory, or do we truly mean #RESET state (in which case FCW_RESET would need using, and in which case leaving xstate_bv bit 0 clear would be wrong). Jan
On 27/03/2026 10:17 am, Jan Beulich wrote: > On 27.03.2026 11:04, Ross Lagerwall wrote: >> On 3/26/26 7:04 PM, Andrew Cooper wrote: >>> xstate_alloc_save_area() configures FCW and MXCSR to #RESET values but misses >>> FTW. Fixing this means that the backing memory always has an architecturally >>> correct value. >>> >>> Adjust the comment to state that it's the #RESET values which we care about. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Ross Lagerwall <ross.lagerwall@citrix.com> >>> >>> I don't understand what the rest of the comment is trying to say, so have left >>> it alone. There's still a lot of cleanup to be done to merge i387 and xstate. >>> --- >>> xen/arch/x86/xstate.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c >>> index e990abc9d18c..747df0b2e9a9 100644 >>> --- a/xen/arch/x86/xstate.c >>> +++ b/xen/arch/x86/xstate.c >>> @@ -550,11 +550,12 @@ int xstate_alloc_save_area(struct vcpu *v) >>> return -ENOMEM; >>> >>> /* >>> - * Set the memory image to default values, but don't force the context >>> + * Set the memory image to #RESET values, but don't force the context >>> * to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv >>> * clear). >>> */ >>> save_area->fpu_sse.fcw = FCW_DEFAULT; >>> + save_area->fpu_sse.ftw = FXSAVE_FTW_RESET; >>> save_area->fpu_sse.mxcsr = MXCSR_DEFAULT; >>> >>> v->arch.xsave_area = save_area; >> Is this comment correct given that it is initializing FCW to FCW_DEFAULT >> which is different from FCW_RESET? > Is the goal here to represent XSAVE init-state in memory, or do we truly mean > #RESET state (in which case FCW_RESET would need using, and in which case > leaving xstate_bv bit 0 clear would be wrong). We're creating the vCPU, so conceptually I think #RESET state is what we want. But, I'd forgotten that #RESET and #INIT FCW are different. Also, we don't really want to be taking the overhead of keeping this properly at the #RESET state until the guest executes an FNINIT/etc. I think it's better to keep using DEFAULT here. I'll rework the comment and commit message. This also suggests that we want to rethink vcpu_reset_fpu(), but we can leave that for later. ~Andrew
Right now, xstate_alloc_save_area() leaves both XSTATE_BV and FTW clear. This
causes a difference in behaviour between FXRSTOR and XRSTOR.
Switch to using using XSTATE's idea of initial configuration which will behave
the same even on pre-XSAVE hardware. Expand the comment to explain why.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
v2:
* Rewrite the commmit message and comment.
---
xen/arch/x86/xstate.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index e990abc9d18c..defe9b3f0cbe 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -550,11 +550,22 @@ int xstate_alloc_save_area(struct vcpu *v)
return -ENOMEM;
/*
- * Set the memory image to default values, but don't force the context
- * to be loaded from memory (i.e. keep save_area->xsave_hdr.xstate_bv
- * clear).
+ * We're creating a vCPU, so conceptually we should be choosing the
+ * architectural #RESET values.
+ *
+ * However for historical reasons of configuring the external
+ * co-processor, FCW's #RESET state is different to what F(N)INIT and
+ * XSTATE consider the "initial configuration".
+ *
+ * Guests won't care about the difference; all software tends to executes
+ * FNINIT very early during setup.
+ *
+ * Use XSTATE's idea of initial configuration. This allows XSTATE_BV to
+ * remain clear and for CPUs to use the INIT optimisation where
+ * applicable.
*/
save_area->fpu_sse.fcw = FCW_DEFAULT;
+ save_area->fpu_sse.ftw = FXSAVE_FTW_RESET;
save_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
v->arch.xsave_area = save_area;
--
2.39.5
© 2016 - 2026 Red Hat, Inc.