Re: [PATCH 5/7] Nested VMX patch 5 Simplify fpu handling

[Avi Kivity <avi@redhat.com>]

On 12/10/2009 08:38 PM, oritw@il.ibm.com wrote:
> From: Orit Wasserman<oritw@il.ibm.com>
>
>    

What exactly is the simplification?  Is it intended to have a functional 
change?

> ---
>   arch/x86/kvm/vmx.c |   27 +++++++++++++++++----------
>   1 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 8745d44..de1f596 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1244,8 +1244,6 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
>   	u32 eb;
>
>   	eb = (1u<<  PF_VECTOR) | (1u<<  UD_VECTOR) | (1u<<  MC_VECTOR);
> -	if (!vcpu->fpu_active)
> -		eb |= 1u<<  NM_VECTOR;
>   	/*
>   	 * Unconditionally intercept #DB so we can maintain dr6 without
>   	 * reading it every exit.
> @@ -1463,10 +1461,6 @@ static void vmx_fpu_activate(struct kvm_vcpu *vcpu)
>   	if (vcpu->fpu_active)
>   		return;
>   	vcpu->fpu_active = 1;
> -	vmcs_clear_bits(GUEST_CR0, X86_CR0_TS);
> -	if (vcpu->arch.cr0&  X86_CR0_TS)
> -		vmcs_set_bits(GUEST_CR0, X86_CR0_TS);
> -	update_exception_bitmap(vcpu);
>   }
>
>   static void vmx_fpu_deactivate(struct kvm_vcpu *vcpu)
> @@ -1474,8 +1468,6 @@ static void vmx_fpu_deactivate(struct kvm_vcpu *vcpu)
>   	if (!vcpu->fpu_active)
>   		return;
>   	vcpu->fpu_active = 0;
> -	vmcs_set_bits(GUEST_CR0, X86_CR0_TS);
> -	update_exception_bitmap(vcpu);
>   }
>
>   static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)
> @@ -2715,8 +2707,10 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
>
>   	vmx_flush_tlb(vcpu);
>   	vmcs_writel(GUEST_CR3, guest_cr3);
> -	if (vcpu->arch.cr0&  X86_CR0_PE)
> -		vmx_fpu_deactivate(vcpu);
> +	if (vcpu->arch.cr0&  X86_CR0_PE) {
> +		if (guest_cr3 != vmcs_readl(GUEST_CR3))
> +			vmx_fpu_deactivate(vcpu);
> +	}
>    

Why the added cr3 check?  It may make sense, but it isn't a simplification.

>   static void vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> @@ -5208,6 +5202,19 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>   	if (vcpu->arch.switch_db_regs)
>   		get_debugreg(vcpu->arch.dr6, 6);
>
> +	if (vcpu->fpu_active) {
> +		if (vmcs_readl(CR0_READ_SHADOW)&  X86_CR0_TS)
> +			vmcs_set_bits(GUEST_CR0, X86_CR0_TS);
> +		else
> +			vmcs_clear_bits(GUEST_CR0, X86_CR0_TS);
> +			vmcs_write32(EXCEPTION_BITMAP,
> +				     vmcs_read32(EXCEPTION_BITMAP)&   ~(1u<<  NM_VECTOR));
> +	} else {
> +		vmcs_set_bits(GUEST_CR0, X86_CR0_TS);
> +		vmcs_write32(EXCEPTION_BITMAP,
> +			     vmcs_read32(EXCEPTION_BITMAP) |  (1u<<  NM_VECTOR));
> +	}
>    

This is executed unconditionally, so the vmreads/vmwrites take place 
every time.  Need to cache the previous fpu_active state and only take 
action if it changed.

Since this is a large piece of code, move it to a function.

Please post this as the first patch (or better, separately), so I can 
apply it independently of the rest.

-- 
error compiling committee.c: too many arguments to function