[Qemu-devel] [PATCH][STABLE] fix CPUID vendor override

[Qemu-devel] [PATCH][STABLE] fix CPUID vendor override

[Andre Przywara]

the meaning of vendor_override is actually the opposite of how it
is currently used :-(
Fix it to allow KVM to export the non-native CPUID vendor if
explicitly requested by the user.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>
---
 target-i386/helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

I will send a refactoring patch including this fix for git HEAD later.

Regards,
Andre.

diff --git a/target-i386/helper.c b/target-i386/helper.c
index 9d7fec3..c17adc1 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1655,7 +1655,7 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
      * this if you want to use KVM's sysenter/syscall emulation
      * in compatibility mode and when doing cross vendor migration
      */
-    if (kvm_enabled() && env->cpuid_vendor_override) {
+    if (kvm_enabled() && ! env->cpuid_vendor_override) {
         host_cpuid(0, 0, NULL, ebx, ecx, edx);
     }
 }
-- 
1.6.4

[Qemu-devel] Re: [PATCH][STABLE] fix CPUID vendor override

[Avi Kivity]

On 04/11/2010 10:21 PM, Andre Przywara wrote:
> the meaning of vendor_override is actually the opposite of how it
> is currently used :-(
> Fix it to allow KVM to export the non-native CPUID vendor if
> explicitly requested by the user.
>
> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
> ---
>   target-i386/helper.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> I will send a refactoring patch including this fix for git HEAD later.
>    

The protocol is to first fix HEAD, then backport, so we don't introduce 
regressions inadvertently.  Also, separate fixes from refactoring.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH][STABLE] fix CPUID vendor override

[Andre Przywara]

Avi Kivity wrote:
> On 04/11/2010 10:21 PM, Andre Przywara wrote:
>> the meaning of vendor_override is actually the opposite of how it
>> is currently used :-(
>> Fix it to allow KVM to export the non-native CPUID vendor if
>> explicitly requested by the user.
>>
>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>> ---
>>   target-i386/helper.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> I will send a refactoring patch including this fix for git HEAD later.
>>    
> 
> The protocol is to first fix HEAD, then backport, so we don't introduce 
> regressions inadvertently.
Well, I just followed the call for 0.12.4. Actually this is the 
backport, one cannot easily cherry-pick the upstream patch, because it 
is in a different file now. I will send the (separated) upstream patch 
in a few minutes. Feel free to apply them in whatever order you prefer ;-)


Regards,
Andre.

P.S. I also have a qemu-kvm patch in the queue (because there was a 
separate workaround for this bug).

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

[Qemu-devel] Re: [PATCH][STABLE] fix CPUID vendor override

[Avi Kivity]

On 04/11/2010 10:21 PM, Andre Przywara wrote:
> the meaning of vendor_override is actually the opposite of how it
> is currently used :-(
> Fix it to allow KVM to export the non-native CPUID vendor if
> explicitly requested by the user.
>
> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
> ---
>   target-i386/helper.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> I will send a refactoring patch including this fix for git HEAD later.
>
> Regards,
> Andre.
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 9d7fec3..c17adc1 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1655,7 +1655,7 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>        * this if you want to use KVM's sysenter/syscall emulation
>        * in compatibility mode and when doing cross vendor migration
>        */
> -    if (kvm_enabled()&&  env->cpuid_vendor_override) {
> +    if (kvm_enabled()&&  ! env->cpuid_vendor_override) {
>           host_cpuid(0, 0, NULL, ebx, ecx, edx);
>       }
>   }
>    

Why is the original code wrong?  I would say vendor_override means 
overriding the qemu-picked vendor ID in favour of the host cpuid.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH][STABLE] fix CPUID vendor override

[Andre Przywara]

Avi Kivity wrote:
> On 04/11/2010 10:21 PM, Andre Przywara wrote:
>> the meaning of vendor_override is actually the opposite of how it
>> is currently used :-(
>> Fix it to allow KVM to export the non-native CPUID vendor if
>> explicitly requested by the user.
>>
>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>> ---
>>   target-i386/helper.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> I will send a refactoring patch including this fix for git HEAD later.
>>
>> Regards,
>> Andre.
>>
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index 9d7fec3..c17adc1 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -1655,7 +1655,7 @@ static void get_cpuid_vendor(CPUX86State *env, 
>> uint32_t *ebx,
>>        * this if you want to use KVM's sysenter/syscall emulation
>>        * in compatibility mode and when doing cross vendor migration
>>        */
>> -    if (kvm_enabled()&&  env->cpuid_vendor_override) {
>> +    if (kvm_enabled()&&  ! env->cpuid_vendor_override) {
>>           host_cpuid(0, 0, NULL, ebx, ecx, edx);
>>       }
>>   }
>>    
> 
> Why is the original code wrong?  I would say vendor_override means 
> overriding the qemu-picked vendor ID in favour of the host cpuid.
I meant it to mean: override the automatically chosen vendor ID (which 
is the host ID in case of KVM). I think the reason for KVM to use the 
host ID is valid, so I wanted to have an explicit override only if the 
user says so.
If you look at the code, you will see that it is initialized to 0 and 
only set to 1 if one specifies an explicit vendor ID on the command line.
Honestly I cannot say how this bug slipped through, I can only guess 
that I tricked myself while making the final version of the patch 
(lost-in-branches(TM))

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12

Re: [Qemu-devel] Re: [PATCH][STABLE] fix CPUID vendor override

[Aurelien Jarno]

On Sun, Apr 11, 2010 at 09:49:40PM +0200, Andre Przywara wrote:
> Avi Kivity wrote:
>> On 04/11/2010 10:21 PM, Andre Przywara wrote:
>>> the meaning of vendor_override is actually the opposite of how it
>>> is currently used :-(
>>> Fix it to allow KVM to export the non-native CPUID vendor if
>>> explicitly requested by the user.
>>>
>>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>>> ---
>>>   target-i386/helper.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> I will send a refactoring patch including this fix for git HEAD later.
>>>
>>> Regards,
>>> Andre.
>>>
>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>> index 9d7fec3..c17adc1 100644
>>> --- a/target-i386/helper.c
>>> +++ b/target-i386/helper.c
>>> @@ -1655,7 +1655,7 @@ static void get_cpuid_vendor(CPUX86State *env,  
>>> uint32_t *ebx,
>>>        * this if you want to use KVM's sysenter/syscall emulation
>>>        * in compatibility mode and when doing cross vendor migration
>>>        */
>>> -    if (kvm_enabled()&&  env->cpuid_vendor_override) {
>>> +    if (kvm_enabled()&&  ! env->cpuid_vendor_override) {
>>>           host_cpuid(0, 0, NULL, ebx, ecx, edx);
>>>       }
>>>   }
>>>    
>>
>> Why is the original code wrong?  I would say vendor_override means  
>> overriding the qemu-picked vendor ID in favour of the host cpuid.
> I meant it to mean: override the automatically chosen vendor ID (which  
> is the host ID in case of KVM). I think the reason for KVM to use the  
> host ID is valid, so I wanted to have an explicit override only if the  
> user says so.
> If you look at the code, you will see that it is initialized to 0 and  
> only set to 1 if one specifies an explicit vendor ID on the command line.
> Honestly I cannot say how this bug slipped through, I can only guess  
> that I tricked myself while making the final version of the patch  
> (lost-in-branches(TM))
>

While it clearly fixes the -cpu vendor option when kvm is enabled, it
also forces the vendor id to the one of the host. Try for example -cpu
qemu64. Before your patch the vendor id is "Authentic AMD" and after
your patch it is "Genuine Intel"

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH][STABLE] fix CPUID vendor override

[Andre Przywara]

Aurelien Jarno wrote:
> On Sun, Apr 11, 2010 at 09:49:40PM +0200, Andre Przywara wrote:
>> Avi Kivity wrote:
>>> On 04/11/2010 10:21 PM, Andre Przywara wrote:
>>>> the meaning of vendor_override is actually the opposite of how it
>>>> is currently used :-(
>>>> Fix it to allow KVM to export the non-native CPUID vendor if
>>>> explicitly requested by the user.
>>>>
>>>> Signed-off-by: Andre Przywara<andre.przywara@amd.com>
>>>> ---
>>>>   target-i386/helper.c |    2 +-
>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> I will send a refactoring patch including this fix for git HEAD later.
>>>>
>>>> Regards,
>>>> Andre.
>>>>
>>>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>>>> index 9d7fec3..c17adc1 100644
>>>> --- a/target-i386/helper.c
>>>> +++ b/target-i386/helper.c
>>>> @@ -1655,7 +1655,7 @@ static void get_cpuid_vendor(CPUX86State *env,  
>>>> uint32_t *ebx,
>>>>        * this if you want to use KVM's sysenter/syscall emulation
>>>>        * in compatibility mode and when doing cross vendor migration
>>>>        */
>>>> -    if (kvm_enabled()&&  env->cpuid_vendor_override) {
>>>> +    if (kvm_enabled()&&  ! env->cpuid_vendor_override) {
>>>>           host_cpuid(0, 0, NULL, ebx, ecx, edx);
>>>>       }
>>>>   }
>>>>    
>>> Why is the original code wrong?  I would say vendor_override means  
>>> overriding the qemu-picked vendor ID in favour of the host cpuid.
>> I meant it to mean: override the automatically chosen vendor ID (which  
>> is the host ID in case of KVM). I think the reason for KVM to use the  
>> host ID is valid, so I wanted to have an explicit override only if the  
>> user says so.
>> If you look at the code, you will see that it is initialized to 0 and  
>> only set to 1 if one specifies an explicit vendor ID on the command line.
>> Honestly I cannot say how this bug slipped through, I can only guess  
>> that I tricked myself while making the final version of the patch  
>> (lost-in-branches(TM))
>>
> 
> While it clearly fixes the -cpu vendor option when kvm is enabled, it
> also forces the vendor id to the one of the host. Try for example -cpu
> qemu64. Before your patch the vendor id is "Authentic AMD" and after
> your patch it is "Genuine Intel"
You are using the wrong CPU ;-)
This is actually correct for KVM. (or are you seeing this with TCG?)
To make sure we are actually talk about the same problem, the intended 
behavior is:
With TCG:
     - always inject the configured vendor (either hard-coded, in config 
files or via ",vendor=" commandline)
With KVM:
     - by default inject the host's vendor
     - if the user specifies ",vendor=" on the commandline, use this 
instead of the host's vendor
     - all pre-configured vendors (hard-coded, config file) are ignored

The reason KVM is injecting the host's vendor is the following:
There is a nasty difference between AMD and Intel chips in supporting 
syscall and sysenter in _compat_ mode (that is: running a 32bit binary 
while the OS is running 64bit). This mode is 99% compatible with the 
legacy protected mode, but AMD does not support sysenter here and Intel 
does not support syscall. So for instance the Linux kernel checks the 
vendor id and tells the userspace to use the supported instruction. On 
TCG we don't care as we properly emulate both(?), but on KVM the 
instruction is called natively. This leads to a crash (#UD exception) if 
the wrong vendor is used. So a long time ago the workaround of always 
propagating the host vendor was introduced.
Later last year I introduced emulation of the missing instructions, so 
to overcome the former limitation (and allow for more hacking with KVM) 
I introduced the vendor override. Sadly my patch was broken (I used a 
slightly different version for development), that's why this fix. 
qemu-kvm had an additional fix to kind of work-around this bug.

Hope that clarifies the issue, if you meant something else, tell me.

Regards,
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12