xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: Shuai Ruan <shuai.ruan@linux.intel.com>
Cc: andrew.cooper3@citrix.com, keir@xen.org, xen-devel@lists.xen.org
Subject: Re: [V4] x86/xsaves: fix overwriting between non-lazy/lazy xsaves
Date: Thu, 10 Mar 2016 02:30:34 -0700	[thread overview]
Message-ID: <56E14CCA02000078000DB219@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1457598165-10393-1-git-send-email-shuai.ruan@linux.intel.com>

>>> On 10.03.16 at 09:22, <shuai.ruan@linux.intel.com> 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@lists.xen.org
http://lists.xen.org/xen-devel

  reply	other threads:[~2016-03-10  9:30 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-10  8:22 [V4] x86/xsaves: fix overwriting between non-lazy/lazy xsaves Shuai Ruan
2016-03-10  9:30 ` Jan Beulich [this message]
2016-03-11  6:45   ` Shuai Ruan
     [not found]   ` <20160311064516.GA11162@shuai.ruan@linux.intel.com>
2016-03-11 10:16     ` Jan Beulich
2016-03-15  9:40       ` Shuai Ruan
     [not found]       ` <20160315094037.GA4682@shuai.ruan@linux.intel.com>
2016-03-15 13:33         ` Jan Beulich
2016-03-16  9:35           ` Shuai Ruan
     [not found]           ` <20160316093436.GA3531@shuai.ruan@linux.intel.com>
2016-03-16 10:08             ` Jan Beulich
2016-03-16 10:51               ` Shuai Ruan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56E14CCA02000078000DB219@prv-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=keir@xen.org \
    --cc=shuai.ruan@linux.intel.com \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).