diff mbox

[V7,2/3] x86/xsaves: fix two remained issues

Message ID 1459414657-7558-3-git-send-email-shuai.ruan@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shuai Ruan March 31, 2016, 8:57 a.m. UTC 
From: Shuai Ruan <shuai.ruan@intel.com>

1. get_xsave_addr() will only be called when
xsave_area_compressed(xsave) is true. So drop the
conditional expression.

2. expand_xsave_states() will memset the area when
get NULL from get_xsave_addr().

Signed-off-by: Shuai Ruan <shuai.ruan@intel.com>
---
 xen/arch/x86/xstate.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Jan Beulich April 4, 2016, 4:03 p.m. UTC | #1 
>>> On 31.03.16 at 10:57, <shuai.ruan@linux.intel.com> wrote:

Considering this isn't the last patch in the series, the subject isn't
really nice (apart from being mis-spelled). If you e.g. replaced
"remained" by "miscellaneous", I wouldn't insist on splitting.

> 1. get_xsave_addr() will only be called when
> xsave_area_compressed(xsave) is true. So drop the
> conditional expression.
> 
> 2. expand_xsave_states() will memset the area when
> get NULL from get_xsave_addr().

Reported-by: ...

> Signed-off-by: Shuai Ruan <shuai.ruan@intel.com>
> 
>  xen/arch/x86/xstate.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index 8c652bc..f4ea54d 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -164,12 +164,8 @@ static void *get_xsave_addr(struct xsave_struct *xsave,
>                              const uint16_t *comp_offsets,
>                              unsigned int xfeature_idx)
>  {
> -    if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
> -        return NULL;
> -
> -    return (void *)xsave + (xsave_area_compressed(xsave) ?
> -                            comp_offsets[xfeature_idx] :
> -                            xstate_offsets[xfeature_idx]);
> +    return (1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv ?
> +           (void *)xsave + comp_offsets[xfeature_idx] : NULL;
>  }

I would really have expected an ASSERT() to get added as
(documenting) replacement.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 8c652bc..f4ea54d 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -164,12 +164,8 @@  static void *get_xsave_addr(struct xsave_struct *xsave,
                             const uint16_t *comp_offsets,
                             unsigned int xfeature_idx)
 {
-    if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
-        return NULL;
-
-    return (void *)xsave + (xsave_area_compressed(xsave) ?
-                            comp_offsets[xfeature_idx] :
-                            xstate_offsets[xfeature_idx]);
+    return (1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv ?
+           (void *)xsave + comp_offsets[xfeature_idx] : NULL;
 }
 
 void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
@@ -211,6 +207,8 @@  void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
             ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
             memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
         }
+        else
+            memset(dest + xstate_offsets[index], 0, xstate_sizes[index]);
 
         valid &= ~feature;
     }