* Hotplug / TSC cleanup and fixing @ 2009-09-29 4:04 Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 4:04 UTC (permalink / raw) To: kvm; +Cc: linux-kernel Greatly simplified version of the patches; allocate and dealloc all required structs at module load and unload time. Clarification: per_cpu(var, cpu) does indeed work for not-present CPUs; the allocations for module specific per-cpu variables are done at module load and unload time, while the per_cpu chunks are setup for all possible CPUs at boot time. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-29 4:04 Hotplug / TSC cleanup and fixing Zachary Amsden @ 2009-09-29 4:04 ` Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 4:04 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/x86.c | 23 +++++++++++++++-------- 1 files changed, 15 insertions(+), 8 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index fedac9d..15d2ace 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3116,9 +3116,22 @@ static struct notifier_block kvmclock_cpufreq_notifier_block = { .notifier_call = kvmclock_cpufreq_notifier }; +static void kvm_timer_init(void) +{ + int cpu; + + for_each_possible_cpu(cpu) + per_cpu(cpu_tsc_khz, cpu) = tsc_khz; + if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { + tsc_khz_ref = tsc_khz; + cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, + CPUFREQ_TRANSITION_NOTIFIER); + } +} + int kvm_arch_init(void *opaque) { - int r, cpu; + int r; struct kvm_x86_ops *ops = (struct kvm_x86_ops *)opaque; if (kvm_x86_ops) { @@ -3150,13 +3163,7 @@ int kvm_arch_init(void *opaque) kvm_mmu_set_mask_ptes(PT_USER_MASK, PT_ACCESSED_MASK, PT_DIRTY_MASK, PT64_NX_MASK, 0); - for_each_possible_cpu(cpu) - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; - if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { - tsc_khz_ref = tsc_khz; - cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, - CPUFREQ_TRANSITION_NOTIFIER); - } + kvm_timer_init(); return 0; -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-09-29 4:04 ` [PATCH v2: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden @ 2009-09-29 4:04 ` Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 3/4] Fix printk name error in svm.c Zachary Amsden 2009-09-29 8:29 ` [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Avi Kivity 0 siblings, 2 replies; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 4:04 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti They are globals, not clearly protected by any ordering or locking, and vulnerable to various startup races. Instead, for variable TSC machines, register the cpufreq notifier and get the TSC frequency directly from the cpufreq machinery. Not only is it always right, it is also perfectly accurate, as no error prone measurement is required. On such machines, when a new CPU online is brought online, it isn't clear what frequency it will start with, and it may not correspond to the reference, thus in hardware_enable we clear the cpu_tsc_khz variable to zero and make sure it is set before running on a VCPU. Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/x86.c | 30 ++++++++++++++++++++---------- 1 files changed, 20 insertions(+), 10 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 15d2ace..9cbd53a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1326,6 +1326,8 @@ out: void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) { kvm_x86_ops->vcpu_load(vcpu, cpu); + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); kvm_request_guest_time_update(vcpu); } @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) /* nothing */ } -static unsigned int ref_freq; -static unsigned long tsc_khz_ref; - static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long val, void *data) { @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va struct kvm_vcpu *vcpu; int i, send_ipi = 0; - if (!ref_freq) - ref_freq = freq->old; - if (val == CPUFREQ_PRECHANGE && freq->old > freq->new) return 0; if (val == CPUFREQ_POSTCHANGE && freq->old < freq->new) return 0; - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, ref_freq, freq->new); + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; spin_lock(&kvm_lock); list_for_each_entry(kvm, &vm_list, vm_list) { @@ -3120,12 +3116,18 @@ static void kvm_timer_init(void) { int cpu; - for_each_possible_cpu(cpu) - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { - tsc_khz_ref = tsc_khz; cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, CPUFREQ_TRANSITION_NOTIFIER); + for_each_online_cpu(cpu) + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); + } else { + for_each_possible_cpu(cpu) + per_cpu(cpu_tsc_khz, cpu) = tsc_khz; + } + for_each_possible_cpu(cpu) { + printk(KERN_DEBUG "kvm: cpu %d = %ld khz\n", + cpu, per_cpu(cpu_tsc_khz, cpu)); } } @@ -4698,6 +4700,14 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) int kvm_arch_hardware_enable(void *garbage) { + /* + * Since this may be called from a hotplug notifcation, + * we can't get the CPU frequency directly. + */ + if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { + int cpu = raw_smp_processor_id(); + per_cpu(cpu_tsc_khz, cpu) = 0; + } return kvm_x86_ops->hardware_enable(garbage); } -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2: kvm 3/4] Fix printk name error in svm.c 2009-09-29 4:04 ` [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden @ 2009-09-29 4:04 ` Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 4/4] Fix hotplug of CPUs for KVM Zachary Amsden 2009-09-29 8:29 ` [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Avi Kivity 1 sibling, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 4:04 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/svm.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 9a4daca..d1036ce 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -330,13 +330,14 @@ static int svm_hardware_enable(void *garbage) return -EBUSY; if (!has_svm()) { - printk(KERN_ERR "svm_cpu_init: err EOPNOTSUPP on %d\n", me); + printk(KERN_ERR "svm_hardware_enable: err EOPNOTSUPP on %d\n", + me); return -EINVAL; } svm_data = per_cpu(svm_data, me); if (!svm_data) { - printk(KERN_ERR "svm_cpu_init: svm_data is NULL on %d\n", + printk(KERN_ERR "svm_hardware_enable: svm_data is NULL on %d\n", me); return -EINVAL; } -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2: kvm 4/4] Fix hotplug of CPUs for KVM. 2009-09-29 4:04 ` [PATCH v2: kvm 3/4] Fix printk name error in svm.c Zachary Amsden @ 2009-09-29 4:04 ` Zachary Amsden 2009-09-29 8:30 ` Avi Kivity 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 4:04 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti Both VMX and SVM require per-cpu memory allocation, which is done at module init time, for only online cpus. Backend was not allocating enough structure for all possible CPUs, so new CPUs coming online could not be hardware enabled. Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/svm.c | 4 ++-- arch/x86/kvm/vmx.c | 6 ++++-- virt/kvm/kvm_main.c | 3 --- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index d1036ce..02a4269 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -482,7 +482,7 @@ static __init int svm_hardware_setup(void) kvm_enable_efer_bits(EFER_SVME); } - for_each_online_cpu(cpu) { + for_each_possible_cpu(cpu) { r = svm_cpu_init(cpu); if (r) goto err; @@ -516,7 +516,7 @@ static __exit void svm_hardware_unsetup(void) { int cpu; - for_each_online_cpu(cpu) + for_each_possible_cpu(cpu) svm_cpu_uninit(cpu); __free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER); diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 3fe0d42..e86f1a6 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1350,15 +1350,17 @@ static void free_kvm_area(void) { int cpu; - for_each_online_cpu(cpu) + for_each_possible_cpu(cpu) { free_vmcs(per_cpu(vmxarea, cpu)); + per_cpu(vmxarea, cpu) = NULL; + } } static __init int alloc_kvm_area(void) { int cpu; - for_each_online_cpu(cpu) { + for_each_possible_cpu(cpu) { struct vmcs *vmcs; vmcs = alloc_vmcs_cpu(cpu); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index e27b7a9..2cd8bc2 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1716,9 +1716,6 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val, { int cpu = (long)v; - if (!kvm_usage_count) - return NOTIFY_OK; - val &= ~CPU_TASKS_FROZEN; switch (val) { case CPU_DYING: -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2: kvm 4/4] Fix hotplug of CPUs for KVM. 2009-09-29 4:04 ` [PATCH v2: kvm 4/4] Fix hotplug of CPUs for KVM Zachary Amsden @ 2009-09-29 8:30 ` Avi Kivity 2009-09-29 15:51 ` Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden 0 siblings, 2 replies; 30+ messages in thread From: Avi Kivity @ 2009-09-29 8:30 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/29/2009 06:04 AM, Zachary Amsden wrote: > Both VMX and SVM require per-cpu memory allocation, which is done at module > init time, for only online cpus. > > Backend was not allocating enough structure for all possible CPUs, so > new CPUs coming online could not be hardware enabled. > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index e27b7a9..2cd8bc2 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1716,9 +1716,6 @@ static int kvm_cpu_hotplug(struct notifier_block *notifier, unsigned long val, > { > int cpu = (long)v; > > - if (!kvm_usage_count) > - return NOTIFY_OK; > - > val&= ~CPU_TASKS_FROZEN; > switch (val) { > case CPU_DYING: > I still don't see how this bit can work. Maybe if we move the notification registration to the point where kvm_usage_count is bumped. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2: kvm 4/4] Fix hotplug of CPUs for KVM. 2009-09-29 8:30 ` Avi Kivity @ 2009-09-29 15:51 ` Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden 1 sibling, 0 replies; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 15:51 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/28/2009 10:30 PM, Avi Kivity wrote: > On 09/29/2009 06:04 AM, Zachary Amsden wrote: >> Both VMX and SVM require per-cpu memory allocation, which is done at >> module >> init time, for only online cpus. >> >> Backend was not allocating enough structure for all possible CPUs, so >> new CPUs coming online could not be hardware enabled. >> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index e27b7a9..2cd8bc2 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -1716,9 +1716,6 @@ static int kvm_cpu_hotplug(struct >> notifier_block *notifier, unsigned long val, >> { >> int cpu = (long)v; >> >> - if (!kvm_usage_count) >> - return NOTIFY_OK; >> - >> val&= ~CPU_TASKS_FROZEN; >> switch (val) { >> case CPU_DYING: > > I still don't see how this bit can work. Maybe if we move the > notification registration to the point where kvm_usage_count is bumped. That was stray junk in the patch. Let me rediff... ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-29 8:30 ` Avi Kivity 2009-09-29 15:51 ` Zachary Amsden @ 2009-09-29 21:38 ` Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden 2009-09-30 8:45 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Avi Kivity 1 sibling, 2 replies; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 21:38 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/x86.c | 23 +++++++++++++++-------- 1 files changed, 15 insertions(+), 8 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index fedac9d..15d2ace 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3116,9 +3116,22 @@ static struct notifier_block kvmclock_cpufreq_notifier_block = { .notifier_call = kvmclock_cpufreq_notifier }; +static void kvm_timer_init(void) +{ + int cpu; + + for_each_possible_cpu(cpu) + per_cpu(cpu_tsc_khz, cpu) = tsc_khz; + if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { + tsc_khz_ref = tsc_khz; + cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, + CPUFREQ_TRANSITION_NOTIFIER); + } +} + int kvm_arch_init(void *opaque) { - int r, cpu; + int r; struct kvm_x86_ops *ops = (struct kvm_x86_ops *)opaque; if (kvm_x86_ops) { @@ -3150,13 +3163,7 @@ int kvm_arch_init(void *opaque) kvm_mmu_set_mask_ptes(PT_USER_MASK, PT_ACCESSED_MASK, PT_DIRTY_MASK, PT64_NX_MASK, 0); - for_each_possible_cpu(cpu) - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; - if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { - tsc_khz_ref = tsc_khz; - cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, - CPUFREQ_TRANSITION_NOTIFIER); - } + kvm_timer_init(); return 0; -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-09-29 21:38 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden @ 2009-09-29 21:38 ` Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 3/4] Fix printk name error in svm.c Zachary Amsden 2009-10-08 23:18 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Jan Kiszka 2009-09-30 8:45 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Avi Kivity 1 sibling, 2 replies; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 21:38 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti They are globals, not clearly protected by any ordering or locking, and vulnerable to various startup races. Instead, for variable TSC machines, register the cpufreq notifier and get the TSC frequency directly from the cpufreq machinery. Not only is it always right, it is also perfectly accurate, as no error prone measurement is required. On such machines, when a new CPU online is brought online, it isn't clear what frequency it will start with, and it may not correspond to the reference, thus in hardware_enable we clear the cpu_tsc_khz variable to zero and make sure it is set before running on a VCPU. Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- 1 files changed, 16 insertions(+), 10 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 15d2ace..de4ce8f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1326,6 +1326,8 @@ out: void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) { kvm_x86_ops->vcpu_load(vcpu, cpu); + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); kvm_request_guest_time_update(vcpu); } @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) /* nothing */ } -static unsigned int ref_freq; -static unsigned long tsc_khz_ref; - static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long val, void *data) { @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va struct kvm_vcpu *vcpu; int i, send_ipi = 0; - if (!ref_freq) - ref_freq = freq->old; - if (val == CPUFREQ_PRECHANGE && freq->old > freq->new) return 0; if (val == CPUFREQ_POSTCHANGE && freq->old < freq->new) return 0; - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, ref_freq, freq->new); + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; spin_lock(&kvm_lock); list_for_each_entry(kvm, &vm_list, vm_list) { @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) { int cpu; - for_each_possible_cpu(cpu) - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { - tsc_khz_ref = tsc_khz; cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, CPUFREQ_TRANSITION_NOTIFIER); + for_each_online_cpu(cpu) + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); + } else { + for_each_possible_cpu(cpu) + per_cpu(cpu_tsc_khz, cpu) = tsc_khz; } } @@ -4698,6 +4696,14 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) int kvm_arch_hardware_enable(void *garbage) { + /* + * Since this may be called from a hotplug notifcation, + * we can't get the CPU frequency directly. + */ + if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { + int cpu = raw_smp_processor_id(); + per_cpu(cpu_tsc_khz, cpu) = 0; + } return kvm_x86_ops->hardware_enable(garbage); } -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4: kvm 3/4] Fix printk name error in svm.c 2009-09-29 21:38 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden @ 2009-09-29 21:38 ` Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 4/4] Fix hotplug of CPUs for KVM Zachary Amsden 2009-10-08 23:18 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Jan Kiszka 1 sibling, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 21:38 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/svm.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 9a4daca..d1036ce 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -330,13 +330,14 @@ static int svm_hardware_enable(void *garbage) return -EBUSY; if (!has_svm()) { - printk(KERN_ERR "svm_cpu_init: err EOPNOTSUPP on %d\n", me); + printk(KERN_ERR "svm_hardware_enable: err EOPNOTSUPP on %d\n", + me); return -EINVAL; } svm_data = per_cpu(svm_data, me); if (!svm_data) { - printk(KERN_ERR "svm_cpu_init: svm_data is NULL on %d\n", + printk(KERN_ERR "svm_hardware_enable: svm_data is NULL on %d\n", me); return -EINVAL; } -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4: kvm 4/4] Fix hotplug of CPUs for KVM. 2009-09-29 21:38 ` [PATCH v4: kvm 3/4] Fix printk name error in svm.c Zachary Amsden @ 2009-09-29 21:38 ` Zachary Amsden 2009-09-30 13:35 ` Marcelo Tosatti 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-29 21:38 UTC (permalink / raw) To: kvm; +Cc: linux-kernel, Zachary Amsden, Avi Kivity, Marcelo Tosatti Both VMX and SVM require per-cpu memory allocation, which is done at module init time, for only online cpus. Backend was not allocating enough structure for all possible CPUs, so new CPUs coming online could not be hardware enabled. Signed-off-by: Zachary Amsden <zamsden@redhat.com> --- arch/x86/kvm/svm.c | 4 ++-- arch/x86/kvm/vmx.c | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index d1036ce..02a4269 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -482,7 +482,7 @@ static __init int svm_hardware_setup(void) kvm_enable_efer_bits(EFER_SVME); } - for_each_online_cpu(cpu) { + for_each_possible_cpu(cpu) { r = svm_cpu_init(cpu); if (r) goto err; @@ -516,7 +516,7 @@ static __exit void svm_hardware_unsetup(void) { int cpu; - for_each_online_cpu(cpu) + for_each_possible_cpu(cpu) svm_cpu_uninit(cpu); __free_pages(pfn_to_page(iopm_base >> PAGE_SHIFT), IOPM_ALLOC_ORDER); diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 3fe0d42..e86f1a6 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1350,15 +1350,17 @@ static void free_kvm_area(void) { int cpu; - for_each_online_cpu(cpu) + for_each_possible_cpu(cpu) { free_vmcs(per_cpu(vmxarea, cpu)); + per_cpu(vmxarea, cpu) = NULL; + } } static __init int alloc_kvm_area(void) { int cpu; - for_each_online_cpu(cpu) { + for_each_possible_cpu(cpu) { struct vmcs *vmcs; vmcs = alloc_vmcs_cpu(cpu); -- 1.6.4.4 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 4/4] Fix hotplug of CPUs for KVM. 2009-09-29 21:38 ` [PATCH v4: kvm 4/4] Fix hotplug of CPUs for KVM Zachary Amsden @ 2009-09-30 13:35 ` Marcelo Tosatti 0 siblings, 0 replies; 30+ messages in thread From: Marcelo Tosatti @ 2009-09-30 13:35 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Avi Kivity On Tue, Sep 29, 2009 at 11:38:37AM -1000, Zachary Amsden wrote: > Both VMX and SVM require per-cpu memory allocation, which is done at module > init time, for only online cpus. > > Backend was not allocating enough structure for all possible CPUs, so > new CPUs coming online could not be hardware enabled. > > Signed-off-by: Zachary Amsden <zamsden@redhat.com> Applied all, thanks. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-09-29 21:38 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 3/4] Fix printk name error in svm.c Zachary Amsden @ 2009-10-08 23:18 ` Jan Kiszka 2009-10-09 20:27 ` Zachary Amsden 1 sibling, 1 reply; 30+ messages in thread From: Jan Kiszka @ 2009-10-08 23:18 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Avi Kivity, Marcelo Tosatti [-- Attachment #1: Type: text/plain, Size: 2661 bytes --] Zachary Amsden wrote: > They are globals, not clearly protected by any ordering or locking, and > vulnerable to various startup races. > > Instead, for variable TSC machines, register the cpufreq notifier and get > the TSC frequency directly from the cpufreq machinery. Not only is it > always right, it is also perfectly accurate, as no error prone measurement > is required. > > On such machines, when a new CPU online is brought online, it isn't clear what > frequency it will start with, and it may not correspond to the reference, thus > in hardware_enable we clear the cpu_tsc_khz variable to zero and make sure > it is set before running on a VCPU. > > Signed-off-by: Zachary Amsden <zamsden@redhat.com> > --- > arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- > 1 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 15d2ace..de4ce8f 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1326,6 +1326,8 @@ out: > void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > { > kvm_x86_ops->vcpu_load(vcpu, cpu); > + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) > + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); > kvm_request_guest_time_update(vcpu); > } > > @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) > /* nothing */ > } > > -static unsigned int ref_freq; > -static unsigned long tsc_khz_ref; > - > static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long val, > void *data) > { > @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va > struct kvm_vcpu *vcpu; > int i, send_ipi = 0; > > - if (!ref_freq) > - ref_freq = freq->old; > - > if (val == CPUFREQ_PRECHANGE && freq->old > freq->new) > return 0; > if (val == CPUFREQ_POSTCHANGE && freq->old < freq->new) > return 0; > - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, ref_freq, freq->new); > + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; > > spin_lock(&kvm_lock); > list_for_each_entry(kvm, &vm_list, vm_list) { > @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) > { > int cpu; > > - for_each_possible_cpu(cpu) > - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; > if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { > - tsc_khz_ref = tsc_khz; > cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, > CPUFREQ_TRANSITION_NOTIFIER); > + for_each_online_cpu(cpu) > + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); This doesn't build for !CONFIG_CPU_FREQ. Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-10-08 23:18 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Jan Kiszka @ 2009-10-09 20:27 ` Zachary Amsden 2009-10-09 20:36 ` Jan Kiszka 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-10-09 20:27 UTC (permalink / raw) To: Jan Kiszka; +Cc: kvm, linux-kernel, Avi Kivity, Marcelo Tosatti On 10/08/2009 01:18 PM, Jan Kiszka wrote: > Zachary Amsden wrote: > >> They are globals, not clearly protected by any ordering or locking, and >> vulnerable to various startup races. >> >> Instead, for variable TSC machines, register the cpufreq notifier and get >> the TSC frequency directly from the cpufreq machinery. Not only is it >> always right, it is also perfectly accurate, as no error prone measurement >> is required. >> >> On such machines, when a new CPU online is brought online, it isn't clear what >> frequency it will start with, and it may not correspond to the reference, thus >> in hardware_enable we clear the cpu_tsc_khz variable to zero and make sure >> it is set before running on a VCPU. >> >> Signed-off-by: Zachary Amsden<zamsden@redhat.com> >> --- >> arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- >> 1 files changed, 16 insertions(+), 10 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 15d2ace..de4ce8f 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -1326,6 +1326,8 @@ out: >> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >> { >> kvm_x86_ops->vcpu_load(vcpu, cpu); >> + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) >> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); >> kvm_request_guest_time_update(vcpu); >> } >> >> @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) >> /* nothing */ >> } >> >> -static unsigned int ref_freq; >> -static unsigned long tsc_khz_ref; >> - >> static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long val, >> void *data) >> { >> @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct notifier_block *nb, unsigned long va >> struct kvm_vcpu *vcpu; >> int i, send_ipi = 0; >> >> - if (!ref_freq) >> - ref_freq = freq->old; >> - >> if (val == CPUFREQ_PRECHANGE&& freq->old> freq->new) >> return 0; >> if (val == CPUFREQ_POSTCHANGE&& freq->old< freq->new) >> return 0; >> - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, ref_freq, freq->new); >> + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; >> >> spin_lock(&kvm_lock); >> list_for_each_entry(kvm,&vm_list, vm_list) { >> @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) >> { >> int cpu; >> >> - for_each_possible_cpu(cpu) >> - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; >> if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { >> - tsc_khz_ref = tsc_khz; >> cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, >> CPUFREQ_TRANSITION_NOTIFIER); >> + for_each_online_cpu(cpu) >> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); >> > This doesn't build for !CONFIG_CPU_FREQ. > And did it before? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-10-09 20:27 ` Zachary Amsden @ 2009-10-09 20:36 ` Jan Kiszka 2009-10-09 20:36 ` Zachary Amsden 0 siblings, 1 reply; 30+ messages in thread From: Jan Kiszka @ 2009-10-09 20:36 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Avi Kivity, Marcelo Tosatti [-- Attachment #1: Type: text/plain, Size: 3284 bytes --] Zachary Amsden wrote: > On 10/08/2009 01:18 PM, Jan Kiszka wrote: >> Zachary Amsden wrote: >> >>> They are globals, not clearly protected by any ordering or locking, and >>> vulnerable to various startup races. >>> >>> Instead, for variable TSC machines, register the cpufreq notifier and >>> get >>> the TSC frequency directly from the cpufreq machinery. Not only is it >>> always right, it is also perfectly accurate, as no error prone >>> measurement >>> is required. >>> >>> On such machines, when a new CPU online is brought online, it isn't >>> clear what >>> frequency it will start with, and it may not correspond to the >>> reference, thus >>> in hardware_enable we clear the cpu_tsc_khz variable to zero and make >>> sure >>> it is set before running on a VCPU. >>> >>> Signed-off-by: Zachary Amsden<zamsden@redhat.com> >>> --- >>> arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- >>> 1 files changed, 16 insertions(+), 10 deletions(-) >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 15d2ace..de4ce8f 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -1326,6 +1326,8 @@ out: >>> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >>> { >>> kvm_x86_ops->vcpu_load(vcpu, cpu); >>> + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) >>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); >>> kvm_request_guest_time_update(vcpu); >>> } >>> >>> @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) >>> /* nothing */ >>> } >>> >>> -static unsigned int ref_freq; >>> -static unsigned long tsc_khz_ref; >>> - >>> static int kvmclock_cpufreq_notifier(struct notifier_block *nb, >>> unsigned long val, >>> void *data) >>> { >>> @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct >>> notifier_block *nb, unsigned long va >>> struct kvm_vcpu *vcpu; >>> int i, send_ipi = 0; >>> >>> - if (!ref_freq) >>> - ref_freq = freq->old; >>> - >>> if (val == CPUFREQ_PRECHANGE&& freq->old> freq->new) >>> return 0; >>> if (val == CPUFREQ_POSTCHANGE&& freq->old< freq->new) >>> return 0; >>> - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, >>> ref_freq, freq->new); >>> + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; >>> >>> spin_lock(&kvm_lock); >>> list_for_each_entry(kvm,&vm_list, vm_list) { >>> @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) >>> { >>> int cpu; >>> >>> - for_each_possible_cpu(cpu) >>> - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; >>> if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { >>> - tsc_khz_ref = tsc_khz; >>> cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, >>> CPUFREQ_TRANSITION_NOTIFIER); >>> + for_each_online_cpu(cpu) >>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); >>> >> This doesn't build for !CONFIG_CPU_FREQ. >> > > And did it before? Yes, because cpufreq_get, which is undefined without CONFIG_CPU_FREQ, did not exist so far. One may argue that this is a deficit of the cpufreq API. However, it needs fixing. Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-10-09 20:36 ` Jan Kiszka @ 2009-10-09 20:36 ` Zachary Amsden 2009-10-09 20:47 ` Jan Kiszka 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-10-09 20:36 UTC (permalink / raw) To: Jan Kiszka; +Cc: kvm, linux-kernel, Avi Kivity, Marcelo Tosatti On 10/09/2009 10:36 AM, Jan Kiszka wrote: > Zachary Amsden wrote: > >> On 10/08/2009 01:18 PM, Jan Kiszka wrote: >> >>> Zachary Amsden wrote: >>> >>> >>>> They are globals, not clearly protected by any ordering or locking, and >>>> vulnerable to various startup races. >>>> >>>> Instead, for variable TSC machines, register the cpufreq notifier and >>>> get >>>> the TSC frequency directly from the cpufreq machinery. Not only is it >>>> always right, it is also perfectly accurate, as no error prone >>>> measurement >>>> is required. >>>> >>>> On such machines, when a new CPU online is brought online, it isn't >>>> clear what >>>> frequency it will start with, and it may not correspond to the >>>> reference, thus >>>> in hardware_enable we clear the cpu_tsc_khz variable to zero and make >>>> sure >>>> it is set before running on a VCPU. >>>> >>>> Signed-off-by: Zachary Amsden<zamsden@redhat.com> >>>> --- >>>> arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- >>>> 1 files changed, 16 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>> index 15d2ace..de4ce8f 100644 >>>> --- a/arch/x86/kvm/x86.c >>>> +++ b/arch/x86/kvm/x86.c >>>> @@ -1326,6 +1326,8 @@ out: >>>> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >>>> { >>>> kvm_x86_ops->vcpu_load(vcpu, cpu); >>>> + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) >>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); >>>> kvm_request_guest_time_update(vcpu); >>>> } >>>> >>>> @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) >>>> /* nothing */ >>>> } >>>> >>>> -static unsigned int ref_freq; >>>> -static unsigned long tsc_khz_ref; >>>> - >>>> static int kvmclock_cpufreq_notifier(struct notifier_block *nb, >>>> unsigned long val, >>>> void *data) >>>> { >>>> @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct >>>> notifier_block *nb, unsigned long va >>>> struct kvm_vcpu *vcpu; >>>> int i, send_ipi = 0; >>>> >>>> - if (!ref_freq) >>>> - ref_freq = freq->old; >>>> - >>>> if (val == CPUFREQ_PRECHANGE&& freq->old> freq->new) >>>> return 0; >>>> if (val == CPUFREQ_POSTCHANGE&& freq->old< freq->new) >>>> return 0; >>>> - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, >>>> ref_freq, freq->new); >>>> + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; >>>> >>>> spin_lock(&kvm_lock); >>>> list_for_each_entry(kvm,&vm_list, vm_list) { >>>> @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) >>>> { >>>> int cpu; >>>> >>>> - for_each_possible_cpu(cpu) >>>> - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; >>>> if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { >>>> - tsc_khz_ref = tsc_khz; >>>> cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, >>>> CPUFREQ_TRANSITION_NOTIFIER); >>>> + for_each_online_cpu(cpu) >>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); >>>> >>>> >>> This doesn't build for !CONFIG_CPU_FREQ. >>> >>> >> And did it before? >> > Yes, because cpufreq_get, which is undefined without CONFIG_CPU_FREQ, > did not exist so far. One may argue that this is a deficit of the > cpufreq API. However, it needs fixing. > I'll send a patch today. I'm going with API deficient and will fix that, but this means I will also require a fix for kvm-kmod :( Zach ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-10-09 20:36 ` Zachary Amsden @ 2009-10-09 20:47 ` Jan Kiszka 2009-10-09 20:47 ` Zachary Amsden 2009-10-10 2:26 ` [PATCH] cpufreq: Make cpufreq_get always defined Zachary Amsden 0 siblings, 2 replies; 30+ messages in thread From: Jan Kiszka @ 2009-10-09 20:47 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Avi Kivity, Marcelo Tosatti [-- Attachment #1: Type: text/plain, Size: 3875 bytes --] Zachary Amsden wrote: > On 10/09/2009 10:36 AM, Jan Kiszka wrote: >> Zachary Amsden wrote: >> >>> On 10/08/2009 01:18 PM, Jan Kiszka wrote: >>> >>>> Zachary Amsden wrote: >>>> >>>> >>>>> They are globals, not clearly protected by any ordering or locking, >>>>> and >>>>> vulnerable to various startup races. >>>>> >>>>> Instead, for variable TSC machines, register the cpufreq notifier and >>>>> get >>>>> the TSC frequency directly from the cpufreq machinery. Not only is it >>>>> always right, it is also perfectly accurate, as no error prone >>>>> measurement >>>>> is required. >>>>> >>>>> On such machines, when a new CPU online is brought online, it isn't >>>>> clear what >>>>> frequency it will start with, and it may not correspond to the >>>>> reference, thus >>>>> in hardware_enable we clear the cpu_tsc_khz variable to zero and make >>>>> sure >>>>> it is set before running on a VCPU. >>>>> >>>>> Signed-off-by: Zachary Amsden<zamsden@redhat.com> >>>>> --- >>>>> arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- >>>>> 1 files changed, 16 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>>> index 15d2ace..de4ce8f 100644 >>>>> --- a/arch/x86/kvm/x86.c >>>>> +++ b/arch/x86/kvm/x86.c >>>>> @@ -1326,6 +1326,8 @@ out: >>>>> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >>>>> { >>>>> kvm_x86_ops->vcpu_load(vcpu, cpu); >>>>> + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) >>>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); >>>>> kvm_request_guest_time_update(vcpu); >>>>> } >>>>> >>>>> @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) >>>>> /* nothing */ >>>>> } >>>>> >>>>> -static unsigned int ref_freq; >>>>> -static unsigned long tsc_khz_ref; >>>>> - >>>>> static int kvmclock_cpufreq_notifier(struct notifier_block *nb, >>>>> unsigned long val, >>>>> void *data) >>>>> { >>>>> @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct >>>>> notifier_block *nb, unsigned long va >>>>> struct kvm_vcpu *vcpu; >>>>> int i, send_ipi = 0; >>>>> >>>>> - if (!ref_freq) >>>>> - ref_freq = freq->old; >>>>> - >>>>> if (val == CPUFREQ_PRECHANGE&& freq->old> freq->new) >>>>> return 0; >>>>> if (val == CPUFREQ_POSTCHANGE&& freq->old< freq->new) >>>>> return 0; >>>>> - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, >>>>> ref_freq, freq->new); >>>>> + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; >>>>> >>>>> spin_lock(&kvm_lock); >>>>> list_for_each_entry(kvm,&vm_list, vm_list) { >>>>> @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) >>>>> { >>>>> int cpu; >>>>> >>>>> - for_each_possible_cpu(cpu) >>>>> - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; >>>>> if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { >>>>> - tsc_khz_ref = tsc_khz; >>>>> cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, >>>>> CPUFREQ_TRANSITION_NOTIFIER); >>>>> + for_each_online_cpu(cpu) >>>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); >>>>> >>>>> >>>> This doesn't build for !CONFIG_CPU_FREQ. >>>> >>>> >>> And did it before? >>> >> Yes, because cpufreq_get, which is undefined without CONFIG_CPU_FREQ, >> did not exist so far. One may argue that this is a deficit of the >> cpufreq API. However, it needs fixing. >> > > I'll send a patch today. I'm going with API deficient and will fix > that, but this means I will also require a fix for kvm-kmod :( That won't be tricky, I will queue up a fix. BTW, is the KVM prepared for cpufreq_get returning 0? Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-10-09 20:47 ` Jan Kiszka @ 2009-10-09 20:47 ` Zachary Amsden 2009-10-09 21:05 ` Jan Kiszka 2009-10-10 2:26 ` [PATCH] cpufreq: Make cpufreq_get always defined Zachary Amsden 1 sibling, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-10-09 20:47 UTC (permalink / raw) To: Jan Kiszka; +Cc: kvm, linux-kernel, Avi Kivity, Marcelo Tosatti On 10/09/2009 10:47 AM, Jan Kiszka wrote: > Zachary Amsden wrote: > >> On 10/09/2009 10:36 AM, Jan Kiszka wrote: >> >>> Zachary Amsden wrote: >>> >>> >>>> On 10/08/2009 01:18 PM, Jan Kiszka wrote: >>>> >>>> >>>>> Zachary Amsden wrote: >>>>> >>>>> >>>>> >>>>>> They are globals, not clearly protected by any ordering or locking, >>>>>> and >>>>>> vulnerable to various startup races. >>>>>> >>>>>> Instead, for variable TSC machines, register the cpufreq notifier and >>>>>> get >>>>>> the TSC frequency directly from the cpufreq machinery. Not only is it >>>>>> always right, it is also perfectly accurate, as no error prone >>>>>> measurement >>>>>> is required. >>>>>> >>>>>> On such machines, when a new CPU online is brought online, it isn't >>>>>> clear what >>>>>> frequency it will start with, and it may not correspond to the >>>>>> reference, thus >>>>>> in hardware_enable we clear the cpu_tsc_khz variable to zero and make >>>>>> sure >>>>>> it is set before running on a VCPU. >>>>>> >>>>>> Signed-off-by: Zachary Amsden<zamsden@redhat.com> >>>>>> --- >>>>>> arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- >>>>>> 1 files changed, 16 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>>>> index 15d2ace..de4ce8f 100644 >>>>>> --- a/arch/x86/kvm/x86.c >>>>>> +++ b/arch/x86/kvm/x86.c >>>>>> @@ -1326,6 +1326,8 @@ out: >>>>>> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >>>>>> { >>>>>> kvm_x86_ops->vcpu_load(vcpu, cpu); >>>>>> + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) >>>>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); >>>>>> kvm_request_guest_time_update(vcpu); >>>>>> } >>>>>> >>>>>> @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) >>>>>> /* nothing */ >>>>>> } >>>>>> >>>>>> -static unsigned int ref_freq; >>>>>> -static unsigned long tsc_khz_ref; >>>>>> - >>>>>> static int kvmclock_cpufreq_notifier(struct notifier_block *nb, >>>>>> unsigned long val, >>>>>> void *data) >>>>>> { >>>>>> @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct >>>>>> notifier_block *nb, unsigned long va >>>>>> struct kvm_vcpu *vcpu; >>>>>> int i, send_ipi = 0; >>>>>> >>>>>> - if (!ref_freq) >>>>>> - ref_freq = freq->old; >>>>>> - >>>>>> if (val == CPUFREQ_PRECHANGE&& freq->old> freq->new) >>>>>> return 0; >>>>>> if (val == CPUFREQ_POSTCHANGE&& freq->old< freq->new) >>>>>> return 0; >>>>>> - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, >>>>>> ref_freq, freq->new); >>>>>> + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; >>>>>> >>>>>> spin_lock(&kvm_lock); >>>>>> list_for_each_entry(kvm,&vm_list, vm_list) { >>>>>> @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) >>>>>> { >>>>>> int cpu; >>>>>> >>>>>> - for_each_possible_cpu(cpu) >>>>>> - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; >>>>>> if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { >>>>>> - tsc_khz_ref = tsc_khz; >>>>>> cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, >>>>>> CPUFREQ_TRANSITION_NOTIFIER); >>>>>> + for_each_online_cpu(cpu) >>>>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); >>>>>> >>>>>> >>>>>> >>>>> This doesn't build for !CONFIG_CPU_FREQ. >>>>> >>>>> >>>>> >>>> And did it before? >>>> >>>> >>> Yes, because cpufreq_get, which is undefined without CONFIG_CPU_FREQ, >>> did not exist so far. One may argue that this is a deficit of the >>> cpufreq API. However, it needs fixing. >>> >>> >> I'll send a patch today. I'm going with API deficient and will fix >> that, but this means I will also require a fix for kvm-kmod :( >> > That won't be tricky, I will queue up a fix. > > BTW, is the KVM prepared for cpufreq_get returning 0? > That will be part of the fix, I think. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-10-09 20:47 ` Zachary Amsden @ 2009-10-09 21:05 ` Jan Kiszka 0 siblings, 0 replies; 30+ messages in thread From: Jan Kiszka @ 2009-10-09 21:05 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Avi Kivity, Marcelo Tosatti [-- Attachment #1: Type: text/plain, Size: 4980 bytes --] Zachary Amsden wrote: > On 10/09/2009 10:47 AM, Jan Kiszka wrote: >> Zachary Amsden wrote: >> >>> On 10/09/2009 10:36 AM, Jan Kiszka wrote: >>> >>>> Zachary Amsden wrote: >>>> >>>> >>>>> On 10/08/2009 01:18 PM, Jan Kiszka wrote: >>>>> >>>>> >>>>>> Zachary Amsden wrote: >>>>>> >>>>>> >>>>>> >>>>>>> They are globals, not clearly protected by any ordering or locking, >>>>>>> and >>>>>>> vulnerable to various startup races. >>>>>>> >>>>>>> Instead, for variable TSC machines, register the cpufreq notifier >>>>>>> and >>>>>>> get >>>>>>> the TSC frequency directly from the cpufreq machinery. Not only >>>>>>> is it >>>>>>> always right, it is also perfectly accurate, as no error prone >>>>>>> measurement >>>>>>> is required. >>>>>>> >>>>>>> On such machines, when a new CPU online is brought online, it isn't >>>>>>> clear what >>>>>>> frequency it will start with, and it may not correspond to the >>>>>>> reference, thus >>>>>>> in hardware_enable we clear the cpu_tsc_khz variable to zero and >>>>>>> make >>>>>>> sure >>>>>>> it is set before running on a VCPU. >>>>>>> >>>>>>> Signed-off-by: Zachary Amsden<zamsden@redhat.com> >>>>>>> --- >>>>>>> arch/x86/kvm/x86.c | 26 ++++++++++++++++---------- >>>>>>> 1 files changed, 16 insertions(+), 10 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>>>>> index 15d2ace..de4ce8f 100644 >>>>>>> --- a/arch/x86/kvm/x86.c >>>>>>> +++ b/arch/x86/kvm/x86.c >>>>>>> @@ -1326,6 +1326,8 @@ out: >>>>>>> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >>>>>>> { >>>>>>> kvm_x86_ops->vcpu_load(vcpu, cpu); >>>>>>> + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) >>>>>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); >>>>>>> kvm_request_guest_time_update(vcpu); >>>>>>> } >>>>>>> >>>>>>> @@ -3061,9 +3063,6 @@ static void bounce_off(void *info) >>>>>>> /* nothing */ >>>>>>> } >>>>>>> >>>>>>> -static unsigned int ref_freq; >>>>>>> -static unsigned long tsc_khz_ref; >>>>>>> - >>>>>>> static int kvmclock_cpufreq_notifier(struct notifier_block *nb, >>>>>>> unsigned long val, >>>>>>> void *data) >>>>>>> { >>>>>>> @@ -3072,14 +3071,11 @@ static int kvmclock_cpufreq_notifier(struct >>>>>>> notifier_block *nb, unsigned long va >>>>>>> struct kvm_vcpu *vcpu; >>>>>>> int i, send_ipi = 0; >>>>>>> >>>>>>> - if (!ref_freq) >>>>>>> - ref_freq = freq->old; >>>>>>> - >>>>>>> if (val == CPUFREQ_PRECHANGE&& freq->old> freq->new) >>>>>>> return 0; >>>>>>> if (val == CPUFREQ_POSTCHANGE&& freq->old< freq->new) >>>>>>> return 0; >>>>>>> - per_cpu(cpu_tsc_khz, freq->cpu) = cpufreq_scale(tsc_khz_ref, >>>>>>> ref_freq, freq->new); >>>>>>> + per_cpu(cpu_tsc_khz, freq->cpu) = freq->new; >>>>>>> >>>>>>> spin_lock(&kvm_lock); >>>>>>> list_for_each_entry(kvm,&vm_list, vm_list) { >>>>>>> @@ -3120,12 +3116,14 @@ static void kvm_timer_init(void) >>>>>>> { >>>>>>> int cpu; >>>>>>> >>>>>>> - for_each_possible_cpu(cpu) >>>>>>> - per_cpu(cpu_tsc_khz, cpu) = tsc_khz; >>>>>>> if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { >>>>>>> - tsc_khz_ref = tsc_khz; >>>>>>> >>>>>>> cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, >>>>>>> CPUFREQ_TRANSITION_NOTIFIER); >>>>>>> + for_each_online_cpu(cpu) >>>>>>> + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); >>>>>>> >>>>>>> >>>>>>> >>>>>> This doesn't build for !CONFIG_CPU_FREQ. >>>>>> >>>>>> >>>>>> >>>>> And did it before? >>>>> >>>>> >>>> Yes, because cpufreq_get, which is undefined without CONFIG_CPU_FREQ, >>>> did not exist so far. One may argue that this is a deficit of the >>>> cpufreq API. However, it needs fixing. >>>> >>>> >>> I'll send a patch today. I'm going with API deficient and will fix >>> that, but this means I will also require a fix for kvm-kmod :( >>> >> That won't be tricky, I will queue up a fix. >> >> BTW, is the KVM prepared for cpufreq_get returning 0? >> > > That will be part of the fix, I think. > That's good, specifically as it looks like cpufreq_get can return 0 even in the CONFIG_CPU_FREQ case, at least theoretically. Jan PS: diff --git a/external-module-compat-comm.h b/external-module-compat-comm.h index 47fdc86..de8ab23 100644 --- a/external-module-compat-comm.h +++ b/external-module-compat-comm.h @@ -993,3 +993,10 @@ unsigned long kvm_vma_kernel_pagesize(struct vm_area_struct *vma) } \ }) #endif + +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,33) && !defined(CONFIG_CPU_FREQ) +static inline unsigned int cpufreq_get(unsigned int cpu) +{ + return 0; +} +#endif [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 257 bytes --] ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH] cpufreq: Make cpufreq_get always defined 2009-10-09 20:47 ` Jan Kiszka 2009-10-09 20:47 ` Zachary Amsden @ 2009-10-10 2:26 ` Zachary Amsden 2009-10-10 2:26 ` [PATCH] KVM: Harden against cpufreq Zachary Amsden 1 sibling, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-10-10 2:26 UTC (permalink / raw) To: kvm, avi; +Cc: Zachary Amsden, linux-kernel, torvalds, Dave Jones, cpufreq Even if not using cpufreq, there is little justification for exporting cpufreq_quick_get and not cpufreq_get, especially when it is so easy and harmless to do. Signed-off-by: Zachary Amsden <zamsden@redhat.com> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 44717eb..684fc4b 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -291,17 +291,22 @@ struct global_attr { int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu); int cpufreq_update_policy(unsigned int cpu); -/* query the current CPU frequency (in kHz). If zero, cpufreq couldn't detect it */ -unsigned int cpufreq_get(unsigned int cpu); - -/* query the last known CPU freq (in kHz). If zero, cpufreq couldn't detect it */ #ifdef CONFIG_CPU_FREQ +/* query the last known CPU freq (in kHz). If zero, cpufreq doesn't know it */ unsigned int cpufreq_quick_get(unsigned int cpu); + +/* query the current CPU frequency (in kHz). If zero, cpufreq doesn't know it */ +unsigned int cpufreq_get(unsigned int cpu); #else static inline unsigned int cpufreq_quick_get(unsigned int cpu) { return 0; } + +static inline unsigned int cpufreq_get(unsigned int cpu) +{ + return 0; +} #endif ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH] KVM: Harden against cpufreq 2009-10-10 2:26 ` [PATCH] cpufreq: Make cpufreq_get always defined Zachary Amsden @ 2009-10-10 2:26 ` Zachary Amsden 2009-10-10 2:26 ` [PATCH] kvm-kmod cpufreq_get fix Zachary Amsden 2009-10-12 19:41 ` [PATCH] KVM: Harden against cpufreq Marcelo Tosatti 0 siblings, 2 replies; 30+ messages in thread From: Zachary Amsden @ 2009-10-10 2:26 UTC (permalink / raw) To: kvm, avi; +Cc: Zachary Amsden, linux-kernel If cpufreq can't determine the CPU khz, or cpufreq is not compiled in, we should fallback to the measured TSC khz. Signed-off-by: Zachary Amsden <zamsden@redhat.com> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 11a6f2f..6604f4c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1345,8 +1345,12 @@ out: void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) { kvm_x86_ops->vcpu_load(vcpu, cpu); - if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) - per_cpu(cpu_tsc_khz, cpu) = cpufreq_quick_get(cpu); + if (unlikely(per_cpu(cpu_tsc_khz, cpu) == 0)) { + unsigned long khz = cpufreq_quick_get(cpu); + if (!khz) + khz = tsc_khz; + per_cpu(cpu_tsc_khz, cpu) = khz; + } kvm_request_guest_time_update(vcpu); } @@ -3140,8 +3144,12 @@ static void kvm_timer_init(void) if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) { cpufreq_register_notifier(&kvmclock_cpufreq_notifier_block, CPUFREQ_TRANSITION_NOTIFIER); - for_each_online_cpu(cpu) - per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); + for_each_online_cpu(cpu) { + unsigned long khz = cpufreq_get(cpu); + if (!khz) + khz = tsc_khz; + per_cpu(cpu_tsc_khz, cpu) = khz; + } } else { for_each_possible_cpu(cpu) per_cpu(cpu_tsc_khz, cpu) = tsc_khz; ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH] kvm-kmod cpufreq_get fix 2009-10-10 2:26 ` [PATCH] KVM: Harden against cpufreq Zachary Amsden @ 2009-10-10 2:26 ` Zachary Amsden 2009-10-12 19:41 ` [PATCH] KVM: Harden against cpufreq Marcelo Tosatti 1 sibling, 0 replies; 30+ messages in thread From: Zachary Amsden @ 2009-10-10 2:26 UTC (permalink / raw) To: kvm, avi; +Cc: Zachary Amsden, Eduardo Habkost Define cpufreq_get for kernels which don't export it when not config'd. Signed-off-by: Zachary Amsden <zamsden@redhat.com> diff --git a/external-module-compat-comm.h b/external-module-compat-comm.h index 47fdc86..6fe7d87 100644 --- a/external-module-compat-comm.h +++ b/external-module-compat-comm.h @@ -993,3 +993,12 @@ unsigned long kvm_vma_kernel_pagesize(struct vm_area_struct *vma) } \ }) #endif + +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,33) +#ifndef CONFIG_CPU_FREQ +static inline unsigned int cpufreq_get(unsigned int cpu) +{ + return 0; +} +#endif +#endif ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] KVM: Harden against cpufreq 2009-10-10 2:26 ` [PATCH] KVM: Harden against cpufreq Zachary Amsden 2009-10-10 2:26 ` [PATCH] kvm-kmod cpufreq_get fix Zachary Amsden @ 2009-10-12 19:41 ` Marcelo Tosatti 1 sibling, 0 replies; 30+ messages in thread From: Marcelo Tosatti @ 2009-10-12 19:41 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, avi, linux-kernel On Fri, Oct 09, 2009 at 04:26:08PM -1000, Zachary Amsden wrote: > If cpufreq can't determine the CPU khz, or cpufreq is not compiled in, > we should fallback to the measured TSC khz. > > Signed-off-by: Zachary Amsden <zamsden@redhat.com> Applied, thanks. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-29 21:38 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden @ 2009-09-30 8:45 ` Avi Kivity 2009-09-30 15:51 ` Zachary Amsden 1 sibling, 1 reply; 30+ messages in thread From: Avi Kivity @ 2009-09-30 8:45 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/29/2009 11:38 PM, Zachary Amsden wrote: > Signed-off-by: Zachary Amsden<zamsden@redhat.com> > Looks good. Is anything preventing us from unifying the constant_tsc and !same paths? We could just do a quick check in the notifier, see the tsc frequency hasn't changed, and return. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-30 8:45 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Avi Kivity @ 2009-09-30 15:51 ` Zachary Amsden 2009-09-30 15:56 ` Avi Kivity 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-30 15:51 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/29/2009 10:45 PM, Avi Kivity wrote: > On 09/29/2009 11:38 PM, Zachary Amsden wrote: >> Signed-off-by: Zachary Amsden<zamsden@redhat.com> > > > Looks good. > > Is anything preventing us from unifying the constant_tsc and !same > paths? We could just do a quick check in the notifier, see the tsc > frequency hasn't changed, and return. Actually, yes. On constant_tsc processors, the processor frequency may still change, however the TSC frequency does not change with it. I actually have both of these kinds of processors (freq changes with constant TSC and freq changes with variable TSC) so I was able to test both of these cases. Zach ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-30 15:51 ` Zachary Amsden @ 2009-09-30 15:56 ` Avi Kivity 2009-09-30 16:06 ` Zachary Amsden 0 siblings, 1 reply; 30+ messages in thread From: Avi Kivity @ 2009-09-30 15:56 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/30/2009 05:51 PM, Zachary Amsden wrote: >> Is anything preventing us from unifying the constant_tsc and !same >> paths? We could just do a quick check in the notifier, see the tsc >> frequency hasn't changed, and return. > > > Actually, yes. On constant_tsc processors, the processor frequency > may still change, however the TSC frequency does not change with it. > > I actually have both of these kinds of processors (freq changes with > constant TSC and freq changes with variable TSC) so I was able to test > both of these cases. If the API allows us to query the tsc frequency, it would simply return the same values in all cases, which we'd ignore. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-30 15:56 ` Avi Kivity @ 2009-09-30 16:06 ` Zachary Amsden 2009-09-30 16:11 ` Avi Kivity 0 siblings, 1 reply; 30+ messages in thread From: Zachary Amsden @ 2009-09-30 16:06 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/30/2009 05:56 AM, Avi Kivity wrote: > On 09/30/2009 05:51 PM, Zachary Amsden wrote: > > If the API allows us to query the tsc frequency, it would simply > return the same values in all cases, which we'd ignore. The API only allows querying the processor frequency. In the constant_tsc case, the highest processor frequency is likely going to be the actual TSC frequency, but I don't think it's a guarantee; theoretically, it could be faster on normal hardware ... or slower on overclocked hardware with an externally clocked TSC. Zach ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-30 16:06 ` Zachary Amsden @ 2009-09-30 16:11 ` Avi Kivity 2009-09-30 16:16 ` Zachary Amsden 0 siblings, 1 reply; 30+ messages in thread From: Avi Kivity @ 2009-09-30 16:11 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/30/2009 06:06 PM, Zachary Amsden wrote: > On 09/30/2009 05:56 AM, Avi Kivity wrote: >> On 09/30/2009 05:51 PM, Zachary Amsden wrote: >> >> If the API allows us to query the tsc frequency, it would simply >> return the same values in all cases, which we'd ignore. > > The API only allows querying the processor frequency. In the > constant_tsc case, the highest processor frequency is likely going to > be the actual TSC frequency, but I don't think it's a guarantee; > theoretically, it could be faster on normal hardware ... or slower on > overclocked hardware with an externally clocked TSC. Well we could add a new API then (or a new tscfreq notifier). Those conditionals don't belong in client code. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function. 2009-09-30 16:11 ` Avi Kivity @ 2009-09-30 16:16 ` Zachary Amsden 0 siblings, 0 replies; 30+ messages in thread From: Zachary Amsden @ 2009-09-30 16:16 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/30/2009 06:11 AM, Avi Kivity wrote: > On 09/30/2009 06:06 PM, Zachary Amsden wrote: >> On 09/30/2009 05:56 AM, Avi Kivity wrote: >>> On 09/30/2009 05:51 PM, Zachary Amsden wrote: >>> >>> If the API allows us to query the tsc frequency, it would simply >>> return the same values in all cases, which we'd ignore. >> >> The API only allows querying the processor frequency. In the >> constant_tsc case, the highest processor frequency is likely going to >> be the actual TSC frequency, but I don't think it's a guarantee; >> theoretically, it could be faster on normal hardware ... or slower on >> overclocked hardware with an externally clocked TSC. > > Well we could add a new API then (or a new tscfreq notifier). Those > conditionals don't belong in client code. It's possible... but it's also possible to run without cpufreq enabled, which won't work properly unless the cpufreq code is aware of the measured tsc_khz... this could be a little ugly architecture wise given the big melting pot of generic code and vendor / arch specific code here. Since we're already very hardware dependent and one of the few clients who care, it seems okay to leave it as is for now. Zach ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables. 2009-09-29 4:04 ` [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 3/4] Fix printk name error in svm.c Zachary Amsden @ 2009-09-29 8:29 ` Avi Kivity 1 sibling, 0 replies; 30+ messages in thread From: Avi Kivity @ 2009-09-29 8:29 UTC (permalink / raw) To: Zachary Amsden; +Cc: kvm, linux-kernel, Marcelo Tosatti On 09/29/2009 06:04 AM, Zachary Amsden wrote: > They are globals, not clearly protected by any ordering or locking, and > vulnerable to various startup races. > > Instead, for variable TSC machines, register the cpufreq notifier and get > the TSC frequency directly from the cpufreq machinery. Not only is it > always right, it is also perfectly accurate, as no error prone measurement > is required. > > On such machines, when a new CPU online is brought online, it isn't clear what > frequency it will start with, and it may not correspond to the reference, thus > in hardware_enable we clear the cpu_tsc_khz variable to zero and make sure > it is set before running on a VCPU. > > CPUFREQ_TRANSITION_NOTIFIER); > + for_each_online_cpu(cpu) > + per_cpu(cpu_tsc_khz, cpu) = cpufreq_get(cpu); > + } else { > + for_each_possible_cpu(cpu) > + per_cpu(cpu_tsc_khz, cpu) = tsc_khz; > + } > + for_each_possible_cpu(cpu) { > + printk(KERN_DEBUG "kvm: cpu %d = %ld khz\n", > + cpu, per_cpu(cpu_tsc_khz, cpu)); > } > } > Leftover debug code? -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2009-10-12 19:42 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-09-29 4:04 Hotplug / TSC cleanup and fixing Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 3/4] Fix printk name error in svm.c Zachary Amsden 2009-09-29 4:04 ` [PATCH v2: kvm 4/4] Fix hotplug of CPUs for KVM Zachary Amsden 2009-09-29 8:30 ` Avi Kivity 2009-09-29 15:51 ` Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 3/4] Fix printk name error in svm.c Zachary Amsden 2009-09-29 21:38 ` [PATCH v4: kvm 4/4] Fix hotplug of CPUs for KVM Zachary Amsden 2009-09-30 13:35 ` Marcelo Tosatti 2009-10-08 23:18 ` [PATCH v4: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Jan Kiszka 2009-10-09 20:27 ` Zachary Amsden 2009-10-09 20:36 ` Jan Kiszka 2009-10-09 20:36 ` Zachary Amsden 2009-10-09 20:47 ` Jan Kiszka 2009-10-09 20:47 ` Zachary Amsden 2009-10-09 21:05 ` Jan Kiszka 2009-10-10 2:26 ` [PATCH] cpufreq: Make cpufreq_get always defined Zachary Amsden 2009-10-10 2:26 ` [PATCH] KVM: Harden against cpufreq Zachary Amsden 2009-10-10 2:26 ` [PATCH] kvm-kmod cpufreq_get fix Zachary Amsden 2009-10-12 19:41 ` [PATCH] KVM: Harden against cpufreq Marcelo Tosatti 2009-09-30 8:45 ` [PATCH v4: kvm 1/4] Code motion. Separate timer intialization into an indepedent function Avi Kivity 2009-09-30 15:51 ` Zachary Amsden 2009-09-30 15:56 ` Avi Kivity 2009-09-30 16:06 ` Zachary Amsden 2009-09-30 16:11 ` Avi Kivity 2009-09-30 16:16 ` Zachary Amsden 2009-09-29 8:29 ` [PATCH v2: kvm 2/4] Kill the confusing tsc_ref_khz and ref_freq variables Avi Kivity
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).