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;
}
