CPUID handling: cleanups and support for storing subleaf values

CPUID handling: cleanups and support for storing subleaf values

[Amit Shah]

Hi Avi,

The first patch replaces the one I sent yesterday.

I'll send out the userspace patches to the qemu-devel list with kvm@ in the CC.

Please apply
Amit.

[PATCH] KVM: x86: Fix typos and whitespace errors

[Amit Shah]

Some typos, comments, whitespace errors corrected in the cpuid code

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 arch/x86/include/asm/kvm.h |    6 ++--
 arch/x86/kvm/x86.c         |   45 +++++++++++++++++++++----------------------
 2 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index a031102..58e28ab 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -177,9 +177,9 @@ struct kvm_cpuid_entry2 {
 	__u32 padding[3];
 };
 
-#define KVM_CPUID_FLAG_SIGNIFCANT_INDEX 1
-#define KVM_CPUID_FLAG_STATEFUL_FUNC    2
-#define KVM_CPUID_FLAG_STATE_READ_NEXT  4
+#define KVM_CPUID_FLAG_SIGNIFICANT_INDEX 1
+#define KVM_CPUID_FLAG_STATEFUL_FUNC     2
+#define KVM_CPUID_FLAG_STATE_READ_NEXT   4
 
 /* for KVM_SET_CPUID2 */
 struct kvm_cpuid2 {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4b17253..06b44fb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1065,7 +1065,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
 			goto out;
 		r = kvm_dev_ioctl_get_supported_cpuid(&cpuid,
-			cpuid_arg->entries);
+						      cpuid_arg->entries);
 		if (r)
 			goto out;
 
@@ -1163,8 +1163,8 @@ out:
 }
 
 static int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
-				    struct kvm_cpuid2 *cpuid,
-				    struct kvm_cpuid_entry2 __user *entries)
+				     struct kvm_cpuid2 *cpuid,
+				     struct kvm_cpuid_entry2 __user *entries)
 {
 	int r;
 
@@ -1183,8 +1183,8 @@ out:
 }
 
 static int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
-				    struct kvm_cpuid2 *cpuid,
-				    struct kvm_cpuid_entry2 __user *entries)
+				     struct kvm_cpuid2 *cpuid,
+				     struct kvm_cpuid_entry2 __user *entries)
 {
 	int r;
 
@@ -1193,7 +1193,7 @@ static int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
 		goto out;
 	r = -EFAULT;
 	if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
-			   vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
+			 vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
 		goto out;
 	return 0;
 
@@ -1203,12 +1203,12 @@ out:
 }
 
 static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
-			  u32 index)
+			   u32 index)
 {
 	entry->function = function;
 	entry->index = index;
 	cpuid_count(entry->function, entry->index,
-		&entry->eax, &entry->ebx, &entry->ecx, &entry->edx);
+		    &entry->eax, &entry->ebx, &entry->ecx, &entry->edx);
 	entry->flags = 0;
 }
 
@@ -1247,7 +1247,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 		bit(X86_FEATURE_LAHF_LM) | bit(X86_FEATURE_CMP_LEGACY) |
 		bit(X86_FEATURE_SVM);
 
-	/* all func 2 cpuid_count() should be called on the same cpu */
+	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
 	do_cpuid_1_ent(entry, function, index);
 	++*nent;
@@ -1280,7 +1280,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 4: {
 		int i, cache_type;
 
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+		entry->flags |= KVM_CPUID_FLAG_SIGNIFICANT_INDEX;
 		/* read more entries until cache_type is zero */
 		for (i = 1; *nent < maxnent; ++i) {
 			cache_type = entry[i - 1].eax & 0x1f;
@@ -1288,7 +1288,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 				break;
 			do_cpuid_1_ent(&entry[i], function, i);
 			entry[i].flags |=
-			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+			       KVM_CPUID_FLAG_SIGNIFICANT_INDEX;
 			++*nent;
 		}
 		break;
@@ -1296,7 +1296,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 0xb: {
 		int i, level_type;
 
-		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+		entry->flags |= KVM_CPUID_FLAG_SIGNIFICANT_INDEX;
 		/* read more entries until level_type is zero */
 		for (i = 1; *nent < maxnent; ++i) {
 			level_type = entry[i - 1].ecx & 0xff00;
@@ -1304,7 +1304,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 				break;
 			do_cpuid_1_ent(&entry[i], function, i);
 			entry[i].flags |=
-			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+			       KVM_CPUID_FLAG_SIGNIFICANT_INDEX;
 			++*nent;
 		}
 		break;
@@ -1321,7 +1321,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 }
 
 static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
-				    struct kvm_cpuid_entry2 __user *entries)
+				     struct kvm_cpuid_entry2 __user *entries)
 {
 	struct kvm_cpuid_entry2 *cpuid_entries;
 	int limit, nent = 0, r = -E2BIG;
@@ -1338,7 +1338,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 	limit = cpuid_entries[0].eax;
 	for (func = 1; func <= limit && nent < cpuid->nent; ++func)
 		do_cpuid_ent(&cpuid_entries[nent], func, 0,
-				&nent, cpuid->nent);
+			     &nent, cpuid->nent);
 	r = -E2BIG;
 	if (nent >= cpuid->nent)
 		goto out_free;
@@ -1347,10 +1347,10 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 	limit = cpuid_entries[nent - 1].eax;
 	for (func = 0x80000001; func <= limit && nent < cpuid->nent; ++func)
 		do_cpuid_ent(&cpuid_entries[nent], func, 0,
-			       &nent, cpuid->nent);
+			     &nent, cpuid->nent);
 	r = -EFAULT;
 	if (copy_to_user(entries, cpuid_entries,
-			nent * sizeof(struct kvm_cpuid_entry2)))
+			 nent * sizeof(struct kvm_cpuid_entry2)))
 		goto out_free;
 	cpuid->nent = nent;
 	r = 0;
@@ -1494,7 +1494,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
 			goto out;
 		r = kvm_vcpu_ioctl_set_cpuid2(vcpu, &cpuid,
-				cpuid_arg->entries);
+					      cpuid_arg->entries);
 		if (r)
 			goto out;
 		break;
@@ -1507,7 +1507,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
 			goto out;
 		r = kvm_vcpu_ioctl_get_cpuid2(vcpu, &cpuid,
-				cpuid_arg->entries);
+					      cpuid_arg->entries);
 		if (r)
 			goto out;
 		r = -EFAULT;
@@ -2855,14 +2855,14 @@ static int move_to_next_stateful_cpuid_entry(struct kvm_vcpu *vcpu, int i)
 /* find an entry with matching function, matching index (if needed), and that
  * should be read next (if it's stateful) */
 static int is_matching_cpuid_entry(struct kvm_cpuid_entry2 *e,
-	u32 function, u32 index)
+				   u32 function, u32 index)
 {
 	if (e->function != function)
 		return 0;
-	if ((e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX) && e->index != index)
+	if ((e->flags & KVM_CPUID_FLAG_SIGNIFICANT_INDEX) && e->index != index)
 		return 0;
 	if ((e->flags & KVM_CPUID_FLAG_STATEFUL_FUNC) &&
-		!(e->flags & KVM_CPUID_FLAG_STATE_READ_NEXT))
+	    !(e->flags & KVM_CPUID_FLAG_STATE_READ_NEXT))
 		return 0;
 	return 1;
 }
@@ -2890,7 +2890,6 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 			if (!best || e->function > best->function)
 				best = e;
 	}
-
 	return best;
 }
 
-- 
1.6.0.6

[PATCH] KVM: x86: Store multiple cpuid entries for a single function

[Amit Shah]

CPUID functions 4, 0xb and 0xd behave differently for different values of ECX.
Store these values if userspace passes them.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 arch/x86/kvm/x86.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 06b44fb..8dc4b29 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1126,7 +1126,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 				    struct kvm_cpuid *cpuid,
 				    struct kvm_cpuid_entry __user *entries)
 {
-	int r, i;
+	int r, i, count;
 	struct kvm_cpuid_entry *cpuid_entries;
 
 	r = -E2BIG;
@@ -1146,8 +1146,20 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
 		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
 		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
-		vcpu->arch.cpuid_entries[i].index = 0;
-		vcpu->arch.cpuid_entries[i].flags = 0;
+		switch (cpuid_entries[i].function) {
+		case 4:
+		case 0xb:
+		case 0xd:
+			vcpu->arch.cpuid_entries[i].index = count++;
+			vcpu->arch.cpuid_entries[i].flags =
+				KVM_CPUID_FLAG_SIGNIFICANT_INDEX;
+			break;
+		default:
+			vcpu->arch.cpuid_entries[i].index = 0;
+			vcpu->arch.cpuid_entries[i].flags = 0;
+		}
+		if (cpuid_entries[i].function != cpuid_entries[i+1].function)
+			count = 0;
 		vcpu->arch.cpuid_entries[i].padding[0] = 0;
 		vcpu->arch.cpuid_entries[i].padding[1] = 0;
 		vcpu->arch.cpuid_entries[i].padding[2] = 0;
-- 
1.6.0.6

Re: [PATCH] KVM: x86: Store multiple cpuid entries for a single function

[Muli Ben-Yehuda]

On Mon, Jan 12, 2009 at 10:49:25AM +0000, Amit Shah wrote:
> CPUID functions 4, 0xb and 0xd behave differently for different values of ECX.
> Store these values if userspace passes them.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  arch/x86/kvm/x86.c |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 06b44fb..8dc4b29 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1126,7 +1126,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  				    struct kvm_cpuid *cpuid,
>  				    struct kvm_cpuid_entry __user *entries)
>  {
> -	int r, i;
> +	int r, i, count;
>  	struct kvm_cpuid_entry *cpuid_entries;
>  
>  	r = -E2BIG;
> @@ -1146,8 +1146,20 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
>  		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
>  		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
> -		vcpu->arch.cpuid_entries[i].index = 0;
> -		vcpu->arch.cpuid_entries[i].flags = 0;
> +		switch (cpuid_entries[i].function) {
> +		case 4:
> +		case 0xb:
> +		case 0xd:
> +			vcpu->arch.cpuid_entries[i].index = count++;

Isn't this using count uninitialized?

Cheers,
Muli
-- 
SYSTOR 2009---The Israeli Experimental Systems Conference
May 4-6, 2009, Haifa, Israel
http://www.haifa.il.ibm.com/conferences/systor2009/

Re: [PATCH] KVM: x86: Store multiple cpuid entries for a single function

[Amit Shah]

On Mon, Jan 12, 2009 at 10:39:31PM +0200, Muli Ben-Yehuda wrote:
> On Mon, Jan 12, 2009 at 10:49:25AM +0000, Amit Shah wrote:
> > +		case 0xd:
> > +			vcpu->arch.cpuid_entries[i].index = count++;
> 
> Isn't this using count uninitialized?

Oops, that's right.

Here's the refreshed patch.

Thanks,
Amit.

>From 0708401594924e2d8ca7bcc1512624a8a0a6e79d Mon Sep 17 00:00:00 2001
From: Amit Shah <amit.shah@redhat.com>
Date: Mon, 12 Jan 2009 15:59:37 +0530
Subject: [PATCH] KVM: x86: Store multiple cpuid entries for a single function

CPUID functions 4, 0xb and 0xd behave differently for different values of ECX.
Store these values if userspace passes them.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 arch/x86/kvm/x86.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 06b44fb..4d731fd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1126,7 +1126,7 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 				    struct kvm_cpuid *cpuid,
 				    struct kvm_cpuid_entry __user *entries)
 {
-	int r, i;
+	int r, i, count;
 	struct kvm_cpuid_entry *cpuid_entries;
 
 	r = -E2BIG;
@@ -1140,14 +1140,27 @@ static int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	if (copy_from_user(cpuid_entries, entries,
 			   cpuid->nent * sizeof(struct kvm_cpuid_entry)))
 		goto out_free;
+	count = 0;
 	for (i = 0; i < cpuid->nent; i++) {
 		vcpu->arch.cpuid_entries[i].function = cpuid_entries[i].function;
 		vcpu->arch.cpuid_entries[i].eax = cpuid_entries[i].eax;
 		vcpu->arch.cpuid_entries[i].ebx = cpuid_entries[i].ebx;
 		vcpu->arch.cpuid_entries[i].ecx = cpuid_entries[i].ecx;
 		vcpu->arch.cpuid_entries[i].edx = cpuid_entries[i].edx;
-		vcpu->arch.cpuid_entries[i].index = 0;
-		vcpu->arch.cpuid_entries[i].flags = 0;
+		switch (cpuid_entries[i].function) {
+		case 4:
+		case 0xb:
+		case 0xd:
+			vcpu->arch.cpuid_entries[i].index = count++;
+			vcpu->arch.cpuid_entries[i].flags =
+				KVM_CPUID_FLAG_SIGNIFICANT_INDEX;
+			break;
+		default:
+			vcpu->arch.cpuid_entries[i].index = 0;
+			vcpu->arch.cpuid_entries[i].flags = 0;
+		}
+		if (cpuid_entries[i].function != cpuid_entries[i+1].function)
+			count = 0;
 		vcpu->arch.cpuid_entries[i].padding[0] = 0;
 		vcpu->arch.cpuid_entries[i].padding[1] = 0;
 		vcpu->arch.cpuid_entries[i].padding[2] = 0;
-- 
1.6.0.6

Re: [PATCH] KVM: x86: Store multiple cpuid entries for a single function

[Avi Kivity]

Amit Shah wrote:
> CPUID functions 4, 0xb and 0xd behave differently for different values of ECX.
> Store these values if userspace passes them.
>   

Userspace has no way of knowing whether the kernel contains this fix or not.

Use the SET_CPUID2 interface instead.



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

Re: [PATCH] KVM: x86: Store multiple cpuid entries for a single function

[Amit Shah]

On Tue, Jan 13, 2009 at 11:27:07AM +0200, Avi Kivity wrote:
> Amit Shah wrote:
>> CPUID functions 4, 0xb and 0xd behave differently for different values of ECX.
>> Store these values if userspace passes them.
>>   
>
> Userspace has no way of knowing whether the kernel contains this fix or not.
>
> Use the SET_CPUID2 interface instead.

OK; So first attempt to use the SET_CPUID2 interface. If that fails (new
userspace on older kernel), what should we do?

Amit

Re: [PATCH] KVM: x86: Store multiple cpuid entries for a single function

[Avi Kivity]

Amit Shah wrote:
>> Use the SET_CPUID2 interface instead.
>>     
>
> OK; So first attempt to use the SET_CPUID2 interface. If that fails (new
> userspace on older kernel), what should we do?
>   

SET_CPUID2 was introduced on 2.6.25; that is now the minimum requirement 
for qemu and kvm-userspace anyway.

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

Re: [PATCH] KVM: x86: Store multiple cpuid entries for a single function

[Amit Shah]

On Tue, Jan 13, 2009 at 11:45:55AM +0200, Avi Kivity wrote:
> Amit Shah wrote:
>>> Use the SET_CPUID2 interface instead.
>>>     
>>
>> OK; So first attempt to use the SET_CPUID2 interface. If that fails (new
>> userspace on older kernel), what should we do?
>>   
>
> SET_CPUID2 was introduced on 2.6.25; that is now the minimum requirement  
> for qemu and kvm-userspace anyway.

OK; that's great. I'll respin the patches.

Amit.