| Message ID | 1355760092-18755-9-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Dec 17, 2012 at 05:01:20PM +0100, Igor Mammedov wrote: [...] > > static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > @@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > } > } > if (kvm_enabled() && name && strcmp(name, "host") == 0) { > +#ifdef CONFIG_KVM > kvm_cpu_fill_host(x86_cpu_def); > +#endif Is this really better than the existing code that generates an empty stub function (that will never be called anyway)? I am not strongly inclined either way, but I prefer the existing style. > } else if (!def) { > return -1; > } else { > @@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > x86_cpu_def->kvm_features &= ~minus_kvm_features; > x86_cpu_def->svm_features &= ~minus_svm_features; > x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; > +#ifdef CONFIG_KVM > if (check_cpuid && kvm_enabled()) { > if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid) > goto error; > } > +#endif > return 0; > > error: > -- > 1.7.1 > >
On Wed, 19 Dec 2012 14:42:31 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Dec 17, 2012 at 05:01:20PM +0100, Igor Mammedov wrote: > [...] > > > > static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void > > *opaque, const char *name, Error **errp) > > @@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t > > *x86_cpu_def, const char *name) } > > } > > if (kvm_enabled() && name && strcmp(name, "host") == 0) { > > +#ifdef CONFIG_KVM > > kvm_cpu_fill_host(x86_cpu_def); > > +#endif > > Is this really better than the existing code that generates an empty > stub function (that will never be called anyway)? Following patch that moves kvm_check_features_against_host() inside of #ifdef CONFIG_KVM in realize_fn(), makes build failure *-user with warning that kvm_check_features_against_host() is unused and if we make stub from kvm_check_features_against_host() as well then we will have to ifdef unavailable_host_feature() as well. As result it makes +2 more ifdefs one of which excludes whole function. This patch makes one big ifdef block of kvm specific functions and the next one keep amount of ifdef the same as before these patches. And as bonus we get cleaner build and won't get unused symbols & calls to empty functions on debug builds. > > I am not strongly inclined either way, but I prefer the existing style. > > > > } else if (!def) { > > return -1; > > } else { > > @@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t > > *x86_cpu_def, char *features) x86_cpu_def->kvm_features &= > > ~minus_kvm_features; x86_cpu_def->svm_features &= ~minus_svm_features; > > x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; > > +#ifdef CONFIG_KVM > > if (check_cpuid && kvm_enabled()) { > > if (kvm_check_features_against_host(x86_cpu_def) && > > enforce_cpuid) goto error; > > } > > +#endif > > return 0; > > > > error: > > -- > > 1.7.1 > > > > >
On Wed, Dec 19, 2012 at 06:16:08PM +0100, Igor Mammedov wrote: > On Wed, 19 Dec 2012 14:42:31 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Dec 17, 2012 at 05:01:20PM +0100, Igor Mammedov wrote: > > [...] > > > > > > static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void > > > *opaque, const char *name, Error **errp) > > > @@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t > > > *x86_cpu_def, const char *name) } > > > } > > > if (kvm_enabled() && name && strcmp(name, "host") == 0) { > > > +#ifdef CONFIG_KVM > > > kvm_cpu_fill_host(x86_cpu_def); > > > +#endif > > > > Is this really better than the existing code that generates an empty > > stub function (that will never be called anyway)? > Following patch that moves kvm_check_features_against_host() inside > of #ifdef CONFIG_KVM in realize_fn(), makes build failure *-user with > warning that kvm_check_features_against_host() is unused and if > we make stub from kvm_check_features_against_host() as well then we will have > to ifdef unavailable_host_feature() as well. As result it makes +2 more > ifdefs one of which excludes whole function. I see. So my question above doesn't apply anymore once patch 09/20 is applied (and the rest of the changes in this patch make sense). I think you could just squash both patches together, and it should be obvious that you are adding/moving #ifdefs around the functions just because the function call is now inside an #ifdef. > > This patch makes one big ifdef block of kvm specific functions and the next > one keep amount of ifdef the same as before these patches. > > And as bonus we get cleaner build and won't get unused symbols & calls to > empty functions on debug builds. > > > > > I am not strongly inclined either way, but I prefer the existing style. > > > > > > > } else if (!def) { > > > return -1; > > > } else { > > > @@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t > > > *x86_cpu_def, char *features) x86_cpu_def->kvm_features &= > > > ~minus_kvm_features; x86_cpu_def->svm_features &= ~minus_svm_features; > > > x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; > > > +#ifdef CONFIG_KVM > > > if (check_cpuid && kvm_enabled()) { > > > if (kvm_check_features_against_host(x86_cpu_def) && > > > enforce_cpuid) goto error; > > > } > > > +#endif > > > return 0; > > > > > > error: > > > -- > > > 1.7.1 > > > > > > > > >
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 418c899..24bfd95 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -862,7 +862,6 @@ static int cpu_x86_fill_model_id(char *str) } return 0; } -#endif /* Fill a x86_def_t struct with information about the host CPU, and * the CPU features supported by the host hardware + host kernel @@ -871,7 +870,6 @@ static int cpu_x86_fill_model_id(char *str) */ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) { -#ifdef CONFIG_KVM KVMState *s = kvm_state; uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0; @@ -930,7 +928,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) * unsupported ones later. */ x86_cpu_def->svm_features = -1; -#endif /* CONFIG_KVM */ } static int unavailable_host_feature(struct model_features_t *f, uint32_t mask) @@ -981,6 +978,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def) } return rv; } +#endif /* CONFIG_KVM */ static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) @@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) } } if (kvm_enabled() && name && strcmp(name, "host") == 0) { +#ifdef CONFIG_KVM kvm_cpu_fill_host(x86_cpu_def); +#endif } else if (!def) { return -1; } else { @@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) x86_cpu_def->kvm_features &= ~minus_kvm_features; x86_cpu_def->svm_features &= ~minus_svm_features; x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; +#ifdef CONFIG_KVM if (check_cpuid && kvm_enabled()) { if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid) goto error; } +#endif return 0; error:
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)