Skip to contentSkip to conversation

get rid of mmu_only parameter in emulator_write_emulated()

PATCH Applied Gleb Natapov 3 participants 7 messages Apr 13, 2010 – Apr 14, 2010

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Gleb Natapov

author
#0 lore raw

PATCH 1/1 get rid of mmu_only parameter in emulator_write_emulated()

May be I am missing something here, but it seams we can call kvm_mmu_pte_write() directly from emulator_cmpxchg_emulated() instead of passing mmu_only down to emulator_write_emulated_onepage() and call it there.

Signed-off-by Gleb Natapov <[email protected]>
arch/x86/kvm/x86.c +11 -25
index a4e55ae..8ab30e1 100644
@@ -3327,8 +3327,7 @@ int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
 static int emulator_write_emulated_onepage(unsigned long addr,
 					   const void *val,
 					   unsigned int bytes,
-					   struct kvm_vcpu *vcpu,
-					   bool mmu_only)
+					   struct kvm_vcpu *vcpu)
 {
 	gpa_t                 gpa;
 	u32 error_code;

@@ -3344,10 +3343,6 @@ static int emulator_write_emulated_onepage(unsigned long addr,
 	if ((gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE)
 		goto mmio;
 
-	if (mmu_only) {
-		kvm_mmu_pte_write(vcpu, gpa, val, bytes, 1);
-		return X86EMUL_CONTINUE;
-	}
 	if (emulator_write_phys(vcpu, gpa, val, bytes))
 		return X86EMUL_CONTINUE;
 

@@ -3368,35 +3363,24 @@ mmio:
 	return X86EMUL_CONTINUE;
 }
 
-int __emulator_write_emulated(unsigned long addr,
-				   const void *val,
-				   unsigned int bytes,
-				   struct kvm_vcpu *vcpu,
-				   bool mmu_only)
+int emulator_write_emulated(unsigned long addr,
+			    const void *val,
+			    unsigned int bytes,
+			    struct kvm_vcpu *vcpu)
 {
 	/* Crossing a page boundary? */
 	if (((addr + bytes - 1) ^ addr) & PAGE_MASK) {
 		int rc, now;
 
 		now = -addr & ~PAGE_MASK;
-		rc = emulator_write_emulated_onepage(addr, val, now, vcpu,
-						     mmu_only);
+		rc = emulator_write_emulated_onepage(addr, val, now, vcpu);
 		if (rc != X86EMUL_CONTINUE)
 			return rc;
 		addr += now;
 		val += now;
 		bytes -= now;
 	}
-	return emulator_write_emulated_onepage(addr, val, bytes, vcpu,
-					       mmu_only);
-}
-
-int emulator_write_emulated(unsigned long addr,
-				   const void *val,
-				   unsigned int bytes,
-				   struct kvm_vcpu *vcpu)
-{
-	return __emulator_write_emulated(addr, val, bytes, vcpu, false);
+	return emulator_write_emulated_onepage(addr, val, bytes, vcpu);
 }
 EXPORT_SYMBOL_GPL(emulator_write_emulated);
 

@@ -3460,7 +3444,9 @@ static int emulator_cmpxchg_emulated(unsigned long addr,
 	if (!exchanged)
 		return X86EMUL_CMPXCHG_FAILED;
 
-	return __emulator_write_emulated(addr, new, bytes, vcpu, true);
+	kvm_mmu_pte_write(vcpu, gpa, new, bytes, 1);
+
+	return X86EMUL_CONTINUE;
 
 emul_write:
 	printk_once(KERN_WARNING "kvm: emulating exchange as write\n");

@@ -4174,7 +4160,7 @@ int kvm_fix_hypercall(struct kvm_vcpu *vcpu)
 
 	kvm_x86_ops->patch_hypercall(vcpu, instruction);
 
-	return __emulator_write_emulated(rip, instruction, 3, vcpu, false);
+	return emulator_write_emulated(rip, instruction, 3, vcpu);
 }
 
 void realmode_lgdt(struct kvm_vcpu *vcpu, u16 limit, unsigned long base)

--
Gleb.

#1 lore raw
Quoted 7 lines · Gleb Natapov

May be I am missing something here, but it seams we can call kvm_mmu_pte_write() directly from emulator_cmpxchg_emulated() instead of passing mmu_only down to emulator_write_emulated_onepage() and call it there.

arch/x86/kvm/x86.c static int emulator_cmpxchg_emulated(unsigned long addr, · line 3451

Quoted 9 lines
arch/x86/kvm/x86.c +3 -1
@@ -3460,7 +3444,9 @@ static int emulator_cmpxchg_emulated(unsigned long addr,
  	if (!exchanged)
  		return X86EMUL_CMPXCHG_FAILED;
 
-	return __emulator_write_emulated(addr, new, bytes, vcpu, true);
+	kvm_mmu_pte_write(vcpu, gpa, new, bytes, 1);
+
+	return X86EMUL_CONTINUE;

The written range might cross a page boundary, which kvm_mmu_pte_write() is not prepared to handle.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
author
#2 lore raw
Quoted 11 lines · Avi Kivity
Quoted 10 lines · Gleb Natapov

May be I am missing something here, but it seams we can call kvm_mmu_pte_write() directly from emulator_cmpxchg_emulated() instead of passing mmu_only down to emulator_write_emulated_onepage() and call it there.

arch/x86/kvm/x86.c +0 -0
@@ -3460,7 +3444,9 @@ static int emulator_cmpxchg_emulated(unsigned long addr,
 	if (!exchanged)
 		return X86EMUL_CMPXCHG_FAILED;
Quoted 9 lines

arch/x86/kvm/x86.c static int emulator_cmpxchg_emulated(unsigned long addr, · line 3450

Quoted 5 lines
arch/x86/kvm/x86.c +3 -1
-	return __emulator_write_emulated(addr, new, bytes, vcpu, true);
+	kvm_mmu_pte_write(vcpu, gpa, new, bytes, 1);
+
+	return X86EMUL_CONTINUE;

The written range might cross a page boundary, which kvm_mmu_pte_write() is not prepared to handle.

Don't we emulate exchange as write in this case?

	if (((gpa + bytes - 1) & PAGE_MASK) != (gpa & PAGE_MASK))
		goto emul_write;

--
Gleb.

#3 lore raw
Quoted 24 lines · Gleb Natapov
Quoted 12 lines · Avi Kivity
Quoted 10 lines · Gleb Natapov

May be I am missing something here, but it seams we can call kvm_mmu_pte_write() directly from emulator_cmpxchg_emulated() instead of passing mmu_only down to emulator_write_emulated_onepage() and call it there.

arch/x86/kvm/x86.c +0 -0
@@ -3460,7 +3444,9 @@ static int emulator_cmpxchg_emulated(unsigned long addr,
  	if (!exchanged)
  		return X86EMUL_CMPXCHG_FAILED;
Quoted 10 lines

arch/x86/kvm/x86.c static int emulator_cmpxchg_emulated(unsigned long addr,

Quoted 6 lines
arch/x86/kvm/x86.c +3 -1
-	return __emulator_write_emulated(addr, new, bytes, vcpu, true);
+	kvm_mmu_pte_write(vcpu, gpa, new, bytes, 1);
+
+	return X86EMUL_CONTINUE;

The written range might cross a page boundary, which kvm_mmu_pte_write() is not prepared to handle.

Quoted 5 lines

Don't we emulate exchange as write in this case?

	if (((gpa + bytes - 1)&  PAGE_MASK) != (gpa&  PAGE_MASK))
		goto emul_write;

We do, but that's unrelated. We still have to invalidate potential ptes on both pages

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
author
#4 lore raw
Quoted 21 lines · Avi Kivity
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov

May be I am missing something here, but it seams we can call kvm_mmu_pte_write() directly from emulator_cmpxchg_emulated() instead of passing mmu_only down to emulator_write_emulated_onepage() and call it there.

arch/x86/kvm/x86.c static int emulator_cmpxchg_emulated(unsigned long addr, · line 3450

Quoted 8 lines
arch/x86/kvm/x86.c +3 -1
@@ -3460,7 +3444,9 @@ static int emulator_cmpxchg_emulated(unsigned long addr,
 	if (!exchanged)
 		return X86EMUL_CMPXCHG_FAILED;
 
-	return __emulator_write_emulated(addr, new, bytes, vcpu, true);
+	kvm_mmu_pte_write(vcpu, gpa, new, bytes, 1);
+
+	return X86EMUL_CONTINUE;

The written range might cross a page boundary, which kvm_mmu_pte_write() is not prepared to handle.

Quoted 8 lines
Quoted 4 lines

Don't we emulate exchange as write in this case?

	if (((gpa + bytes - 1)&  PAGE_MASK) != (gpa&  PAGE_MASK))
		goto emul_write;

We do, but that's unrelated. We still have to invalidate potential ptes on both pages

The code path executed in case of cmpxchg crossing page boundary is not touched by the patch as far as I can see. In this case emulator_write_emulated() is executed with mmu_only false and kvm_mmu_pte_write() is called from emulator_write_phys().

--
Gleb.

#5 lore raw
Quoted 6 lines · Gleb Natapov

The code path executed in case of cmpxchg crossing page boundary is not touched by the patch as far as I can see. In this case emulator_write_emulated() is executed with mmu_only false and kvm_mmu_pte_write() is called from emulator_write_phys().

Yes. Patch is correct.

Signature · 1 line
error compiling committee.c: too many arguments to function