[PATCH] mask out clflush

[PATCH] mask out clflush

[Glauber Costa]

clflush is a non-privileged instruction that flushes the cacheline
given by its parameter, in terms of linear address. As it is non-privileged,
it is quite tricky, because a guest doing clflush will actually be trying to
flush a host kernel address.

Signed-off-by: Glauber Costa <gcosta@redhat.com>
---
 qemu/qemu-kvm-x86.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index 5daedd1..7f90fc2 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -505,13 +505,17 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
 	e->ecx = bcd[1];
 	e->edx = bcd[2];
     }
-    // "Hypervisor present" bit for Microsoft guests
-    if (function == 1)
-	e->ecx |= (1u << 31);
+
+    if (function == 1) {
+        // "Hypervisor present" bit for Microsoft guests
+        e->ecx |= (1u << 31);
+        e->edx &= ~(1u << 19);
+    }
 
     // 3dnow isn't properly emulated yet
     if (function == 0x80000001)
-	e->edx &= ~0xc0000000;
+        e->edx &= ~0xc0000000;
+
 }
 
 struct kvm_para_features {
-- 
1.5.5.1

Re: [PATCH] mask out clflush

[Anthony Liguori]

Glauber Costa wrote:
> clflush is a non-privileged instruction that flushes the cacheline
> given by its parameter, in terms of linear address. As it is non-privileged,
> it is quite tricky, because a guest doing clflush will actually be trying to
> flush a host kernel address.
>   

Is this the case still with NPT/EPT?

Regards,

Anthony Liguori

> Signed-off-by: Glauber Costa <gcosta@redhat.com>
> ---
>  qemu/qemu-kvm-x86.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
> index 5daedd1..7f90fc2 100644
> --- a/qemu/qemu-kvm-x86.c
> +++ b/qemu/qemu-kvm-x86.c
> @@ -505,13 +505,17 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e, uint32_t function,
>  	e->ecx = bcd[1];
>  	e->edx = bcd[2];
>      }
> -    // "Hypervisor present" bit for Microsoft guests
> -    if (function == 1)
> -	e->ecx |= (1u << 31);
> +
> +    if (function == 1) {
> +        // "Hypervisor present" bit for Microsoft guests
> +        e->ecx |= (1u << 31);
> +        e->edx &= ~(1u << 19);
> +    }
>  
>      // 3dnow isn't properly emulated yet
>      if (function == 0x80000001)
> -	e->edx &= ~0xc0000000;
> +        e->edx &= ~0xc0000000;
> +
>  }
>  
>  struct kvm_para_features {
>   

Re: [PATCH] mask out clflush

[Glauber Costa]

On Tue, Jul 8, 2008 at 4:34 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Glauber Costa wrote:
>>
>> clflush is a non-privileged instruction that flushes the cacheline
>> given by its parameter, in terms of linear address. As it is
>> non-privileged,
>> it is quite tricky, because a guest doing clflush will actually be trying
>> to
>> flush a host kernel address.
>>
>
> Is this the case still with NPT/EPT?

Honestly don't know, and don't have the hardware to test. I don't know
exactly how NPT works. But probably isn't an issue. Do we have any
kind of mechanism to test for npt from userspace? A quick grep didn't
show me.

> Regards,
>
> Anthony Liguori
>
>> Signed-off-by: Glauber Costa <gcosta@redhat.com>
>> ---
>>  qemu/qemu-kvm-x86.c |   12 ++++++++----
>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
>> index 5daedd1..7f90fc2 100644
>> --- a/qemu/qemu-kvm-x86.c
>> +++ b/qemu/qemu-kvm-x86.c
>> @@ -505,13 +505,17 @@ static void do_cpuid_ent(struct kvm_cpuid_entry *e,
>> uint32_t function,
>>        e->ecx = bcd[1];
>>        e->edx = bcd[2];
>>     }
>> -    // "Hypervisor present" bit for Microsoft guests
>> -    if (function == 1)
>> -       e->ecx |= (1u << 31);
>> +
>> +    if (function == 1) {
>> +        // "Hypervisor present" bit for Microsoft guests
>> +        e->ecx |= (1u << 31);
>> +        e->edx &= ~(1u << 19);
>> +    }
>>       // 3dnow isn't properly emulated yet
>>     if (function == 0x80000001)
>> -       e->edx &= ~0xc0000000;
>> +        e->edx &= ~0xc0000000;
>> +
>>  }
>>   struct kvm_para_features {
>>
>
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH] mask out clflush

[Yang, Sheng]

On Wednesday 09 July 2008 02:29:44 Glauber Costa wrote:
> clflush is a non-privileged instruction that flushes the cacheline
> given by its parameter, in terms of linear address. As it is
> non-privileged, it is quite tricky, because a guest doing clflush
> will actually be trying to flush a host kernel address.

The linear address was convert to host physical address, then cache 
line was flushed. Of course the host physical address was used by 
guest at the time. I don't understand why we need to prevent guest 
from flushing cache line related to itself...

--
Thanks
Yang, Sheng

Re: [PATCH] mask out clflush

[Anthony Liguori]

Yang, Sheng wrote:
> On Wednesday 09 July 2008 02:29:44 Glauber Costa wrote:
>   
>> clflush is a non-privileged instruction that flushes the cacheline
>> given by its parameter, in terms of linear address. As it is
>> non-privileged, it is quite tricky, because a guest doing clflush
>> will actually be trying to flush a host kernel address.
>>     
>
> The linear address was convert to host physical address, then cache 
> line was flushed. Of course the host physical address was used by 
> guest at the time. I don't understand why we need to prevent guest 
> from flushing cache line related to itself...
>   

The problem turned out to be that we aren't emulating clflush in 
x86_emulate.

Regards,

Anthony Liguori

> --
> Thanks
> Yang, Sheng
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH] mask out clflush

[Avi Kivity]

Anthony Liguori wrote:
> Yang, Sheng wrote:
>> On Wednesday 09 July 2008 02:29:44 Glauber Costa wrote:
>>  
>>> clflush is a non-privileged instruction that flushes the cacheline
>>> given by its parameter, in terms of linear address. As it is
>>> non-privileged, it is quite tricky, because a guest doing clflush
>>> will actually be trying to flush a host kernel address.
>>>     
>>
>> The linear address was convert to host physical address, then cache 
>> line was flushed. Of course the host physical address was used by 
>> guest at the time. I don't understand why we need to prevent guest 
>> from flushing cache line related to itself...
>>   
>
> The problem turned out to be that we aren't emulating clflush in 
> x86_emulate.
>

Why would clflush trap?  Is it called from real mode?

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

Re: [PATCH] mask out clflush

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Yang, Sheng wrote:
>>> On Wednesday 09 July 2008 02:29:44 Glauber Costa wrote:
>>>  
>>>> clflush is a non-privileged instruction that flushes the cacheline
>>>> given by its parameter, in terms of linear address. As it is
>>>> non-privileged, it is quite tricky, because a guest doing clflush
>>>> will actually be trying to flush a host kernel address.
>>>>     
>>>
>>> The linear address was convert to host physical address, then cache 
>>> line was flushed. Of course the host physical address was used by 
>>> guest at the time. I don't understand why we need to prevent guest 
>>> from flushing cache line related to itself...
>>>   
>>
>> The problem turned out to be that we aren't emulating clflush in 
>> x86_emulate.
>>
>
> Why would clflush trap?  Is it called from real mode?

It's equivalent to a read from a VT perspective so if the read would 
trap, the clflush instruction will trap.

Regards,

Anthony Liguori

Re: [PATCH] mask out clflush

[Avi Kivity]

Anthony Liguori wrote:
>
> It's equivalent to a read from a VT perspective so if the read would 
> trap, the clflush instruction will trap.
>

Reads don't normally go through the emulator.  Is the guest clflush()ing 
mmio addresses?  Strange as these are not normally cached.


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

Re: [PATCH] mask out clflush

[Avi Kivity]

Glauber Costa wrote:
> clflush is a non-privileged instruction that flushes the cacheline
> given by its parameter, in terms of linear address. As it is non-privileged,
> it is quite tricky, because a guest doing clflush will actually be trying to
> flush a host kernel address.
>   

We need to allow clflush for pci device assignment.

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

Re: [PATCH] mask out clflush

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>
>> It's equivalent to a read from a VT perspective so if the read would 
>> trap, the clflush instruction will trap.
>>
>
> Reads don't normally go through the emulator.  Is the guest 
> clflush()ing mmio addresses?  Strange as these are not normally cached.

It seems so, Glauber mentioned that the address was an MMIO address.

Regards,

Anthony Liguori

Re: [PATCH] mask out clflush

[Glauber Costa]

On Thu, Jul 10, 2008 at 11:20 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Avi Kivity wrote:
>>
>> Anthony Liguori wrote:
>>>
>>> It's equivalent to a read from a VT perspective so if the read would
>>> trap, the clflush instruction will trap.
>>>
>>
>> Reads don't normally go through the emulator.  Is the guest clflush()ing
>> mmio addresses?  Strange as these are not normally cached.
>
> It seems so, Glauber mentioned that the address was an MMIO address.

yes. It is address 0xc8821000, apparently part of a pci controller
initialization.

> Regards,
>
> Anthony Liguori
>
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [PATCH] mask out clflush

[Avi Kivity]

Glauber Costa wrote:
> On Thu, Jul 10, 2008 at 11:20 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>   
>> Avi Kivity wrote:
>>     
>>> Anthony Liguori wrote:
>>>       
>>>> It's equivalent to a read from a VT perspective so if the read would
>>>> trap, the clflush instruction will trap.
>>>>
>>>>         
>>> Reads don't normally go through the emulator.  Is the guest clflush()ing
>>> mmio addresses?  Strange as these are not normally cached.
>>>       
>> It seems so, Glauber mentioned that the address was an MMIO address.
>>     
>
> yes. It is address 0xc8821000, apparently part of a pci controller
> initialization.
>   

qemu pci starts at 0xe0000000 IIRC.  So maybe the guest is flushing 
random addresses just to be annoying.

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

Re: [PATCH] mask out clflush

[Anthony Liguori]

Avi Kivity wrote:
> Glauber Costa wrote:
>> On Thu, Jul 10, 2008 at 11:20 AM, Anthony Liguori 
>> <anthony@codemonkey.ws> wrote:
>>  
>>> Avi Kivity wrote:
>>>    
>>>> Anthony Liguori wrote:
>>>>      
>>>>> It's equivalent to a read from a VT perspective so if the read would
>>>>> trap, the clflush instruction will trap.
>>>>>
>>>>>         
>>>> Reads don't normally go through the emulator.  Is the guest 
>>>> clflush()ing
>>>> mmio addresses?  Strange as these are not normally cached.
>>>>       
>>> It seems so, Glauber mentioned that the address was an MMIO address.
>>>     
>>
>> yes. It is address 0xc8821000, apparently part of a pci controller
>> initialization.
>>   
>
> qemu pci starts at 0xe0000000 IIRC.  So maybe the guest is flushing 
> random addresses just to be annoying.

That's a virtual address, not a physical address IIUC.

Regards,

Anthony Liguori

Re: [PATCH] mask out clflush

[Avi Kivity]

Anthony Liguori wrote:
>>>
>>> yes. It is address 0xc8821000, apparently part of a pci controller
>>> initialization.
>>>   
>>
>> qemu pci starts at 0xe0000000 IIRC.  So maybe the guest is flushing 
>> random addresses just to be annoying.
>
> That's a virtual address, not a physical address IIUC.
>

Ah, of course.


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

Re: [PATCH] mask out clflush

[Glauber Costa]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Thu, Jul 10, 2008 at 12:39 PM, Avi Kivity <avi@qumranet.com> wrote:
> Anthony Liguori wrote:
>>>>
>>>> yes. It is address 0xc8821000, apparently part of a pci controller
>>>> initialization.
>>>>
>>>
>>> qemu pci starts at 0xe0000000 IIRC.  So maybe the guest is flushing
>>> random addresses just to be annoying.
>>
>> That's a virtual address, not a physical address IIUC.
>>
>
> Ah, of course.

How's that one ?
>
> --
> error compiling committee.c: too many arguments to function
>
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

[-- Attachment #2: 0001-properly-decode-clflush.patch --]
[-- Type: application/octet-stream, Size: 1155 bytes --]

From 7f60c9c828c7907625f634baf740376bdee7726b Mon Sep 17 00:00:00 2001
From: Glauber Costa <gcosta@redhat.com>
Date: Thu, 10 Jul 2008 17:08:15 -0300
Subject: [PATCH] properly decode clflush

If the guest issues a clflush in a mmio address, the instruction
can trap into the hypervisor. Currently, we do not decode clflush
properly, causing the guest to hang. This patch fixes this by
adding the "ModRM" flag into position 0xAE of twobyte_table.

Signed-off-by: Glauber Costa <gcosta@redhat.com>
---
 arch/x86/kvm/x86_emulate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index 932f216..ee4cebf 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -215,7 +215,7 @@ static u16 twobyte_table[256] = {
 	/* 0xA0 - 0xA7 */
 	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, 0, 0,
 	/* 0xA8 - 0xAF */
-	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, 0, 0,
+	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, ModRM, 0,
 	/* 0xB0 - 0xB7 */
 	ByteOp | DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM, 0,
 	    DstMem | SrcReg | ModRM | BitOp,
-- 
1.5.5.1

Re: [PATCH] mask out clflush

[Avi Kivity]

Glauber Costa wrote:
> On Thu, Jul 10, 2008 at 12:39 PM, Avi Kivity <avi@qumranet.com> wrote:
>   
>> Anthony Liguori wrote:
>>     
>>>>> yes. It is address 0xc8821000, apparently part of a pci controller
>>>>> initialization.
>>>>>
>>>>>           
>>>> qemu pci starts at 0xe0000000 IIRC.  So maybe the guest is flushing
>>>> random addresses just to be annoying.
>>>>         
>>> That's a virtual address, not a physical address IIUC.
>>>
>>>       
>> Ah, of course.
>>     
>
> How's that one ?
>   

It abuses the lack of a 'default:' statement in the switch.  I fixed 
that up and applied.

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