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

[Avi Kivity <avi@redhat.com>]

On 05/26/2010 12:19 PM, Sheng Yang wrote:
> From: Dexuan Cui<dexuan.cui@intel.com>
>
> 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