From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/1] KVM: X86: add the support of XSAVE/XRSTOR to guest Date: Thu, 06 May 2010 11:14:18 +0300 Message-ID: <4BE27A5A.2020903@redhat.com> References: <1272518554-20357-1-git-send-email-dexuan.cui@intel.com> <4BDD8896.2000607@redhat.com> <1865303E0DED764181A9D882DEF65FB61A82B51D4A@shsmsx502.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" , "Yang, Sheng" To: "Cui, Dexuan" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:25936 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751383Ab0EFIOV (ORCPT ); Thu, 6 May 2010 04:14:21 -0400 In-Reply-To: <1865303E0DED764181A9D882DEF65FB61A82B51D4A@shsmsx502.ccr.corp.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On 05/06/2010 07:23 AM, Cui, Dexuan wrote: > >>> + goto err; >>> + vcpu->arch.guest_xstate_mask = new_bv; >>> + xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.guest_xstate_mask); >>> >>> >> Can't we run with the host xcr0? isn't it guaranteed to be a superset >> of the guest xcr0? >> > I agree host xcr0 is a superset of guest xcr0. > In the case guest xcr0 has less bits set than host xcr0, I suppose running with guest > xcr0 would be a bit faster. However, switching xcr0 may be slow. That's our experience with msrs. Can you measure its latency? Can also be avoided if guest and host xcr0 match. > If you think simplying the code by removing the field > guest_xstate_mask is more important, we can do it. > Well we need guest_xstate_mask, it's the guest's xcr0, no? btw, it needs save/restore for live migration, as well as save/restore for the new fpu state. >>> + skip_emulated_instruction(vcpu); >>> + return 1; >>> +err: >>> + kvm_inject_gp(vcpu, 0); >>> >>> >> Need to #UD in some circumstances. >> > I don't think so. When the CPU doesn't suport XSAVE, or guest doesn't set guest > CR4.OSXSAVE, guest's attempt of exectuing xsetbv would get a #UD immediately > and no VMexit would occur. > Ok. >>> @@ -62,6 +64,7 @@ >>> (~(unsigned long)(X86_CR4_VME | X86_CR4_PVI | X86_CR4_TSD | >>> X86_CR4_DE\ | X86_CR4_PSE | X86_CR4_PAE | X86_CR4_MCE \ >>> | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR \ >>> + | (cpu_has_xsave ? X86_CR4_OSXSAVE : 0) \ >>> | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE)) >>> >>> >> It also depends on the guest cpuid value. Please add it outside the >> > If cpu_has_xsave is true, we always present xsave to guest via cpuid, so I > think the code is correct here. > We don't pass all host cpuid values to the guest. We expose them to userspace via KVM_GET_SUPPORTED_CPUID, and then userspace decides what cpuid to present to the guest via KVM_SET_CPUID2. So the guest may run with fewer features than the host. > I saw the 2 patches you sent. They (like the new APIs fpu_alloc/fpu_free) will simplify > the implementation of kvm xsave support. Thanks a lot! > Thanks. To simplify things, please separate host xsave support (switching to the fpu api) and guest xsave support (cpuid, xsetbv, new ioctls) into separate patches. In fact, guest xsave support is probably best split into patches as well. -- error compiling committee.c: too many arguments to function