Re: [PATCH V4 2/4] x86/xsaves: enable xsaves/xrstors in xen

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

On 26/08/15 12:50, Jan Beulich wrote:
>>>> On 26.08.15 at 12:12, <andrew.cooper3@citrix.com> wrote:
>> On 25/08/15 11:54, Shuai Ruan wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -2148,8 +2148,12 @@ static int hvm_save_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
>>>          ctxt->xfeature_mask = xfeature_mask;
>>>          ctxt->xcr0 = v->arch.xcr0;
>>>          ctxt->xcr0_accum = v->arch.xcr0_accum;
>>> -        memcpy(&ctxt->save_area, v->arch.xsave_area,
>>> -               size - offsetof(struct hvm_hw_cpu_xsave, save_area));
>>> +	if ( cpu_has_xsaves )
>> Stray hard tab.
>>
>>> +            save_xsave_states(v, (void *)&ctxt->save_area,
>>> +                              size - offsetof(struct hvm_hw_cpu_xsave,save_area));
>> These offsetof()s can become far shorter by using offsetof(*ctxt,
>> save_area).
> Are you mixing this up with sizeof()? If anything, offsetof(typeof(),).

Oops sorry yes.  I did mean to include the use of typeof(), which still
makes it shorter.

>
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -936,9 +936,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
>>>          if ( regs->_ecx == 1 )
>>>          {
>>>              a &= XSTATE_FEATURE_XSAVEOPT |
>>> -                 XSTATE_FEATURE_XSAVEC |
>>> -                 (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
>>> -                 (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0);
>>> +                 XSTATE_FEATURE_XSAVEC;
>>> +	         /* PV guest will not support xsaves. */
>>> +                 /* (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
>>> +                 (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0); */
>> Don't leave this code commented out like this.  Just delete it.
> Agreed, but - mind reminding me again why supporting them for
> PV guests isn't going to work?

xsaves is a cpl0 instruction used to manage state which userspace can't
be trusted to handle alone.  Xen therefore can't trust PV guests to use
it either.

The first of these features is Processor Trace.  A PV guest able to use
xsaves/xrstors would be able to gather trace data of hypervisor
execution, or cause the trace buffers to clobber arbitrary physical memory.

The features covered by MSR_IA32_XSS can safely be exposed to PV guests,
but via a hypercall interface.

~Andrew