From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40986 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OHDJb-0005Vq-GA for qemu-devel@nongnu.org; Wed, 26 May 2010 05:54:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OHDJa-0002SQ-8o for qemu-devel@nongnu.org; Wed, 26 May 2010 05:54:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37093) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OHDJa-0002SI-1K for qemu-devel@nongnu.org; Wed, 26 May 2010 05:54:18 -0400 Message-ID: <4BFCEFC4.5080800@redhat.com> Date: Wed, 26 May 2010 12:54:12 +0300 From: Avi Kivity MIME-Version: 1.0 References: <1274865557-31137-1-git-send-email-sheng@linux.intel.com> In-Reply-To: <1274865557-31137-1-git-send-email-sheng@linux.intel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v5] KVM: VMX: Enable XSAVE/XRSTORE for guest List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sheng Yang Cc: Marcelo Tosatti , Dexuan Cui , kvm@vger.kernel.org, qemu-devel@nongnu.org On 05/26/2010 12:19 PM, Sheng Yang wrote: > From: Dexuan Cui > > This patch enable guest to use XSAVE/XRSTORE instructions. > > We assume that host_xcr0 would use all possible bits that OS supported. > > And we loaded xcr0 in the same way we handled fpu - do it as late as we can. > > > Looks really good now, only a couple of minor comments and I think we're done. > I've done a prototype of LM support, would send out tomorrow. But the test > case in QEmu side seems got something wrong. I always got an segfault at: > qemu-kvm/hw/fw_cfg.c:223 > 223 s->entries[arch][key].data = data; > > Haven't looked into it yet. But maybe someone knows the reason... > I saw this too, then it disappeared, can't remember why. Perhaps a clean build is needed? > > +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& ~host_xcr0) > + goto err; > + vcpu->arch.xcr0 = new_bv; > + xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0); > Please move all the code above to kvm_set_xcr0() in x86.c, since it's not vendor specific. This would also allow you to make host_xcr0 local to x86.c. > + skip_emulated_instruction(vcpu); > + return 1; > +err: > + kvm_inject_gp(vcpu, 0); > + return 1; > +} > > /* > * List of msr numbers which we expose to userspace through KVM_GET_MSRS > * and KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST. > @@ -1813,6 +1847,14 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu, > r = 0; > kvm_apic_set_version(vcpu); > kvm_x86_ops->cpuid_update(vcpu); > + update_cpuid(vcpu); > + > + /* > + * Ensure guest xcr0 is valid for loading, also using as > + * the indicator of if guest cpuid has XSAVE > + */ > + if (guest_cpuid_has_xsave(vcpu)) > + vcpu->arch.xcr0 = XSTATE_FP; > This is problematic because it enforces an ordering between KVM_SET_XCR and KVM_SET_CPUID. So I think you can use kvm_read_cr4_bits(OSXSAVE) instead of checking vcpu->arch.xcr0. Sorry for the bad advice earlier. > @@ -5134,12 +5207,26 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu) > > vcpu->guest_fpu_loaded = 1; > unlazy_fpu(current); > + /* > + * Restore all possible states in the guest, > + * and assume host would use all available bits. > + * Guest xcr0 would be loaded later. > + */ > + if (cpu_has_xsave&& vcpu->arch.xcr0) { > + xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0); > + vcpu->guest_xcr0_loaded = 0; > + } > Has to be before unlazy_fpu(), so host fpu uses host xcr0. It's sufficient to check for guest cr4.osxsave, no need to check for cpu_has_xsave. But you need to check for guest_xcr0_loaded! > fpu_restore_checking(&vcpu->arch.guest_fpu); > trace_kvm_fpu(1); > } > > void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) > { > + if (vcpu->guest_xcr0_loaded) { > + vcpu->guest_xcr0_loaded = 0; > + xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0); > + } > + > This duplicates the above. So better to have kvm_load_guest_xcr0()/kvm_put_guest_xcr0(). -- error compiling committee.c: too many arguments to function