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: [V5] x86/xsaves: fix overwriting between non-lazy/lazy xsaves
Date: Tue, 22 Mar 2016 08:34:33 -0600	[thread overview]
Message-ID: <56F1660902000078000DF38B@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1458270080-19493-1-git-send-email-shuai.ruan@linux.intel.com>

>>> On 18.03.16 at 04:01, <shuai.ruan@linux.intel.com> wrote:
> v5: Address comments from Jan
> 1. Add XSTATE_XSAVES_ONLY and using xsaves depend on whether this bits are
>    set in xcr0_accum
> 2. Change compress logic in compress_xsave_states() depend on 
>    !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) && !xsave_area_compressed(src)).
> 3. XSTATE_COMPACTION_ENABLED only set in xrstor().
> 4. Rebase the code on
>    [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv
>    (already sent out) For they both change same code. 
>    (I am not sure whether this rebase is ok or not).

Such re-basing is okay, but the dependency on the other patch
shouldn't get hidden in the revision log. Best would really be if this
was a series (consisting of both patches).

> @@ -222,22 +222,21 @@ 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 ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) &&
> +         !xsave_area_compressed(src) )
>      {
>          memcpy(xsave, src, size);
>          return;
>      }
>  
> -    ASSERT(!xsave_area_compressed(src));

This is bogus: The function here gets called only after
validate_xstate() already succeeded. Hence the ASSERT()
should imo simply get moved ahead of the if().

>      /*
>       * Copy legacy XSAVE area, to avoid complications with CPUID
>       * leaves 0 and 1 in the loop below.
>       */
>      memcpy(xsave, src, FXSAVE_SIZE);
>  
> -    /* Set XSTATE_BV and XCOMP_BV.  */
> +    /* Set XSTATE_BV.  */
>      xsave->xsave_hdr.xstate_bv = xstate_bv;
> -    xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
>      setup_xstate_comp(xstate_comp_offsets, xstate_bv);

I see you set xcomp_bv (and hence the compaction bit) in xrstor()
now, but afaict that doesn't allow you to completely drop initializing
the field here, as the code there looks at the compaction bit.

> @@ -267,31 +266,35 @@ void xsave(struct vcpu *v, uint64_t mask)
>      uint32_t hmask = mask >> 32;
>      uint32_t lmask = mask;
>      unsigned int fip_width = v->domain->arch.x87_fip_width;
> -#define XSAVE(pfx) \
> -        alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
> -                         ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
> -                         X86_FEATURE_XSAVEOPT, \
> -                         ".byte " pfx "0x0f,0xc7,0x27\n", /* xsavec */ \
> -                         X86_FEATURE_XSAVEC, \
> -                         ".byte " pfx "0x0f,0xc7,0x2f\n", /* xsaves */ \
> -                         X86_FEATURE_XSAVES, \
> -                         "=m" (*ptr), \
> -                         "a" (lmask), "d" (hmask), "D" (ptr))
> +#define XSAVE(pfx, xsave_ins) \
> +        asm volatile ( ".byte " pfx xsave_ins \
> +                       : "=m" (*ptr) \
> +                       : "a" (lmask), "d" (hmask), "D" (ptr) )
>  
>      if ( fip_width == 8 || !(mask & XSTATE_FP) )
>      {
> -        XSAVE("0x48,");
> +        if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY )
> +            XSAVE("0x48,", "0x0f,0xc7,0x2f"); /* xsaves */
> +        else if ( cpu_has_xsaveopt )
> +            XSAVE("0x48,", "0x0f,0xae,0x37"); /* xsaveopt */
> +        else
> +            XSAVE("0x48,", "0x0f,0xae,0x27"); /* xsave */

The latter two should still use alternative insn patching.

>      }
>      else if ( fip_width == 4 )
>      {
> -        XSAVE("");
> +        if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY )
> +            XSAVE("", "0x0f,0xc7,0x2f");
> +        else if ( cpu_has_xsaveopt )
> +            XSAVE("", "0x0f,0xae,0x37");
> +        else
> +            XSAVE("", "0x0f,0xae,0x27");

And this logic being repeated here (and another time below) should
have made obvious that you want to leave the code here untouched
and do all of your changes just to the XSAVE() macro definition.

> @@ -378,25 +387,42 @@ void xrstor(struct vcpu *v, uint64_t mask)
>          switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
>          {
>              BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
> -#define XRSTOR(pfx) \
> -        alternative_io("1: .byte " pfx "0x0f,0xae,0x2f\n" \
> +#define XRSTOR(pfx, xrstor_ins) \
> +        asm volatile ( "1: .byte " pfx xrstor_ins"\n" \
>                         "3:\n" \
>                         "   .section .fixup,\"ax\"\n" \
>                         "2: incl %[faults]\n" \
>                         "   jmp 3b\n" \
>                         "   .previous\n" \
> -                       _ASM_EXTABLE(1b, 2b), \
> -                       ".byte " pfx "0x0f,0xc7,0x1f\n", \
> -                       X86_FEATURE_XSAVES, \
> -                       ASM_OUTPUT2([mem] "+m" (*ptr), [faults] "+g" (faults)), \
> -                       [lmask] "a" (lmask), [hmask] "d" (hmask), \
> -                       [ptr] "D" (ptr))
> +                       _ASM_EXTABLE(1b, 2b) \
> +                       : [mem] "+m" (*ptr), [faults] "+g" (faults) \
> +                       : [lmask] "a" (lmask), [hmask] "d" (hmask), \
> +                         [ptr] "D" (ptr) )
>  
>          default:
> -            XRSTOR("0x48,");
> +            if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY )
> +            {
> +                if ( unlikely(!(ptr->xsave_hdr.xcomp_bv
> +                                & XSTATE_COMPACTION_ENABLED)) )
> +                    ptr->xsave_hdr.xcomp_bv = ptr->xsave_hdr.xstate_bv
> +                                              | XSTATE_COMPACTION_ENABLED;
> +
> +                XRSTOR("0x48,","0x0f,0xc7,0x1f"); /* xrstors */
> +            }
> +            else
> +                XRSTOR("0x48,","0x0f,0xae,0x2f"); /* xrstor */

At this point, what guarantees that xcomp_bv is zero, no matter
where the state to be loaded originates from? I think at least in
arch_set_info_guest(), hvm_load_cpu_ctxt(), and
hvm_vcpu_reset_state() you went too far deleting code, and you
really need to keep the storing of zero there. Did you draw, just
for yourself, mentally or on a sheet of paper, a diagram illustrating
the various state transitions?

>              break;
>          case 4: case 2:
> -            XRSTOR("");
> +            if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY )
> +            {
> +                if ( unlikely(!(ptr->xsave_hdr.xcomp_bv
> +                                & XSTATE_COMPACTION_ENABLED)) )
> +                    ptr->xsave_hdr.xcomp_bv = ptr->xsave_hdr.xstate_bv
> +                                              | XSTATE_COMPACTION_ENABLED;
> +                XRSTOR("","0x0f,0xc7,0x1f");
> +            }
> +            else
> +                XRSTOR("","0x0f,0xae,0x2f");
>              break;

Since again you repeat the same logic twice, this should again have
been a signal that all your changes should go into the XRSTOR()
macro. Or alternatively, since the exception fixup also differs, you
may want to convert the whole logic into an XSAVES and an XSAVE
path. My only really sincere request here is - as little redundancy as
possible, since having to change the same thing twice in more than
one place is always calling for trouble.

Jan

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

  reply	other threads:[~2016-03-22 14:34 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-18  3:01 Shuai Ruan
2016-03-22 14:34 ` Jan Beulich [this message]
2016-03-23  2:02   ` Shuai Ruan
     [not found]   ` <20160323020224.GB4131@shuai.ruan@linux.intel.com>
2016-03-23  6:14     ` Shuai Ruan
     [not found]     ` <20160323061455.GA12388@shuai.ruan@linux.intel.com>
2016-03-23 10:21       ` Jan Beulich
2016-03-23 11:01         ` 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=56F1660902000078000DF38B@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 \
    --subject='Re: [V5] x86/xsaves: fix overwriting between non-lazy/lazy xsaves' \
    /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

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).