Re: [Xen-devel] [V4] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.16 at 09:22, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -948,7 +948,7 @@ int arch_set_info_guest(
>          fpu_sse->fcw = FCW_DEFAULT;
>          fpu_sse->mxcsr = MXCSR_DEFAULT;
>      }
> -    if ( cpu_has_xsaves )
> +    if ( using_xsave_compact )
>      {
>          ASSERT(v->arch.xsave_area);
>          v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED |

With the common pattern being that cpu_has_xsaves ||
cpu_has_xsavec gets replaced by using_xsave_compact this
looks wrong - imo it needs to be using_xsave_compact &&
cpu_has_xsaves, since when using plain XRSTOR we don't
need to set the compaction bit. The same would apply
elsewhere in the patch.

> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -118,7 +118,21 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu 
> *v)
>      if ( v->fpu_dirtied )
>          return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
>  
> -    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;

Considering this being deleted, wouldn't you better add
ASSERT(v->arch.nonlazy_xstate_used) in its stead, making
more obvious that (taking into account the other return path
above) in the using_xsave_compact case both paths will
produce XSTATE_ALL?

> +    /*
> +     * The offsets of components which live in the extended region of
> +     * compact xsave area are not fixed. Xsave area may be overwritten
> +     * when v->fpu_dirtied set is followed by one with v->fpu_dirtied
> +     * clear.

I think this sentence still lack a few words, e.g. "... when a save
with v->fpu_dirtied set is followed ..."

> +     * In such case, if hypervisor uses compact xsave area and guest
> +     * has ever used lazy states (checking xcr0_accum also excluding

I'm not sure about the "also" here. Perhaps just drop it? Or replace
it by "yet"? A native speaker's input would be appreciated.

> +     * XSTATE_FP_SSE), vcpu_xsave_mask will return XSTATE_ALL. Otherwise
> +     * return XSTATE_NONLAZY.
> +     * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
> +     * (in the legacy region of xsave area) are fixed, so saving
> +     * XSTATE_FS_SSE will not cause overwriting problem.

s/FS/FP/

> +     */
> +    return using_xsave_compact && (v->arch.xcr0_accum & XSTATE_LAZY & 
> ~XSTATE_FP_SSE) ?
> +           XSTATE_ALL : XSTATE_NONLAZY;

Long line and indentation.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -165,7 +165,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
> unsigned int size)
>      u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
>      u64 valid;
>  
> -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +    if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
>      {
>          memcpy(dest, xsave, size);
>          return;

This one looks correct, but ...

> @@ -206,7 +206,7 @@ void compress_xsave_states(struct vcpu *v, const void 
> *src, unsigned int size)
>      u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
>      u64 valid;
>  
> -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +    if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
>      {
>          memcpy(xsave, src, size);
>          return;

... how can this one be? You are in the process of compressing
known uncompressed input.

> @@ -370,7 +368,7 @@ void xrstor(struct vcpu *v, uint64_t mask)
>                         "   .previous\n" \
>                         _ASM_EXTABLE(1b, 2b), \
>                         ".byte " pfx "0x0f,0xc7,0x1f\n", \
> -                       X86_FEATURE_XSAVES, \
> +                       X86_FEATURE_XSAVE_COMPACT, \
>                         ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" 
> (faults)), \
>                         [lmask] "a" (lmask), [hmask] "d" (hmask), \
>                         [ptr] "D" (ptr))

I don't think you can stick to alternative patching here - whether
to use XRSTORS now depends on what state is to be restored.

Or maybe (to amend the first comment above)
"using_xsave_compact" is actually the wrong term now, and this
really needs to become "using_xsaves" (in which case the change
suggested in that first comment wouldn't be needed anymore). In
the end, at least the code outside of xstate.c should be in a state
where xstate.c's choice of whether to use XSAVEC doesn't matter
(and ideally this would also extend to all code in that file except
for the relevant parts of xsave()).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel