Re: [Qemu-devel] [PATCH 08/20] target-i386: compile kvm only functions if CONFIG_KVM is defined

[Eduardo Habkost]

On Wed, Dec 19, 2012 at 06:16:08PM +0100, Igor Mammedov wrote:
> On Wed, 19 Dec 2012 14:42:31 -0200
> Eduardo Habkost <address@hidden> 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
> > > 
> > > 
> > 
> 

-- 
Eduardo