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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 29.10.15 at 08:58, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
>> >>> On 23.10.15 at 11:48, <shuai.ruan@linux.intel.com> 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