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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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, <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?
>>
> 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