Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno <aurelien-rXXEIb44qovR7s880joybQ@public.gmane.org>]

Aurelien Jarno a écrit :
> Avi Kivity a écrit :
>> Aurelien Jarno wrote:
>>> On Tue, Oct 16, 2007 at 12:12:18PM +0200, Avi Kivity wrote:
>>>   
>>>> Aurelien Jarno wrote:
>>>>     
>>>>> I have marked the invlpg instruction the same way as it is done in
>>>>> kvm-37 to know what happens. I get either modrm_reg = 4 or = 6 when the
>>>>> invlpg instruction is executed, but never = 7.
>>>>>
>>>>>  
>>>>>       
>>>> Then it isn't the invlpg instruction at all.  Rather smsw (modrm_reg == 
>>>> 4) or lmsw ( == 6).
>>>>
>>>> I'm confused.
>>>>
>>>> (looks)
>>>>
>>>> Okay.  What we have here is total breakage when emulating an instruction 
>>>> that uses a mod r/m encoding that actually refers to a register 
>>>> (modrm_mod == 3).  In x86_decode_insn() we set src.type as OP_MEM, and 
>>>> in x86_emulate_insn() we happily fetch it even though it's a register, 
>>>> generating a fault.
>>>>
>>>> It usually doesn't bite us because these instructions are directly executed.
>>>>
>>>> The fix is probably to switch to OP_REG if SrcMem and ModRM and 
>>>> modrm_mod == 3 (similarly for DstMem).
>>>>
>>>>     
>>> Ok, I have tried to implement that for SrcMem, and it works in my case.
>>> I am able to boot FreeBSD, and I am seeing no regression for Linux or
>>> Hurd VMs. Does the patch below looks ok to you?
>>>
>>>
>>>
>>> KVM: Fix the invlpg instruction emulation on AMD64
>>>
>>> The patch below correctly detects the invlpg instruction, and switch 
>>> src.type to OP_REG. This fixes the boot of FreeBSD on an AMD64 CPU, it has
>>> been broken since commit aa38840d3d2e0a804e628077df8d8879b496d741.
>>>
>>> It also moves the assignation of c->src.bytes after the test as it is
>>> not needed for the invlpg instruction.
>>>
>>> Signed-off-by: Aurelien Jarno <aurelien-rXXEIb44qovR7s880joybQ@public.gmane.org>
>>>
>>> diff --git a/drivers/kvm/x86_emulate.c b/drivers/kvm/x86_emulate.c
>>> index e974ace..06e183b 100644
>>> --- a/drivers/kvm/x86_emulate.c
>>> +++ b/drivers/kvm/x86_emulate.c
>>> @@ -825,12 +825,12 @@ modrm_done:
>>>  		c->src.bytes = 4;
>>>  		goto srcmem_common;
>>>  	case SrcMem:
>>> -		c->src.bytes = (c->d & ByteOp) ? 1 :
>>> -							   c->op_bytes;
>>>  		/* Don't fetch the address for invlpg: it could be unmapped. */
>>> -		if (c->twobyte && c->b == 0x01
>>> -				    && c->modrm_reg == 7)
>>> +		if ((c->d & ModRM) && c->modrm_mod == 3) {
>>> +			c->src.type = OP_REG;
>>>  			break;
>>> +		}
>>> +		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>>>  srcmem_common:
>>>  		c->src.type = OP_MEM;
>>>  		break;
>>>   
>> Why did you remove the check for invlpg?  It need not be executed with
>> modrm_mod == 3 (in fact invlpg with modrm_mod == 3 is useless), so it
>> won't be caught by this.
> 
> Because I really thing this check is buggy. On my tests, modrm_reg is
> always different than 7 for the invlpg instruction, so the test is
> always false.

JFTR, in kvm-37 (the latest version that work out of the box for
FreeBSD), SrcMem is removed from twobyte[1] on AMD.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org         | aurelien-rXXEIb44qovR7s880joybQ@public.gmane.org
   `-    people.debian.org/~aurel32 | www.aurel32.net

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/