diff mbox series

[02/61] KVM: x86: Refactor loop around do_cpuid_func() to separate helper

Message ID 20200201185218.24473-3-sean.j.christopherson@intel.com
State New, archived
Headers show
Series KVM: x86: Introduce KVM cpu caps | expand

Commit Message

Sean Christopherson Feb. 1, 2020, 6:51 p.m. UTC
Move the guts of kvm_dev_ioctl_get_cpuid()'s CPUID func loop to a
separate helper to improve code readability and pave the way for future
cleanup.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c | 45 ++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

Comments

Vitaly Kuznetsov Feb. 6, 2020, 2:59 p.m. UTC | #1
Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Move the guts of kvm_dev_ioctl_get_cpuid()'s CPUID func loop to a
> separate helper to improve code readability and pave the way for future
> cleanup.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 45 ++++++++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 47ce04762c20..f49fdd06f511 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -839,6 +839,29 @@ static bool is_centaur_cpu(const struct kvm_cpuid_param *param)
>  	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
>  }
>  
> +static int get_cpuid_func(struct kvm_cpuid_entry2 *entries, u32 func,
> +			  int *nent, int maxnent, unsigned int type)
> +{
> +	u32 limit;
> +	int r;
> +
> +	r = do_cpuid_func(&entries[*nent], func, nent, maxnent, type);
> +	if (r)
> +		return r;
> +
> +	limit = entries[*nent - 1].eax;
> +	for (func = func + 1; func <= limit; ++func) {
> +		if (*nent >= maxnent)
> +			return -E2BIG;
> +
> +		r = do_cpuid_func(&entries[*nent], func, nent, maxnent, type);
> +		if (r)
> +			break;
> +	}
> +
> +	return r;
> +}
> +
>  static bool sanity_check_entries(struct kvm_cpuid_entry2 __user *entries,
>  				 __u32 num_entries, unsigned int ioctl_type)
>  {
> @@ -871,8 +894,8 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>  			    unsigned int type)
>  {
>  	struct kvm_cpuid_entry2 *cpuid_entries;
> -	int limit, nent = 0, r = -E2BIG, i;
> -	u32 func;
> +	int nent = 0, r = -E2BIG, i;

Not this patches fault, but I just noticed that '-E2BIG' initializer
here is only being used for 

 'if (cpuid->nent < 1)'

case so I have two suggestion:
1) Return directly without the 'goto' , drop the initializer.
2) Return -EINVAL instead.

> +
>  	static const struct kvm_cpuid_param param[] = {
>  		{ .func = 0 },
>  		{ .func = 0x80000000 },
> @@ -901,22 +924,8 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>  		if (ent->qualifier && !ent->qualifier(ent))
>  			continue;
>  
> -		r = do_cpuid_func(&cpuid_entries[nent], ent->func,
> -				  &nent, cpuid->nent, type);
> -
> -		if (r)
> -			goto out_free;
> -
> -		limit = cpuid_entries[nent - 1].eax;
> -		for (func = ent->func + 1; func <= limit && r == 0; ++func) {
> -			if (nent >= cpuid->nent) {
> -				r = -E2BIG;
> -				goto out_free;
> -			}
> -			r = do_cpuid_func(&cpuid_entries[nent], func,
> -				          &nent, cpuid->nent, type);
> -		}
> -
> +		r = get_cpuid_func(cpuid_entries, ent->func, &nent,
> +				   cpuid->nent, type);
>  		if (r)
>  			goto out_free;
>  	}

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Sean Christopherson Feb. 7, 2020, 7:53 p.m. UTC | #2
On Thu, Feb 06, 2020 at 03:59:49PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > Move the guts of kvm_dev_ioctl_get_cpuid()'s CPUID func loop to a
> > separate helper to improve code readability and pave the way for future
> > cleanup.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 45 ++++++++++++++++++++++++++------------------
> >  1 file changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 47ce04762c20..f49fdd06f511 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -839,6 +839,29 @@ static bool is_centaur_cpu(const struct kvm_cpuid_param *param)
> >  	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
> >  }
> >  
> > +static int get_cpuid_func(struct kvm_cpuid_entry2 *entries, u32 func,
> > +			  int *nent, int maxnent, unsigned int type)
> > +{
> > +	u32 limit;
> > +	int r;
> > +
> > +	r = do_cpuid_func(&entries[*nent], func, nent, maxnent, type);
> > +	if (r)
> > +		return r;
> > +
> > +	limit = entries[*nent - 1].eax;
> > +	for (func = func + 1; func <= limit; ++func) {
> > +		if (*nent >= maxnent)
> > +			return -E2BIG;
> > +
> > +		r = do_cpuid_func(&entries[*nent], func, nent, maxnent, type);
> > +		if (r)
> > +			break;
> > +	}
> > +
> > +	return r;
> > +}
> > +
> >  static bool sanity_check_entries(struct kvm_cpuid_entry2 __user *entries,
> >  				 __u32 num_entries, unsigned int ioctl_type)
> >  {
> > @@ -871,8 +894,8 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
> >  			    unsigned int type)
> >  {
> >  	struct kvm_cpuid_entry2 *cpuid_entries;
> > -	int limit, nent = 0, r = -E2BIG, i;
> > -	u32 func;
> > +	int nent = 0, r = -E2BIG, i;
> 
> Not this patches fault, but I just noticed that '-E2BIG' initializer
> here is only being used for 
> 
>  'if (cpuid->nent < 1)'
> 
> case so I have two suggestion:
> 1) Return directly without the 'goto' , drop the initializer.

Great minds think alike ;-)

> 2) Return -EINVAL instead.

I agree that it _should_ be -EINVAL, but I just don't think it's worth
the possibility of breaking (stupid) userspace that was doing something
like:

	for (i = 0; i < max_cpuid_size; i++) {
		cpuid.nent = i;

		r = ioctl(fd, KVM_GET_SUPPORTED_CPUID, &cpuid);
		if (!r || r != -E2BIG)
			break;
	}
Paolo Bonzini Feb. 25, 2020, 2:37 p.m. UTC | #3
On 07/02/20 20:53, Sean Christopherson wrote:
> 
>> 2) Return -EINVAL instead.
> I agree that it _should_ be -EINVAL, but I just don't think it's worth
> the possibility of breaking (stupid) userspace that was doing something
> like:
> 
> 	for (i = 0; i < max_cpuid_size; i++) {
> 		cpuid.nent = i;
> 
> 		r = ioctl(fd, KVM_GET_SUPPORTED_CPUID, &cpuid);
> 		if (!r || r != -E2BIG)
> 			break;
> 	}
> 

Apart from the stupidity of the above case, why would it be EINVAL?

I can do the change to drop the initializer when applying.

Paolo
Vitaly Kuznetsov Feb. 25, 2020, 3:09 p.m. UTC | #4
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/02/20 20:53, Sean Christopherson wrote:
>> 
>>> 2) Return -EINVAL instead.
>> I agree that it _should_ be -EINVAL, but I just don't think it's worth
>> the possibility of breaking (stupid) userspace that was doing something
>> like:
>> 
>> 	for (i = 0; i < max_cpuid_size; i++) {
>> 		cpuid.nent = i;
>> 
>> 		r = ioctl(fd, KVM_GET_SUPPORTED_CPUID, &cpuid);
>> 		if (!r || r != -E2BIG)
>> 			break;
>> 	}
>> 
>
> Apart from the stupidity of the above case, why would it be EINVAL?
>

I suggested -EINVAL because issuing KVM_GET_SUPPORTED_CPUID with nent=0
looks more like a completely invalid input and not 'too many
entries'(-E2BIG) to me (but -E2BIG is already there, let's keep it, it's
not a big deal).

> I can do the change to drop the initializer when applying.

We're agreed with Sean on a few cosmetic changes in other patches of
this series, wait for v2)
Paolo Bonzini Feb. 26, 2020, 11:35 a.m. UTC | #5
On 25/02/20 16:09, Vitaly Kuznetsov wrote:
>> Apart from the stupidity of the above case, why would it be EINVAL?
>>
> I suggested -EINVAL because issuing KVM_GET_SUPPORTED_CPUID with nent=0
> looks more like a completely invalid input and not 'too many
> entries'(-E2BIG) to me (but -E2BIG is already there, let's keep it, it's
> not a big deal).

Yes, and in fact he already does that change a few patches later.

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 47ce04762c20..f49fdd06f511 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -839,6 +839,29 @@  static bool is_centaur_cpu(const struct kvm_cpuid_param *param)
 	return boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR;
 }
 
+static int get_cpuid_func(struct kvm_cpuid_entry2 *entries, u32 func,
+			  int *nent, int maxnent, unsigned int type)
+{
+	u32 limit;
+	int r;
+
+	r = do_cpuid_func(&entries[*nent], func, nent, maxnent, type);
+	if (r)
+		return r;
+
+	limit = entries[*nent - 1].eax;
+	for (func = func + 1; func <= limit; ++func) {
+		if (*nent >= maxnent)
+			return -E2BIG;
+
+		r = do_cpuid_func(&entries[*nent], func, nent, maxnent, type);
+		if (r)
+			break;
+	}
+
+	return r;
+}
+
 static bool sanity_check_entries(struct kvm_cpuid_entry2 __user *entries,
 				 __u32 num_entries, unsigned int ioctl_type)
 {
@@ -871,8 +894,8 @@  int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 			    unsigned int type)
 {
 	struct kvm_cpuid_entry2 *cpuid_entries;
-	int limit, nent = 0, r = -E2BIG, i;
-	u32 func;
+	int nent = 0, r = -E2BIG, i;
+
 	static const struct kvm_cpuid_param param[] = {
 		{ .func = 0 },
 		{ .func = 0x80000000 },
@@ -901,22 +924,8 @@  int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 		if (ent->qualifier && !ent->qualifier(ent))
 			continue;
 
-		r = do_cpuid_func(&cpuid_entries[nent], ent->func,
-				  &nent, cpuid->nent, type);
-
-		if (r)
-			goto out_free;
-
-		limit = cpuid_entries[nent - 1].eax;
-		for (func = ent->func + 1; func <= limit && r == 0; ++func) {
-			if (nent >= cpuid->nent) {
-				r = -E2BIG;
-				goto out_free;
-			}
-			r = do_cpuid_func(&cpuid_entries[nent], func,
-				          &nent, cpuid->nent, type);
-		}
-
+		r = get_cpuid_func(cpuid_entries, ent->func, &nent,
+				   cpuid->nent, type);
 		if (r)
 			goto out_free;
 	}