Re: [PATCH v4] KVM: VMX: Enable XSAVE/XRSTORE for guest

[Avi Kivity <avi@redhat.com>]

On 05/25/2010 09:28 AM, Sheng Yang wrote:
>>
>>> @@ -3354,6 +3356,29 @@ static int handle_wbinvd(struct kvm_vcpu *vcpu)
>>>
>>>    	return 1;
>>>
>>>    }
>>>
>>> +static int handle_xsetbv(struct kvm_vcpu *vcpu)
>>> +{
>>> +	u64 new_bv = kvm_read_edx_eax(vcpu);
>>> +
>>> +	if (kvm_register_read(vcpu, VCPU_REGS_RCX) != 0)
>>> +		goto err;
>>> +	if (vmx_get_cpl(vcpu) != 0)
>>> +		goto err;
>>> +	if (!(new_bv&   XSTATE_FP))
>>> +		goto err;
>>> +	if ((new_bv&   XSTATE_YMM)&&   !(new_bv&   XSTATE_SSE))
>>> +		goto err;
>>> +	if (new_bv&   ~XCNTXT_MASK)
>>> +		goto err;
>>>        
>> Ok.  This means we must update kvm immediately when XCNTXT_MASK changes.
>>
>> (Otherwise we would use KVM_XCNTXT_MASK which is always smaller than
>> than XCNTXT_MASK).
>>      
> I guess use host_xcr0 here is better?
>    

Yes - it might be smaller than XCNTXT_MASK

>> may be simplified if we move xcr0 reload back to guest entry (... :)
>> but make it lazy:
>>
>>     save_host_state: nothing
>>     set cr4.osxsave: nothing
>>     clear cr4.osxsave: nothing
>>     guest entry: if (gcr4.osxsave&&  !guest_xcr0_loaded) {
>> guest_xcr0_loaded = true, load gxcr0 }
>>     load_host_state: if (guest_xcr0_loaded) { guest_xcr0_loaded = false;
>> load host xcr0 }
>>     fpu switching:  if (guest_xcr0_loaded) { guest_xcr0_loaded = false;
>> load host xcr0 }, do fpu stuff
>>
>> So we delay xcr0 reload as late as possible for both entry and exit.
>>      
> I think I got it. But why we need do it at "load_host_state()"? I guess just put
> code before fpu testing in kvm_put_guest_fpu() is fine?
>    

Right, load_host_state() is bad because it is vmx specific.  
kvm_put_guest_fpu() (or perhaps kvm_arch_vcpu_put()) is better.

-- 
error compiling committee.c: too many arguments to function