diff mbox

[1/7,v2] target-i386: Eliminate CONFIG_KVM #ifdefs

Message ID 20131210185506.GF27737@otherpad.lan.raisama.net
State New
Headers show

Commit Message

Eduardo Habkost Dec. 10, 2013, 6:55 p.m. UTC 
The compiler is capable of eliminating the KVM-specific function calls
as long as the calling function has an assert(kvm_enabled()) line, so we
don't need to wrap all KVM-specific code inside #ifdefs.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 * Check for __i386__ on host_cpuid() so the code compiles properly
   on non-x86 hosts
---
 target-i386/cpu.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Comments

Richard Henderson Dec. 11, 2013, 1:36 a.m. UTC | #1 
On 12/10/2013 10:55 AM, Eduardo Habkost wrote:
> The compiler is capable of eliminating the KVM-specific function calls
> as long as the calling function has an assert(kvm_enabled()) line, so we
> don't need to wrap all KVM-specific code inside #ifdefs.

Really?  In tcg/tcg.h we force NDEBUG if not CONFIG_TCG_DEBUG, which makes
assert expand to nothing.  This statement may be true for some files, but
almost everything under target-i386 includes tcg.h.

Although I know we've talked within glibc and gcc the de-optimization of
missing out on assert info, and how we ought to use __builtin_gcc_unreachable
in order to retain that, we've still not done anything official with <assert.h>.

That said, I don't disagree with the changes, if they work with a forced
-DNDEBUG, i.e. unreachable code that still compiles.


r~
Eduardo Habkost Dec. 11, 2013, 4:11 p.m. UTC | #2 
On Tue, Dec 10, 2013 at 05:36:10PM -0800, Richard Henderson wrote:
> On 12/10/2013 10:55 AM, Eduardo Habkost wrote:
> > The compiler is capable of eliminating the KVM-specific function calls
> > as long as the calling function has an assert(kvm_enabled()) line, so we
> > don't need to wrap all KVM-specific code inside #ifdefs.
> 
> Really?  In tcg/tcg.h we force NDEBUG if not CONFIG_TCG_DEBUG, which makes
> assert expand to nothing.  This statement may be true for some files, but
> almost everything under target-i386 includes tcg.h.
> 
> Although I know we've talked within glibc and gcc the de-optimization of
> missing out on assert info, and how we ought to use __builtin_gcc_unreachable
> in order to retain that, we've still not done anything official with <assert.h>.
> 
> That said, I don't disagree with the changes, if they work with a forced
> -DNDEBUG, i.e. unreachable code that still compiles.

Oops, I didn't consider -DNDEBUG. You are right and assert() can't be
considered a sufficient hint to the compiler.

I remember making some tests where assert(kvm_enabled()) helped the
compiler, but I can't reproduce it anymore (maybe it was in an older
version of the code). I have just tested this patch using -DNEBUG
combined with: -O, -O0, -O1, -O2, and without -O. In all cases, QEMU was
built successfully with CONFIG_KVM disabled.

I will resubmit the patch without the assert() line and with a more
accurate commit message.
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 783b3d0..211cdf1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -373,7 +373,6 @@  void disable_kvm_pv_eoi(void)
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
-#if defined(CONFIG_KVM)
     uint32_t vec[4];
 
 #ifdef __x86_64__
@@ -381,7 +380,7 @@  void host_cpuid(uint32_t function, uint32_t count,
                  : "=a"(vec[0]), "=b"(vec[1]),
                    "=c"(vec[2]), "=d"(vec[3])
                  : "0"(function), "c"(count) : "cc");
-#else
+#elif defined(__i386__)
     asm volatile("pusha \n\t"
                  "cpuid \n\t"
                  "mov %%eax, 0(%2) \n\t"
@@ -391,6 +390,8 @@  void host_cpuid(uint32_t function, uint32_t count,
                  "popa"
                  : : "a"(function), "c"(count), "S"(vec)
                  : "memory", "cc");
+#else
+    abort();
 #endif
 
     if (eax)
@@ -401,7 +402,6 @@  void host_cpuid(uint32_t function, uint32_t count,
         *ecx = vec[2];
     if (edx)
         *edx = vec[3];
-#endif
 }
 
 #define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
@@ -1118,7 +1118,6 @@  void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
     }
 }
 
-#ifdef CONFIG_KVM
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1133,7 +1132,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
@@ -1142,7 +1140,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;
 
@@ -1172,8 +1169,6 @@  static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
             kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
                                          wi->cpuid_reg);
     }
-
-#endif /* CONFIG_KVM */
 }
 
 static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
@@ -1798,13 +1793,14 @@  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
-#ifdef CONFIG_KVM
 static void filter_features_for_kvm(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
     KVMState *s = kvm_state;
     FeatureWord w;
 
+    assert(kvm_enabled());
+
     for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
         uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
@@ -1815,7 +1811,6 @@  static void filter_features_for_kvm(X86CPU *cpu)
         cpu->filtered_features[w] = requested_features & ~env->features[w];
     }
 }
-#endif
 
 static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
 {
@@ -2525,9 +2520,7 @@  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                        "Host's CPU doesn't support requested features");
             goto out;
         }
-#ifdef CONFIG_KVM
         filter_features_for_kvm(cpu);
-#endif
     }
 
 #ifndef CONFIG_USER_ONLY