Patchwork [v2,17/17] kvm: Drop dependencies on very old capabilities

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 3, 2011, 8:33 a.m.
Message ID <4ffa4f23bc93aa5af90d836986771bb6d9856bf9.1294043582.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/77234/
State New
Headers show

Comments

Jan Kiszka - Jan. 3, 2011, 8:33 a.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

COALESCED_MMIO, SYNC_MMU, EXT_CPUID, CLOCKSOURCE, NOP_IO_DELAY, PV_MMU -
all these caps predate features on which we already depend at build
time. Moreover, the check for KVM_CAP_EXT_CPUID is unneeded as we
already test & fail is a more recent feature is missing.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c         |   16 ----------------
 target-i386/kvm.c |   22 ----------------------
 2 files changed, 0 insertions(+), 38 deletions(-)
Avi Kivity - Jan. 3, 2011, 4:08 p.m.
On 01/03/2011 10:33 AM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> COALESCED_MMIO, SYNC_MMU, EXT_CPUID, CLOCKSOURCE, NOP_IO_DELAY, PV_MMU -
> all these caps predate features on which we already depend at build
> time. Moreover, the check for KVM_CAP_EXT_CPUID is unneeded as we
> already test&  fail is a more recent feature is missing.

No.  Each test documents a dependency of qemu on a kvm feature.  Even 
though something like SYNC_MMU is unlikely to go away, as long as we 
depend on it, we require the feature.
Jan Kiszka - Jan. 3, 2011, 4:54 p.m.
Am 03.01.2011 17:08, Avi Kivity wrote:
> On 01/03/2011 10:33 AM, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> COALESCED_MMIO, SYNC_MMU, EXT_CPUID, CLOCKSOURCE, NOP_IO_DELAY, PV_MMU -
>> all these caps predate features on which we already depend at build
>> time. Moreover, the check for KVM_CAP_EXT_CPUID is unneeded as we
>> already test&  fail is a more recent feature is missing.
> 
> No.  Each test documents a dependency of qemu on a kvm feature.  Even
> though something like SYNC_MMU is unlikely to go away, as long as we
> depend on it, we require the feature.
> 

Then at least move all those KVM_CAPs we need at build time into
configure. I really see no value in keeping ugly conditional code
around, A) because those paths won't be tested and B) none of the CAPs
touched here are to pass away without a replacement that will require
user space adaption anyway.

Jan
Avi Kivity - Jan. 3, 2011, 5:01 p.m.
On 01/03/2011 06:54 PM, Jan Kiszka wrote:
> Am 03.01.2011 17:08, Avi Kivity wrote:
> >  On 01/03/2011 10:33 AM, Jan Kiszka wrote:
> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
> >>
> >>  COALESCED_MMIO, SYNC_MMU, EXT_CPUID, CLOCKSOURCE, NOP_IO_DELAY, PV_MMU -
> >>  all these caps predate features on which we already depend at build
> >>  time. Moreover, the check for KVM_CAP_EXT_CPUID is unneeded as we
> >>  already test&   fail is a more recent feature is missing.
> >
> >  No.  Each test documents a dependency of qemu on a kvm feature.  Even
> >  though something like SYNC_MMU is unlikely to go away, as long as we
> >  depend on it, we require the feature.
> >
>
> Then at least move all those KVM_CAPs we need at build time into
> configure.

Need a run time check as well (build on new kernel, run on old kernel, 
or run on even newer kernel that lost a feature).

> I really see no value in keeping ugly conditional code
> around, A) because those paths won't be tested and B) none of the CAPs
> touched here are to pass away without a replacement that will require
> user space adaption anyway.

I'm fine with a series of checks during init time with no fallback.  I'm 
not fine with just dropping those away.  Reducing code size is great, 
but not at the cost of undiagnosed runtime failures.
Jan Kiszka - Jan. 3, 2011, 5:24 p.m.
Am 03.01.2011 18:01, Avi Kivity wrote:
> On 01/03/2011 06:54 PM, Jan Kiszka wrote:
>> Am 03.01.2011 17:08, Avi Kivity wrote:
>> >  On 01/03/2011 10:33 AM, Jan Kiszka wrote:
>> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
>> >>
>> >>  COALESCED_MMIO, SYNC_MMU, EXT_CPUID, CLOCKSOURCE, NOP_IO_DELAY,
>> PV_MMU -
>> >>  all these caps predate features on which we already depend at build
>> >>  time. Moreover, the check for KVM_CAP_EXT_CPUID is unneeded as we
>> >>  already test&   fail is a more recent feature is missing.
>> >
>> >  No.  Each test documents a dependency of qemu on a kvm feature.  Even
>> >  though something like SYNC_MMU is unlikely to go away, as long as we
>> >  depend on it, we require the feature.
>> >
>>
>> Then at least move all those KVM_CAPs we need at build time into
>> configure.
> 
> Need a run time check as well (build on new kernel, run on old kernel,
> or run on even newer kernel that lost a feature).
> 
>> I really see no value in keeping ugly conditional code
>> around, A) because those paths won't be tested and B) none of the CAPs
>> touched here are to pass away without a replacement that will require
>> user space adaption anyway.
> 
> I'm fine with a series of checks during init time with no fallback.  I'm
> not fine with just dropping those away.  Reducing code size is great,
> but not at the cost of undiagnosed runtime failures.

My worry was not code size but untested code. And looking at the
EXT_CPUID case again, this is what would happen:
kvm_x86_get_supported_cpuid will return -1 if we lack EXT_CPUID, but
that value is interpreted by the callers as "all requested features
available" - likely not that helpful in all cases. This also affects
qemu-kvm.

I will add generic and per-arch check sections at build and runtime for
CAPs we don't want to miss.

Jan

Patch

diff --git a/kvm-all.c b/kvm-all.c
index 190fcdf..15d5f32 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -57,9 +57,7 @@  static struct KVMState {
     int fd;
     int vmfd;
     int coalesced_mmio;
-#ifdef KVM_CAP_COALESCED_MMIO
     struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
-#endif
     int broken_set_mem_region;
     int migration_log;
     int vcpu_events;
@@ -214,12 +212,10 @@  int kvm_init_vcpu(CPUState *env)
         goto err;
     }
 
-#ifdef KVM_CAP_COALESCED_MMIO
     if (kvm_state.coalesced_mmio && !kvm_state.coalesced_mmio_ring) {
         kvm_state.coalesced_mmio_ring =
             (void *)env->kvm_run + kvm_state.coalesced_mmio * PAGE_SIZE;
     }
-#endif
 
     ret = kvm_arch_init_vcpu(env);
     if (ret == 0) {
@@ -386,7 +382,6 @@  int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
 {
     int ret = -ENOSYS;
 
-#ifdef KVM_CAP_COALESCED_MMIO
     if (kvm_state.coalesced_mmio) {
         struct kvm_coalesced_mmio_zone zone;
 
@@ -395,7 +390,6 @@  int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
 
         ret = kvm_vm_ioctl(KVM_REGISTER_COALESCED_MMIO, &zone);
     }
-#endif
 
     return ret;
 }
@@ -404,7 +398,6 @@  int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
 {
     int ret = -ENOSYS;
 
-#ifdef KVM_CAP_COALESCED_MMIO
     if (kvm_state.coalesced_mmio) {
         struct kvm_coalesced_mmio_zone zone;
 
@@ -413,7 +406,6 @@  int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
 
         ret = kvm_vm_ioctl(KVM_UNREGISTER_COALESCED_MMIO, &zone);
     }
-#endif
 
     return ret;
 }
@@ -654,9 +646,7 @@  int kvm_init(void)
         goto err;
     }
 
-#ifdef KVM_CAP_COALESCED_MMIO
     kvm_state.coalesced_mmio = kvm_check_extension(KVM_CAP_COALESCED_MMIO);
-#endif
 
     kvm_state.broken_set_mem_region = 1;
 #ifdef KVM_CAP_JOIN_MEMORY_REGIONS_WORKS
@@ -777,7 +767,6 @@  static int kvm_handle_internal_error(CPUState *env, struct kvm_run *run)
 
 void kvm_flush_coalesced_mmio_buffer(void)
 {
-#ifdef KVM_CAP_COALESCED_MMIO
     if (kvm_state.coalesced_mmio_ring) {
         struct kvm_coalesced_mmio_ring *ring = kvm_state.coalesced_mmio_ring;
         while (ring->first != ring->last) {
@@ -790,7 +779,6 @@  void kvm_flush_coalesced_mmio_buffer(void)
             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
         }
     }
-#endif
 }
 
 static void do_kvm_cpu_synchronize_state(void *_env)
@@ -988,11 +976,7 @@  int kvm_vcpu_ioctl(CPUState *env, int type, ...)
 
 int kvm_has_sync_mmu(void)
 {
-#ifdef KVM_CAP_SYNC_MMU
     return kvm_check_extension(KVM_CAP_SYNC_MMU);
-#else
-    return 0;
-#endif
 }
 
 int kvm_has_vcpu_events(void)
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index caca407..b4a3463 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -59,8 +59,6 @@  static bool has_msr_star;
 static bool has_msr_hsave_pa;
 static int lm_capable_kernel;
 
-#ifdef KVM_CAP_EXT_CPUID
-
 static struct kvm_cpuid2 *try_get_cpuid(int max)
 {
     struct kvm_cpuid2 *cpuid;
@@ -94,10 +92,6 @@  uint32_t kvm_x86_get_supported_cpuid(uint32_t function, uint32_t index,
     uint32_t ret = 0;
     uint32_t cpuid_1_edx;
 
-    if (!kvm_check_extension(KVM_CAP_EXT_CPUID)) {
-        return -1U;
-    }
-
     max = 1;
     while ((cpuid = try_get_cpuid(max)) == NULL) {
         max *= 2;
@@ -141,30 +135,14 @@  uint32_t kvm_x86_get_supported_cpuid(uint32_t function, uint32_t index,
     return ret;
 }
 
-#else
-
-uint32_t kvm_x86_get_supported_cpuid(uint32_t function, uint32_t index,
-                                     int reg)
-{
-    return -1U;
-}
-
-#endif
-
 #ifdef CONFIG_KVM_PARA
 struct kvm_para_features {
     int cap;
     int feature;
 } para_features[] = {
-#ifdef KVM_CAP_CLOCKSOURCE
     { KVM_CAP_CLOCKSOURCE, KVM_FEATURE_CLOCKSOURCE },
-#endif
-#ifdef KVM_CAP_NOP_IO_DELAY
     { KVM_CAP_NOP_IO_DELAY, KVM_FEATURE_NOP_IO_DELAY },
-#endif
-#ifdef KVM_CAP_PV_MMU
     { KVM_CAP_PV_MMU, KVM_FEATURE_MMU_OP },
-#endif
 #ifdef KVM_CAP_ASYNC_PF
     { KVM_CAP_ASYNC_PF, KVM_FEATURE_ASYNC_PF },
 #endif