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

[Avi Kivity <avi@redhat.com>]

On 05/21/2010 10:26 AM, Sheng Yang wrote:
>>
>>> +
>>> +static void update_cpuid(struct kvm_vcpu *vcpu)
>>> +{
>>> +	struct kvm_cpuid_entry2 *best;
>>> +
>>> +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>>> +	if (!best)
>>> +		return;
>>> +
>>> +	/* Update OSXSAVE bit */
>>> +	if (cpu_has_xsave&&   best->function == 0x1) {
>>> +		best->ecx&= ~(bit(X86_FEATURE_OSXSAVE));
>>> +		if (kvm_read_cr4(vcpu)&   X86_CR4_OSXSAVE)
>>> +			best->ecx |= bit(X86_FEATURE_OSXSAVE);
>>> +	}
>>> +}
>>>        
>> Note: need to update after userspace writes cpuid as well.
>>      
> Not quite understand. Userspace set OSXSAVE should be trimmed IMO...
>    

Two cases: userspace does KVM_SET_CPUID2 with osxsave set but cr4.xsave 
clear, or the other way round.

So we should set cpuid.osxsave depending to cr4.xsave whenever cr4 OR 
cpuid is modified, and completely ignore userspace setting for that bit.

>>> @@ -497,6 +532,9 @@ int __kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned
>>> long cr4)
>>>
>>>    	if ((cr4 ^ old_cr4)&   pdptr_bits)
>>>    	
>>>    		kvm_mmu_reset_context(vcpu);
>>>
>>> +	if ((cr4 ^ old_cr4)&   X86_CR4_OSXSAVE)
>>> +		update_cpuid(vcpu);
>>> +
>>>        
>> I think we need to reload the guest's xcr0 at this point.
>> Alternatively, call vmx_load_host_state() to ensure the the next entry
>> will reload it.
>>      
> Current xcr0 would be loaded when next vmentry.
>    

True.

> And if we use prepare_guest_switch(), how about SVM?
>    

kvm_arch_vcpu_load() looks like a good place, as long as interrupts 
don't use the fpu.

>>
>>> @@ -5134,6 +5197,10 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
>>>
>>>    	vcpu->guest_fpu_loaded = 1;
>>>    	unlazy_fpu(current);
>>>
>>> +	/* Restore all possible states in the guest */
>>> +	if (cpu_has_xsave&&   guest_cpuid_has_xsave(vcpu))
>>> +		xsetbv(XCR_XFEATURE_ENABLED_MASK,
>>> +			cpuid_get_possible_xcr0(vcpu));
>>>        
>> Best to calculate it out of the fast path, when guest cpuid is set.
>> Need to check it at this time as well.
>>      
> You mean guest_cpuid_has_xsave()? Not quite understand the point here...
>    

Also cpuid_get_possible_cr0().  So we have something like

    if (vcpu->save_xcr0)
        xsetbv(vcpu->save_xcr0);

Those cpuid functions have loops, we don't want them running every 
context switch.

>> Also can avoid it if guest xcr0 == host xcr0.
>>      
> I don't know the assumption that "host use all possible xcr0 bits" can apply. If
> so, only use host_xcr0 should be fine.
>    

I think we can rely on it.  Those bits are a service to userspace and 
the guest is just a different kind of userspace, so it makes sense to 
expose the same set.

> Would update other points. Thanks.
>    

Thanks.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.