* [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID
@ 2011-11-09 13:44 Avi Kivity
2011-11-09 17:56 ` Anthony Liguori
2011-11-09 19:45 ` Anthony Liguori
0 siblings, 2 replies; 6+ messages in thread
From: Avi Kivity @ 2011-11-09 13:44 UTC (permalink / raw)
To: Marcelo Tosatti, Anthony Liguori, kvm, qemu-devel
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(-)
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
}
--
1.7.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID
2011-11-09 13:44 [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID Avi Kivity
@ 2011-11-09 17:56 ` Anthony Liguori
2011-11-09 18:00 ` Avi Kivity
2011-11-09 19:45 ` Anthony Liguori
1 sibling, 1 reply; 6+ messages in thread
From: Anthony Liguori @ 2011-11-09 17:56 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, qemu-devel, kvm
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
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID
2011-11-09 17:56 ` Anthony Liguori
@ 2011-11-09 18:00 ` Avi Kivity
2011-11-09 18:21 ` Sasha Levin
0 siblings, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2011-11-09 18:00 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Marcelo Tosatti, qemu-devel, kvm
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.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID
2011-11-09 18:00 ` Avi Kivity
@ 2011-11-09 18:21 ` Sasha Levin
2011-11-10 13:18 ` Avi Kivity
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2011-11-09 18:21 UTC (permalink / raw)
To: Avi Kivity
Cc: Cyrill Gorcunov, kvm, Marcelo Tosatti, qemu-devel, Pekka Enberg
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?
--
Sasha.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID
2011-11-09 18:21 ` Sasha Levin
@ 2011-11-10 13:18 ` Avi Kivity
0 siblings, 0 replies; 6+ messages in thread
From: Avi Kivity @ 2011-11-10 13:18 UTC (permalink / raw)
To: Sasha Levin
Cc: Cyrill Gorcunov, kvm, Marcelo Tosatti, qemu-devel, Pekka Enberg
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?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID
2011-11-09 13:44 [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID Avi Kivity
2011-11-09 17:56 ` Anthony Liguori
@ 2011-11-09 19:45 ` Anthony Liguori
1 sibling, 0 replies; 6+ messages in thread
From: Anthony Liguori @ 2011-11-09 19:45 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, qemu-devel, kvm
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
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-10 13:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-09 13:44 [Qemu-devel] [PATCH] i386: derive '-cpu host' from KVM_GET_SUPPORTED_CPUID Avi Kivity
2011-11-09 17:56 ` Anthony Liguori
2011-11-09 18:00 ` Avi Kivity
2011-11-09 18:21 ` Sasha Levin
2011-11-10 13:18 ` Avi Kivity
2011-11-09 19:45 ` Anthony Liguori
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).