[PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[Andrew Cooper]

validate_xsave() is call from the HVM and PV codepaths which load new vcpu
xsave state, usually as part of migration.  In both cases, this is the
xfeature_mask of the saving Xen rather than the restoring Xen.

Given that the xsave state itself is checked for consistency and validity on
the current cpus, checking whether it was valid for the cpu before migration
is not interesting (or indeed relevant, as the error can't be distinguished
from the other validity checking).

This change removes the need to pass the saving Xen's xfeature_mask,
simplifying the toolstack code and migration stream format in this area.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domctl.c                  |    3 +--
 xen/arch/x86/hvm/hvm.c                 |    3 +--
 xen/arch/x86/xstate.c                  |    5 ++---
 xen/include/asm-x86/xstate.h           |    3 +--
 xen/include/public/arch-x86/hvm/save.h |    2 +-
 xen/include/public/domctl.h            |    2 +-
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index f8b0a79..1285dd0 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1247,8 +1247,7 @@ long arch_do_domctl(
             {
                 if ( evc->size >= 2 * sizeof(uint64_t) + XSTATE_AREA_MIN_SIZE )
                     ret = validate_xstate(_xcr0, _xcr0_accum,
-                                          _xsave_area->xsave_hdr.xstate_bv,
-                                          evc->xfeature_mask);
+                                          _xsave_area->xsave_hdr.xstate_bv);
             }
             else if ( !_xcr0 )
                 ret = 0;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index efbf6d9..c797c0c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1434,8 +1434,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
     h->cur += desc->length;
 
     err = validate_xstate(ctxt->xcr0, ctxt->xcr0_accum,
-                          ctxt->save_area.xsave_hdr.xstate_bv,
-                          ctxt->xfeature_mask);
+                          ctxt->save_area.xsave_hdr.xstate_bv);
     if ( err )
     {
         printk(XENLOG_G_WARNING
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 7d9b92b..e202344 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -346,10 +346,9 @@ static bool_t valid_xcr0(u64 xcr0)
     return !(xcr0 & XSTATE_BNDREGS) == !(xcr0 & XSTATE_BNDCSR);
 }
 
-int validate_xstate(u64 xcr0, u64 xcr0_accum, u64 xstate_bv, u64 xfeat_mask)
+int validate_xstate(u64 xcr0, u64 xcr0_accum, u64 xstate_bv)
 {
-    if ( (xcr0_accum & ~xfeat_mask) ||
-         (xstate_bv & ~xcr0_accum) ||
+    if ( (xstate_bv & ~xcr0_accum) ||
          (xcr0 & ~xcr0_accum) ||
          !valid_xcr0(xcr0) ||
          !valid_xcr0(xcr0_accum) )
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 1a92ac3..8d21349 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -81,8 +81,7 @@ uint64_t get_xcr0(void);
 void xsave(struct vcpu *v, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
 bool_t xsave_enabled(const struct vcpu *v);
-int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum, u64 xstate_bv,
-                                 u64 xfeat_mask);
+int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum, u64 xstate_bv);
 int __must_check handle_xsetbv(u32 index, u64 new_bv);
 
 /* extended state init and cleanup functions */
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 5a13795..16d85a3 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -544,7 +544,7 @@ DECLARE_HVM_SAVE_TYPE(MTRR, 14, struct hvm_hw_mtrr);
  */
 
 struct hvm_hw_cpu_xsave {
-    uint64_t xfeature_mask;
+    uint64_t xfeature_mask;        /* Ignored */
     uint64_t xcr0;                 /* Updated by XSETBV */
     uint64_t xcr0_accum;           /* Updated by XSETBV */
     struct {
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 565fa4c..385b053 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -839,7 +839,7 @@ struct xen_domctl_vcpuextstate {
     /* IN: VCPU that this call applies to. */
     uint32_t         vcpu;
     /*
-     * SET: xfeature support mask of struct (IN)
+     * SET: Ignored.
      * GET: xfeature support mask of struct (IN/OUT)
      * xfeature mask is served as identifications of the saving format
      * so that compatible CPUs can have a check on format to decide
-- 
1.7.10.4

Re: [PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 05/30/14 10:39 AM >>>
>validate_xsave() is call from the HVM and PV codepaths which load new vcpu
>xsave state, usually as part of migration.  In both cases, this is the
>xfeature_mask of the saving Xen rather than the restoring Xen.
>
>Given that the xsave state itself is checked for consistency and validity on
>the current cpus, checking whether it was valid for the cpu before migration
>is not interesting (or indeed relevant, as the error can't be distinguished
>from the other validity checking).

While I'm not entirely opposed to this, I'm also not fully convinced - the data
being available, it can as well be used for additional sanity checking.

Jan

Re: [PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[Andrew Cooper]

On 30/05/2014 12:39, Jan Beulich wrote:
>>>> Andrew Cooper <andrew.cooper3@citrix.com> 05/30/14 10:39 AM >>>
>> validate_xsave() is call from the HVM and PV codepaths which load new vcpu
>> xsave state, usually as part of migration.  In both cases, this is the
>> xfeature_mask of the saving Xen rather than the restoring Xen.
>>
>> Given that the xsave state itself is checked for consistency and validity on
>> the current cpus, checking whether it was valid for the cpu before migration
>> is not interesting (or indeed relevant, as the error can't be distinguished
> >from the other validity checking).
>
> While I'm not entirely opposed to this, I'm also not fully convinced - the data
> being available, it can as well be used for additional sanity checking.
>
> Jan
>

What further sanity checking would be wanted/needed?

The sending Xen must have gotten this correct else it wouldn't have an 
xsave area to send in the first place.  If the receiving the Xen found 
parts it didn't like, the local validity checks would fail.

As far as I can see, the only case this might do something unexpected is 
if the individual xfeature_mask got changed on transit, at which point 
the receiving Xen would fail the xsave load, despite the xsave area 
being valid for the current cpu.

~Andrew

Re: [PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[Jan Beulich]

>>> On 30.05.14 at 13:57, <andrew.cooper3@citrix.com> wrote:
> What further sanity checking would be wanted/needed?
> 
> The sending Xen must have gotten this correct else it wouldn't have an 
> xsave area to send in the first place.  If the receiving the Xen found 
> parts it didn't like, the local validity checks would fail.
> 
> As far as I can see, the only case this might do something unexpected is 
> if the individual xfeature_mask got changed on transit, at which point 
> the receiving Xen would fail the xsave load, despite the xsave area 
> being valid for the current cpu.

Whether the loading would fail really depends on what exactly became
corrupted.

But in the end the question is - are you intending to no longer
communicate this bit of information in the v2 migration stream?

Jan

Re: [PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[Andrew Cooper]

On 02/06/14 07:43, Jan Beulich wrote:
>>>> On 30.05.14 at 13:57, <andrew.cooper3@citrix.com> wrote:
>> What further sanity checking would be wanted/needed?
>>
>> The sending Xen must have gotten this correct else it wouldn't have an 
>> xsave area to send in the first place.  If the receiving the Xen found 
>> parts it didn't like, the local validity checks would fail.
>>
>> As far as I can see, the only case this might do something unexpected is 
>> if the individual xfeature_mask got changed on transit, at which point 
>> the receiving Xen would fail the xsave load, despite the xsave area 
>> being valid for the current cpu.
> Whether the loading would fail really depends on what exactly became
> corrupted.

The current behaviour is that the load would fail, as validate_xstate()
would fail.

>
> But in the end the question is - are you intending to no longer
> communicate this bit of information in the v2 migration stream?
>
> Jan
>

Ideally I would like to cease communicating this in the v2 stream.

~Andrew

Re: [PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[Jan Beulich]

>>> On 02.06.14 at 12:07, <andrew.cooper3@citrix.com> wrote:
> On 02/06/14 07:43, Jan Beulich wrote:
>>>>> On 30.05.14 at 13:57, <andrew.cooper3@citrix.com> wrote:
>>> What further sanity checking would be wanted/needed?
>>>
>>> The sending Xen must have gotten this correct else it wouldn't have an 
>>> xsave area to send in the first place.  If the receiving the Xen found 
>>> parts it didn't like, the local validity checks would fail.
>>>
>>> As far as I can see, the only case this might do something unexpected is 
>>> if the individual xfeature_mask got changed on transit, at which point 
>>> the receiving Xen would fail the xsave load, despite the xsave area 
>>> being valid for the current cpu.
>> Whether the loading would fail really depends on what exactly became
>> corrupted.
> 
> The current behaviour is that the load would fail, as validate_xstate()
> would fail.

Oh, then we meant different things with "load" (I was assuming you
meant the actual xrstor).

>> But in the end the question is - are you intending to no longer
>> communicate this bit of information in the v2 migration stream?
> 
> Ideally I would like to cease communicating this in the v2 stream.

Okay, on that basis I'm willing to take the patch; any chance you
could add a note to this effect to the commit message?

Jan

Re: [PATCH] x86/xsave: Remove xfeat_mask checking from validate_xstate()

[Andrew Cooper]

On 02/06/14 11:43, Jan Beulich wrote:
>>>> On 02.06.14 at 12:07, <andrew.cooper3@citrix.com> wrote:
>> On 02/06/14 07:43, Jan Beulich wrote:
>>>>>> On 30.05.14 at 13:57, <andrew.cooper3@citrix.com> wrote:
>>>> What further sanity checking would be wanted/needed?
>>>>
>>>> The sending Xen must have gotten this correct else it wouldn't have an 
>>>> xsave area to send in the first place.  If the receiving the Xen found 
>>>> parts it didn't like, the local validity checks would fail.
>>>>
>>>> As far as I can see, the only case this might do something unexpected is 
>>>> if the individual xfeature_mask got changed on transit, at which point 
>>>> the receiving Xen would fail the xsave load, despite the xsave area 
>>>> being valid for the current cpu.
>>> Whether the loading would fail really depends on what exactly became
>>> corrupted.
>> The current behaviour is that the load would fail, as validate_xstate()
>> would fail.
> Oh, then we meant different things with "load" (I was assuming you
> meant the actual xrstor).

Ah no.  This is strictly only for loading the vcpu xsave state using
XEN_DOMCTL_setvcpuextstate or XEN_DOMCTL_sethvmcontext

There is no change for guest xsave interaction with Xen.


>
>>> But in the end the question is - are you intending to no longer
>>> communicate this bit of information in the v2 migration stream?
>> Ideally I would like to cease communicating this in the v2 stream.
> Okay, on that basis I'm willing to take the patch; any chance you
> could add a note to this effect to the commit message?
>
> Jan
>

I will reword it.

~Andrew