[PATCH 0/3] Fixes for PPC Book3S KVM

[PATCH 0/3] Fixes for PPC Book3S KVM

[Paul Mackerras]

Here are three small fixes for the PPC Book 3S code.  The first should
go into 3.16 if possible, I think, or if not, certainly 3.17.  The
remaining two are less urgent and should go into 3.17.

The patch series is against Alex Graf's kvm-ppc-queue branch.

Paul.

 Documentation/virtual/kvm/api.txt   |  3 ++-
 arch/powerpc/include/uapi/asm/kvm.h |  1 +
 arch/powerpc/kvm/book3s.c           | 25 ++++++++++++-------------
 arch/powerpc/kvm/book3s_hv.c        | 13 +++++++++++--
 arch/powerpc/kvm/book3s_pr.c        |  2 ++
 arch/powerpc/kvm/book3s_pr_papr.c   |  9 +++++++--
 6 files changed, 35 insertions(+), 18 deletions(-)

[PATCH 1/3] KVM: PPC: Book3S: Fix LPCR one_reg interface

[Paul Mackerras]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

Unfortunately, the LPCR got defined as a 32-bit register in the
one_reg interface.  This is unfortunate because KVM allows userspace
to control the DPFD (default prefetch depth) field, which is in the
upper 32 bits.  The result is that DPFD always get set to 0, which
reduces performance in the guest.

We can't just change KVM_REG_PPC_LPCR to be a 64-bit register ID,
since that would break existing userspace binaries.  Instead we define
a new KVM_REG_PPC_LPCR_64 id which is 64-bit.  Userspace can still use
the old KVM_REG_PPC_LPCR id, but it now only modifies those fields in
the bottom 32 bits that userspace can modify (ILE, TC and AIL).
If userspace uses the new KVM_REG_PPC_LPCR_64 id, it can modify DPFD
as well.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 Documentation/virtual/kvm/api.txt   |  3 ++-
 arch/powerpc/include/uapi/asm/kvm.h |  1 +
 arch/powerpc/kvm/book3s_hv.c        | 13 +++++++++++--
 arch/powerpc/kvm/book3s_pr.c        |  2 ++
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 6955318..884f819 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1869,7 +1869,8 @@ registers, find a list below:
   PPC   | KVM_REG_PPC_PID	| 64
   PPC   | KVM_REG_PPC_ACOP	| 64
   PPC   | KVM_REG_PPC_VRSAVE	| 32
-  PPC   | KVM_REG_PPC_LPCR	| 64
+  PPC   | KVM_REG_PPC_LPCR	| 32
+  PPC   | KVM_REG_PPC_LPCR_64	| 64
   PPC   | KVM_REG_PPC_PPR	| 64
   PPC   | KVM_REG_PPC_ARCH_COMPAT 32
   PPC   | KVM_REG_PPC_DABRX     | 32
diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
index 2bc4a94..de7d426 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -548,6 +548,7 @@ struct kvm_get_htab_header {
 
 #define KVM_REG_PPC_VRSAVE	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb4)
 #define KVM_REG_PPC_LPCR	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xb5)
+#define KVM_REG_PPC_LPCR_64	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb5)
 #define KVM_REG_PPC_PPR		(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xb6)
 
 /* Architecture compatibility level */
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index f1281c4..0c5266e 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -863,7 +863,8 @@ static int kvm_arch_vcpu_ioctl_set_sregs_hv(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static void kvmppc_set_lpcr(struct kvm_vcpu *vcpu, u64 new_lpcr)
+static void kvmppc_set_lpcr(struct kvm_vcpu *vcpu, u64 new_lpcr,
+		bool preserve_top32)
 {
 	struct kvmppc_vcore *vc = vcpu->arch.vcore;
 	u64 mask;
@@ -898,6 +899,10 @@ static void kvmppc_set_lpcr(struct kvm_vcpu *vcpu, u64 new_lpcr)
 	mask = LPCR_DPFD | LPCR_ILE | LPCR_TC;
 	if (cpu_has_feature(CPU_FTR_ARCH_207S))
 		mask |= LPCR_AIL;
+
+	/* Broken 32-bit version of LPCR must not clear top bits */
+	if (preserve_top32)
+		mask &= 0xFFFFFFFF;
 	vc->lpcr = (vc->lpcr & ~mask) | (new_lpcr & mask);
 	spin_unlock(&vc->lock);
 }
@@ -1011,6 +1016,7 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
 		*val = get_reg_val(id, vcpu->arch.vcore->tb_offset);
 		break;
 	case KVM_REG_PPC_LPCR:
+	case KVM_REG_PPC_LPCR_64:
 		*val = get_reg_val(id, vcpu->arch.vcore->lpcr);
 		break;
 	case KVM_REG_PPC_PPR:
@@ -1216,7 +1222,10 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
 			ALIGN(set_reg_val(id, *val), 1UL << 24);
 		break;
 	case KVM_REG_PPC_LPCR:
-		kvmppc_set_lpcr(vcpu, set_reg_val(id, *val));
+		kvmppc_set_lpcr(vcpu, set_reg_val(id, *val), true);
+		break;
+	case KVM_REG_PPC_LPCR_64:
+		kvmppc_set_lpcr(vcpu, set_reg_val(id, *val), false);
 		break;
 	case KVM_REG_PPC_PPR:
 		vcpu->arch.ppr = set_reg_val(id, *val);
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index e40765f..84c7a47 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -1310,6 +1310,7 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
 		*val = get_reg_val(id, to_book3s(vcpu)->hior);
 		break;
 	case KVM_REG_PPC_LPCR:
+	case KVM_REG_PPC_LPCR_64:
 		/*
 		 * We are only interested in the LPCR_ILE bit
 		 */
@@ -1345,6 +1346,7 @@ static int kvmppc_set_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
 		to_book3s(vcpu)->hior_explicit = true;
 		break;
 	case KVM_REG_PPC_LPCR:
+	case KVM_REG_PPC_LPCR_64:
 		kvmppc_set_lpcr_pr(vcpu, set_reg_val(id, *val));
 		break;
 	default:
-- 
2.0.1

[PATCH 2/3] KVM: PPC: Book3S PR: Take SRCU read lock around RTAS kvm_read_guest() call

[Paul Mackerras]

This does for PR KVM what c9438092cae4 ("KVM: PPC: Book3S HV: Take SRCU
read lock around kvm_read_guest() call") did for HV KVM, that is,
eliminate a "suspicious rcu_dereference_check() usage!" warning by
taking the SRCU lock around the call to kvmppc_rtas_hcall().

It also fixes a return of RESUME_HOST to return EMULATE_FAIL instead,
since kvmppc_h_pr() is supposed to return EMULATE_* values.

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/kvm/book3s_pr_papr.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_pr_papr.c b/arch/powerpc/kvm/book3s_pr_papr.c
index 6d0143f..ce3c893 100644
--- a/arch/powerpc/kvm/book3s_pr_papr.c
+++ b/arch/powerpc/kvm/book3s_pr_papr.c
@@ -267,6 +267,8 @@ static int kvmppc_h_pr_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd)
 
 int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd)
 {
+	int rc, idx;
+
 	if (cmd <= MAX_HCALL_OPCODE &&
 	    !test_bit(cmd/4, vcpu->kvm->arch.enabled_hcalls))
 		return EMULATE_FAIL;
@@ -299,8 +301,11 @@ int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned long cmd)
 		break;
 	case H_RTAS:
 		if (list_empty(&vcpu->kvm->arch.rtas_tokens))
-			return RESUME_HOST;
-		if (kvmppc_rtas_hcall(vcpu))
+			break;
+		idx = srcu_read_lock(&vcpu->kvm->srcu);
+		rc = kvmppc_rtas_hcall(vcpu);
+		srcu_read_unlock(&vcpu->kvm->srcu, idx);
+		if (rc)
 			break;
 		kvmppc_set_gpr(vcpu, 3, 0);
 		return EMULATE_DONE;
-- 
2.0.1

[PATCH 3/3] KVM: PPC: Book3S: Make kvmppc_ld return a more accurate error indication

[Paul Mackerras]

At present, kvmppc_ld calls kvmppc_xlate, and if kvmppc_xlate returns
any error indication, it returns -ENOENT, which is taken to mean an
HPTE not found error.  However, the error could have been a segment
found (no SLB entry) or a permission error.  Similarly,
kvmppc_pte_to_hva currently does permission checking, but any error
from it is taken by kvmppc_ld to mean that the access is an emulated
MMIO access.  Also, kvmppc_ld does no execute permission checking.

This fixes these problems by (a) returning any error from kvmppc_xlate
directly, (b) moving the permission check from kvmppc_pte_to_hva
into kvmppc_ld, and (c) adding an execute permission check to kvmppc_ld.

This is similar to what was done for kvmppc_st() by commit 82ff911317c3
("KVM: PPC: Deflect page write faults properly in kvmppc_st").

Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/kvm/book3s.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 31facfc..087f8f9 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -413,17 +413,10 @@ static hva_t kvmppc_bad_hva(void)
 	return PAGE_OFFSET;
 }
 
-static hva_t kvmppc_pte_to_hva(struct kvm_vcpu *vcpu, struct kvmppc_pte *pte,
-			       bool read)
+static hva_t kvmppc_pte_to_hva(struct kvm_vcpu *vcpu, struct kvmppc_pte *pte)
 {
 	hva_t hpage;
 
-	if (read && !pte->may_read)
-		goto err;
-
-	if (!read && !pte->may_write)
-		goto err;
-
 	hpage = gfn_to_hva(vcpu->kvm, pte->raddr >> PAGE_SHIFT);
 	if (kvm_is_error_hva(hpage))
 		goto err;
@@ -462,15 +455,23 @@ int kvmppc_ld(struct kvm_vcpu *vcpu, ulong *eaddr, int size, void *ptr,
 {
 	struct kvmppc_pte pte;
 	hva_t hva = *eaddr;
+	int rc;
 
 	vcpu->stat.ld++;
 
-	if (kvmppc_xlate(vcpu, *eaddr, data, false, &pte))
-		goto nopte;
+	rc = kvmppc_xlate(vcpu, *eaddr, data, false, &pte);
+	if (rc)
+		return rc;
 
 	*eaddr = pte.raddr;
 
-	hva = kvmppc_pte_to_hva(vcpu, &pte, true);
+	if (!pte.may_read)
+		return -EPERM;
+
+	if (!data && !pte.may_execute)
+		return -ENOEXEC;
+
+	hva = kvmppc_pte_to_hva(vcpu, &pte);
 	if (kvm_is_error_hva(hva))
 		goto mmio;
 
@@ -481,8 +482,6 @@ int kvmppc_ld(struct kvm_vcpu *vcpu, ulong *eaddr, int size, void *ptr,
 
 	return EMULATE_DONE;
 
-nopte:
-	return -ENOENT;
 mmio:
 	return EMULATE_DO_MMIO;
 }
-- 
2.0.1

Re: [PATCH 3/3] KVM: PPC: Book3S: Make kvmppc_ld return a more accurate error indication

[Alexander Graf]

On 19.07.14 09:59, Paul Mackerras wrote:
> At present, kvmppc_ld calls kvmppc_xlate, and if kvmppc_xlate returns
> any error indication, it returns -ENOENT, which is taken to mean an
> HPTE not found error.  However, the error could have been a segment
> found (no SLB entry) or a permission error.  Similarly,
> kvmppc_pte_to_hva currently does permission checking, but any error
> from it is taken by kvmppc_ld to mean that the access is an emulated
> MMIO access.  Also, kvmppc_ld does no execute permission checking.
>
> This fixes these problems by (a) returning any error from kvmppc_xlate
> directly, (b) moving the permission check from kvmppc_pte_to_hva
> into kvmppc_ld, and (c) adding an execute permission check to kvmppc_ld.
>
> This is similar to what was done for kvmppc_st() by commit 82ff911317c3
> ("KVM: PPC: Deflect page write faults properly in kvmppc_st").
>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
>   arch/powerpc/kvm/book3s.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 31facfc..087f8f9 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -413,17 +413,10 @@ static hva_t kvmppc_bad_hva(void)
>   	return PAGE_OFFSET;
>   }
>   
> -static hva_t kvmppc_pte_to_hva(struct kvm_vcpu *vcpu, struct kvmppc_pte *pte,
> -			       bool read)
> +static hva_t kvmppc_pte_to_hva(struct kvm_vcpu *vcpu, struct kvmppc_pte *pte)
>   {
>   	hva_t hpage;
>   
> -	if (read && !pte->may_read)
> -		goto err;
> -
> -	if (!read && !pte->may_write)
> -		goto err;
> -
>   	hpage = gfn_to_hva(vcpu->kvm, pte->raddr >> PAGE_SHIFT);
>   	if (kvm_is_error_hva(hpage))
>   		goto err;
> @@ -462,15 +455,23 @@ int kvmppc_ld(struct kvm_vcpu *vcpu, ulong *eaddr, int size, void *ptr,
>   {
>   	struct kvmppc_pte pte;
>   	hva_t hva = *eaddr;
> +	int rc;
>   
>   	vcpu->stat.ld++;
>   
> -	if (kvmppc_xlate(vcpu, *eaddr, data, false, &pte))
> -		goto nopte;
> +	rc = kvmppc_xlate(vcpu, *eaddr, data, false, &pte);
> +	if (rc)
> +		return rc;
>   
>   	*eaddr = pte.raddr;
>   
> -	hva = kvmppc_pte_to_hva(vcpu, &pte, true);
> +	if (!pte.may_read)
> +		return -EPERM;
> +
> +	if (!data && !pte.may_execute)
> +		return -ENOEXEC;

We should probably do a full audit of all that code and decide who is 
responsible for returning errors where. IIRC our MMU frontends already 
check pte.may* and return error codes respectively.

However, double-checking doesn't hurt for now, so I've applied this 
patch regardless.


Alex

Re: [PATCH 0/3] Fixes for PPC Book3S KVM

[Alexander Graf]

On 19.07.14 09:59, Paul Mackerras wrote:
> Here are three small fixes for the PPC Book 3S code.  The first should
> go into 3.16 if possible, I think, or if not, certainly 3.17.  The
> remaining two are less urgent and should go into 3.17.
>
> The patch series is against Alex Graf's kvm-ppc-queue branch.

Thanks, applied all to kvm-ppc-queue with the first two marked as CC: 
stable.


Alex