Re: [PATCH 25/28] nVMX: Further fixes for lazy FPU loading

[Avi Kivity <avi@redhat.com>]

On 12/08/2010 07:12 PM, Nadav Har'El wrote:
> KVM's "Lazy FPU loading" means that sometimes L0 needs to set CR0.TS, even
> if a guest didn't set it. Moreover, L0 must also trap CR0.TS changes and
> NM exceptions, even if we have a guest hypervisor (L1) who didn't want these
> traps. And of course, conversely: If L1 wanted to trap these events, we
> must let it, even if L0 is not interested in them.
>
> This patch fixes some existing KVM code (in update_exception_bitmap(),
> vmx_fpu_activate(), vmx_fpu_deactivate()) to do the correct merging of L0's
> and L1's needs. Note that handle_cr() was already fixed in the above patch,
> and that new code in introduced in previous patches already handles CR0
> correctly (see prepare_vmcs02(), prepare_vmcs12(), and nested_vmx_vmexit()).
>
>
> @@ -1415,8 +1424,19 @@ static void vmx_fpu_activate(struct kvm_
>   	cr0&= ~(X86_CR0_TS | X86_CR0_MP);
>   	cr0 |= kvm_read_cr0_bits(vcpu, X86_CR0_TS | X86_CR0_MP);
>   	vmcs_writel(GUEST_CR0, cr0);
> -	update_exception_bitmap(vcpu);
>   	vcpu->arch.cr0_guest_owned_bits = X86_CR0_TS;
> +	if (is_guest_mode(vcpu)) {
> +		/* While we (L0) no longer care about NM exceptions or cr0.TS
> +		 * changes, our guest hypervisor (L1) might care in which case
> +		 * we must trap them for it.
> +		 */
> +		u32 eb = vmcs_read32(EXCEPTION_BITMAP)&  ~(1u<<  NM_VECTOR);
> +		struct vmcs_fields *vmcs12 = get_vmcs12_fields(vcpu);
> +		eb |= vmcs12->exception_bitmap;
> +		vcpu->arch.cr0_guest_owned_bits&= ~vmcs12->cr0_guest_host_mask;
> +		vmcs_write32(EXCEPTION_BITMAP, eb);
> +	} else
> +		update_exception_bitmap(vcpu);

Isn't update_exception_bitmap() sufficient for both cases?

>   	vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
>   }
>
> @@ -1424,12 +1444,24 @@ static void vmx_decache_cr0_guest_bits(s
>
>   static void vmx_fpu_deactivate(struct kvm_vcpu *vcpu)
>   {
> +	/* Note that there is no vcpu->fpu_active = 0 here. The caller must
> +	 * set this *before* calling this function.
> +	 */
>   	vmx_decache_cr0_guest_bits(vcpu);
>   	vmcs_set_bits(GUEST_CR0, X86_CR0_TS | X86_CR0_MP);
> -	update_exception_bitmap(vcpu);
> +	vmcs_write32(EXCEPTION_BITMAP,
> +		vmcs_read32(EXCEPTION_BITMAP) | (1u<<  NM_VECTOR));

Why not fold the logic into update_exception_bitmap()?

>   	vcpu->arch.cr0_guest_owned_bits = 0;
>   	vmcs_writel(CR0_GUEST_HOST_MASK, ~vcpu->arch.cr0_guest_owned_bits);
> -	vmcs_writel(CR0_READ_SHADOW, vcpu->arch.cr0);
> +	if (is_guest_mode(vcpu))
> +		/* Unfortunately in nested mode we play with arch.cr0's PG
> +		 * bit, so we musn't copy it all, just the relevant TS bit
> +		 */
> +		vmcs_writel(CR0_READ_SHADOW,
> +			(vmcs_readl(CR0_READ_SHADOW)&  ~X86_CR0_TS) |
> +			(vcpu->arch.cr0&  X86_CR0_TS));
> +	else
> +		vmcs_writel(CR0_READ_SHADOW, vcpu->arch.cr0);

Didn't you have a nice guest_readable_cr0() function that did this?

>   }
>
>   static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)


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