| Message ID | 1271013662-7640-1-git-send-email-andre.przywara@amd.com |
|---|---|
| State | New |
| Headers | show |
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.
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 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.
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); } }
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.