KVM: VMX: Execute WBINVD to keep data consistency with assigned devices
PATCH v3 1/1 KVM: VMX: Execute WBINVD to keep data consistency with assigned devices
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
---
OK, I've checked AMD's spec, they do intercept WBINVD. So we can do it like this.
arch/x86/include/asm/kvm_host.h | 6 ++++++ arch/x86/kvm/emulate.c | 5 ++++- arch/x86/kvm/svm.c | 7 +++++++ arch/x86/kvm/vmx.c | 10 +++++++++- arch/x86/kvm/x86.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 2 deletions(-)
arch/x86/include/asm/kvm_host.h +6 -0
@@ -15,6 +15,7 @@#include <linux/mm.h> #include <linux/mmu_notifier.h> #include <linux/tracepoint.h> +#include <linux/cpumask.h> #include <linux/kvm.h> #include <linux/kvm_para.h>
@@ -358,6 +359,8 @@ struct kvm_vcpu_arch {/* fields used by HYPER-V emulation */ u64 hv_vapic; + +cpumask_t wbinvd_dirty_mask; };struct kvm_arch {
@@ -514,6 +517,8 @@ struct kvm_x86_ops {void (*set_supported_cpuid)(u32 func,struct kvm_cpuid_entry2 *entry); +bool (*has_wbinvd_exit)(void ); +const struct trace_print_flags *exit_reasons_str; };
@@ -571,6 +576,7 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu);int kvm_emulate_halt (struct kvm_vcpu *vcpu);int emulate_invlpg (struct kvm_vcpu *vcpu,gva_t address);int emulate_clts (struct kvm_vcpu *vcpu); +int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu);void kvm_get_segment (struct kvm_vcpu *vcpu,struct kvm_segment *var,int seg);int kvm_load_segment_descriptor (struct kvm_vcpu *vcpu,u16 selector,int seg);
arch/x86/kvm/emulate.c +4 -1
@@ -3138,8 +3138,11 @@ twobyte_insn:emulate_clts (ctxt->vcpu); c->dst.type =OP_NONE ;break ; -case 0x08 :/* invd */ case 0x09 :/* wbinvd */ +kvm_emulate_wbinvd (ctxt->vcpu); + c->dst.type =OP_NONE ; +break ; +case 0x08 :/* invd */ case 0x0d :/* GrpP (prefetch) */ case 0x18 :/* Grp16 (prefetch/nop) */ c->dst.type =OP_NONE ;
arch/x86/kvm/svm.c +7 -0
@@ -3424,6 +3424,11 @@ static bool svm_rdtscp_supported(void)return false; } +static bool svm_has_wbinvd_exit (void ) +{ +return true; +} +static void svm_fpu_deactivate (struct kvm_vcpu *vcpu) {struct vcpu_svm *svm =to_svm (vcpu);
@@ -3508,6 +3513,8 @@ static struct kvm_x86_ops svm_x86_ops = { .rdtscp_supported = svm_rdtscp_supported, .set_supported_cpuid = svm_set_supported_cpuid, + + .has_wbinvd_exit = svm_has_wbinvd_exit, };static int __init svm_init (void )
arch/x86/kvm/vmx.c +9 -1
@@ -412,6 +412,12 @@ static inline bool cpu_has_virtual_nmis(void)return vmcs_config.pin_based_exec_ctrl &PIN_BASED_VIRTUAL_NMIS ; } +static inline bool cpu_has_vmx_wbinvd_exit (void ) +{ +return vmcs_config.cpu_based_2nd_exec_ctrl & +SECONDARY_EXEC_WBINVD_EXITING ; +} +static inline bool report_flexpriority (void ) {return flexpriority_enabled;
@@ -3400,7 +3406,7 @@ static int handle_invlpg(struct kvm_vcpu *vcpu)static int handle_wbinvd (struct kvm_vcpu *vcpu) {skip_emulated_instruction (vcpu); -/* TODO: Add support for VT-d/pass-through device */ +kvm_emulate_wbinvd (vcpu);return 1 ; }
@@ -4350,6 +4356,8 @@ static struct kvm_x86_ops vmx_x86_ops = { .rdtscp_supported = vmx_rdtscp_supported, .set_supported_cpuid = vmx_set_supported_cpuid, + + .has_wbinvd_exit = cpu_has_vmx_wbinvd_exit, };static int __init vmx_init (void )
arch/x86/kvm/x86.c +29 -0
@@ -1783,8 +1783,22 @@ out:return r; } +static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +} +void kvm_arch_vcpu_load (struct kvm_vcpu *vcpu,int cpu) { +/* Address WBINVD may be executed by guest */ +if (vcpu->kvm->arch.iommu_domain) { +if (kvm_x86_ops->has_wbinvd_exit ()) +cpu_set (cpu, vcpu->arch.wbinvd_dirty_mask); +else if (vcpu->cpu !=-1 ) +smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 ); + } + kvm_x86_ops->vcpu_load (vcpu, cpu);if (unlikely (per_cpu (cpu_tsc_khz, cpu) ==0 )) {unsigned long khz =cpufreq_quick_get (cpu);
@@ -3650,6 +3664,21 @@ int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)return X86EMUL_CONTINUE ; } +int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu) +{ +if (!vcpu->kvm->arch.iommu_domain) +return X86EMUL_CONTINUE ; + +if (kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (&vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 ); +cpus_clear (vcpu->arch.wbinvd_dirty_mask); + }else +wbinvd (); +return X86EMUL_CONTINUE ; +} +EXPORT_SYMBOL_GPL (kvm_emulate_wbinvd); +int emulate_clts (struct kvm_vcpu *vcpu) { kvm_x86_ops->set_cr0 (vcpu,kvm_read_cr0_bits (vcpu, ~X86_CR0_TS ));
Signature · 1 line
1.7.0.1
Quoted 4 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
Don't we always force enable snooping? Or is that only for the processor, and you're worried about devices?
Quoted 12 lines
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits./* fields used by HYPER-V emulation */
u64hv_vapic;
arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 364
Quoted 3 lines
arch/x86/include/asm/kvm_host.h +2
-0+ +cpumask_t wbinvd_dirty_mask; };
Need alloc_cpumask_var()/free_cpumask_var() for very large hosts.
arch/x86/kvm/x86.c out: · line 1789
Quoted 6 lines
arch/x86/kvm/x86.c +4
-0+static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +}
Like Jan mentioned, this is quite heavy. What about a clflush() loop instead? That may take more time, but at least it's preemptible. Of course, it isn't preemptible in an IPI.
arch/x86/kvm/x86.c out: · line 1799
Quoted 11 lines
arch/x86/kvm/x86.c +8
-0+void kvm_arch_vcpu_load (struct kvm_vcpu *vcpu,int cpu) { +/* Address WBINVD may be executed by guest */ +if (vcpu->kvm->arch.iommu_domain) { +if (kvm_x86_ops->has_wbinvd_exit ()) +cpu_set (cpu, vcpu->arch.wbinvd_dirty_mask); +else if (vcpu->cpu !=-1 ) +smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 );
Is there any point to doing this if !has_wbinvd_exit()? The vcpu might not have migrated in time, so the cache is flushed too late.
Quoted 9 lines
arch/x86/kvm/x86.c +2
-0+ } + kvm_x86_ops->vcpu_load (vcpu, cpu);if (unlikely (per_cpu (cpu_tsc_khz, cpu) ==0 )) {unsigned long khz =cpufreq_quick_get (cpu);@@ -3650,6 +3664,21 @@ int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)return X86EMUL_CONTINUE ; }
arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address) · line 3676
Quoted 10 lines
arch/x86/kvm/x86.c +9
-0+int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu) +{ +if (!vcpu->kvm->arch.iommu_domain) +return X86EMUL_CONTINUE ; + +if (kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (&vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 ); +cpus_clear (vcpu->arch.wbinvd_dirty_mask);
Race - a migration may set a new bit in wbinvd_dirty_mask after the s_c_f_m().
However, it's probably benign, since we won't be entering the guest in that period.
Quoted 10 lines
arch/x86/kvm/x86.c +6
-0+ }else +wbinvd (); +return X86EMUL_CONTINUE ; +} +EXPORT_SYMBOL_GPL (kvm_emulate_wbinvd); +int emulate_clts (struct kvm_vcpu *vcpu) {kvm_x86_ops->set_cr0(vcpu, kvm_read_cr0_bits(vcpu, ~X86_CR0_TS));
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 8 lines · On Monday 28 June 2010 11:56:08 Avi Kivity wrote:
Quoted 5 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or
CLFLUSH, we need to maintain data consistency either by:
Don't we always force enable snooping? Or is that only for the processor, and you're worried about devices?
We only force enabling snooping for capable VT-d engine(with KVM_IOMMU_CACHE_COHERENCY flag, on most recent server board). And you're right, with the snooping capable VT-d engine we don't need to do all these. Would address it in the next version.
Quoted 7 lines
Quoted 7 lines
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits./* fields used by HYPER-V emulation */
u64hv_vapic;
Quoted 6 lines
arch/x86/include/asm/kvm_host.h struct kvm_vcpu_arch {
Quoted 4 lines
arch/x86/include/asm/kvm_host.h +2
-0+ +cpumask_t wbinvd_dirty_mask; };Need alloc_cpumask_var()/free_cpumask_var() for very large hosts.
OK.
Quoted 9 lines
arch/x86/kvm/x86.c out: · line 1789
Quoted 5 lines
arch/x86/kvm/x86.c +4
-0+static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +}Like Jan mentioned, this is quite heavy. What about a
clflush()loop instead? That may take more time, but at least it's preemptible. Of course, it isn't preemptible in an IPI.
I think this kind of behavior happened rarely, and most recent processor should have WBINVD exit which means it's an IPI... So I think it's maybe acceptable here.
Quoted 5 lines
Quoted 5 lines
+
void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) {
Quoted 10 lines
arch/x86/kvm/x86.c out: · line 1799
Quoted 7 lines
arch/x86/kvm/x86.c +7
-0+/* Address WBINVD may be executed by guest */ +if (vcpu->kvm->arch.iommu_domain) { +if (kvm_x86_ops->has_wbinvd_exit ()) +cpu_set (cpu, vcpu->arch.wbinvd_dirty_mask); +else if (vcpu->cpu !=-1 ) +smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 );Is there any point to doing this if !has_wbinvd_exit()? The vcpu might not have migrated in time, so the cache is flushed too late.
For the !has_wbinvd_exit(), the instruction would be executed by guest and flush the current processor immediately. And we can ensure that it's clean in the last CPU, so we're fine.
Quoted 28 lines
Quoted 14 lines
arch/x86/kvm/x86.c +2
-0+ } + kvm_x86_ops->vcpu_load (vcpu, cpu);if (unlikely (per_cpu (cpu_tsc_khz, cpu) ==0 )) {unsigned long khz =cpufreq_quick_get (cpu);@@ -3650,6 +3664,21 @@ int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)return X86EMUL_CONTINUE ; }arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t · line 3678
Quoted 10 lines
arch/x86/kvm/x86.c +9
-0+int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu) +{ +if (!vcpu->kvm->arch.iommu_domain) +return X86EMUL_CONTINUE ; + +if (kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (&vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 ); +cpus_clear (vcpu->arch.wbinvd_dirty_mask);Race - a migration may set a new bit in
wbinvd_dirty_maskafter thes_c_f_m().
Quoted 2 lines
However, it's probably benign, since we won't be entering the guest in that period.
Yes. :)
--
regards
Yang, Sheng
Quoted 12 lines
Quoted 12 lines
arch/x86/kvm/x86.c +6
-0+ }else +wbinvd (); +return X86EMUL_CONTINUE ; +} +EXPORT_SYMBOL_GPL (kvm_emulate_wbinvd); +int emulate_clts(struct kvm_vcpu *vcpu) { kvm_x86_ops->set_cr0(vcpu, kvm_read_cr0_bits(vcpu, ~X86_CR0_TS));
Quoted 10 lines · Sheng Yang
Quoted 9 lines
arch/x86/kvm/x86.c out: · line 1789
Quoted 5 lines
arch/x86/kvm/x86.c +4
-0+static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +}Like Jan mentioned, this is quite heavy. What about a
clflush()loop instead? That may take more time, but at least it's preemptible. Of course, it isn't preemptible in an IPI.
Quoted 4 lines
I think this kind of behavior happened rarely, and most recent processor should have WBINVD exit which means it's an IPI... So I think it's maybe acceptable here.
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think queue_work_on() and a clflush loop is better than an IPI and wbinvd.
Quoted 16 lines
Quoted 5 lines
Quoted 5 lines
+
void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) {Quoted 11 lines
arch/x86/kvm/x86.c out: · line 1799
Quoted 8 lines
arch/x86/kvm/x86.c +7
-0+/* Address WBINVD may be executed by guest */ +if (vcpu->kvm->arch.iommu_domain) { +if (kvm_x86_ops->has_wbinvd_exit ()) +cpu_set (cpu, vcpu->arch.wbinvd_dirty_mask); +else if (vcpu->cpu !=-1 ) +smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 );Is there any point to doing this if !has_wbinvd_exit()? The vcpu might not have migrated in time, so the cache is flushed too late.
Quoted 4 lines
For the !has_wbinvd_exit(), the instruction would be executed by guest and flush the current processor immediately. And we can ensure that it's clean in the last CPU, so we're fine.
Ah, yes.
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 10 lines · On Monday 28 June 2010 14:56:38 Avi Kivity wrote:
Quoted 9 lines · Sheng Yang
Quoted 8 lines
arch/x86/kvm/x86.c out: · line 1789
Quoted 4 lines
arch/x86/kvm/x86.c +4
-0+static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +}Like Jan mentioned, this is quite heavy. What about a
clflush()loop instead? That may take more time, but at least it's preemptible. Of course, it isn't preemptible in an IPI.
Quoted 7 lines
Quoted 3 lines
I think this kind of behavior happened rarely, and most recent processor should have WBINVD exit which means it's an IPI... So I think it's maybe acceptable here.
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think
queue_work_on()and a clflush loop is better than an IPI and wbinvd.
OK... Would update it in the next version.
--
regards
Yang, Sheng
Quoted 22 lines
Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
+
void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) {Quoted 10 lines
arch/x86/kvm/x86.c out: · line 1799
Quoted 7 lines
arch/x86/kvm/x86.c +7
-0+/* Address WBINVD may be executed by guest */ +if (vcpu->kvm->arch.iommu_domain) { +if (kvm_x86_ops->has_wbinvd_exit ()) +cpu_set (cpu, vcpu->arch.wbinvd_dirty_mask); +else if (vcpu->cpu !=-1 ) +smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 );Is there any point to doing this if !has_wbinvd_exit()? The vcpu might not have migrated in time, so the cache is flushed too late.
Quoted 3 lines
For the !has_wbinvd_exit(), the instruction would be executed by guest and flush the current processor immediately. And we can ensure that it's clean in the last CPU, so we're fine.
Ah, yes.
Quoted 17 lines · Sheng Yang
Quoted 15 lines · On Monday 28 June 2010 14:56:38 Avi Kivity wrote:
Quoted 13 lines · Sheng Yang
Quoted 9 lines
arch/x86/kvm/x86.c out: · line 1789
Quoted 5 lines
arch/x86/kvm/x86.c +4
-0+static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +}Like Jan mentioned, this is quite heavy. What about a
clflush()loop instead? That may take more time, but at least it's preemptible. Of course, it isn't preemptible in an IPI.I think this kind of behavior happened rarely, and most recent processor should have WBINVD exit which means it's an IPI... So I think it's maybe acceptable here.
Quoted 7 lines
Quoted 4 lines
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think
queue_work_on()and a clflush loop is better than an IPI and wbinvd.OK... Would update it in the next version.
Hm, the manual says (regarding clflush):
Quoted 5 lines
Invalidates the cache line that contains the linear address specified with the source operand from all levels of the processor cache hierarchy (data and instruction). The invalidation is broadcast throughout the cache coherence domain. If, at any level of the cache hierarchy, the line is inconsistent with memory (dirty) it is written to memory before invalidation.
So I don't think you need to queue_work_on(), instead you can work in vcpu thread context. But better check with someone that really knows.
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 33 lines · On Monday 28 June 2010 15:08:56 Avi Kivity wrote:
Quoted 15 lines · Sheng Yang
Quoted 14 lines · On Monday 28 June 2010 14:56:38 Avi Kivity wrote:
Quoted 12 lines · Sheng Yang
Quoted 8 lines
arch/x86/kvm/x86.c out: · line 1789
Quoted 4 lines
arch/x86/kvm/x86.c +4
-0+static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +}Like Jan mentioned, this is quite heavy. What about a
clflush()loop instead? That may take more time, but at least it's preemptible. Of course, it isn't preemptible in an IPI.I think this kind of behavior happened rarely, and most recent processor should have WBINVD exit which means it's an IPI... So I think it's maybe acceptable here.
Quoted 5 lines
Quoted 3 lines
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think
queue_work_on()and a clflush loop is better than an IPI and wbinvd.OK... Would update it in the next version.
Hm, the manual says (regarding clflush):
Quoted 9 lines
Invalidates the cache line that contains the linear address specified with the source
operand from all levels of the processor cache hierarchy (data and instruction). The
invalidation is broadcast throughout the cache coherence domain. If, at any level of
the cache hierarchy, the line is inconsistent with memory (dirty) it is written to
memory before invalidation.
Quoted 2 lines
So I don't think you need to
queue_work_on(), instead you can work in vcpu thread context. But better check with someone that really knows.
Yeah, I've just checked the instruction as well. For it would be boardcasted, seems we even don't need(and can't have) a dirty bitmap. So the overhead on the large machine should be big.
And I've calculated the times we need to execute clflush for whole guest memory. If I calculate it right, for a 64bit guest, clflush can only cover 64 bytes one time, so for a typical 4G guest, we would need to execute the command for 4G / 64 = 64M times. The cycles used by clflush can be vary, suppose it would use 10 cycles each (which sounds impossible, for involving boardcast and writeback, and not including cache refill time for all processors), it would cost more than 0.2 seconds one time on an 3.2Ghz machine...
--
regards
Yang, Sheng
--
regards
Yang, Sheng
Quoted 20 lines · Sheng Yang
Quoted 13 lines
Hm, the manual says (regarding clflush):
Quoted 10 lines
Invalidates the cache line that contains the linear address specified with the source
operand from all levels of the processor cache hierarchy (data and instruction). The
invalidation is broadcast throughout the cache coherence domain. If, at any level of
the cache hierarchy, the line is inconsistent with memory (dirty) it is written to
memory before invalidation.Quoted 3 lines
So I don't think you need to
queue_work_on(), instead you can work in vcpu thread context. But better check with someone that really knows.Yeah, I've just checked the instruction as well. For it would be boardcasted, seems we even don't need(and can't have) a dirty bitmap. So the overhead on the large machine should be big.
Quoted 8 lines
And I've calculated the times we need to execute clflush for whole guest memory. If I calculate it right, for a 64bit guest, clflush can only cover 64 bytes one time, so for a typical 4G guest, we would need to execute the command for 4G / 64 = 64M times. The cycles used by clflush can be vary, suppose it would use 10 cycles each (which sounds impossible, for involving boardcast and writeback, and not including cache refill time for all processors), it would cost more than 0.2 seconds one time on an 3.2Ghz machine...
Right, so clflush can't be made to work.
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH v4 1/1 KVM: VMX: Execute WBINVD to keep data consistency with assigned devices
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
--- arch/x86/include/asm/kvm_host.h | 6 +++++ arch/x86/kvm/emulate.c | 5 +++- arch/x86/kvm/svm.c | 7 ++++++ arch/x86/kvm/vmx.c | 10 ++++++++- arch/x86/kvm/x86.c | 44 ++++++++++++++++++++++++++++++++++++++- 5 files changed, 69 insertions(+), 3 deletions(-)
arch/x86/include/asm/kvm_host.h +6 -0
@@ -15,6 +15,7 @@#include <linux/mm.h> #include <linux/mmu_notifier.h> #include <linux/tracepoint.h> +#include <linux/cpumask.h> #include <linux/kvm.h> #include <linux/kvm_para.h>
@@ -358,6 +359,8 @@ struct kvm_vcpu_arch {/* fields used by HYPER-V emulation */ u64 hv_vapic; + +cpumask_var_t wbinvd_dirty_mask; };struct kvm_arch {
@@ -514,6 +517,8 @@ struct kvm_x86_ops {void (*set_supported_cpuid)(u32 func,struct kvm_cpuid_entry2 *entry); +bool (*has_wbinvd_exit)(void ); +const struct trace_print_flags *exit_reasons_str; };
@@ -571,6 +576,7 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu);int kvm_emulate_halt (struct kvm_vcpu *vcpu);int emulate_invlpg (struct kvm_vcpu *vcpu,gva_t address);int emulate_clts (struct kvm_vcpu *vcpu); +int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu);void kvm_get_segment (struct kvm_vcpu *vcpu,struct kvm_segment *var,int seg);int kvm_load_segment_descriptor (struct kvm_vcpu *vcpu,u16 selector,int seg);
arch/x86/kvm/emulate.c +4 -1
@@ -3138,8 +3138,11 @@ twobyte_insn:emulate_clts (ctxt->vcpu); c->dst.type =OP_NONE ;break ; -case 0x08 :/* invd */ case 0x09 :/* wbinvd */ +kvm_emulate_wbinvd (ctxt->vcpu); + c->dst.type =OP_NONE ; +break ; +case 0x08 :/* invd */ case 0x0d :/* GrpP (prefetch) */ case 0x18 :/* Grp16 (prefetch/nop) */ c->dst.type =OP_NONE ;
arch/x86/kvm/svm.c +7 -0
@@ -3424,6 +3424,11 @@ static bool svm_rdtscp_supported(void)return false; } +static bool svm_has_wbinvd_exit (void ) +{ +return true; +} +static void svm_fpu_deactivate (struct kvm_vcpu *vcpu) {struct vcpu_svm *svm =to_svm (vcpu);
@@ -3508,6 +3513,8 @@ static struct kvm_x86_ops svm_x86_ops = { .rdtscp_supported = svm_rdtscp_supported, .set_supported_cpuid = svm_set_supported_cpuid, + + .has_wbinvd_exit = svm_has_wbinvd_exit, };static int __init svm_init (void )
arch/x86/kvm/vmx.c +9 -1
@@ -412,6 +412,12 @@ static inline bool cpu_has_virtual_nmis(void)return vmcs_config.pin_based_exec_ctrl &PIN_BASED_VIRTUAL_NMIS ; } +static inline bool cpu_has_vmx_wbinvd_exit (void ) +{ +return vmcs_config.cpu_based_2nd_exec_ctrl & +SECONDARY_EXEC_WBINVD_EXITING ; +} +static inline bool report_flexpriority (void ) {return flexpriority_enabled;
@@ -3400,7 +3406,7 @@ static int handle_invlpg(struct kvm_vcpu *vcpu)static int handle_wbinvd (struct kvm_vcpu *vcpu) {skip_emulated_instruction (vcpu); -/* TODO: Add support for VT-d/pass-through device */ +kvm_emulate_wbinvd (vcpu);return 1 ; }
@@ -4350,6 +4356,8 @@ static struct kvm_x86_ops vmx_x86_ops = { .rdtscp_supported = vmx_rdtscp_supported, .set_supported_cpuid = vmx_set_supported_cpuid, + + .has_wbinvd_exit = cpu_has_vmx_wbinvd_exit, };static int __init vmx_init (void )
arch/x86/kvm/x86.c +43 -1
70 lines not on this page · raw message
Signature · 1 line
1.7.0.1
Quoted 10 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.arch/x86/kvm/x86.c +1
-0+int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu)
arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address) · line 3683
Quoted 10 lines
arch/x86/kvm/x86.c +10
-0+{ +if (need_emulate_wbinvd (vcpu)&& kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 ); +cpumask_clear (vcpu->arch.wbinvd_dirty_mask); + }else +wbinvd (); +return X86EMUL_CONTINUE ; +} +EXPORT_SYMBOL_GPL (kvm_emulate_wbinvd);
Why check for has_wbinvd_exit()? If it's false, we don't get here anyway.
If !need_emulate_wbinvd(), why call wbinvd()?
Quoted 20 lines
arch/x86/kvm/x86.c +5
-1+int emulate_clts (struct kvm_vcpu *vcpu) { kvm_x86_ops->set_cr0 (vcpu,kvm_read_cr0_bits (vcpu, ~X86_CR0_TS ));@@ -5255,6 +5287,7 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu) vcpu->arch.time_page =NULL ; } +free_cpumask_var (vcpu->arch.wbinvd_dirty_mask);fx_free (vcpu); kvm_x86_ops->vcpu_free (vcpu); }@@ -5262,7 +5295,16 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)struct kvm_vcpu *kvm_arch_vcpu_create (struct kvm *kvm,unsigned int id) { -return kvm_x86_ops->vcpu_create (kvm, id); +struct kvm_vcpu *vcpu = kvm_x86_ops->vcpu_create (kvm, id); + +if (IS_ERR (vcpu))
arch/x86/kvm/x86.c void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu) · line 5310
Quoted 10 lines
arch/x86/kvm/x86.c +7
-0+return vcpu; + +if (!zalloc_cpumask_var (&vcpu->arch.wbinvd_dirty_mask,GFP_KERNEL )) { + kvm_x86_ops->vcpu_free (vcpu); +return ERR_PTR (-ENOMEM ); + } +return vcpu; }
Better place is kvm_arch_vcpu_init().
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 23 lines · Avi Kivity
Quoted 9 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.arch/x86/kvm/x86.c +1
-0+int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu)arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address) · line 3683
Quoted 10 lines
arch/x86/kvm/x86.c +10
-0+{ +if (need_emulate_wbinvd (vcpu)&& kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 ); +cpumask_clear (vcpu->arch.wbinvd_dirty_mask); + }else +wbinvd (); +return X86EMUL_CONTINUE ; +} +EXPORT_SYMBOL_GPL (kvm_emulate_wbinvd);Why check for
has_wbinvd_exit()? If it's false, we don't get here anyway.
Quoted 3 lines
If !need_emulate_wbinvd(), why call
wbinvd()?
The function is called from emulator too. I guess that is why.
Quoted 23 lines
Quoted 23 lines
arch/x86/kvm/x86.c +8
-1+int emulate_clts (struct kvm_vcpu *vcpu) { kvm_x86_ops->set_cr0 (vcpu,kvm_read_cr0_bits (vcpu, ~X86_CR0_TS ));@@ -5255,6 +5287,7 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu) vcpu->arch.time_page =NULL ; } +free_cpumask_var (vcpu->arch.wbinvd_dirty_mask);fx_free (vcpu); kvm_x86_ops->vcpu_free (vcpu); }@@ -5262,7 +5295,16 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)struct kvm_vcpu *kvm_arch_vcpu_create (struct kvm *kvm,unsigned int id) { -return kvm_x86_ops->vcpu_create (kvm, id); +struct kvm_vcpu *vcpu = kvm_x86_ops->vcpu_create (kvm, id); + +if (IS_ERR (vcpu)) +return vcpu; + +if (!zalloc_cpumask_var (&vcpu->arch.wbinvd_dirty_mask,GFP_KERNEL )) {
Quoted 16 lines
arch/x86/kvm/x86.c void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu) · line 5309
Quoted 6 lines
arch/x86/kvm/x86.c +4
-0+ kvm_x86_ops->vcpu_free (vcpu); +return ERR_PTR (-ENOMEM ); + } +return vcpu; }Better place is
kvm_arch_vcpu_init().Signature · 6 lines
error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.
Quoted 26 lines · Gleb Natapov
Quoted 24 lines · Avi Kivity
Quoted 10 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.arch/x86/kvm/x86.c +1
-0+int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu)arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address) · line 3683
Quoted 10 lines
arch/x86/kvm/x86.c +10
-0+{ +if (need_emulate_wbinvd (vcpu)&& kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 ); +cpumask_clear (vcpu->arch.wbinvd_dirty_mask); + }else +wbinvd (); +return X86EMUL_CONTINUE ; +} +EXPORT_SYMBOL_GPL (kvm_emulate_wbinvd);Why check for
has_wbinvd_exit()? If it's false, we don't get here anyway.
Quoted 6 lines
Quoted 4 lines
If !need_emulate_wbinvd(), why call
wbinvd()?The function is called from emulator too. I guess that is why.
Ah, yes. But it needs to return, if !n_e_wbinvd(), not call wbinvd().
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH v5 1/1 KVM: VMX: Execute WBINVD to keep data consistency with assigned devices
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
--- arch/x86/include/asm/kvm_host.h | 6 +++++ arch/x86/kvm/emulate.c | 5 +++- arch/x86/kvm/svm.c | 7 ++++++ arch/x86/kvm/vmx.c | 10 ++++++++- arch/x86/kvm/x86.c | 41 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 67 insertions(+), 2 deletions(-)
arch/x86/include/asm/kvm_host.h +6 -0
@@ -15,6 +15,7 @@#include <linux/mm.h> #include <linux/mmu_notifier.h> #include <linux/tracepoint.h> +#include <linux/cpumask.h> #include <linux/kvm.h> #include <linux/kvm_para.h>
@@ -358,6 +359,8 @@ struct kvm_vcpu_arch {/* fields used by HYPER-V emulation */ u64 hv_vapic; + +cpumask_var_t wbinvd_dirty_mask; };struct kvm_arch {
@@ -514,6 +517,8 @@ struct kvm_x86_ops {void (*set_supported_cpuid)(u32 func,struct kvm_cpuid_entry2 *entry); +bool (*has_wbinvd_exit)(void ); +const struct trace_print_flags *exit_reasons_str; };
@@ -571,6 +576,7 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu);int kvm_emulate_halt (struct kvm_vcpu *vcpu);int emulate_invlpg (struct kvm_vcpu *vcpu,gva_t address);int emulate_clts (struct kvm_vcpu *vcpu); +int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu);void kvm_get_segment (struct kvm_vcpu *vcpu,struct kvm_segment *var,int seg);int kvm_load_segment_descriptor (struct kvm_vcpu *vcpu,u16 selector,int seg);
arch/x86/kvm/emulate.c +4 -1
@@ -3138,8 +3138,11 @@ twobyte_insn:emulate_clts (ctxt->vcpu); c->dst.type =OP_NONE ;break ; -case 0x08 :/* invd */ case 0x09 :/* wbinvd */ +kvm_emulate_wbinvd (ctxt->vcpu); + c->dst.type =OP_NONE ; +break ; +case 0x08 :/* invd */ case 0x0d :/* GrpP (prefetch) */ case 0x18 :/* Grp16 (prefetch/nop) */ c->dst.type =OP_NONE ;
arch/x86/kvm/svm.c +7 -0
@@ -3424,6 +3424,11 @@ static bool svm_rdtscp_supported(void)return false; } +static bool svm_has_wbinvd_exit (void ) +{ +return true; +} +static void svm_fpu_deactivate (struct kvm_vcpu *vcpu) {struct vcpu_svm *svm =to_svm (vcpu);
@@ -3508,6 +3513,8 @@ static struct kvm_x86_ops svm_x86_ops = { .rdtscp_supported = svm_rdtscp_supported, .set_supported_cpuid = svm_set_supported_cpuid, + + .has_wbinvd_exit = svm_has_wbinvd_exit, };static int __init svm_init (void )
arch/x86/kvm/vmx.c +9 -1
@@ -412,6 +412,12 @@ static inline bool cpu_has_virtual_nmis(void)return vmcs_config.pin_based_exec_ctrl &PIN_BASED_VIRTUAL_NMIS ; } +static inline bool cpu_has_vmx_wbinvd_exit (void ) +{ +return vmcs_config.cpu_based_2nd_exec_ctrl & +SECONDARY_EXEC_WBINVD_EXITING ; +} +static inline bool report_flexpriority (void ) {return flexpriority_enabled;
@@ -3400,7 +3406,7 @@ static int handle_invlpg(struct kvm_vcpu *vcpu)static int handle_wbinvd (struct kvm_vcpu *vcpu) {skip_emulated_instruction (vcpu); -/* TODO: Add support for VT-d/pass-through device */ +kvm_emulate_wbinvd (vcpu);return 1 ; }
@@ -4350,6 +4356,8 @@ static struct kvm_x86_ops vmx_x86_ops = { .rdtscp_supported = vmx_rdtscp_supported, .set_supported_cpuid = vmx_set_supported_cpuid, + + .has_wbinvd_exit = cpu_has_vmx_wbinvd_exit, };static int __init vmx_init (void )
arch/x86/kvm/x86.c +41 -0
68 lines not on this page · raw message
Signature · 1 line
1.7.0.1
Quoted 8 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
Looks good.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 10 lines · Avi Kivity
Quoted 8 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
Quoted 2 lines
Looks good.
There is just the question remaining if we want to add some disable knob, maybe as an option in the device assignment configuration.
I wonder what the performance impact of this feature is when using CPUs without wbinvd exiting. Can we afford to enable it unconditionally (in the presence of pass-through) even if the guest doesn't need it?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Avi Kivity
Quoted 9 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
Quoted 6 lines
Quoted 3 lines
Looks good.
There is just the question remaining if we want to add some disable knob, maybe as an option in the device assignment configuration.
Patches welcome. Also an alternative implementation that uses clflush could also work.
Quoted 4 lines
I wonder what the performance impact of this feature is when using CPUs without wbinvd exiting. Can we afford to enable it unconditionally (in the presence of pass-through) even if the guest doesn't need it?
Correctness is more important than performance. Since we don't know whether the guest needs it or not, we have to enable it. The user may disable it if he likes.
Maybe we should emit a warning that performance may degrade when pci snooping is not available.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 5 lines · Avi Kivity
On 06/29/2010 01:32 PM, Jan Kiszka wrote:
Correctness is more important than performance. Since we don't know whether the guest needs it or not, we have to enable it. The user may disable it if he likes.
Can't this code only be enabled if VT-d hardware is detected that does not support the snoop force bit? So the user does not have to struggle with configuration options that are hard to understand for a non-developer.
Joerg
Quoted 5 lines · Joerg
Quoted 2 lines · Avi Kivity
On 06/29/2010 01:32 PM, Jan Kiszka wrote:
Quoted 10 lines
Quoted 4 lines
Correctness is more important than performance. Since we don't know whether the guest needs it or not, we have to enable it. The user may disable it if he likes.
Can't this code only be enabled if VT-d hardware is detected that does not support the snoop force bit? So the user does not have to struggle with configuration options that are hard to understand for a non-developer.
It's already this way, that's why we test for KVM_IOMMU_CACHE_COHERENCY. Jan wants an override for older hardware which doesn't support a coherent iommu (the wbinvd ipi on task migrations, on processors that don't support wbinvd exits, is particularly expensive).
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 6 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
Sorry for jumping in late. This code is not required on AMD platforms because the io-page-tables for the AMD IOMMU have a FC bit (force coherent) that must just be set. The current code does not set this bit but I will prepare a patch for that. This wbinvd emulation code should be avoided where possible.
Joerg
Quoted 9 lines · Joerg
Quoted 7 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
Quoted 6 lines
Sorry for jumping in late. This code is not required on AMD platforms because the io-page-tables for the AMD IOMMU have a FC bit (force coherent) that must just be set. The current code does not set this bit but I will prepare a patch for that. This wbinvd emulation code should be avoided where possible.
Do you mean, KVM_IOMMU_CACHE_COHERENCY should be set for the AMD IOMMU?
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 10 lines · Avi Kivity
Quoted 9 lines · Joerg
Quoted 7 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.
Quoted 8 lines
Quoted 6 lines
Sorry for jumping in late. This code is not required on AMD platforms because the io-page-tables for the AMD IOMMU have a FC bit (force coherent) that must just be set. The current code does not set this bit but I will prepare a patch for that. This wbinvd emulation code should be avoided where possible.
Do you mean,
KVM_IOMMU_CACHE_COHERENCYshould be set for the AMD IOMMU?
No, as far as I understand it the KVM_IOMMU_CACHE_COHERENCY flag is only there because there are VT-d IOMMUs that does not support the snoop force bit. In the AMD IOMMU case all hardware has this feature, the IOMMU driver just has to use it for IOMMU-API page-tables too. This is currently not the case. So this is only an IOMMU driver change.
Joerg
Quoted 9 lines · Joerg
Quoted 3 lines
Do you mean,
KVM_IOMMU_CACHE_COHERENCYshould be set for the AMD IOMMU?No, as far as I understand it the
KVM_IOMMU_CACHE_COHERENCYflag is only there because there are VT-d IOMMUs that does not support the snoop force bit. In the AMD IOMMU case all hardware has this feature, the IOMMU driver just has to use it for IOMMU-API page-tables too. This is currently not the case. So this is only an IOMMU driver change.
The flag indicates to kvm that it doesn't need to worry about iommu cache coherency issues (for example, it can ignore wbinvd), so it needs to be set. THe following code
if (iommu_domain_has_cap(kvm->arch.iommu_domain,
IOMMU_CAP_CACHE_COHERENCY))
kvm->arch.iommu_flags |= KVM_IOMMU_CACHE_COHERENCY;does this, so it looks like you need to return true for iommu_domain_has_cap() after the change. So far only the intel iommu supports it.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 6 lines · Avi Kivity
On 06/29/2010 03:28 PM, Roedel, Joerg wrote:
The flag indicates to kvm that it doesn't need to worry about iommu cache coherency issues (for example, it can ignore wbinvd), so it needs to be set. THe following code
Quoted 7 lines
if (iommu_domain_has_cap(kvm->arch.iommu_domain, IOMMU_CAP_CACHE_COHERENCY)) kvm->arch.iommu_flags |= KVM_IOMMU_CACHE_COHERENCY;does this, so it looks like you need to return true for
iommu_domain_has_cap()after the change. So far only the intel iommu supports it.
Ah ok, good to know. Thanks.
Joerg
Quoted 148 lines · Sheng Yang
Some guest device driver may leverage the "Non-Snoop" I/O, and explicitly WBINVD or CLFLUSH to a RAM space. Since migration may occur before WBINVD or CLFLUSH, we need to maintain data consistency either by:
1: flushing cache (wbinvd) when the guest is scheduled out if there is no wbinvd exit, or
2: execute wbinvd on all dirty physical CPUs when guest wbinvd exits.Signed-off-by Yaozu (Eddie) Dong <[email protected]>Signed-off-by Sheng Yang <[email protected]>--- arch/x86/include/asm/kvm_host.h | 6 +++++ arch/x86/kvm/emulate.c | 5 +++- arch/x86/kvm/svm.c | 7 ++++++ arch/x86/kvm/vmx.c | 10 ++++++++- arch/x86/kvm/x86.c | 41 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 67 insertions(+), 2 deletions(-)arch/x86/include/asm/kvm_host.h +6
-0@@ -15,6 +15,7 @@#include <linux/mm.h> #include <linux/mmu_notifier.h> #include <linux/tracepoint.h> +#include <linux/cpumask.h> #include <linux/kvm.h> #include <linux/kvm_para.h> @@ -358,6 +359,8 @@ struct kvm_vcpu_arch {/* fields used by HYPER-V emulation */ u64 hv_vapic; + +cpumask_var_t wbinvd_dirty_mask; };struct kvm_arch {@@ -514,6 +517,8 @@ struct kvm_x86_ops {void (*set_supported_cpuid)(u32 func,struct kvm_cpuid_entry2 *entry); +bool (*has_wbinvd_exit)(void ); +const struct trace_print_flags *exit_reasons_str; };@@ -571,6 +576,7 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu);int kvm_emulate_halt (struct kvm_vcpu *vcpu);int emulate_invlpg (struct kvm_vcpu *vcpu,gva_t address);int emulate_clts (struct kvm_vcpu *vcpu); +int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu);void kvm_get_segment (struct kvm_vcpu *vcpu,struct kvm_segment *var,int seg);int kvm_load_segment_descriptor (struct kvm_vcpu *vcpu,u16 selector,int seg);arch/x86/kvm/emulate.c +4
-1@@ -3138,8 +3138,11 @@ twobyte_insn:emulate_clts (ctxt->vcpu); c->dst.type =OP_NONE ;break ; -case 0x08 :/* invd */ case 0x09 :/* wbinvd */ +kvm_emulate_wbinvd (ctxt->vcpu); + c->dst.type =OP_NONE ; +break ; +case 0x08 :/* invd */ case 0x0d :/* GrpP (prefetch) */ case 0x18 :/* Grp16 (prefetch/nop) */ c->dst.type =OP_NONE ;arch/x86/kvm/svm.c +7
-0@@ -3424,6 +3424,11 @@ static bool svm_rdtscp_supported(void)return false; } +static bool svm_has_wbinvd_exit (void ) +{ +return true; +} +static void svm_fpu_deactivate (struct kvm_vcpu *vcpu) {struct vcpu_svm *svm =to_svm (vcpu);@@ -3508,6 +3513,8 @@ static struct kvm_x86_ops svm_x86_ops = { .rdtscp_supported = svm_rdtscp_supported, .set_supported_cpuid = svm_set_supported_cpuid, + + .has_wbinvd_exit = svm_has_wbinvd_exit, };static int __init svm_init (void )arch/x86/kvm/vmx.c +9
-1@@ -412,6 +412,12 @@ static inline bool cpu_has_virtual_nmis(void)return vmcs_config.pin_based_exec_ctrl &PIN_BASED_VIRTUAL_NMIS ; } +static inline bool cpu_has_vmx_wbinvd_exit (void ) +{ +return vmcs_config.cpu_based_2nd_exec_ctrl & +SECONDARY_EXEC_WBINVD_EXITING ; +} +static inline bool report_flexpriority (void ) {return flexpriority_enabled;@@ -3400,7 +3406,7 @@ static int handle_invlpg(struct kvm_vcpu *vcpu)static int handle_wbinvd (struct kvm_vcpu *vcpu) {skip_emulated_instruction (vcpu); -/* TODO: Add support for VT-d/pass-through device */ +kvm_emulate_wbinvd (vcpu);return 1 ; }@@ -4350,6 +4356,8 @@ static struct kvm_x86_ops vmx_x86_ops = { .rdtscp_supported = vmx_rdtscp_supported, .set_supported_cpuid = vmx_set_supported_cpuid, + + .has_wbinvd_exit = cpu_has_vmx_wbinvd_exit, };static int __init vmx_init (void )arch/x86/kvm/x86.c +8
-0@@ -1783,8 +1783,28 @@ out:return r; } +static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +} + +static bool need_emulate_wbinvd (struct kvm_vcpu *vcpu) +{ + return vcpu->kvm->arch.iommu_domain &&
arch/x86/kvm/x86.c out: · line 1803
Quoted 10 lines
arch/x86/kvm/x86.c +8
-0+ !(vcpu->kvm->arch.iommu_flags &KVM_IOMMU_CACHE_COHERENCY ); +} +void kvm_arch_vcpu_load (struct kvm_vcpu *vcpu,int cpu) { +/* Address WBINVD may be executed by guest */ +if (need_emulate_wbinvd (vcpu)) { +if (kvm_x86_ops->has_wbinvd_exit ()) +cpumask_set_cpu (cpu, vcpu->arch.wbinvd_dirty_mask); +else if (vcpu->cpu !=-1 )
&& vcpu->cpu != cpu
Quoted 9 lines
arch/x86/kvm/x86.c +4
-0+smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 ); + } + kvm_x86_ops->vcpu_load (vcpu, cpu);if (unlikely (per_cpu (cpu_tsc_khz, cpu) ==0 )) {unsigned long khz =cpufreq_quick_get (cpu);@@ -3650,6 +3670,21 @@ int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)return X86EMUL_CONTINUE ;
arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address) · line 3680
Quoted 10 lines
arch/x86/kvm/x86.c +8
-0} +int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu) +{ +if (!need_emulate_wbinvd (vcpu)) +return X86EMUL_CONTINUE ; + +if (kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 );
work_on_cpu() loop instead of smp_call_function_many(), to avoid executing wbinvd with interrupts disabled.
Quoted 12 lines · Marcelo Tosatti
Quoted 12 lines
arch/x86/kvm/x86.c +4
-0+smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 ); + } + kvm_x86_ops->vcpu_load (vcpu, cpu);if (unlikely (per_cpu (cpu_tsc_khz, cpu) ==0 )) {unsigned long khz =cpufreq_quick_get (cpu);@@ -3650,6 +3670,21 @@ int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)return X86EMUL_CONTINUE ; }
Quoted 12 lines
arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)
Quoted 9 lines
arch/x86/kvm/x86.c +8
-0+int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu) +{ +if (!need_emulate_wbinvd (vcpu)) +return X86EMUL_CONTINUE ; + +if (kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 );
work_on_cpu()loop instead ofsmp_call_function_many(), to avoid executing wbinvd with interrupts disabled.
Why? wbinvd is not interruptible.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 24 lines · Avi Kivity
Quoted 12 lines · Marcelo Tosatti
Quoted 12 lines
arch/x86/kvm/x86.c +4
-0+smp_call_function_single (vcpu->cpu, + wbinvd_ipi,NULL ,1 ); + } + kvm_x86_ops->vcpu_load (vcpu, cpu);if (unlikely (per_cpu (cpu_tsc_khz, cpu) ==0 )) {unsigned long khz =cpufreq_quick_get (cpu);@@ -3650,6 +3670,21 @@ int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address)return X86EMUL_CONTINUE ; }Quoted 10 lines
arch/x86/kvm/x86.c int emulate_invlpg(struct kvm_vcpu *vcpu, gva_t address) · line 3680
Quoted 8 lines
arch/x86/kvm/x86.c +8
-0+int kvm_emulate_wbinvd (struct kvm_vcpu *vcpu) +{ +if (!need_emulate_wbinvd (vcpu)) +return X86EMUL_CONTINUE ; + +if (kvm_x86_ops->has_wbinvd_exit ()) { +smp_call_function_many (vcpu->arch.wbinvd_dirty_mask, + wbinvd_ipi,NULL ,1 );
work_on_cpu()loop instead ofsmp_call_function_many(), to avoid executing wbinvd with interrupts disabled.
Quoted 1 line
Why? wbinvd is not interruptible.
Right. But still, smp_call_function_many() is going to busy-spin until the target CPUs finish their work, while work_on_cpu() will schedule.
Also the IPI request has to handled immediately, bypassing the scheduler.
Quoted 10 lines · Marcelo Tosatti
Quoted 6 lines
Quoted 4 lines
work_on_cpu()loop instead ofsmp_call_function_many(), to avoid executing wbinvd with interrupts disabled.Why? wbinvd is not interruptible.
Right. But still,
smp_call_function_many()is going to busy-spin until the target CPUs finish their work, whilework_on_cpu()will schedule.
Good point. So in the worst case we double the hit.
Quoted 3 lines
Also the IPI request has to handled immediately, bypassing the scheduler.
We're screwed on that point in any case, once wbinvd starts the scheduler is bypassed.
But work_on_cpu() has its own problems. It creates a kthread (perhaps not too bad). Maybe queue_work_on()?
btw, smp_call_function_many() appears not to run the function on the current cpu.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 11 lines · Avi Kivity
Quoted 7 lines · Marcelo Tosatti
Quoted 4 lines
Quoted 3 lines
work_on_cpu()loop instead ofsmp_call_function_many(), to avoid executing wbinvd with interrupts disabled.Why? wbinvd is not interruptible.
Right. But still,
smp_call_function_many()is going to busy-spin until the target CPUs finish their work, whilework_on_cpu()will schedule.Good point. So in the worst case we double the hit.
Quoted 8 lines
Quoted 2 lines
Also the IPI request has to handled immediately, bypassing the scheduler.
We're screwed on that point in any case, once wbinvd starts the scheduler is bypassed.
But
work_on_cpu()has its own problems. It creates a kthread (perhaps not too bad). Maybequeue_work_on()?
queue_work_on + flush_work should be equivalent to work_on_cpu (without thread creation).
Quoted 17 lines · Avi Kivity
Quoted 10 lines · Sheng Yang
Quoted 9 lines
arch/x86/kvm/x86.c out: · line 1789
Quoted 5 lines
arch/x86/kvm/x86.c +4
-0+static void wbinvd_ipi (void *garbage) +{ +wbinvd (); +}Like Jan mentioned, this is quite heavy. What about a
clflush()loop instead? That may take more time, but at least it's preemptible. Of course, it isn't preemptible in an IPI.Quoted 5 lines
I think this kind of behavior happened rarely, and most recent processor should have WBINVD exit which means it's an IPI... So I think it's maybe acceptable here.
Quoted 4 lines
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think
queue_work_on()and a clflush loop is better than an IPI and wbinvd.
Probably we should make it configurable. For RT usage models, we do care about responsiveness more than performance, but for typical server useg model, we'd better focus on performance in this issue. WBINVD may perform much much better than CLFLUSH, and a mallicious guest repeatedly issuing wbinvd may greatly impact the system performance.
thx, eddie
Quoted 8 lines · Eddie
Quoted 6 lines
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think
queue_work_on()and a clflush loop is better than an IPI and wbinvd.Probably we should make it configurable. For RT usage models, we do care about responsiveness more than performance, but for typical server useg model, we'd better focus on performance in this issue. WBINVD may perform much much better than CLFLUSH, and a mallicious guest repeatedly issuing wbinvd may greatly impact the system performance.
I'm not even sure clflush can work. I thought you could loop on just the cache size, but it appears you'll need to loop over the entire guest address space, which could take ages.
So I guess we'll have to settle for wbinvd, just avoiding it when the hardware allows us to.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 15 lines · Avi Kivity
Quoted 6 lines · Eddie
Quoted 6 lines
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think
queue_work_on()and a clflush loop is better than an IPI and wbinvd.Quoted 7 lines
Probably we should make it configurable. For RT usage models, we do care about responsiveness more than performance, but for typical server useg model, we'd better focus on performance in this issue. WBINVD may perform much much better than CLFLUSH, and a mallicious guest repeatedly issuing wbinvd may greatly impact the system performance.
Quoted 3 lines
I'm not even sure clflush can work. I thought you could loop on just the cache size, but it appears you'll need to loop over the entire guest address space, which could take ages.
If RT usage model comes into reality, we may have to do in this way though pay with huge overhead :) Is there any RT customers here?
Quoted 3 lines
So I guess we'll have to settle for wbinvd, just avoiding it when the hardware allows us to.
Yes, for now I agree we can just use wbinvd to emulate wbinvd :)
Thx, Eddie
Quoted 14 lines · Eddie
Quoted 13 lines · Avi Kivity
Quoted 12 lines · Eddie
Quoted 5 lines
Several milliseconds of non-responsiveness may not be acceptable for some applications. So I think
queue_work_on()and a clflush loop is better than an IPI and wbinvd.Probably we should make it configurable. For RT usage models, we do care about responsiveness more than performance, but for typical server useg model, we'd better focus on performance in this issue. WBINVD may perform much much better than CLFLUSH, and a mallicious guest repeatedly issuing wbinvd may greatly impact the system performance.
Quoted 6 lines
Quoted 3 lines
I'm not even sure clflush can work. I thought you could loop on just the cache size, but it appears you'll need to loop over the entire guest address space, which could take ages.
If RT usage model comes into reality, we may have to do in this way though pay with huge overhead :) Is there any RT customers here?
Yes, I know of a few (RT host + The Typical non-RT guest).
One case already has to deal with wbinvd because it runs on older HW without trapping support. The issue is mitigated here by CPU isolation. But shared caches remain problematic, also with this new approach here (wbinvd not only flushes KVM's memory...).
Quoted 5 lines
Quoted 3 lines
So I guess we'll have to settle for wbinvd, just avoiding it when the hardware allows us to.
Yes, for now I agree we can just use wbinvd to emulate wbinvd :)
Having a switch even for the case the guest may have a potential need would be useful. Maybe controllable by user space, maybe something like emulate / skip / skip+report.
There might be guests issuing wbinvd from drivers of devices that aren't passed through (I'm thinking of graphic adapters) while they don't for the passed-through ones. In that case, ignoring should be fine.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux