* [PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid
@ 2011-11-25 8:12 Sasha Levin
2011-11-27 14:44 ` Avi Kivity
0 siblings, 1 reply; 3+ messages in thread
From: Sasha Levin @ 2011-11-25 8:12 UTC (permalink / raw)
To: kvm; +Cc: Sasha Levin, Avi Kivity, Marcelo Tosatti
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 = ¶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;
r = -EFAULT;
--
1.7.8.rc3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid
2011-11-25 8:12 [PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid Sasha Levin
@ 2011-11-27 14:44 ` Avi Kivity
2011-11-27 14:55 ` Sasha Levin
0 siblings, 1 reply; 3+ messages in thread
From: Avi Kivity @ 2011-11-27 14:44 UTC (permalink / raw)
To: Sasha Levin; +Cc: kvm, Marcelo Tosatti
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 = ¶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?
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid
2011-11-27 14:44 ` Avi Kivity
@ 2011-11-27 14:55 ` Sasha Levin
0 siblings, 0 replies; 3+ messages in thread
From: Sasha Levin @ 2011-11-27 14:55 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, Marcelo Tosatti
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 = ¶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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-11-27 14:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-25 8:12 [PATCH] KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid Sasha Levin
2011-11-27 14:44 ` Avi Kivity
2011-11-27 14:55 ` Sasha Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox