Re: KVM: Fix the invlpg instruction emulation on AMD64

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

Avi Kivity a écrit :
> Aurelien Jarno wrote:
>>>> 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.
>>
>>   
> 
> 
> But invlpg is defined as having reg == 7! The manual says:
> 
> 0F 01/7 INVLPG m
> 
> where "/7" is the notation for "reg in the mod reg r/m byte:
> 
> " /digit — A digit between 0 and 7 indicates that the ModR/M byte of the
> instruction uses
> only the r/m (register or memory) operand. The reg field contains the
> digit that provides an
> extension to the instruction's opcode."
> 
> 
> So what you're seeing is not the invlpg instruction but something else
> in the same group.
> 

I see.

Anyway the previous behaviour on AMD was to disable SrcMem on all 0F 01
instructions. It's actually what I tried to do in the first version of
the patch, ie have the same behaviour as the SrcNone case for the 0F 01
instructions.

The 0F 01 instruction is then correctly emulated, but another of this
group is probably not, as the patch clearly fix the problem 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/