Patchwork [08/20] target-i386: compile kvm only functions if CONFIG_KVM is defined

login
register
mail settings
Submitter Igor Mammedov
Date Dec. 17, 2012, 4:01 p.m.
Message ID <1355760092-18755-9-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/206903/
State New
Headers show

Comments

Igor Mammedov - Dec. 17, 2012, 4:01 p.m.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
Eduardo Habkost - Dec. 19, 2012, 4:42 p.m.
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
> 
>
Igor Mammedov - Dec. 19, 2012, 5:16 p.m.
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
> > 
> > 
>
Eduardo Habkost - Dec. 20, 2012, 5:03 p.m.
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
> > > 
> > > 
> > 
>

Patch

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: