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

[Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>]

On Thu, Oct 29, 2015 at 02:59:38AM -0600, Jan Beulich wrote:
> >>> On 29.10.15 at 08:58, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> > On Mon, Oct 26, 2015 at 08:03:55AM -0600, Jan Beulich wrote:
> >> >>> On 23.10.15 at 11:48, <shuai.ruan@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
> 

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