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

[Avi Kivity <avi@redhat.com>]

On 05/24/2010 01:03 PM, Sheng Yang wrote:
> From: Dexuan Cui<dexuan.cui@intel.com>
>
> Enable XSAVE/XRSTORE for guest.
>
> Change from V3:
> 1. Enforced the assumption that host OS would use all available xstate bits.
> 2. Various fixes, addressed Avi's comments.
>
> I am still not clear about why we need to reload guest xcr0 when cr4.osxsave
> set...
>    

When cr4.osxsave=0, then the guest executes with the host xcr0 (since 
xgetbv will trap; this is similar to the guest running with the host fpu 
if cr0.ts=0).  So if cr4.osxsave transtions, we need to transition xcr0 
as well.

> @@ -3354,6 +3356,29 @@ 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);
> +
> +	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&  ~XCNTXT_MASK)
> +		goto err;
>    

Ok.  This means we must update kvm immediately when XCNTXT_MASK changes.

(Otherwise we would use KVM_XCNTXT_MASK which is always smaller than 
than XCNTXT_MASK).

> +	vcpu->arch.xcr0 = new_bv;
> +	xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
> +	skip_emulated_instruction(vcpu);
> +	return 1;
> +err:
> +	kvm_inject_gp(vcpu, 0);
> +	return 1;
> +}
> +
>
>
> @@ -4124,6 +4176,8 @@ int kvm_arch_init(void *opaque)
>
>   	perf_register_guest_info_callbacks(&kvm_guest_cbs);
>
> +	host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
> +
>   	return 0;
>    

Will fault on old cpu.

>   EXPORT_SYMBOL_GPL(fx_init);
> @@ -5134,6 +5195,12 @@ 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.
> +	 */
> +	if (cpu_has_xsave&&  vcpu->arch.xcr0)
> +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
>   	fpu_restore_checking(&vcpu->arch.guest_fpu);
>    

I think we need to reload xcr0 now to the guest's value.

>   	trace_kvm_fpu(1);
>   }
> @@ -5144,6 +5211,13 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
>   		return;
>
>   	vcpu->guest_fpu_loaded = 0;
> +	/*
> +	 * Save all possible states in the guest,
> +	 * and assume host would use all available bits.
> +	 * Also load host_xcr0 for host usage.
> +	 */
> +	if (cpu_has_xsave&&  vcpu->arch.xcr0)
> +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
>   	fpu_save_init(&vcpu->arch.guest_fpu);
>   	++vcpu->stat.fpu_reload;
>   	set_bit(KVM_REQ_DEACTIVATE_FPU,&vcpu->requests);
>    

This might be unnecessary.

So far xcr0 life cycle is almost that of 
save_host_state()/load_host_state(), but not exactly.  When loading the 
guest fpu we switch temporarily to host xcr0, then we have to switch 
back, but only if gcr4.osxsave.  When saving the guest fpu, we're 
already using the host xcr0:

> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> {
>     kvm_x86_ops->vcpu_put(vcpu);
>     kvm_put_guest_fpu(vcpu);
> }

One way to simplify this is to have a vcpu->guest_xcr0_loaded flag and 
check it when needed.  So the transition matrix is:

   save_host_state: if gcr4.osxsave, set guest_xcr0_loaded, load it
   set gcr4.osxsave: ditto
   clear gcr4.osxsave: do nothing
   load_host_state: if guest_xcr0_loaded, clear it, reload host xcr0
   fpu switching: if (switched) switch; reload fpu; if (switched) switch

may be simplified if we move xcr0 reload back to guest entry (... :)  
but make it lazy:

   save_host_state: nothing
   set cr4.osxsave: nothing
   clear cr4.osxsave: nothing
   guest entry: if (gcr4.osxsave && !guest_xcr0_loaded) { 
guest_xcr0_loaded = true, load gxcr0 }
   load_host_state: if (guest_xcr0_loaded) { guest_xcr0_loaded = false; 
load host xcr0 }
   fpu switching:  if (guest_xcr0_loaded) { guest_xcr0_loaded = false; 
load host xcr0 }, do fpu stuff

So we delay xcr0 reload as late as possible for both entry and exit.

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