Re: [kvm-devel] [BUG] Oops with KVM-27

[Avi Kivity <avi@qumranet.com>]

Luca Tettamanti wrote:
>> Actually we haven't; just before the memcpy(), we can put a memcmp() to
>> guard the kvm_mmu_pte_write(), which is the really expensive operation,
>> especially with guest smp.
>>     
>
> Yup, but it seemed wasteful to map (at least when highmem is in use) a
> page just to check for something that we already knew. That was a
> preemptive optmization though, I haven't actually benchmarked the cost
> of setting up the mapping ;-)
>
>   

It's negligible compared to the vmexit cost and to the emulation  (which 
does a kmap_atomic() for every byte of the instruction; this can be 
easily optimized away).

In any case, I expect that performance sensitive uses will use x86_64, 
whereas i386 is mostly for desktops.


>> I think we can simply remove the if ().  For the register case, the
>> check is more expensive that the write; for mmio, we don't want it; and
>> for memory writes, we can put it in emulator_write_phys().
>>     
>
> Ok, this way it's simpler. How does this look:
>
> --- a/kernel/x86_emulate.c	2007-06-15 21:13:51.000000000 +0200
> +++ b/kernel/x86_emulate.c	2007-06-17 16:57:50.000000000 +0200
> @@ -1057,40 +1057,38 @@
>  	}
>  
>  writeback:
> -	if ((d & Mov) || (dst.orig_val != dst.val)) {
> -		switch (dst.type) {
> -		case OP_REG:
> -			/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
> -			switch (dst.bytes) {
> -			case 1:
> -				*(u8 *)dst.ptr = (u8)dst.val;
> -				break;
> -			case 2:
> -				*(u16 *)dst.ptr = (u16)dst.val;
> -				break;
> -			case 4:
> -				*dst.ptr = (u32)dst.val;
> -				break;	/* 64b: zero-ext */
> -			case 8:
> -				*dst.ptr = dst.val;
> -				break;
> -			}
> +	switch (dst.type) {
> +	case OP_REG:
> +		/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
> +		switch (dst.bytes) {
> +		case 1:
> +			*(u8 *)dst.ptr = (u8)dst.val;
>  			break;
> -		case OP_MEM:
> -			if (lock_prefix)
> -				rc = ops->cmpxchg_emulated((unsigned long)dst.
> -							   ptr, &dst.orig_val,
> -							   &dst.val, dst.bytes,
> -							   ctxt);
> -			else
> -				rc = ops->write_emulated((unsigned long)dst.ptr,
> -							 &dst.val, dst.bytes,
> -							 ctxt);
> -			if (rc != 0)
> -				goto done;
> -		default:
> +		case 2:
> +			*(u16 *)dst.ptr = (u16)dst.val;
> +			break;
> +		case 4:
> +			*dst.ptr = (u32)dst.val;
> +			break;	/* 64b: zero-ext */
> +		case 8:
> +			*dst.ptr = dst.val;
>  			break;
>  		}
> +		break;
> +	case OP_MEM:
> +		if (lock_prefix)
> +			rc = ops->cmpxchg_emulated((unsigned long)dst.
> +						   ptr, &dst.orig_val,
> +						   &dst.val, dst.bytes,
> +						   ctxt);
> +		else
> +			rc = ops->write_emulated((unsigned long)dst.ptr,
> +						 &dst.val, dst.bytes,
> +						 ctxt);
> +		if (rc != 0)
> +			goto done;
> +	default:
> +		break;
>  	}
>  
>  	/* Commit shadow register state. */
>
> --- a/kernel/kvm_main.c	2007-06-15 21:18:08.000000000 +0200
> +++ b/kernel/kvm_main.c	2007-06-17 16:59:33.000000000 +0200
> @@ -1139,8 +1139,10 @@
>  		return 0;
>  	mark_page_dirty(vcpu->kvm, gpa >> PAGE_SHIFT);
>  	virt = kmap_atomic(page, KM_USER0);
> -	kvm_mmu_pte_write(vcpu, gpa, virt + offset, val, bytes);
> -	memcpy(virt + offset_in_page(gpa), val, bytes);
> +	if (memcmp(virt + offset_in_page(gpa), val, bytes)) {
> +		kvm_mmu_pte_write(vcpu, gpa, virt + offset, val, bytes);
> +		memcpy(virt + offset_in_page(gpa), val, bytes);
> +	}
>  	kunmap_atomic(virt, KM_USER0);
>  	return 1;
>  }
>
>
>   

Excellent.  We win back a precious indentation level and fix a bug at 
the same time.  Please test, send me a changelog and a signoff and I'll 
commit it.


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