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 <[email protected]> > > Cc: Marcelo Tosatti <[email protected]> > > Signed-off-by: Sasha Levin <[email protected]> > > --- > > 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 = ¶m[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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
