[PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid

[PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid

[Sasha Levin]

This patch cleans and simplifies kvm_dev_ioctl_get_supported_cpuid by using a table
instead of duplicating code as Avi suggested.

This patch also fixes a bug where kvm_dev_ioctl_get_supported_cpuid would return
-E2BIG when amount of entries passed was just right.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 arch/x86/kvm/cpuid.c |   78 ++++++++++++++++++++++---------------------------
 1 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index bbaa6d8..c9ede4d 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -425,12 +425,31 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 
 #undef F
 
+struct kvm_cpuid_param {
+	u32 func;
+	u32 idx;
+	u32 leaves;
+	int (*qualifier)(struct kvm_cpuid_param *param);
+};
+
+int is_centaur_cpu(struct kvm_cpuid_param *param)
+{
+	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
+}
+
 int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 				      struct kvm_cpuid_entry2 __user *entries)
 {
 	struct kvm_cpuid_entry2 *cpuid_entries;
-	int limit, nent = 0, r = -E2BIG;
+	int limit, nent = 0, r = -E2BIG, i;
 	u32 func;
+	struct kvm_cpuid_param param[] = {
+		{ .func = 0, .leaves = 1 },
+		{ .func = 0x80000000, .leaves = 1 },
+		{ .func = 0xC0000000, .qualifier = is_centaur_cpu, .leaves = 1 },
+		{ .func = KVM_CPUID_SIGNATURE },
+		{ .func = KVM_CPUID_FEATURES },
+	};
 
 	if (cpuid->nent < 1)
 		goto out;
@@ -441,59 +460,32 @@ int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 	if (!cpuid_entries)
 		goto out;
 
-	do_cpuid_ent(&cpuid_entries[0], 0, 0, &nent, cpuid->nent);
-	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);
-	r = -E2BIG;
-	if (nent >= cpuid->nent)
-		goto out_free;
-
-	do_cpuid_ent(&cpuid_entries[nent], 0x80000000, 0, &nent, cpuid->nent);
-	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);
-
+	for (i = 0; i < ARRAY_SIZE(param); i++) {
+		struct kvm_cpuid_param *ent = &param[i];
 
+		if (ent->qualifier && !ent->qualifier(ent))
+			continue;
 
-	r = -E2BIG;
-	if (nent >= cpuid->nent)
-		goto out_free;
+		if (nent >= cpuid->nent)
+			break;
 
-	/* Add support for Centaur's CPUID instruction. */
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR) {
-		do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,
+		do_cpuid_ent(&cpuid_entries[nent], ent->func, ent->idx,
 				&nent, cpuid->nent);
 
-		r = -E2BIG;
-		if (nent >= cpuid->nent)
-			goto out_free;
+		if (!ent->leaves)
+			continue;
 
 		limit = cpuid_entries[nent - 1].eax;
-		for (func = 0xC0000001;
-			func <= limit && nent < cpuid->nent; ++func)
-			do_cpuid_ent(&cpuid_entries[nent], func, 0,
-					&nent, cpuid->nent);
+		for (func = ent->func + 1; func <= limit && nent < cpuid->nent; ++func)
+			do_cpuid_ent(&cpuid_entries[nent], func, ent->idx,
+				     &nent, cpuid->nent);
 
-		r = -E2BIG;
-		if (nent >= cpuid->nent)
-			goto out_free;
+		if (func <= limit)
+			break;
 	}
 
-	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0, &nent,
-		     cpuid->nent);
-
-	r = -E2BIG;
-	if (nent >= cpuid->nent)
-		goto out_free;
-
-	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_FEATURES, 0, &nent,
-		     cpuid->nent);
-
 	r = -E2BIG;
-	if (nent >= cpuid->nent)
+	if (i < ARRAY_SIZE(param) || nent > cpuid->nent)
 		goto out_free;
 
 	r = -EFAULT;
-- 
1.7.8.rc3

Re: [PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid

[Avi Kivity]

On 11/25/2011 10:12 AM, Sasha Levin wrote:
> This patch cleans and simplifies kvm_dev_ioctl_get_supported_cpuid by using a table
> instead of duplicating code as Avi suggested.
>
> This patch also fixes a bug where kvm_dev_ioctl_get_supported_cpuid would return
> -E2BIG when amount of entries passed was just right.
>
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  arch/x86/kvm/cpuid.c |   78 ++++++++++++++++++++++---------------------------
>  1 files changed, 35 insertions(+), 43 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index bbaa6d8..c9ede4d 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -425,12 +425,31 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  
>  #undef F
>  
> +struct kvm_cpuid_param {
> +	u32 func;
> +	u32 idx;
> +	u32 leaves;

What does 'leaves' mean?  That it has a leaf count in eax?  if so it
want a better name and to be a bool.

> +	int (*qualifier)(struct kvm_cpuid_param *param);

bool

> +};
> +
> +int is_centaur_cpu(struct kvm_cpuid_param *param)
> +{
> +	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
> +}
> +
static

>  int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
>  				      struct kvm_cpuid_entry2 __user *entries)
>  {
>  	struct kvm_cpuid_entry2 *cpuid_entries;
> -	int limit, nent = 0, r = -E2BIG;
> +	int limit, nent = 0, r = -E2BIG, i;
>  	u32 func;
> +	struct kvm_cpuid_param param[] = {
> +		{ .func = 0, .leaves = 1 },
> +		{ .func = 0x80000000, .leaves = 1 },
> +		{ .func = 0xC0000000, .qualifier = is_centaur_cpu, .leaves = 1 },
> +		{ .func = KVM_CPUID_SIGNATURE },
> +		{ .func = KVM_CPUID_FEATURES },
> +	};

static

>  
>  	if (cpuid->nent < 1)
>  		goto out;
> @@ -441,59 +460,32 @@ int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
>  	if (!cpuid_entries)
>  		goto out;
>  
> -	do_cpuid_ent(&cpuid_entries[0], 0, 0, &nent, cpuid->nent);
> -	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);
> -	r = -E2BIG;
> -	if (nent >= cpuid->nent)
> -		goto out_free;
> -
> -	do_cpuid_ent(&cpuid_entries[nent], 0x80000000, 0, &nent, cpuid->nent);
> -	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);
> -
> +	for (i = 0; i < ARRAY_SIZE(param); i++) {
> +		struct kvm_cpuid_param *ent = &param[i];
>  
> +		if (ent->qualifier && !ent->qualifier(ent))
> +			continue;
>  
> -	r = -E2BIG;
> -	if (nent >= cpuid->nent)
> -		goto out_free;
> +		if (nent >= cpuid->nent)
> +			break;
>  
> -	/* Add support for Centaur's CPUID instruction. */
> -	if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR) {
> -		do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,
> +		do_cpuid_ent(&cpuid_entries[nent], ent->func, ent->idx,
>  				&nent, cpuid->nent);
>  
> -		r = -E2BIG;
> -		if (nent >= cpuid->nent)
> -			goto out_free;
> +		if (!ent->leaves)
> +			continue;
>  
>  		limit = cpuid_entries[nent - 1].eax;
> -		for (func = 0xC0000001;
> -			func <= limit && nent < cpuid->nent; ++func)
> -			do_cpuid_ent(&cpuid_entries[nent], func, 0,
> -					&nent, cpuid->nent);
> +		for (func = ent->func + 1; func <= limit && nent < cpuid->nent; ++func)
> +			do_cpuid_ent(&cpuid_entries[nent], func, ent->idx,
> +				     &nent, cpuid->nent);
>  
> -		r = -E2BIG;
> -		if (nent >= cpuid->nent)
> -			goto out_free;
> +		if (func <= limit)
> +			break;
>  	}
>  
> -	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0, &nent,
> -		     cpuid->nent);
> -
> -	r = -E2BIG;
> -	if (nent >= cpuid->nent)
> -		goto out_free;
> -
> -	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_FEATURES, 0, &nent,
> -		     cpuid->nent);
> -
>  	r = -E2BIG;
> -	if (nent >= cpuid->nent)
> +	if (i < ARRAY_SIZE(param) || nent > cpuid->nent)
>  		goto out_free;
>  

Isn't the first condition sufficient?

The code may be more readable if do_cpuid_ent() (and do_cpuid_1_ent)
returned -E2BIG on overflow instead of all this cleverness.

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

Re: [PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid

[Sasha Levin]

On Sun, 2011-11-27 at 16:44 +0200, Avi Kivity wrote:
> On 11/25/2011 10:12 AM, Sasha Levin wrote:
> > This patch cleans and simplifies kvm_dev_ioctl_get_supported_cpuid by using a table
> > instead of duplicating code as Avi suggested.
> >
> > This patch also fixes a bug where kvm_dev_ioctl_get_supported_cpuid would return
> > -E2BIG when amount of entries passed was just right.
> >
> > Cc: Avi Kivity <avi@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > ---
> >  arch/x86/kvm/cpuid.c |   78 ++++++++++++++++++++++---------------------------
> >  1 files changed, 35 insertions(+), 43 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index bbaa6d8..c9ede4d 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -425,12 +425,31 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> >  
> >  #undef F
> >  
> > +struct kvm_cpuid_param {
> > +	u32 func;
> > +	u32 idx;
> > +	u32 leaves;
> 
> What does 'leaves' mean?  That it has a leaf count in eax?  if so it
> want a better name and to be a bool.
> 
> > +	int (*qualifier)(struct kvm_cpuid_param *param);
> 
> bool
> 
> > +};
> > +
> > +int is_centaur_cpu(struct kvm_cpuid_param *param)
> > +{
> > +	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
> > +}
> > +
> static
> 
> >  int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
> >  				      struct kvm_cpuid_entry2 __user *entries)
> >  {
> >  	struct kvm_cpuid_entry2 *cpuid_entries;
> > -	int limit, nent = 0, r = -E2BIG;
> > +	int limit, nent = 0, r = -E2BIG, i;
> >  	u32 func;
> > +	struct kvm_cpuid_param param[] = {
> > +		{ .func = 0, .leaves = 1 },
> > +		{ .func = 0x80000000, .leaves = 1 },
> > +		{ .func = 0xC0000000, .qualifier = is_centaur_cpu, .leaves = 1 },
> > +		{ .func = KVM_CPUID_SIGNATURE },
> > +		{ .func = KVM_CPUID_FEATURES },
> > +	};
> 
> static
> 
> >  
> >  	if (cpuid->nent < 1)
> >  		goto out;
> > @@ -441,59 +460,32 @@ int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
> >  	if (!cpuid_entries)
> >  		goto out;
> >  
> > -	do_cpuid_ent(&cpuid_entries[0], 0, 0, &nent, cpuid->nent);
> > -	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);
> > -	r = -E2BIG;
> > -	if (nent >= cpuid->nent)
> > -		goto out_free;
> > -
> > -	do_cpuid_ent(&cpuid_entries[nent], 0x80000000, 0, &nent, cpuid->nent);
> > -	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);
> > -
> > +	for (i = 0; i < ARRAY_SIZE(param); i++) {
> > +		struct kvm_cpuid_param *ent = &param[i];
> >  
> > +		if (ent->qualifier && !ent->qualifier(ent))
> > +			continue;
> >  
> > -	r = -E2BIG;
> > -	if (nent >= cpuid->nent)
> > -		goto out_free;
> > +		if (nent >= cpuid->nent)
> > +			break;
> >  
> > -	/* Add support for Centaur's CPUID instruction. */
> > -	if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR) {
> > -		do_cpuid_ent(&cpuid_entries[nent], 0xC0000000, 0,
> > +		do_cpuid_ent(&cpuid_entries[nent], ent->func, ent->idx,
> >  				&nent, cpuid->nent);
> >  
> > -		r = -E2BIG;
> > -		if (nent >= cpuid->nent)
> > -			goto out_free;
> > +		if (!ent->leaves)
> > +			continue;
> >  
> >  		limit = cpuid_entries[nent - 1].eax;
> > -		for (func = 0xC0000001;
> > -			func <= limit && nent < cpuid->nent; ++func)
> > -			do_cpuid_ent(&cpuid_entries[nent], func, 0,
> > -					&nent, cpuid->nent);
> > +		for (func = ent->func + 1; func <= limit && nent < cpuid->nent; ++func)
> > +			do_cpuid_ent(&cpuid_entries[nent], func, ent->idx,
> > +				     &nent, cpuid->nent);
> >  
> > -		r = -E2BIG;
> > -		if (nent >= cpuid->nent)
> > -			goto out_free;
> > +		if (func <= limit)
> > +			break;
> >  	}
> >  
> > -	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_SIGNATURE, 0, &nent,
> > -		     cpuid->nent);
> > -
> > -	r = -E2BIG;
> > -	if (nent >= cpuid->nent)
> > -		goto out_free;
> > -
> > -	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_FEATURES, 0, &nent,
> > -		     cpuid->nent);
> > -
> >  	r = -E2BIG;
> > -	if (nent >= cpuid->nent)
> > +	if (i < ARRAY_SIZE(param) || nent > cpuid->nent)
> >  		goto out_free;
> >  
> 
> Isn't the first condition sufficient?

Probably, but with all the possible cases of termination from that loop
I felt it was better going for safe than sorry.

> 
> The code may be more readable if do_cpuid_ent() (and do_cpuid_1_ent)
> returned -E2BIG on overflow instead of all this cleverness.
> 

I'll send a new version with fixes.

-- 

Sasha.