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

[Sean Christopherson]

On Thu, Feb 06, 2020 at 03:59:49PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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;
	}