Re: KVM: Fix the invlpg instruction emulation on AMD64

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Aurelien Jarno wrote:
> 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.
>
>   

It had this little beauty:

> /*
>  * Tell the emulator that of the Group 7 instructions (sgdt, lidt,
> etc.) we
>  * are interested only in invlpg and not in any of the rest.
>  *
>  * invlpg is a special instruction in that the data it references may not
>  * be mapped.
>  */
> void kvm_emulator_want_group7_invlpg(void)
> {
>         twobyte_table[1] &= ~SrcMem;
> }
> EXPORT_SYMBOL_GPL(kvm_emulator_want_group7_invlpg);

It took advantage of the fact that AMD doesn't need to emulate sgdt and
lidt in real mode, so it wouldn't get called for these instructions. 
But they are called on Intel so we can't break them.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
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/