From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH v7] KVM: VMX: Enable XSAVE/XRSTOR for guest
Date: Tue, 01 Jun 2010 21:18:43 +0300
Message-ID: <4C054F03.7030406@redhat.com>
References: <1275306851-30131-1-git-send-email-sheng@linux.intel.com> <20100601171159.GA23918@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Sheng Yang <sheng@linux.intel.com>, kvm@vger.kernel.org,
	Dexuan Cui <dexuan.cui@intel.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:43619 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753647Ab0FASSu (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 1 Jun 2010 14:18:50 -0400
In-Reply-To: <20100601171159.GA23918@amt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 06/01/2010 08:12 PM, Marcelo Tosatti wrote:
> On Mon, May 31, 2010 at 07:54:11PM +0800, Sheng Yang wrote:
>    
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 99ae513..8649627 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -36,6 +36,8 @@
>>   #include<asm/vmx.h>
>>   #include<asm/virtext.h>
>>   #include<asm/mce.h>
>> +#include<asm/i387.h>
>> +#include<asm/xcr.h>
>>
>>   #include "trace.h"
>>
>> @@ -3354,6 +3356,16 @@ 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);
>> +	u32 index = kvm_register_read(vcpu, VCPU_REGS_RCX);
>> +
>> +	kvm_set_xcr(vcpu, index, new_bv);
>> +	skip_emulated_instruction(vcpu);
>>      
> Should only skip_emulated_instruction if no exception was injected.
>    

Good catch.  So, unit testing failure cases _is_ important.

>    
>> +int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
>> +{
>> +	u64 xcr0;
>> +
>> +	/* Only support XCR_XFEATURE_ENABLED_MASK(xcr0) now  */
>> +	if (index != XCR_XFEATURE_ENABLED_MASK)
>> +		return 1;
>> +	xcr0 = xcr;
>> +	if (kvm_x86_ops->get_cpl(vcpu) != 0)
>> +		return 1;
>> +	if (!(xcr0&  XSTATE_FP))
>> +		return 1;
>> +	if ((xcr0&  XSTATE_YMM)&&  !(xcr0&  XSTATE_SSE))
>> +		return 1;
>> +	if (xcr0&  ~host_xcr0)
>> +		return 1;
>> +	vcpu->arch.xcr0 = xcr0;
>> +	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
>>      
> Won't this happen on guest entry, since vcpu->guest_xcr0_loaded == 0?
>    

In fact we don't know what vcpu->guest_xcr0_loaded is.  Best to unload 
it explicitly (and drop the xsetbv() call).

>> +		vcpu->guest_xcr0_loaded = 1;
>> +	}
>> +}
>> +
>> +static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
>> +{
>> +	if (kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE)&&
>> +			vcpu->guest_xcr0_loaded) {
>> +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
>> +		vcpu->guest_xcr0_loaded = 0;
>> +	}
>> +}
>>      
> What if you load guest's XCR0, then guest clears CR4.OSXSAVE? (restore
> of host_xcr0 should be conditional on guest_xcr0_loaded only).
>    

Good catch again.

>> @@ -5140,6 +5250,8 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
>>
>>   void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
>> {
>> +	kvm_put_guest_xcr0(vcpu);
>> +
>>      
> Should be in kvm_arch_vcpu_put?
>    

That's called unconditionally from kvm_arch_vcpu_put(), so equivalent.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.