KVM: x86: fix missed memory synchronization when patch hypercall

From: Xiao Guangrong <[email protected]>

Currently, memory synchronization is missed in emulator_fix_hypercall, please see the commit 758ccc89b83
(KVM: x86: drop calling kvm_mmu_zap_all in emulator_fix_hypercall)

This patch fixes it by introducing kvm_vcpus_hang_on_page_start() and kvm_vcpus_hang_on_page_end which unmap the patched page from guest and use kvm_flush_remote_tlbs() as the serializing instruction to ensure the memory coherence
[ The SDM said that INVEPT, INVVPID and MOV (to control register, with
the exception of MOV CR8) are the serializing instructions. ]

The mmu-lock is held during host patches the page so that it stops vcpus to fix its further page fault

Quoted 13 lines · Xiao Guangrong

From: Xiao Guangrong <[email protected]>

Currently, memory synchronization is missed in emulator_fix_hypercall, please see the commit 758ccc89b83
(KVM: x86: drop calling kvm_mmu_zap_all in emulator_fix_hypercall)

This patch fixes it by introducing kvm_vcpus_hang_on_page_start() and kvm_vcpus_hang_on_page_end which unmap the patched page from guest and use kvm_flush_remote_tlbs() as the serializing instruction to ensure the memory coherence
[ The SDM said that INVEPT, INVVPID and MOV (to control register, with
the exception of MOV CR8) are the serializing instructions. ]

Quoted 3 lines

The mmu-lock is held during host patches the page so that it stops vcpus to fix its further page fault

I have a patch to implement is much simple and in generic way, not relying on MMU internals.

Quoted 17 lines · Gleb Natapov

Quoted 16 lines · Xiao Guangrong

From: Xiao Guangrong <[email protected]>

Currently, memory synchronization is missed in emulator_fix_hypercall, please see the commit 758ccc89b83
(KVM: x86: drop calling kvm_mmu_zap_all in emulator_fix_hypercall)

This patch fixes it by introducing kvm_vcpus_hang_on_page_start() and kvm_vcpus_hang_on_page_end which unmap the patched page from guest and use kvm_flush_remote_tlbs() as the serializing instruction to ensure the memory coherence
[ The SDM said that INVEPT, INVVPID and MOV (to control register, with
the exception of MOV CR8) are the serializing instructions. ]

The mmu-lock is held during host patches the page so that it stops vcpus to fix its further page fault

Quoted 2 lines

I have a patch to implement is much simple and in generic way, not relying on MMU internals.

I have considered this way but it seems not simple - it needs a new type of request and it forces all vcpus to hang when host is patching the page.

My approach is just reusing the mmu code and requires vcpus to hang only when the patched page is bing accessed.

Quoted 21 lines · Xiao Guangrong

Quoted 19 lines · Gleb Natapov

Quoted 13 lines · Xiao Guangrong

From: Xiao Guangrong <[email protected]>

Currently, memory synchronization is missed in emulator_fix_hypercall, please see the commit 758ccc89b83
(KVM: x86: drop calling kvm_mmu_zap_all in emulator_fix_hypercall)

This patch fixes it by introducing kvm_vcpus_hang_on_page_start() and kvm_vcpus_hang_on_page_end which unmap the patched page from guest and use kvm_flush_remote_tlbs() as the serializing instruction to ensure the memory coherence
[ The SDM said that INVEPT, INVVPID and MOV (to control register, with
the exception of MOV CR8) are the serializing instructions. ]

Quoted 3 lines

The mmu-lock is held during host patches the page so that it stops vcpus to fix its further page fault

I have a patch to implement is much simple and in generic way, not relying on MMU internals.

Quoted 5 lines

I have considered this way but it seems not simple - it needs a new type of request and it forces all vcpus to hang when host is patching the page.

My approach is just reusing the mmu code and requires vcpus to hang only when the patched page is bing accessed.

That's very rare, no point to optimize this code path.

arch/x86/kvm/x86.c +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Quoted 12 lines · Gleb Natapov

arch/x86/kvm/x86.c +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

arch/x86/kvm/x86.c +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

arch/x86/kvm/x86.c +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Quoted 45 lines · Xiao Guangrong

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

arch/x86/kvm/x86.c +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

So flags and registers modifications on a host are obviously irrelevant for a guest.

And for memory ordering we have smp_mb() on a guest entry.

Quoted 61 lines · Gleb Natapov

Quoted 45 lines · Xiao Guangrong

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

arch/x86/kvm/x86.c +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

arch/x86/kvm/x86.c static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Quoted 2 lines

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

"The following instructions are memory-ordering instructions, not serializing instruc- tions. These drain the data memory subsystem. They do not serialize the instruction execution stream:"

No?

Quoted 62 lines · Xiao Guangrong

Quoted 61 lines · Gleb Natapov

Quoted 45 lines · Xiao Guangrong

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

(quoted diff) +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Review static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

(quoted diff) +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 3 lines

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Memory barrier should guaranty that as I said bellow.

Quoted 5 lines

Quoted 2 lines

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

What is "instruction buffer"?

Quoted 7 lines

"The following instructions are memory-ordering instructions, not serializing instruc- tions. These drain the data memory subsystem. They do not serialize the instruction execution stream:"

No?

Yes, but we have no issue with instruction execution stream as I said above. No guest instruction can be in a pipeline while all vcpus are in a host.

Quoted 69 lines · Gleb Natapov

Quoted 62 lines · Xiao Guangrong

Quoted 61 lines · Gleb Natapov

Quoted 45 lines · Xiao Guangrong

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

(quoted diff) +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Review static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

(quoted diff) +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 3 lines

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Memory barrier should guaranty that as I said bellow.

Quoted 1 line

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

What is "instruction buffer"?

I mean "Instruction Cache" (icache). Can memory ordering drain icache? The "data memory subsystem" confused me, does it mean dcache?

Quoted 70 lines · Xiao Guangrong

Quoted 69 lines · Gleb Natapov

Quoted 62 lines · Xiao Guangrong

Quoted 61 lines · Gleb Natapov

Quoted 45 lines · Xiao Guangrong

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

(quoted diff) +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Review static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

(quoted diff) +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 3 lines

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Memory barrier should guaranty that as I said bellow.

Quoted 1 line

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

What is "instruction buffer"?

I mean "Instruction Cache" (icache). Can memory ordering drain icache? The "data memory subsystem" confused me, does it mean dcache?

I think it means all caches.
11.6 says:

A write to a memory location in a code segment that is currently cached in the processor causes the associated cache line (or lines) to be invalidated. This check is based on the physical address of the instruction. In addition, the P6 family and Pentium processors check whether a write to a code segment may modify an instruction that has been prefetched for execution. If the write affects a prefetched instruction, the prefetch queue is invalidated. This latter check is based on the linear address of the instruction. For the Pentium 4 and Intel Xeon processors, a write or a snoop of an instruction in a code segment, where the target instruction is already decoded and resident in the trace cache, invalidates the entire trace cache. The latter behavior means that programs that self-modify code can cause severe degradation of performance when run on the Pentium 4 and Intel Xeon processors.

So icache line is invalidate based on physical address so we are OK.

Prefetched instruction is invalidated based on linear address, but if all vcpus are in a host guest instruction cannot be prefetched.

Quoted 97 lines · Gleb Natapov

Quoted 70 lines · Xiao Guangrong

Quoted 69 lines · Gleb Natapov

Quoted 62 lines · Xiao Guangrong

Quoted 61 lines · Gleb Natapov

Quoted 45 lines · Xiao Guangrong

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

(quoted diff) +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Review static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

(quoted diff) +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 3 lines

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Memory barrier should guaranty that as I said bellow.

Quoted 1 line

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

What is "instruction buffer"?

I mean "Instruction Cache" (icache). Can memory ordering drain icache? The "data memory subsystem" confused me, does it mean dcache?

I think it means all caches.
11.6 says:

A write to a memory location in a code segment that is currently cached in the processor causes the associated cache line (or lines) to be invalidated. This check is based on the physical address of the instruction. In addition, the P6 family and Pentium processors check whether a write to a code segment may modify an instruction that has been prefetched for execution. If the write affects a prefetched instruction, the prefetch queue is invalidated. This latter check is based on the linear address of the instruction. For the Pentium 4 and Intel Xeon processors, a write or a snoop of an instruction in a code segment, where the target instruction is already decoded and resident in the trace cache, invalidates the entire trace cache. The latter behavior means that programs that self-modify code can cause severe degradation of performance when run on the Pentium 4 and Intel Xeon processors.

Quoted 1 line

So icache line is invalidate based on physical address so we are OK.

Yes.

Quoted 2 lines

Prefetched instruction is invalidated based on linear address, but if all vcpus are in a host guest instruction cannot be prefetched.

But what happen if the instruction has been prefetched before vcpu exits to host? Then, after returns to guest, it executes the old instruction.

Can it happen?

Quoted 105 lines · Xiao Guangrong

Quoted 97 lines · Gleb Natapov

Quoted 70 lines · Xiao Guangrong

Quoted 69 lines · Gleb Natapov

Quoted 62 lines · Xiao Guangrong

Quoted 61 lines · Gleb Natapov

Quoted 45 lines · Xiao Guangrong

Quoted 39 lines · Gleb Natapov

Quoted 29 lines · Xiao Guangrong

Quoted 12 lines · Gleb Natapov

(quoted diff) +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Review static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

(quoted diff) +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 3 lines

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Memory barrier should guaranty that as I said bellow.

Quoted 1 line

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

What is "instruction buffer"?

I mean "Instruction Cache" (icache). Can memory ordering drain icache? The "data memory subsystem" confused me, does it mean dcache?

I think it means all caches.
11.6 says:

A write to a memory location in a code segment that is currently cached in the processor causes the associated cache line (or lines) to be invalidated. This check is based on the physical address of the instruction. In addition, the P6 family and Pentium processors check whether a write to a code segment may modify an instruction that has been prefetched for execution. If the write affects a prefetched instruction, the prefetch queue is invalidated. This latter check is based on the linear address of the instruction. For the Pentium 4 and Intel Xeon processors, a write or a snoop of an instruction in a code segment, where the target instruction is already decoded and resident in the trace cache, invalidates the entire trace cache. The latter behavior means that programs that self-modify code can cause severe degradation of performance when run on the Pentium 4 and Intel Xeon processors.

Quoted 1 line

So icache line is invalidate based on physical address so we are OK.

Yes.

Quoted 2 lines

Prefetched instruction is invalidated based on linear address, but if all vcpus are in a host guest instruction cannot be prefetched.

Quoted 4 lines

But what happen if the instruction has been prefetched before vcpu exits to host? Then, after returns to guest, it executes the old instruction.

Can it happen?

I do not thing so, prefetched instructions is not a cache, but I'll ask Intel.

Quoted 105 lines · Gleb Natapov

Quoted 97 lines · Xiao Guangrong

Quoted 70 lines · Gleb Natapov

Quoted 69 lines · Xiao Guangrong

Quoted 62 lines · Gleb Natapov

Quoted 61 lines · Xiao Guangrong

Quoted 45 lines · Gleb Natapov

Quoted 39 lines · Xiao Guangrong

Quoted 29 lines · Gleb Natapov

Quoted 13 lines

(quoted diff) +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Review static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

(quoted diff) +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 3 lines

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Memory barrier should guaranty that as I said bellow.

Quoted 1 line

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

What is "instruction buffer"?

I mean "Instruction Cache" (icache). Can memory ordering drain icache? The "data memory subsystem" confused me, does it mean dcache?

I think it means all caches.
11.6 says:

A write to a memory location in a code segment that is currently cached in the processor causes the associated cache line (or lines) to be invalidated. This check is based on the physical address of the instruction. In addition, the P6 family and Pentium processors check whether a write to a code segment may modify an instruction that has been prefetched for execution. If the write affects a prefetched instruction, the prefetch queue is invalidated. This latter check is based on the linear address of the instruction. For the Pentium 4 and Intel Xeon processors, a write or a snoop of an instruction in a code segment, where the target instruction is already decoded and resident in the trace cache, invalidates the entire trace cache. The latter behavior means that programs that self-modify code can cause severe degradation of performance when run on the Pentium 4 and Intel Xeon processors.

Quoted 1 line

So icache line is invalidate based on physical address so we are OK.

Yes.

Quoted 2 lines

Prefetched instruction is invalidated based on linear address, but if all vcpus are in a host guest instruction cannot be prefetched.

Quoted 4 lines

But what happen if the instruction has been prefetched before vcpu exits to host? Then, after returns to guest, it executes the old instruction.

Can it happen?

I do not thing so, prefetched instructions is not a cache, but I'll ask Intel.

Il 09/06/2013 14:27, Gleb Natapov ha scritto:

Quoted 106 lines · Xiao Guangrong

Quoted 105 lines · Gleb Natapov

Quoted 97 lines · Xiao Guangrong

Quoted 70 lines · Gleb Natapov

Quoted 69 lines · Xiao Guangrong

Quoted 62 lines · Gleb Natapov

Quoted 61 lines · Xiao Guangrong

Quoted 45 lines · Gleb Natapov

Quoted 39 lines · Xiao Guangrong

Quoted 29 lines · Gleb Natapov

Quoted 13 lines

(quoted diff) +8 -0

+static int emulator_fix_hypercall(struct x86_emulate_ctxt *ctxt)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+	return kvm_exec_with_stopped_vcpu(vcpu->kvm,
+			emulator_fix_hypercall_cb, ctxt);
+}
+
+
 /*
  * Check if userspace requested an interrupt window, and that the
  * interrupt window is open.

Review static int vcpu_enter_guest(struct kvm_vcpu *vcpu) · line 5774

Quoted 6 lines

(quoted diff) +3 -0

@@ -5761,6 +5769,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_deliver_pmi(vcpu);
 		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
 			vcpu_scan_ioapic(vcpu);
+		if (kvm_check_request(KVM_REQ_STOP_VCPU, vcpu)){
+			mutex_lock(&vcpu->kvm->lock);
+			mutex_unlock(&vcpu->kvm->lock);

We should execute a serializing instruction here?

Quoted 6 lines

virt/kvm/kvm_main.c +0 -0

@@ -222,6 +222,18 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 

Quoted 9 lines

virt/kvm/kvm_main.c void kvm_make_scan_ioapic_request(struct kvm *kvm) · line 231

Quoted 7 lines

virt/kvm/kvm_main.c +7 -0

+int kvm_exec_with_stopped_vcpu(struct kvm *kvm, int (*cb)(void *), void *data)
+{
+	int r;
+
+	mutex_lock(&kvm->lock);
+	make_all_cpus_request(kvm, KVM_REQ_STOP_VCPU);
+	r = cb(data);

And here?

Quoted 4 lines

Since the serialisation instruction the SDM suggest to use is CPUID I think the point here is to flush CPU pipeline. Since all vcpus are out of a guest mode I think out of order execution of modified instruction is no an issue here.

Quoted 7 lines

I checked the SDM that it did not said VMLAUNCH/VMRESUME are the serializing instructions both in VM-Entry description and Instruction reference, instead it said the VMX related serializing instructions are:
INVEPT, INVVPID.

So, i guess the explicit serializing instruction is needed here.

Again the question is what for? SDM says:

The Intel 64 and IA-32 architectures define several serializing instructions. These instructions force the processor to complete all modifications to flags, registers, and memory by previous instructions and to drain all buffered writes to memory before the next instruction is fetched and executed.

Quoted 3 lines

Quoted 1 line

So flags and registers modifications on a host are obviously irrelevant for a guest.

Okay. Hmm... but what can guarantee that "drain all buffered writes to memory"?

Memory barrier should guaranty that as I said bellow.

Quoted 1 line

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

And for memory ordering we have smp_mb() on a guest entry.

If i understand the SDM correctly, memory-ordering instructions can not drain instruction buffer, it only drains "data memory subsystem":

What is "instruction buffer"?

I mean "Instruction Cache" (icache). Can memory ordering drain icache? The "data memory subsystem" confused me, does it mean dcache?

I think it means all caches.
11.6 says:

A write to a memory location in a code segment that is currently cached in the processor causes the associated cache line (or lines) to be invalidated. This check is based on the physical address of the instruction. In addition, the P6 family and Pentium processors check whether a write to a code segment may modify an instruction that has been prefetched for execution. If the write affects a prefetched instruction, the prefetch queue is invalidated. This latter check is based on the linear address of the instruction. For the Pentium 4 and Intel Xeon processors, a write or a snoop of an instruction in a code segment, where the target instruction is already decoded and resident in the trace cache, invalidates the entire trace cache. The latter behavior means that programs that self-modify code can cause severe degradation of performance when run on the Pentium 4 and Intel Xeon processors.

Quoted 1 line

So icache line is invalidate based on physical address so we are OK.

Yes.

Quoted 2 lines

Prefetched instruction is invalidated based on linear address, but if all vcpus are in a host guest instruction cannot be prefetched.

Quoted 6 lines

Quoted 4 lines

But what happen if the instruction has been prefetched before vcpu exits to host? Then, after returns to guest, it executes the old instruction.

Can it happen?

I do not thing so, prefetched instructions is not a cache, but I'll ask Intel.

Any news?

Anyway, if this were the case (which seems strange, but you never know), CPUID would not help. The hypothetical guest prefetch queue would not be flushed, and you'd need INVEPT/INVVPID as Xiao mentioned upthread.