Patchwork [v9,2/4] KVM: PPC: epapr: Add idle hcall support for host

login
register
mail settings
Submitter Stuart Yoder
Date March 7, 2012, 11:12 p.m.
Message ID <1331161979-12759-3-git-send-email-stuart.yoder@freescale.com>
Download mbox | patch
Permalink /patch/145360/
State New
Headers show

Comments

Stuart Yoder - March 7, 2012, 11:12 p.m.
From: Liu Yu-B13201 <Yu.Liu@freescale.com>

And add a new flag definition in kvm_ppc_pvinfo to indicate
whether host support EV_IDLE hcall.

Signed-off-by: Liu Yu <yu.liu@freescale.com>
[stuart.yoder@freescale.com: fixes for conditions allowing idle]
Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
---
-v9: pad hcall instruction array with nops

 arch/powerpc/include/asm/Kbuild     |    1 +
 arch/powerpc/include/asm/kvm_para.h |   14 ++++++++++++--
 arch/powerpc/kvm/booke.c            |    6 ++++++
 arch/powerpc/kvm/powerpc.c          |   19 ++++++++++++++++---
 include/linux/kvm.h                 |    2 ++
 5 files changed, 37 insertions(+), 5 deletions(-)
Alexander Graf - March 7, 2012, 11:27 p.m.
On 08.03.2012, at 00:12, Stuart Yoder wrote:

> From: Liu Yu-B13201 <Yu.Liu@freescale.com>
> 
> And add a new flag definition in kvm_ppc_pvinfo to indicate
> whether host support EV_IDLE hcall.
> 
> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> [stuart.yoder@freescale.com: fixes for conditions allowing idle]
> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
> ---
> -v9: pad hcall instruction array with nops
> 
> arch/powerpc/include/asm/Kbuild     |    1 +
> arch/powerpc/include/asm/kvm_para.h |   14 ++++++++++++--
> arch/powerpc/kvm/booke.c            |    6 ++++++
> arch/powerpc/kvm/powerpc.c          |   19 ++++++++++++++++---
> include/linux/kvm.h                 |    2 ++
> 5 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
> index d51df17..b68cb1c 100644
> --- a/arch/powerpc/include/asm/Kbuild
> +++ b/arch/powerpc/include/asm/Kbuild
> @@ -34,3 +34,4 @@ header-y += termios.h
> header-y += types.h
> header-y += ucontext.h
> header-y += unistd.h
> +header-y += epapr_hcalls.h
> diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
> index b3b4667..5617c8c 100644
> --- a/arch/powerpc/include/asm/kvm_para.h
> +++ b/arch/powerpc/include/asm/kvm_para.h
> @@ -90,9 +90,19 @@ struct kvm_vcpu_arch_shared {
> };
> 
> #define KVM_SC_MAGIC_R0		0x4b564d21 /* "KVM!" */
> -#define HC_VENDOR_KVM		(42 << 16)
> +
> +#include <asm/epapr_hcalls.h>
> +
> +/* ePAPR Hypercall Vendor ID */
> +#define HC_VENDOR_EPAPR		(EV_EPAPR_VENDOR_ID << 16)
> +#define HC_VENDOR_KVM		(EV_KVM_VENDOR_ID << 16)
> +
> +/* ePAPR Hypercall Token */
> +#define HC_EV_IDLE		EV_IDLE
> +
> +/* ePAPR Hypercall Return Codes */
> #define HC_EV_SUCCESS		0
> -#define HC_EV_UNIMPLEMENTED	12
> +#define HC_EV_UNIMPLEMENTED	EV_UNIMPLEMENTED

Shouldn't all this rather be in an epapr header file?

> 
> #define KVM_FEATURE_MAGIC_PAGE	1
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index f0ef0be..9fde645 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -593,6 +593,12 @@ static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
> 	unsigned int priority;
> 
> 	if (vcpu->requests) {
> +		/* kvm_vcpu_block() sets KVM_REQ_UNHALT, but it is
> + 		 * not cleared elsewhere as on x86.  Clear it here
> + 		 * for now, otherwise we never go idle.
> + 		 */
> +		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);

Shouldn't the same thing hit us on non-booke as well? Also, it sounds unrelated to me and probably shouldn't be in this patch.

> +
> 		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
> 			smp_mb();
> 			update_timer_ints(vcpu);
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index ee489f4..2595916 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -48,8 +48,7 @@ static unsigned int perfmon_refcount;
> 
> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> {
> -	bool ret = !(v->arch.shared->msr & MSR_WE) ||
> -		   !!(v->arch.pending_exceptions) ||
> +	bool ret = !!(v->arch.pending_exceptions) ||
> 		   v->requests;

Huh?


Alex

--
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
Scott Wood - March 7, 2012, 11:37 p.m.
On 03/07/2012 05:27 PM, Alexander Graf wrote:
> On 08.03.2012, at 00:12, Stuart Yoder wrote:
>>
>> 	if (vcpu->requests) {
>> +		/* kvm_vcpu_block() sets KVM_REQ_UNHALT, but it is
>> + 		 * not cleared elsewhere as on x86.  Clear it here
>> + 		 * for now, otherwise we never go idle.
>> + 		 */
>> +		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
> 
> Shouldn't the same thing hit us on non-booke as well? Also, it sounds unrelated to me and probably shouldn't be in this patch.

Until recently we didn't check for requests in kvm_arch_vcpu_runnable().

And yes, book3s will need this too.

>> 		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>> 			smp_mb();
>> 			update_timer_ints(vcpu);
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index ee489f4..2595916 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -48,8 +48,7 @@ static unsigned int perfmon_refcount;
>>
>> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>> {
>> -	bool ret = !(v->arch.shared->msr & MSR_WE) ||
>> -		   !!(v->arch.pending_exceptions) ||
>> +	bool ret = !!(v->arch.pending_exceptions) ||
>> 		   v->requests;
> 
> Huh?

MSR_WE is not going to get set if the idle hcall is used, so this check
was preventing us from blocking.

The check isn't needed anyway, as nothing can actually change MSR_WE
while we're in kvm_vcpu_block(), which is the only user of
kvm_arch_vcpu_runnable(), and the MSR_WE path won't call
kvm_vcpu_block() if MSR_WE isn't set.

-Scott

--
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
Alexander Graf - March 7, 2012, 11:38 p.m.
On 08.03.2012, at 00:37, Scott Wood wrote:

> On 03/07/2012 05:27 PM, Alexander Graf wrote:
>> On 08.03.2012, at 00:12, Stuart Yoder wrote:
>>> 
>>> 	if (vcpu->requests) {
>>> +		/* kvm_vcpu_block() sets KVM_REQ_UNHALT, but it is
>>> + 		 * not cleared elsewhere as on x86.  Clear it here
>>> + 		 * for now, otherwise we never go idle.
>>> + 		 */
>>> +		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
>> 
>> Shouldn't the same thing hit us on non-booke as well? Also, it sounds unrelated to me and probably shouldn't be in this patch.
> 
> Until recently we didn't check for requests in kvm_arch_vcpu_runnable().
> 
> And yes, book3s will need this too.
> 
>>> 		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>>> 			smp_mb();
>>> 			update_timer_ints(vcpu);
>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>> index ee489f4..2595916 100644
>>> --- a/arch/powerpc/kvm/powerpc.c
>>> +++ b/arch/powerpc/kvm/powerpc.c
>>> @@ -48,8 +48,7 @@ static unsigned int perfmon_refcount;
>>> 
>>> int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
>>> {
>>> -	bool ret = !(v->arch.shared->msr & MSR_WE) ||
>>> -		   !!(v->arch.pending_exceptions) ||
>>> +	bool ret = !!(v->arch.pending_exceptions) ||
>>> 		   v->requests;
>> 
>> Huh?
> 
> MSR_WE is not going to get set if the idle hcall is used, so this check
> was preventing us from blocking.
> 
> The check isn't needed anyway, as nothing can actually change MSR_WE
> while we're in kvm_vcpu_block(), which is the only user of
> kvm_arch_vcpu_runnable(), and the MSR_WE path won't call
> kvm_vcpu_block() if MSR_WE isn't set.

Ah, this is only removing the MSR_WE check. Ok.


Alex

--
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
Stuart Yoder - March 15, 2012, 3:56 p.m.
On Wed, Mar 7, 2012 at 5:27 PM, Alexander Graf <agraf@suse.de> wrote:
>> #define KVM_SC_MAGIC_R0               0x4b564d21 /* "KVM!" */
>> -#define HC_VENDOR_KVM                (42 << 16)
>> +
>> +#include <asm/epapr_hcalls.h>
>> +
>> +/* ePAPR Hypercall Vendor ID */
>> +#define HC_VENDOR_EPAPR              (EV_EPAPR_VENDOR_ID << 16)
>> +#define HC_VENDOR_KVM                (EV_KVM_VENDOR_ID << 16)
>> +
>> +/* ePAPR Hypercall Token */
>> +#define HC_EV_IDLE           EV_IDLE
>> +
>> +/* ePAPR Hypercall Return Codes */
>> #define HC_EV_SUCCESS         0
>> -#define HC_EV_UNIMPLEMENTED  12
>> +#define HC_EV_UNIMPLEMENTED  EV_UNIMPLEMENTED
>
> Shouldn't all this rather be in an epapr header file?

Think I see a cleaner way of handling this, will address in next respin.

Stuart
--
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/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
index d51df17..b68cb1c 100644
--- a/arch/powerpc/include/asm/Kbuild
+++ b/arch/powerpc/include/asm/Kbuild
@@ -34,3 +34,4 @@  header-y += termios.h
 header-y += types.h
 header-y += ucontext.h
 header-y += unistd.h
+header-y += epapr_hcalls.h
diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h
index b3b4667..5617c8c 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h
@@ -90,9 +90,19 @@  struct kvm_vcpu_arch_shared {
 };
 
 #define KVM_SC_MAGIC_R0		0x4b564d21 /* "KVM!" */
-#define HC_VENDOR_KVM		(42 << 16)
+
+#include <asm/epapr_hcalls.h>
+
+/* ePAPR Hypercall Vendor ID */
+#define HC_VENDOR_EPAPR		(EV_EPAPR_VENDOR_ID << 16)
+#define HC_VENDOR_KVM		(EV_KVM_VENDOR_ID << 16)
+
+/* ePAPR Hypercall Token */
+#define HC_EV_IDLE		EV_IDLE
+
+/* ePAPR Hypercall Return Codes */
 #define HC_EV_SUCCESS		0
-#define HC_EV_UNIMPLEMENTED	12
+#define HC_EV_UNIMPLEMENTED	EV_UNIMPLEMENTED
 
 #define KVM_FEATURE_MAGIC_PAGE	1
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index f0ef0be..9fde645 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -593,6 +593,12 @@  static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
 	unsigned int priority;
 
 	if (vcpu->requests) {
+		/* kvm_vcpu_block() sets KVM_REQ_UNHALT, but it is
+ 		 * not cleared elsewhere as on x86.  Clear it here
+ 		 * for now, otherwise we never go idle.
+ 		 */
+		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+
 		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
 			smp_mb();
 			update_timer_ints(vcpu);
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index ee489f4..2595916 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -48,8 +48,7 @@  static unsigned int perfmon_refcount;
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
-	bool ret = !(v->arch.shared->msr & MSR_WE) ||
-		   !!(v->arch.pending_exceptions) ||
+	bool ret = !!(v->arch.pending_exceptions) ||
 		   v->requests;
 
 #ifdef CONFIG_BOOKE
@@ -100,6 +99,10 @@  int kvmppc_kvm_pv(struct kvm_vcpu *vcpu)
 
 		/* Second return value is in r4 */
 		break;
+	case HC_VENDOR_EPAPR | HC_EV_IDLE:
+		r = HC_EV_SUCCESS;
+		kvm_vcpu_block(vcpu);
+		break;
 	default:
 		r = HC_EV_UNIMPLEMENTED;
 		break;
@@ -888,9 +891,16 @@  out:
 
 static int kvm_vm_ioctl_get_pvinfo(struct kvm_ppc_pvinfo *pvinfo)
 {
+	u32 inst_nop = 0x60000000;
+#ifdef CONFIG_KVM_BOOKE_HV
+	u32 inst_sc1 = 0x44000022;
+	pvinfo->hcall[0] = inst_sc1;
+	pvinfo->hcall[1] = inst_nop;
+	pvinfo->hcall[2] = inst_nop;
+	pvinfo->hcall[3] = inst_nop;
+#else
 	u32 inst_lis = 0x3c000000;
 	u32 inst_ori = 0x60000000;
-	u32 inst_nop = 0x60000000;
 	u32 inst_sc = 0x44000002;
 	u32 inst_imm_mask = 0xffff;
 
@@ -907,6 +917,9 @@  static int kvm_vm_ioctl_get_pvinfo(struct kvm_ppc_pvinfo *pvinfo)
 	pvinfo->hcall[1] = inst_ori | (KVM_SC_MAGIC_R0 & inst_imm_mask);
 	pvinfo->hcall[2] = inst_sc;
 	pvinfo->hcall[3] = inst_nop;
+#endif
+
+	pvinfo->flags = KVM_PPC_PVINFO_FLAGS_EV_IDLE;
 
 	return 0;
 }
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 28ced78..32b8f54 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -438,6 +438,8 @@  struct kvm_ppc_pvinfo {
 	__u8  pad[108];
 };
 
+#define KVM_PPC_PVINFO_FLAGS_EV_IDLE   (1<<0)
+
 #define KVMIO 0xAE
 
 /*