Message ID | 4ffa4f23bc93aa5af90d836986771bb6d9856bf9.1294043582.git.jan.kiszka@web.de |
---|---|
State | New |
Headers | show |
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.
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
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.
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
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