KVM: Fix the invlpg instruction emulation on AMD64

KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

The patch below removes the check for c->modrm_reg == 7 to detect the 
invlpg instruction, as it was the case before before commit 
aa38840d3d2e0a804e628077df8d8879b496d741. This fixes the boot of FreeBSD
on an AMD64 CPU.

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 fa33fcd..01aa952 100644
--- a/drivers/kvm/x86_emulate.c
+++ b/drivers/kvm/x86_emulate.c
@@ -824,12 +824,10 @@ 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->twobyte && c->b == 0x01)
 			break;
+		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
 srcmem_common:
 		c->src.type = OP_MEM;
 		break;

-- 
  .''`.  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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Avi Kivity]

Aurelien Jarno wrote:
> The patch below removes the check for c->modrm_reg == 7 to detect the 
> invlpg instruction, as it was the case before before commit 
> aa38840d3d2e0a804e628077df8d8879b496d741. This fixes the boot of FreeBSD
> on an AMD64 CPU.
>
> 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 fa33fcd..01aa952 100644
> --- a/drivers/kvm/x86_emulate.c
> +++ b/drivers/kvm/x86_emulate.c
> @@ -824,12 +824,10 @@ 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->twobyte && c->b == 0x01)
>  			break;
> +		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>
>   

I don't understand why this helps.  All of the other instructions in 
this group either have modrm_mod == 3 or do require evaluation of the 
source.  invlpg is the only one that doesn't.

It looks like this patch would break lgdt and lidt (which are only 
needed on Intel).

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


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

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

Avi Kivity a écrit :
> Aurelien Jarno wrote:
>> The patch below removes the check for c->modrm_reg == 7 to detect the 
>> invlpg instruction, as it was the case before before commit 
>> aa38840d3d2e0a804e628077df8d8879b496d741. This fixes the boot of FreeBSD
>> on an AMD64 CPU.
>>
>> 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 fa33fcd..01aa952 100644
>> --- a/drivers/kvm/x86_emulate.c
>> +++ b/drivers/kvm/x86_emulate.c
>> @@ -824,12 +824,10 @@ 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->twobyte && c->b == 0x01)
>>  			break;
>> +		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>>
>>   
> 
> I don't understand why this helps.  All of the other instructions in 
> this group either have modrm_mod == 3 or do require evaluation of the 
                         ^^^^^^^^^
The test actually concerns modrm_reg and not modrm_mod. Maybe it is wrong?

> source.  invlpg is the only one that doesn't.

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.

> It looks like this patch would break lgdt and lidt (which are only 
> needed on Intel).
> 

I admit I haven't tried on Intel.

-- 
  .''`.  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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Avi Kivity]

Aurelien Jarno wrote:
> Avi Kivity a écrit :
>   
>> Aurelien Jarno wrote:
>>     
>>> The patch below removes the check for c->modrm_reg == 7 to detect the 
>>> invlpg instruction, as it was the case before before commit 
>>> aa38840d3d2e0a804e628077df8d8879b496d741. This fixes the boot of FreeBSD
>>> on an AMD64 CPU.
>>>
>>> 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 fa33fcd..01aa952 100644
>>> --- a/drivers/kvm/x86_emulate.c
>>> +++ b/drivers/kvm/x86_emulate.c
>>> @@ -824,12 +824,10 @@ 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->twobyte && c->b == 0x01)
>>>  			break;
>>> +		c->src.bytes = (c->d & ByteOp) ? 1 : c->op_bytes;
>>>
>>>   
>>>       
>> I don't understand why this helps.  All of the other instructions in 
>> this group either have modrm_mod == 3 or do require evaluation of the 
>>     
>                          ^^^^^^^^^
> The test actually concerns modrm_reg and not modrm_mod. Maybe it is wrong?
>
>   

I meant, the instructions all require modrm_mod == 3 which means they 
reference registers, and not memory.


>> source.  invlpg is the only one that doesn't.
>>     
>
> 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).

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


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

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

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;


-- 
  .''`.  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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Avi Kivity]

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.

Also, we need to initialize the fields of src in this case.


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

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

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.

> Also, we need to initialize the fields of src in this case.
> 

Correct.

-- 
  .''`.  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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Avi Kivity]

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.

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

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Avi Kivity]

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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

Aurelien Jarno a écrit :
> Avi Kivity a écrit :
>>
>> 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
Oops s/0F 01/invlpg/
> 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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

Aurelien Jarno a écrit :
> Avi Kivity a écrit :
>>
>> 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
Oops s/0F 01/invlpg/
> 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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

Aurelien Jarno a écrit :
> Avi Kivity a écrit :
>>
>> 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
Oops s/0F 01/invlpg/
> 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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

Aurelien Jarno a écrit :
> Avi Kivity a écrit :
>>
>> 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
Oops s/0F 01/invlpg/
> 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/

Re: KVM: Fix the invlpg instruction emulation on AMD64

[Aurelien Jarno]

Aurelien Jarno a écrit :
> Aurelien Jarno a écrit :
>> Avi Kivity a écrit :
>>> 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
> Oops s/0F 01/invlpg/
>> group is probably not, as the patch clearly fix the problem on AMD.
> 

I confirm the problem. SVM does not virtualize smsw and lmsw that have
to be emulated. And it looks like the decoding or/and emulation of those
instructions is currently broken in KVM.

-- 
  .''`.  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/