Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >>> On 23.10.15 at 11:48,  wrote:
> > This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> > to perform the xsave_area switching so that xen itself
> > can benefit from them when available.
> > 
> > For xsaves/xrstors/xsavec only use compact format. Add format conversion
> > support when perform guest os migration.
> > 
> > Also, pv guest will not support xsaves/xrstors.
> > @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
> >  
> >  /* Mask out features not currently understood by Xen. */
> >  eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> > -cpufeat_mask(X86_FEATURE_XSAVEC));
> > +cpufeat_mask(X86_FEATURE_XSAVEC) |
> > +cpufeat_mask(X86_FEATURE_XGETBV1) |
> > +cpufeat_mask(X86_FEATURE_XSAVES));
> >  
> >  c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
> >  
> >  BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
> > +
> > +if ( setup_xstate_features(bsp) )
> > +BUG();
> 
> BUG()? On the BSP maybe, but APs should simply fail being
> brought up instead of bringing down the whole system.
> 
For APs, setup_xsate_features will never return error. Just BSP can
return error as -ENOMEM.

> > --- a/xen/include/public/arch-x86/hvm/save.h
> > +++ b/xen/include/public/arch-x86/hvm/save.h
> > @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
> >  uint64_t msr_syscall_mask;
> >  uint64_t msr_efer;
> >  uint64_t msr_tsc_aux;
> > +uint64_t msr_xss;
> 
> You can't extend a public interface structure like this. New MSRs
> shouldn't be saved/restored this way anyway - that's what we
> have struct hvm_msr for.
Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" in 
hvm_save_cpu_msrs. Is that Ok ?

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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Jan Beulich]

>>> On 29.10.15 at 08:58,  wrote:
> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>> >>> On 23.10.15 at 11:48,  wrote:
>> > This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
>> > to perform the xsave_area switching so that xen itself
>> > can benefit from them when available.
>> > 
>> > For xsaves/xrstors/xsavec only use compact format. Add format conversion
>> > support when perform guest os migration.
>> > 
>> > Also, pv guest will not support xsaves/xrstors.
>> > @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
>> >  
>> >  /* Mask out features not currently understood by Xen. */
>> >  eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>> > -cpufeat_mask(X86_FEATURE_XSAVEC));
>> > +cpufeat_mask(X86_FEATURE_XSAVEC) |
>> > +cpufeat_mask(X86_FEATURE_XGETBV1) |
>> > +cpufeat_mask(X86_FEATURE_XSAVES));
>> >  
>> >  c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>> >  
>> >  BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 
>> > 32]);
>> > +
>> > +if ( setup_xstate_features(bsp) )
>> > +BUG();
>> 
>> BUG()? On the BSP maybe, but APs should simply fail being
>> brought up instead of bringing down the whole system.
>> 
> For APs, setup_xsate_features will never return error. Just BSP can
> return error as -ENOMEM.

This may be the case now, but will whoever ends up editing the
function remember to audit the code here?

>> > --- a/xen/include/public/arch-x86/hvm/save.h
>> > +++ b/xen/include/public/arch-x86/hvm/save.h
>> > @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
>> >  uint64_t msr_syscall_mask;
>> >  uint64_t msr_efer;
>> >  uint64_t msr_tsc_aux;
>> > +uint64_t msr_xss;
>> 
>> You can't extend a public interface structure like this. New MSRs
>> shouldn't be saved/restored this way anyway - that's what we
>> have struct hvm_msr for.
> Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
> intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" 
> in 
> hvm_save_cpu_msrs. Is that Ok ?

No, the code belongs in vmx_save_msr() (and its sibling functions).

Jan


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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

On Thu, Oct 29, 2015 at 02:59:38AM -0600, Jan Beulich wrote:
> >>> On 29.10.15 at 08:58,  wrote:
> > On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >> >>> On 23.10.15 at 11:48,  wrote:
> >> > This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> >> > to perform the xsave_area switching so that xen itself
> >> > can benefit from them when available.
> >> > 
> >> > For xsaves/xrstors/xsavec only use compact format. Add format conversion
> >> > support when perform guest os migration.
> >> > 
> >> > Also, pv guest will not support xsaves/xrstors.
> >> > @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
> >> >  
> >> >  /* Mask out features not currently understood by Xen. */
> >> >  eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> >> > -cpufeat_mask(X86_FEATURE_XSAVEC));
> >> > +cpufeat_mask(X86_FEATURE_XSAVEC) |
> >> > +cpufeat_mask(X86_FEATURE_XGETBV1) |
> >> > +cpufeat_mask(X86_FEATURE_XSAVES));
> >> >  
> >> >  c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
> >> >  
> >> >  BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 
> >> > 32]);
> >> > +
> >> > +if ( setup_xstate_features(bsp) )
> >> > +BUG();
> >> 
> >> BUG()? On the BSP maybe, but APs should simply fail being
> >> brought up instead of bringing down the whole system.
> >> 
> > For APs, setup_xsate_features will never return error. Just BSP can
> > return error as -ENOMEM.
> 
> This may be the case now, but will whoever ends up editing the
> function remember to audit the code here?
> 
Ok.
> >> > --- a/xen/include/public/arch-x86/hvm/save.h
> >> > +++ b/xen/include/public/arch-x86/hvm/save.h
> >> > @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
> >> >  uint64_t msr_syscall_mask;
> >> >  uint64_t msr_efer;
> >> >  uint64_t msr_tsc_aux;
> >> > +uint64_t msr_xss;
> >> 
> >> You can't extend a public interface structure like this. New MSRs
> >> shouldn't be saved/restored this way anyway - that's what we
> >> have struct hvm_msr for.
> > Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
> > intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" 
> > in 
> > hvm_save_cpu_msrs. Is that Ok ?
> 
> No, the code belongs in vmx_save_msr() (and its sibling functions).
> 
Ok.
For there is no new area added in vmcs for xss_msr, I will use
"
if ( cpu_has_xsaves)
{
ctxt->msr[ctxt->count].val = v->arch.hvm_vcpu.msr_xss;
if ( ctxt->msr[ctxt->count].val )
 ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
}
" to save xss_msr. Is it ok to add the save logic between "vmx_vmcs_enter(v);" 
and "vmx_vmcs_exit(v);" ? Or just add the save logic after "vmx_vmcs_exit(v);" ?

Thanks 


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

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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Jan Beulich]

>>> On 29.10.15 at 10:47,  wrote:
> On Thu, Oct 29, 2015 at 02:59:38AM -0600, Jan Beulich wrote:
>> >>> On 29.10.15 at 08:58,  wrote:
>> > Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
>> > intend to add save msr logic before "ASSERT(ctxt->count <= 
>> > msr_count_max);" in 
> 
>> > hvm_save_cpu_msrs. Is that Ok ?
>> 
>> No, the code belongs in vmx_save_msr() (and its sibling functions).
>> 
> Ok.
> For there is no new area added in vmcs for xss_msr, I will use
> "
> if ( cpu_has_xsaves)
> {
> ctxt->msr[ctxt->count].val = v->arch.hvm_vcpu.msr_xss;
> if ( ctxt->msr[ctxt->count].val )
>  ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
> }
> " to save xss_msr. Is it ok to add the save logic between 
> "vmx_vmcs_enter(v);" and "vmx_vmcs_exit(v);" ? Or just add the save logic 
> after "vmx_vmcs_exit(v);" ?

I think it'd be fine either way, but obviously the VMX maintainers
will have the final say.

Jan


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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Andrew Cooper]

On 29/10/15 09:47, Shuai Ruan wrote:
> On Thu, Oct 29, 2015 at 02:59:38AM -0600, Jan Beulich wrote:
> On 29.10.15 at 08:58,  wrote:
>>> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>>> On 23.10.15 at 11:48,  wrote:
> This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> to perform the xsave_area switching so that xen itself
> can benefit from them when available.
>
> For xsaves/xrstors/xsavec only use compact format. Add format conversion
> support when perform guest os migration.
>
> Also, pv guest will not support xsaves/xrstors.
> @@ -343,11 +520,18 @@ void xstate_init(struct cpuinfo_x86 *c)
>  
>  /* Mask out features not currently understood by Xen. */
>  eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> -cpufeat_mask(X86_FEATURE_XSAVEC));
> +cpufeat_mask(X86_FEATURE_XSAVEC) |
> +cpufeat_mask(X86_FEATURE_XGETBV1) |
> +cpufeat_mask(X86_FEATURE_XSAVES));
>  
>  c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>  
>  BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 
> 32]);
> +
> +if ( setup_xstate_features(bsp) )
> +BUG();
 BUG()? On the BSP maybe, but APs should simply fail being
 brought up instead of bringing down the whole system.

>>> For APs, setup_xsate_features will never return error. Just BSP can
>>> return error as -ENOMEM.
>> This may be the case now, but will whoever ends up editing the
>> function remember to audit the code here?
>>
> Ok.
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
>  uint64_t msr_syscall_mask;
>  uint64_t msr_efer;
>  uint64_t msr_tsc_aux;
> +uint64_t msr_xss;
 You can't extend a public interface structure like this. New MSRs
 shouldn't be saved/restored this way anyway - that's what we
 have struct hvm_msr for.
>>> Yes. I will use the exist function "hvm_save_cpu_msrs" to save this msr. I 
>>> intend to add save msr logic before "ASSERT(ctxt->count <= msr_count_max);" 
>>> in 
>>> hvm_save_cpu_msrs. Is that Ok ?
>> No, the code belongs in vmx_save_msr() (and its sibling functions).
>>
> Ok.
> For there is no new area added in vmcs for xss_msr, I will use
> "
> if ( cpu_has_xsaves)
> {
> ctxt->msr[ctxt->count].val = v->arch.hvm_vcpu.msr_xss;
> if ( ctxt->msr[ctxt->count].val )
>  ctxt->msr[ctxt->count++].index = MSR_IA32_XSS;
> }
> " to save xss_msr. Is it ok to add the save logic between 
> "vmx_vmcs_enter(v);" and "vmx_vmcs_exit(v);" ? Or just add the save logic 
> after "vmx_vmcs_exit(v);" ?

That looks ok (after fixing the whitespace issue in the if)

As it doesn't rely on the vmcs, it would be better to be outside the
enter/exit pair so as to prevent needless holdup of another cpu trying
to get at this vmcs.

You also need to modify vmx_init_msr() to indicate that the maximum
possible count of MSRs has increased.

~Andrew

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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Jan Beulich]

>>> On 27.10.15 at 02:06,  wrote:
> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>> >>> On 23.10.15 at 11:48,  wrote:
>> > -a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
>> > -if ( !cpu_has_xsaves )
>> > -b = c = d = 0;
>> > +a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>> > + cpufeat_mask(X86_FEATURE_XSAVEC) |
>> > + (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 
>> > 0);
>> 
>> Why the cpu_has_xgetbv1 dependency? If you want to do it this
>> way, it will get unreadable with two or three more features. Why
>> don't you simply and with the known mask on top of the and with
>> the capability flags that was already there?
>> 
>> And actually I realize I may have misguided you: xstate_init()
>> already makes sure boot_cpu_data.x86_capability[] doesn't
>> contain any unsupported flags, so keeping just the masking
>> that's already there should be enough.
>> 
> In this patch in xstate_init I have add xsaves understood by xen. This
> boot_cpu_data.x86_capability[] contain support for xsaves. And in my
> previous patch V7 I use 
> "a &= (boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32] & 
> ~cpufeat_mask(X86_FEATURE_XSAVES))"
> to mask out xsaves for pv guest. You think this should use white listing
> way.

Oh, right, you mean to hide more features than those Xen supports.

> So will the way I used in V7  ok ?

Conceptionally yes, but the first paragraph of my reply still applies,
i.e. the use of cpu_has_xgetbv1 should go away.

>> > +void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>> > +{
>> > +struct xsave_struct *xsave = v->arch.xsave_area;
>> > +u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
>> > +u64 valid;
>> > +
>> > +if ( !cpu_has_xsaves && !cpu_has_xsavec )
>> > +{
>> > +memcpy(dest, xsave, size);
>> > +return;
>> > +}
>> > +
>> > +ASSERT(xsave_area_compressed(xsave));
>> > +/*
>> > + * Copy legacy XSAVE area, to avoid complications with CPUID
>> > + * leaves 0 and 1 in the loop below.
>> > + */
>> > +memcpy(dest, xsave, FXSAVE_SIZE);
>> > +
>> > +((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
>> > +((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
>> 
>> Wouldn't it be more logical to also memcpy() the header? Which
>> would do away with at least one of these ugly casts, would
>> allow folding with the memcpy() done right before, _and_ would
>> eliminate an (apparent or real I'm not sure without looking in
>> more detail) information leak (the remainder of the header).
>> 
> For machine with no-xsaves support, xcomp_bv should be zero or it will cause
> GP fault. So we can not just memcpy the header when perform save.

See how I limited my reply by saying "at least one", which I added
after realizing that fact? IOW I'd expect you to use memcpy() and
then only zero that one field. Or you'd memset() the whole header
to zero, and then only store xstate_bv. As said, otherwise it looks
like you're adding an information leak.

Jan


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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

On Tue, Oct 27, 2015 at 02:13:03AM -0600, Jan Beulich wrote:
> >> > +
> >> > +((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> >> > +((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
> >> 
> >> Wouldn't it be more logical to also memcpy() the header? Which
> >> would do away with at least one of these ugly casts, would
> >> allow folding with the memcpy() done right before, _and_ would
> >> eliminate an (apparent or real I'm not sure without looking in
> >> more detail) information leak (the remainder of the header).
> >> 
> > For machine with no-xsaves support, xcomp_bv should be zero or it will cause
> > GP fault. So we can not just memcpy the header when perform save.
> 
> See how I limited my reply by saying "at least one", which I added
> after realizing that fact? IOW I'd expect you to use memcpy() and
> then only zero that one field. Or you'd memset() the whole header
> to zero, and then only store xstate_bv. As said, otherwise it looks
> like you're adding an information leak.
> 
> Jan
Ok And sorry for misunderstanding your meaning.
> 
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >>> On 23.10.15 at 11:48,  wrote:
> > Signed-off-by: Shuai Ruan 
> > Reviewed-by: Andrew Cooper 
> 
> There were actual bugs fixed from v7 to v8, plus there's a public
> interface change, so retaining this tag is wrong.
> 
Ok
> > -a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
> > -if ( !cpu_has_xsaves )
> > -b = c = d = 0;
> > +a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> > + cpufeat_mask(X86_FEATURE_XSAVEC) |
> > + (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);
> 
> Why the cpu_has_xgetbv1 dependency? If you want to do it this
> way, it will get unreadable with two or three more features. Why
> don't you simply and with the known mask on top of the and with
> the capability flags that was already there?
> 
> And actually I realize I may have misguided you: xstate_init()
> already makes sure boot_cpu_data.x86_capability[] doesn't
> contain any unsupported flags, so keeping just the masking
> that's already there should be enough.
> 
In this patch in xstate_init I have add xsaves understood by xen. This
boot_cpu_data.x86_capability[] contain support for xsaves. And in my
previous patch V7 I use 
"a &= (boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32] & 
~cpufeat_mask(X86_FEATURE_XSAVES))"
to mask out xsaves for pv guest. You think this should use white listing
way. So will the way I used in V7  ok ?

> > +void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> > +{
> > +struct xsave_struct *xsave = v->arch.xsave_area;
> > +u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> > +u64 valid;
> > +
> > +if ( !cpu_has_xsaves && !cpu_has_xsavec )
> > +{
> > +memcpy(dest, xsave, size);
> > +return;
> > +}
> > +
> > +ASSERT(xsave_area_compressed(xsave));
> > +/*
> > + * Copy legacy XSAVE area, to avoid complications with CPUID
> > + * leaves 0 and 1 in the loop below.
> > + */
> > +memcpy(dest, xsave, FXSAVE_SIZE);
> > +
> > +((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> > +((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
> 
> Wouldn't it be more logical to also memcpy() the header? Which
> would do away with at least one of these ugly casts, would
> allow folding with the memcpy() done right before, _and_ would
> eliminate an (apparent or real I'm not sure without looking in
> more detail) information leak (the remainder of the header).
> 
For machine with no-xsaves support, xcomp_bv should be zero or it will cause
GP fault. So we can not just memcpy the header when perform save.

Thanks 
Shuai
> 

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

Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Jan Beulich]

>>> On 23.10.15 at 11:48,  wrote:
> This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> to perform the xsave_area switching so that xen itself
> can benefit from them when available.
> 
> For xsaves/xrstors/xsavec only use compact format. Add format conversion
> support when perform guest os migration.
> 
> Also, pv guest will not support xsaves/xrstors.
> 
> Signed-off-by: Shuai Ruan 
> Reviewed-by: Andrew Cooper 

There were actual bugs fixed from v7 to v8, plus there's a public
interface change, so retaining this tag is wrong.

> @@ -2025,6 +2026,11 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  
>  v->arch.hvm_vcpu.msr_tsc_aux = ctxt.msr_tsc_aux;
>  
> +if ( cpu_has_xsaves )
> +v->arch.hvm_vcpu.msr_xss = ctxt.msr_xss;
> +else
> +v->arch.hvm_vcpu.msr_xss = 0;

Cases like this call for use of ?:.

> @@ -2257,10 +2266,10 @@ static int hvm_load_cpu_xsave_states(struct domain 
> *d, hvm_domain_context_t *h)
>  v->arch.xcr0_accum = ctxt->xcr0_accum;
>  if ( ctxt->xcr0_accum & XSTATE_NONLAZY )
>  v->arch.nonlazy_xstate_used = 1;
> -memcpy(v->arch.xsave_area, >save_area,
> -   min(desc->length, size) - offsetof(struct hvm_hw_cpu_xsave,
> -   save_area));
>  
> +compress_xsave_states(v, >save_area,
> +  min(desc->length, size) -
> +  offsetof(struct hvm_hw_cpu_xsave,save_area));
>  return 0;

This change now misplaces the blank line. Please - a little more care
when comments on formatting were already given.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -935,9 +935,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
>  goto unsupported;
>  if ( regs->_ecx == 1 )
>  {
> -a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
> -if ( !cpu_has_xsaves )
> -b = c = d = 0;
> +a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> + cpufeat_mask(X86_FEATURE_XSAVEC) |
> + (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);

Why the cpu_has_xgetbv1 dependency? If you want to do it this
way, it will get unreadable with two or three more features. Why
don't you simply and with the known mask on top of the and with
the capability flags that was already there?

And actually I realize I may have misguided you: xstate_init()
already makes sure boot_cpu_data.x86_capability[] doesn't
contain any unsupported flags, so keeping just the masking
that's already there should be enough.

> +void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> +{
> +struct xsave_struct *xsave = v->arch.xsave_area;
> +u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> +u64 valid;
> +
> +if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +{
> +memcpy(dest, xsave, size);
> +return;
> +}
> +
> +ASSERT(xsave_area_compressed(xsave));
> +/*
> + * Copy legacy XSAVE area, to avoid complications with CPUID
> + * leaves 0 and 1 in the loop below.
> + */
> +memcpy(dest, xsave, FXSAVE_SIZE);
> +
> +((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> +((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;

Wouldn't it be more logical to also memcpy() the header? Which
would do away with at least one of these ugly casts, would
allow folding with the memcpy() done right before, _and_ would
eliminate an (apparent or real I'm not sure without looking in
more detail) information leak (the remainder of the header).

> +/*
> + * Copy each region from the possibly compacted offset to the
> + * non-compacted offset.
> + */
> +valid = xstate_bv & ~XSTATE_FP_SSE;
> +while ( valid )
> +{
> +u64 feature = valid & -valid;
> +unsigned int index = fls(feature) - 1;
> +const void *src = get_xsave_addr(xsave, index);
> +
> +if ( src )
> +{
> +ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> +memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
> +}
> +
> +valid &= ~feature;
> +}
> +
> +}

Stray blank line.

> @@ -187,39 +379,24 @@ void xrstor(struct vcpu *v, uint64_t mask)
>  switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
>  {
>  default:
> -asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
> -   ".section .fixup,\"ax\"  \n"
> -   "2: mov %5,%%ecx \n"
> -   "   xor %1,%1\n"
> -   "   rep stosb\n"
> -   "   lea %2,%0\n"
> -   "   mov %3,%1\n"
> -   "   jmp 1b   \n"
> -   ".previous 

[Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Shuai Ruan]

This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
to perform the xsave_area switching so that xen itself
can benefit from them when available.

For xsaves/xrstors/xsavec only use compact format. Add format conversion
support when perform guest os migration.

Also, pv guest will not support xsaves/xrstors.

Signed-off-by: Shuai Ruan 
Reviewed-by: Andrew Cooper 
---
 xen/arch/x86/domain.c  |   7 +
 xen/arch/x86/domctl.c  |  31 -
 xen/arch/x86/hvm/hvm.c |  24 +++-
 xen/arch/x86/i387.c|   4 +
 xen/arch/x86/traps.c   |   7 +-
 xen/arch/x86/xstate.c  | 248 -
 xen/include/asm-x86/xstate.h   |   2 +
 xen/include/public/arch-x86/hvm/save.h |   1 +
 8 files changed, 279 insertions(+), 45 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fe3be30..108d4f8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -883,7 +883,12 @@ int arch_set_info_guest(
 {
 memcpy(v->arch.fpu_ctxt, >fpu_ctxt, sizeof(c.nat->fpu_ctxt));
 if ( v->arch.xsave_area )
+{
  v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
+ if ( cpu_has_xsaves || cpu_has_xsavec )
+  v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE |
+   
XSTATE_COMPACTION_ENABLED;
+}
 }
 
 if ( !compat )
@@ -1568,6 +1573,8 @@ static void __context_switch(void)
 if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
 BUG();
 }
+if ( cpu_has_xsaves && has_hvm_container_vcpu(n) )
+set_msr_xss(n->arch.hvm_vcpu.msr_xss);
 vcpu_restore_fpu_eager(n);
 n->arch.ctxt_switch_to(n);
 }
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 0f6fdb9..551dde2 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -897,9 +897,30 @@ long arch_do_domctl(
 ret = -EFAULT;
 
 offset += sizeof(v->arch.xcr0_accum);
-if ( !ret && copy_to_guest_offset(evc->buffer, offset,
-  (void *)v->arch.xsave_area,
-  size - 2 * sizeof(uint64_t)) )
+
+if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) )
+{
+void *xsave_area;
+
+xsave_area = xmalloc_bytes(size);
+if ( !xsave_area )
+{
+ret = -ENOMEM;
+vcpu_unpause(v);
+goto vcpuextstate_out;
+}
+
+expand_xsave_states(v, xsave_area,
+size - 2 * sizeof(uint64_t));
+
+if ( copy_to_guest_offset(evc->buffer, offset, xsave_area,
+  size - 2 * sizeof(uint64_t)) )
+ ret = -EFAULT;
+xfree(xsave_area);
+   }
+   else if ( !ret && copy_to_guest_offset(evc->buffer, offset,
+  (void *)v->arch.xsave_area,
+  size - 2 * sizeof(uint64_t)) 
)
 ret = -EFAULT;
 
 vcpu_unpause(v);
@@ -955,8 +976,8 @@ long arch_do_domctl(
 v->arch.xcr0_accum = _xcr0_accum;
 if ( _xcr0_accum & XSTATE_NONLAZY )
 v->arch.nonlazy_xstate_used = 1;
-memcpy(v->arch.xsave_area, _xsave_area,
-   evc->size - 2 * sizeof(uint64_t));
+compress_xsave_states(v, _xsave_area,
+  evc->size - 2 * sizeof(uint64_t));
 vcpu_unpause(v);
 }
 else
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3fa2280..0140d34 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1735,6 +1735,7 @@ static int hvm_save_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 hvm_funcs.save_cpu_ctxt(v, );
 
 ctxt.msr_tsc_aux = hvm_msr_tsc_aux(v);
+ctxt.msr_xss = v->arch.hvm_vcpu.msr_xss;
 
 hvm_get_segment_register(v, x86_seg_idtr, );
 ctxt.idtr_limit = seg.limit;
@@ -2025,6 +2026,11 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 
 v->arch.hvm_vcpu.msr_tsc_aux = ctxt.msr_tsc_aux;
 
+if ( cpu_has_xsaves )
+v->arch.hvm_vcpu.msr_xss = ctxt.msr_xss;
+else
+v->arch.hvm_vcpu.msr_xss = 0;
+
 seg.limit = ctxt.idtr_limit;
 seg.base = ctxt.idtr_base;
 hvm_set_segment_register(v, x86_seg_idtr, );
@@ -2088,6 +2094,9 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 
 memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));