diff mbox

[v2,03/22] KVM: x86: fix broken read emulation spans a page boundary

Message ID 4E01FC39.2080709@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiao Guangrong June 22, 2011, 2:29 p.m. UTC
If the range spans a boundary, the mmio access can be broke, fix it as
write emulation.

And we already get the guest physical address, so use it to read guest data
directly to avoid walking guest page table again

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 arch/x86/kvm/x86.c |   41 ++++++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 9 deletions(-)

Comments

Avi Kivity June 29, 2011, 8:21 a.m. UTC | #1
On 06/22/2011 05:29 PM, Xiao Guangrong wrote:
> If the range spans a boundary, the mmio access can be broke, fix it as
> write emulation.
>
> And we already get the guest physical address, so use it to read guest data
> directly to avoid walking guest page table again
>
> Signed-off-by: Xiao Guangrong<xiaoguangrong@cn.fujitsu.com>
> ---
>   arch/x86/kvm/x86.c |   41 ++++++++++++++++++++++++++++++++---------
>   1 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0b803f0..eb27be4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3944,14 +3944,13 @@ out:
>   }
>   EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
>
> -static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
> -				  unsigned long addr,
> -				  void *val,
> -				  unsigned int bytes,
> -				  struct x86_exception *exception)
> +static int emulator_read_emulated_onepage(unsigned long addr,
> +					  void *val,
> +					  unsigned int bytes,
> +					  struct x86_exception *exception,
> +					  struct kvm_vcpu *vcpu)
>   {
> -	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
> -	gpa_t                 gpa;
> +	gpa_t gpa;
>   	int handled;
>
>   	if (vcpu->mmio_read_completed) {
> @@ -3971,8 +3970,7 @@ static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
>   	if ((gpa&  PAGE_MASK) == APIC_DEFAULT_PHYS_BASE)
>   		goto mmio;
>
> -	if (kvm_read_guest_virt(ctxt, addr, val, bytes, exception)
> -	    == X86EMUL_CONTINUE)
> +	if (!kvm_read_guest(vcpu->kvm, gpa, val, bytes))
>   		return X86EMUL_CONTINUE;

This doesn't perform the cpl check.

I suggest dropping this part for now and doing it later.
Xiao Guangrong June 29, 2011, 10:53 a.m. UTC | #2
On 06/29/2011 04:21 PM, Avi Kivity wrote:

>>
>> -    if (kvm_read_guest_virt(ctxt, addr, val, bytes, exception)
>> -        == X86EMUL_CONTINUE)
>> +    if (!kvm_read_guest(vcpu->kvm, gpa, val, bytes))
>>           return X86EMUL_CONTINUE;
> 
> This doesn't perform the cpl check.
> 

Firstly, it calls kvm_mmu_gva_to_gpa_read to translate gva to gpa, and cpl
is checked in this function, it is not enough?

> I suggest dropping this part for now and doing it later.
> 

OK, i will post this part in the separate patchset. :-)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity June 29, 2011, 11:19 a.m. UTC | #3
On 06/29/2011 01:53 PM, Xiao Guangrong wrote:
> On 06/29/2011 04:21 PM, Avi Kivity wrote:
>
> >>
> >>  -    if (kvm_read_guest_virt(ctxt, addr, val, bytes, exception)
> >>  -        == X86EMUL_CONTINUE)
> >>  +    if (!kvm_read_guest(vcpu->kvm, gpa, val, bytes))
> >>            return X86EMUL_CONTINUE;
> >
> >  This doesn't perform the cpl check.
> >
>
> Firstly, it calls kvm_mmu_gva_to_gpa_read to translate gva to gpa, and cpl
> is checked in this function, it is not enough?

You are right, it is enough.  I don't know how I missed it.
diff mbox

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0b803f0..eb27be4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3944,14 +3944,13 @@  out:
 }
 EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
 
-static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
-				  unsigned long addr,
-				  void *val,
-				  unsigned int bytes,
-				  struct x86_exception *exception)
+static int emulator_read_emulated_onepage(unsigned long addr,
+					  void *val,
+					  unsigned int bytes,
+					  struct x86_exception *exception,
+					  struct kvm_vcpu *vcpu)
 {
-	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
-	gpa_t                 gpa;
+	gpa_t gpa;
 	int handled;
 
 	if (vcpu->mmio_read_completed) {
@@ -3971,8 +3970,7 @@  static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
 	if ((gpa & PAGE_MASK) == APIC_DEFAULT_PHYS_BASE)
 		goto mmio;
 
-	if (kvm_read_guest_virt(ctxt, addr, val, bytes, exception)
-	    == X86EMUL_CONTINUE)
+	if (!kvm_read_guest(vcpu->kvm, gpa, val, bytes))
 		return X86EMUL_CONTINUE;
 
 mmio:
@@ -4001,6 +3999,31 @@  mmio:
 	return X86EMUL_IO_NEEDED;
 }
 
+static int emulator_read_emulated(struct x86_emulate_ctxt *ctxt,
+				  unsigned long addr,
+				  void *val,
+				  unsigned int bytes,
+				  struct x86_exception *exception)
+{
+	struct kvm_vcpu *vcpu = emul_to_vcpu(ctxt);
+
+	/* Crossing a page boundary? */
+	if (((addr + bytes - 1) ^ addr) & PAGE_MASK) {
+		int rc, now;
+
+		now = -addr & ~PAGE_MASK;
+		rc = emulator_read_emulated_onepage(addr, val, now, exception,
+							vcpu);
+		if (rc != X86EMUL_CONTINUE)
+			return rc;
+		addr += now;
+		val += now;
+		bytes -= now;
+	}
+	return emulator_read_emulated_onepage(addr, val, bytes, exception,
+						vcpu);
+}
+
 int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa,
 			const void *val, int bytes)
 {