| Message ID | 1320846276-19659-1-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/09/2011 07:44 AM, Avi Kivity wrote: > The fact that a host cpu supports a feature doesn't mean that QEMU and KVM > will also support it, yet -cpuid host brings host features wholesale. > > We need to whitelist each feature separately to make sure we support it. > This patch adds KVM whitelisting (by simply using KVM_GET_SUPPORTED_CPUID > instead of the CPUID instruction). > > Signed-off-by: Avi Kivity<avi@redhat.com> This seems like a 1.0 candidate, yes? Regards, Anthony Liguori > --- > target-i386/cpuid.c | 27 ++++----------------------- > 1 files changed, 4 insertions(+), 23 deletions(-) > > diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c > index 1e8bcff..edac377 100644 > --- a/target-i386/cpuid.c > +++ b/target-i386/cpuid.c > @@ -107,33 +107,14 @@ void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > { > #if defined(CONFIG_KVM) > - uint32_t vec[4]; > - > -#ifdef __x86_64__ > - asm volatile("cpuid" > - : "=a"(vec[0]), "=b"(vec[1]), > - "=c"(vec[2]), "=d"(vec[3]) > - : "0"(function), "c"(count) : "cc"); > -#else > - asm volatile("pusha \n\t" > - "cpuid \n\t" > - "mov %%eax, 0(%2) \n\t" > - "mov %%ebx, 4(%2) \n\t" > - "mov %%ecx, 8(%2) \n\t" > - "mov %%edx, 12(%2) \n\t" > - "popa" > - : : "a"(function), "c"(count), "S"(vec) > - : "memory", "cc"); > -#endif > - > if (eax) > - *eax = vec[0]; > + *eax = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EAX); > if (ebx) > - *ebx = vec[1]; > + *ebx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EBX); > if (ecx) > - *ecx = vec[2]; > + *ecx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_ECX); > if (edx) > - *edx = vec[3]; > + *edx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EDX); > #endif > } >
On 11/09/2011 07:56 PM, Anthony Liguori wrote: > On 11/09/2011 07:44 AM, Avi Kivity wrote: >> The fact that a host cpu supports a feature doesn't mean that QEMU >> and KVM >> will also support it, yet -cpuid host brings host features wholesale. >> >> We need to whitelist each feature separately to make sure we support it. >> This patch adds KVM whitelisting (by simply using >> KVM_GET_SUPPORTED_CPUID >> instead of the CPUID instruction). >> >> Signed-off-by: Avi Kivity<avi@redhat.com> > > This seems like a 1.0 candidate, yes? There is a distinct possibility this will uncover bugs in kvm's KVM_GET_SUPPORTED_CPUID. Those won't be qemu bugs, so I think it's good for 1.0.
On Wed, 2011-11-09 at 20:00 +0200, Avi Kivity wrote: > On 11/09/2011 07:56 PM, Anthony Liguori wrote: > > On 11/09/2011 07:44 AM, Avi Kivity wrote: > >> The fact that a host cpu supports a feature doesn't mean that QEMU > >> and KVM > >> will also support it, yet -cpuid host brings host features wholesale. > >> > >> We need to whitelist each feature separately to make sure we support it. > >> This patch adds KVM whitelisting (by simply using > >> KVM_GET_SUPPORTED_CPUID > >> instead of the CPUID instruction). > >> > >> Signed-off-by: Avi Kivity<avi@redhat.com> > > > > This seems like a 1.0 candidate, yes? > > There is a distinct possibility this will uncover bugs in kvm's > KVM_GET_SUPPORTED_CPUID. Those won't be qemu bugs, so I think it's good > for 1.0. > Avi, we have a problem in the KVM tool of KVM_GET_SUPPORTED_CPUID sometimes returning -E2BIG. I've sent a mail about it some time ago, but we couldn't really find the reason. It's somewhat non-deterministic, and theres no sure way to reproduce it, but it doesn't happen that rarely. The block of code that uses it from usermode it pretty simple: struct kvm_cpuid2 *kvm_cpuid; kvm_cpuid = calloc(1, sizeof(*kvm_cpuid) + MAX_KVM_CPUID_ENTRIES * sizeof(*kvm_cpuid->entries)); kvm_cpuid->nent = MAX_KVM_CPUID_ENTRIES; if (ioctl(vcpu->kvm->sys_fd, KVM_GET_SUPPORTED_CPUID, kvm_cpuid) < 0) die_perror("KVM_GET_SUPPORTED_CPUID failed"); MAX_KVM_CPUID_ENTRIES is set to 100, which is more than the 80 defined in the kernel, so it shouldn't be an issue. It wouldn't explain the non deterministic behavior either. QEMU's code around it allows it to hide the bug if it does happen: uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, uint32_t index, int reg) { struct kvm_cpuid2 *cpuid; int i, max; uint32_t ret = 0; uint32_t cpuid_1_edx; int has_kvm_features = 0; max = 1; while ((cpuid = try_get_cpuid(s, max)) == NULL) { max *= 2; } [snip] Which means that if it fails it will silently retry until it makes it. Any guess on why it might happen?
On 11/09/2011 07:44 AM, Avi Kivity wrote: > The fact that a host cpu supports a feature doesn't mean that QEMU and KVM > will also support it, yet -cpuid host brings host features wholesale. > > We need to whitelist each feature separately to make sure we support it. > This patch adds KVM whitelisting (by simply using KVM_GET_SUPPORTED_CPUID > instead of the CPUID instruction). > > Signed-off-by: Avi Kivity<avi@redhat.com> Applied. Thanks. Regards, Anthony Liguori > --- > target-i386/cpuid.c | 27 ++++----------------------- > 1 files changed, 4 insertions(+), 23 deletions(-) > > diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c > index 1e8bcff..edac377 100644 > --- a/target-i386/cpuid.c > +++ b/target-i386/cpuid.c > @@ -107,33 +107,14 @@ void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > { > #if defined(CONFIG_KVM) > - uint32_t vec[4]; > - > -#ifdef __x86_64__ > - asm volatile("cpuid" > - : "=a"(vec[0]), "=b"(vec[1]), > - "=c"(vec[2]), "=d"(vec[3]) > - : "0"(function), "c"(count) : "cc"); > -#else > - asm volatile("pusha \n\t" > - "cpuid \n\t" > - "mov %%eax, 0(%2) \n\t" > - "mov %%ebx, 4(%2) \n\t" > - "mov %%ecx, 8(%2) \n\t" > - "mov %%edx, 12(%2) \n\t" > - "popa" > - : : "a"(function), "c"(count), "S"(vec) > - : "memory", "cc"); > -#endif > - > if (eax) > - *eax = vec[0]; > + *eax = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EAX); > if (ebx) > - *ebx = vec[1]; > + *ebx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EBX); > if (ecx) > - *ecx = vec[2]; > + *ecx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_ECX); > if (edx) > - *edx = vec[3]; > + *edx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EDX); > #endif > } >
On 11/09/2011 08:21 PM, Sasha Levin wrote: > On Wed, 2011-11-09 at 20:00 +0200, Avi Kivity wrote: > > On 11/09/2011 07:56 PM, Anthony Liguori wrote: > > > On 11/09/2011 07:44 AM, Avi Kivity wrote: > > >> The fact that a host cpu supports a feature doesn't mean that QEMU > > >> and KVM > > >> will also support it, yet -cpuid host brings host features wholesale. > > >> > > >> We need to whitelist each feature separately to make sure we support it. > > >> This patch adds KVM whitelisting (by simply using > > >> KVM_GET_SUPPORTED_CPUID > > >> instead of the CPUID instruction). > > >> > > >> Signed-off-by: Avi Kivity<avi@redhat.com> > > > > > > This seems like a 1.0 candidate, yes? > > > > There is a distinct possibility this will uncover bugs in kvm's > > KVM_GET_SUPPORTED_CPUID. Those won't be qemu bugs, so I think it's good > > for 1.0. > > > > Avi, we have a problem in the KVM tool of KVM_GET_SUPPORTED_CPUID > sometimes returning -E2BIG. I've sent a mail about it some time ago, but > we couldn't really find the reason. > > It's somewhat non-deterministic, and theres no sure way to reproduce it, > but it doesn't happen that rarely. > > The block of code that uses it from usermode it pretty simple: > > struct kvm_cpuid2 *kvm_cpuid; > > kvm_cpuid = calloc(1, sizeof(*kvm_cpuid) + > MAX_KVM_CPUID_ENTRIES * sizeof(*kvm_cpuid->entries)); > > kvm_cpuid->nent = MAX_KVM_CPUID_ENTRIES; > if (ioctl(vcpu->kvm->sys_fd, KVM_GET_SUPPORTED_CPUID, kvm_cpuid) < 0) > die_perror("KVM_GET_SUPPORTED_CPUID failed"); > > MAX_KVM_CPUID_ENTRIES is set to 100, which is more than the 80 defined > in the kernel, so it shouldn't be an issue. It wouldn't explain the non > deterministic behavior either. > > QEMU's code around it allows it to hide the bug if it does happen: > > uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > uint32_t index, int reg) > { > struct kvm_cpuid2 *cpuid; > int i, max; > uint32_t ret = 0; > uint32_t cpuid_1_edx; > int has_kvm_features = 0; > > max = 1; > while ((cpuid = try_get_cpuid(s, max)) == NULL) { > max *= 2; > } > [snip] > > Which means that if it fails it will silently retry until it makes it. > > Any guess on why it might happen? > No idea. If you run your code block in a loop, how soon will it reproduce?
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c index 1e8bcff..edac377 100644 --- a/target-i386/cpuid.c +++ b/target-i386/cpuid.c @@ -107,33 +107,14 @@ void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) { #if defined(CONFIG_KVM) - uint32_t vec[4]; - -#ifdef __x86_64__ - asm volatile("cpuid" - : "=a"(vec[0]), "=b"(vec[1]), - "=c"(vec[2]), "=d"(vec[3]) - : "0"(function), "c"(count) : "cc"); -#else - asm volatile("pusha \n\t" - "cpuid \n\t" - "mov %%eax, 0(%2) \n\t" - "mov %%ebx, 4(%2) \n\t" - "mov %%ecx, 8(%2) \n\t" - "mov %%edx, 12(%2) \n\t" - "popa" - : : "a"(function), "c"(count), "S"(vec) - : "memory", "cc"); -#endif - if (eax) - *eax = vec[0]; + *eax = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EAX); if (ebx) - *ebx = vec[1]; + *ebx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EBX); if (ecx) - *ecx = vec[2]; + *ecx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_ECX); if (edx) - *edx = vec[3]; + *edx = kvm_arch_get_supported_cpuid(kvm_state, function, count, R_EDX); #endif }
The fact that a host cpu supports a feature doesn't mean that QEMU and KVM will also support it, yet -cpuid host brings host features wholesale. We need to whitelist each feature separately to make sure we support it. This patch adds KVM whitelisting (by simply using KVM_GET_SUPPORTED_CPUID instead of the CPUID instruction). Signed-off-by: Avi Kivity <avi@redhat.com> --- target-i386/cpuid.c | 27 ++++----------------------- 1 files changed, 4 insertions(+), 23 deletions(-)