Patchwork [3/5] KVM: PPC: Book3S PR: Don't keep scanning HPTEG after we find a match

login
register
mail settings
Submitter Paul Mackerras
Date June 22, 2013, 7:14 a.m.
Message ID <20130622071448.GD2791@iris.ozlabs.ibm.com>
Download mbox | patch
Permalink /patch/253355/
State New
Headers show

Comments

Paul Mackerras - June 22, 2013, 7:14 a.m.
The loop in kvmppc_mmu_book3s_64_xlate() that looks up a translation
in the guest hashed page table (HPT) keeps going if it finds an
HPTE that matches but doesn't allow access.  This is incorrect; it
is different from what the hardware does, and there should never be
more than one matching HPTE anyway.  This fixes it to stop when any
matching HPTE is found.

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/kvm/book3s_64_mmu.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
Alexander Graf - June 22, 2013, 5:42 p.m.
On 22.06.2013, at 09:14, Paul Mackerras wrote:

> The loop in kvmppc_mmu_book3s_64_xlate() that looks up a translation
> in the guest hashed page table (HPT) keeps going if it finds an
> HPTE that matches but doesn't allow access.  This is incorrect; it
> is different from what the hardware does, and there should never be
> more than one matching HPTE anyway.  This fixes it to stop when any
> matching HPTE is found.

IIRC I put in that logic to make it compatible with how QEMU handled HTAB lookups. Since then this has changed in QEMU though, and doing it this way is architecturally as correct as the other (the spec just says undefined behavior for duplicate entries), so I'm fine with the change.

However, does book3s_32 differ here? I would like to keep them at least behave identically for undefined behavior. So in this case, unless you know of a difference for 32bit, please also provide a patch that does this change in book3s_32_mmu.c.


Alex

> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
> arch/powerpc/kvm/book3s_64_mmu.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_mmu.c b/arch/powerpc/kvm/book3s_64_mmu.c
> index b871721..2e93bb5 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu.c
> @@ -167,7 +167,6 @@ static int kvmppc_mmu_book3s_64_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
> 	int i;
> 	u8 key = 0;
> 	bool found = false;
> -	bool perm_err = false;
> 	int second = 0;
> 	ulong mp_ea = vcpu->arch.magic_page_ea;
> 
> @@ -248,11 +247,6 @@ do_second:
> 				break;
> 			}
> 
> -			if (!gpte->may_read) {
> -				perm_err = true;
> -				continue;
> -			}
> -
> 			dprintk("KVM MMU: Translated 0x%lx [0x%llx] -> 0x%llx "
> 				"-> 0x%lx\n",
> 				eaddr, avpn, gpte->vpage, gpte->raddr);
> @@ -281,6 +275,8 @@ do_second:
> 		if (pteg[i+1] != oldr)
> 			copy_to_user((void __user *)ptegp, pteg, sizeof(pteg));
> 
> +		if (!gpte->may_read)
> +			return -EPERM;
> 		return 0;
> 	} else {
> 		dprintk("KVM MMU: No PTE found (ea=0x%lx sdr1=0x%llx "
> @@ -296,13 +292,7 @@ do_second:
> 		}
> 	}
> 
> -
> no_page_found:
> -
> -
> -	if (perm_err)
> -		return -EPERM;
> -
> 	return -ENOENT;
> 
> no_seg_found:
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/book3s_64_mmu.c b/arch/powerpc/kvm/book3s_64_mmu.c
index b871721..2e93bb5 100644
--- a/arch/powerpc/kvm/book3s_64_mmu.c
+++ b/arch/powerpc/kvm/book3s_64_mmu.c
@@ -167,7 +167,6 @@  static int kvmppc_mmu_book3s_64_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
 	int i;
 	u8 key = 0;
 	bool found = false;
-	bool perm_err = false;
 	int second = 0;
 	ulong mp_ea = vcpu->arch.magic_page_ea;
 
@@ -248,11 +247,6 @@  do_second:
 				break;
 			}
 
-			if (!gpte->may_read) {
-				perm_err = true;
-				continue;
-			}
-
 			dprintk("KVM MMU: Translated 0x%lx [0x%llx] -> 0x%llx "
 				"-> 0x%lx\n",
 				eaddr, avpn, gpte->vpage, gpte->raddr);
@@ -281,6 +275,8 @@  do_second:
 		if (pteg[i+1] != oldr)
 			copy_to_user((void __user *)ptegp, pteg, sizeof(pteg));
 
+		if (!gpte->may_read)
+			return -EPERM;
 		return 0;
 	} else {
 		dprintk("KVM MMU: No PTE found (ea=0x%lx sdr1=0x%llx "
@@ -296,13 +292,7 @@  do_second:
 		}
 	}
 
-
 no_page_found:
-
-
-	if (perm_err)
-		return -EPERM;
-
 	return -ENOENT;
 
 no_seg_found: