Patchwork [16/30] KVM: PPC: e500mc: Add doorbell emulation support

login
register
mail settings
Submitter Alexander Graf
Date Feb. 17, 2012, 5:13 p.m.
Message ID <1329498837-11717-17-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/141974/
State Not Applicable
Headers show

Comments

Alexander Graf - Feb. 17, 2012, 5:13 p.m.
When one vcpu wants to kick another, it can issue a special IPI instruction
called msgsnd. This patch emulates this instruction, its clearing counterpart
and the infrastructure required to actually trigger that interrupt inside
a guest vcpu.

With this patch, SMP guests on e500mc work.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/booke.c        |    6 +++
 arch/powerpc/kvm/e500_emulate.c |   68 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 0 deletions(-)
Scott Wood - Feb. 17, 2012, 9:55 p.m.
On 02/17/2012 11:13 AM, Alexander Graf wrote:
> When one vcpu wants to kick another, it can issue a special IPI instruction
> called msgsnd. This patch emulates this instruction, its clearing counterpart
> and the infrastructure required to actually trigger that interrupt inside
> a guest vcpu.
> 
> With this patch, SMP guests on e500mc work.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/booke.c        |    6 +++
>  arch/powerpc/kvm/e500_emulate.c |   68 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 3dd200d..ce1599d 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -326,6 +326,9 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
>  		int_class = INT_CLASS_NONCRIT;
>  		break;
>  	case BOOKE_IRQPRIO_CRITICAL:
> +#ifdef CONFIG_KVM_E500MC
> +	case BOOKE_IRQPRIO_DBELL_CRIT:
> +#endif
>  		allowed = vcpu->arch.shared->msr & MSR_CE;
>  		allowed = allowed && !crit;
>  		msr_mask = MSR_GS | MSR_ME;
> @@ -342,6 +345,9 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
>  		keep_irq = true;
>  		/* fall through */
>  	case BOOKE_IRQPRIO_EXTERNAL:
> +#ifdef CONFIG_KVM_E500MC
> +	case BOOKE_IRQPRIO_DBELL:
> +#endif

This isn't e500mc specific -- it's in the ISA as "Embedded.Processor
Control".

Any harm in just removing the ifdef (similar to tlbilx)?

>  		allowed = vcpu->arch.shared->msr & MSR_EE;
>  		allowed = allowed && !crit;
>  		msr_mask = MSR_GS | MSR_CE | MSR_ME | MSR_DE;
> diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
> index 98b6c1c..29d5604 100644
> --- a/arch/powerpc/kvm/e500_emulate.c
> +++ b/arch/powerpc/kvm/e500_emulate.c
> @@ -14,16 +14,74 @@
>  
>  #include <asm/kvm_ppc.h>
>  #include <asm/disassemble.h>
> +#include <asm/dbell.h>
>  
>  #include "booke.h"
>  #include "e500.h"
>  
> +#define XOP_MSGSND  206
> +#define XOP_MSGCLR  238
>  #define XOP_TLBIVAX 786
>  #define XOP_TLBSX   914
>  #define XOP_TLBRE   946
>  #define XOP_TLBWE   978
>  #define XOP_TLBILX  18
>  
> +#ifdef CONFIG_KVM_E500MC
> +static int dbell2prio(ulong param)
> +{
> +	int msg = param & PPC_DBELL_TYPE(-1);

Maybe introduce PPC_DBELL_TYPE_MASK or GET_PPC_DBELL_TYPE?

> +	int prio = -1;
> +
> +	switch (msg) {
> +	case PPC_DBELL_TYPE(PPC_DBELL):
> +		prio = BOOKE_IRQPRIO_DBELL;
> +		break;
> +	case PPC_DBELL_TYPE(PPC_DBELL_CRIT):
> +		prio = BOOKE_IRQPRIO_DBELL_CRIT;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return prio;
> +}
> +
> +static int kvmppc_e500_emul_msgclr(struct kvm_vcpu *vcpu, int rb)
> +{
> +	ulong param = vcpu->arch.gpr[rb];
> +	int prio = dbell2prio(param);
> +
> +	if (prio < 0)
> +		return EMULATE_FAIL;
> +
> +	clear_bit(prio, &vcpu->arch.pending_exceptions);
> +	return EMULATE_DONE;
> +}
> +
> +static int kvmppc_e500_emul_msgsnd(struct kvm_vcpu *vcpu, int rb)
> +{
> +	ulong param = vcpu->arch.gpr[rb];
> +	int prio = dbell2prio(rb);
> +	int pir = param & 0x3fff;

Introduce PPC_DBELL_PIR_MASK or GET_PPC_DBELL_PIR?

> +	int i;
> +	struct kvm_vcpu *cvcpu;
> +
> +	if (prio < 0)
> +		return EMULATE_FAIL;
> +
> +	kvm_for_each_vcpu(i, cvcpu, vcpu->kvm) {
> +		int cpir = cvcpu->arch.shared->pir;
> +		if ((param & PPC_DBELL_MSG_BRDCAST) || (cpir == pir)) {
> +			set_bit(prio, &cvcpu->arch.pending_exceptions);
> +			kvm_vcpu_kick(cvcpu);
> +		}
> +	}

Should this be a kvm_make_request instead (with a separate
pending_doorbell bool in vcpu that msgclr can act on), considering
earlier discussion of phasing out atomics on pending_exceptions, in
favor of requests?

-Scott
Scott Wood - Feb. 17, 2012, 9:57 p.m.
On 02/17/2012 03:55 PM, Scott Wood wrote:
> Should this be a kvm_make_request instead (with a separate
> pending_doorbell bool in vcpu that msgclr can act on), considering
> earlier discussion of phasing out atomics on pending_exceptions, in
> favor of requests?

Ignore the bit about msgclr -- it acts on the local vcpu, so no need for
pending_doorbell.  It can just be a plain request.

-Scott
Alexander Graf - Feb. 20, 2012, 11:49 a.m.
On 17.02.2012, at 22:55, Scott Wood wrote:

> On 02/17/2012 11:13 AM, Alexander Graf wrote:
>> When one vcpu wants to kick another, it can issue a special IPI instruction
>> called msgsnd. This patch emulates this instruction, its clearing counterpart
>> and the infrastructure required to actually trigger that interrupt inside
>> a guest vcpu.
>> 
>> With this patch, SMP guests on e500mc work.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/kvm/booke.c        |    6 +++
>> arch/powerpc/kvm/e500_emulate.c |   68 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 74 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index 3dd200d..ce1599d 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -326,6 +326,9 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
>> 		int_class = INT_CLASS_NONCRIT;
>> 		break;
>> 	case BOOKE_IRQPRIO_CRITICAL:
>> +#ifdef CONFIG_KVM_E500MC
>> +	case BOOKE_IRQPRIO_DBELL_CRIT:
>> +#endif
>> 		allowed = vcpu->arch.shared->msr & MSR_CE;
>> 		allowed = allowed && !crit;
>> 		msr_mask = MSR_GS | MSR_ME;
>> @@ -342,6 +345,9 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
>> 		keep_irq = true;
>> 		/* fall through */
>> 	case BOOKE_IRQPRIO_EXTERNAL:
>> +#ifdef CONFIG_KVM_E500MC
>> +	case BOOKE_IRQPRIO_DBELL:
>> +#endif
> 
> This isn't e500mc specific -- it's in the ISA as "Embedded.Processor
> Control".
> 
> Any harm in just removing the ifdef (similar to tlbilx)?

Well, to me this is more of an indication "this should become a runtime check one day" in case we want to combine the two targets. On e500v2, we don't know what a doorbell interrupt is, so we really shouldn't be delivering one either.

> 
>> 		allowed = vcpu->arch.shared->msr & MSR_EE;
>> 		allowed = allowed && !crit;
>> 		msr_mask = MSR_GS | MSR_CE | MSR_ME | MSR_DE;
>> diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
>> index 98b6c1c..29d5604 100644
>> --- a/arch/powerpc/kvm/e500_emulate.c
>> +++ b/arch/powerpc/kvm/e500_emulate.c
>> @@ -14,16 +14,74 @@
>> 
>> #include <asm/kvm_ppc.h>
>> #include <asm/disassemble.h>
>> +#include <asm/dbell.h>
>> 
>> #include "booke.h"
>> #include "e500.h"
>> 
>> +#define XOP_MSGSND  206
>> +#define XOP_MSGCLR  238
>> #define XOP_TLBIVAX 786
>> #define XOP_TLBSX   914
>> #define XOP_TLBRE   946
>> #define XOP_TLBWE   978
>> #define XOP_TLBILX  18
>> 
>> +#ifdef CONFIG_KVM_E500MC
>> +static int dbell2prio(ulong param)
>> +{
>> +	int msg = param & PPC_DBELL_TYPE(-1);
> 
> Maybe introduce PPC_DBELL_TYPE_MASK or GET_PPC_DBELL_TYPE?

TYPE_MASK I'd say.

> 
>> +	int prio = -1;
>> +
>> +	switch (msg) {
>> +	case PPC_DBELL_TYPE(PPC_DBELL):
>> +		prio = BOOKE_IRQPRIO_DBELL;
>> +		break;
>> +	case PPC_DBELL_TYPE(PPC_DBELL_CRIT):
>> +		prio = BOOKE_IRQPRIO_DBELL_CRIT;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return prio;
>> +}
>> +
>> +static int kvmppc_e500_emul_msgclr(struct kvm_vcpu *vcpu, int rb)
>> +{
>> +	ulong param = vcpu->arch.gpr[rb];
>> +	int prio = dbell2prio(param);
>> +
>> +	if (prio < 0)
>> +		return EMULATE_FAIL;
>> +
>> +	clear_bit(prio, &vcpu->arch.pending_exceptions);
>> +	return EMULATE_DONE;
>> +}
>> +
>> +static int kvmppc_e500_emul_msgsnd(struct kvm_vcpu *vcpu, int rb)
>> +{
>> +	ulong param = vcpu->arch.gpr[rb];
>> +	int prio = dbell2prio(rb);
>> +	int pir = param & 0x3fff;
> 
> Introduce PPC_DBELL_PIR_MASK or GET_PPC_DBELL_PIR?

Good point :)

> 
>> +	int i;
>> +	struct kvm_vcpu *cvcpu;
>> +
>> +	if (prio < 0)
>> +		return EMULATE_FAIL;
>> +
>> +	kvm_for_each_vcpu(i, cvcpu, vcpu->kvm) {
>> +		int cpir = cvcpu->arch.shared->pir;
>> +		if ((param & PPC_DBELL_MSG_BRDCAST) || (cpir == pir)) {
>> +			set_bit(prio, &cvcpu->arch.pending_exceptions);
>> +			kvm_vcpu_kick(cvcpu);
>> +		}
>> +	}
> 
> Should this be a kvm_make_request instead (with a separate
> pending_doorbell bool in vcpu that msgclr can act on), considering
> earlier discussion of phasing out atomics on pending_exceptions, in
> favor of requests?

Yeah, I was thinking about that too, but right now we already have some direct use of pending_exceptions in different places around the code. So today, that is our public interface.

I'd rather go in and clean up the whole thing to make pending_exceptions private in a separate cleanup round, rather than having it part of e500mc support.


Alex
Scott Wood - Feb. 20, 2012, 3:39 p.m.
On Mon, Feb 20, 2012 at 12:49:46PM +0100, Alexander Graf wrote:
> 
> On 17.02.2012, at 22:55, Scott Wood wrote:
> 
> > On 02/17/2012 11:13 AM, Alexander Graf wrote:
> >> 	case BOOKE_IRQPRIO_EXTERNAL:
> >> +#ifdef CONFIG_KVM_E500MC
> >> +	case BOOKE_IRQPRIO_DBELL:
> >> +#endif
> > 
> > This isn't e500mc specific -- it's in the ISA as "Embedded.Processor
> > Control".
> > 
> > Any harm in just removing the ifdef (similar to tlbilx)?
> 
> Well, to me this is more of an indication "this should become a runtime
> check one day" in case we want to combine the two targets.  On e500v2,
> we don't know what a doorbell interrupt is, so we really shouldn't be
> delivering one either.

Should at least have a comment saying that eventually the decision should
be based on ISA category support rather than e500mc.

> > Should this be a kvm_make_request instead (with a separate
> > pending_doorbell bool in vcpu that msgclr can act on), considering
> > earlier discussion of phasing out atomics on pending_exceptions, in
> > favor of requests?
> 
> Yeah, I was thinking about that too, but right now we already have some
> direct use of pending_exceptions in different places around the code. 
> So today, that is our public interface.
> 
> I'd rather go in and clean up the whole thing to make
> pending_exceptions private in a separate cleanup round, rather than
> having it part of e500mc support.

We already use requests for timers, and it seems simple enough to add one
for doorbells now rather than come back and clean it up later (it's not
tied to what we'd have to do for external IRQs), but if you'd rather do
it later that's fine with me.

-Scott
Alexander Graf - Feb. 20, 2012, 3:42 p.m.
On 20.02.2012, at 16:39, Scott Wood wrote:

> On Mon, Feb 20, 2012 at 12:49:46PM +0100, Alexander Graf wrote:
>> 
>> On 17.02.2012, at 22:55, Scott Wood wrote:
>> 
>>> On 02/17/2012 11:13 AM, Alexander Graf wrote:
>>>> 	case BOOKE_IRQPRIO_EXTERNAL:
>>>> +#ifdef CONFIG_KVM_E500MC
>>>> +	case BOOKE_IRQPRIO_DBELL:
>>>> +#endif
>>> 
>>> This isn't e500mc specific -- it's in the ISA as "Embedded.Processor
>>> Control".
>>> 
>>> Any harm in just removing the ifdef (similar to tlbilx)?
>> 
>> Well, to me this is more of an indication "this should become a runtime
>> check one day" in case we want to combine the two targets.  On e500v2,
>> we don't know what a doorbell interrupt is, so we really shouldn't be
>> delivering one either.
> 
> Should at least have a comment saying that eventually the decision should
> be based on ISA category support rather than e500mc.

Hrm. How about a new #define that is a bit more verbose?

#ifdef CONFIG_KVM_E500MC
#define KVM_CAT_DOORBELL
#endif

> 
>>> Should this be a kvm_make_request instead (with a separate
>>> pending_doorbell bool in vcpu that msgclr can act on), considering
>>> earlier discussion of phasing out atomics on pending_exceptions, in
>>> favor of requests?
>> 
>> Yeah, I was thinking about that too, but right now we already have some
>> direct use of pending_exceptions in different places around the code. 
>> So today, that is our public interface.
>> 
>> I'd rather go in and clean up the whole thing to make
>> pending_exceptions private in a separate cleanup round, rather than
>> having it part of e500mc support.
> 
> We already use requests for timers, and it seems simple enough to add one
> for doorbells now rather than come back and clean it up later (it's not
> tied to what we'd have to do for external IRQs), but if you'd rather do
> it later that's fine with me.

Yeah, because I want the external interface to include the kick, which we don't model today. So instead of touching basically every line of code again, I'd rather do it in one roll.


Alex

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 3dd200d..ce1599d 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -326,6 +326,9 @@  static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
 		int_class = INT_CLASS_NONCRIT;
 		break;
 	case BOOKE_IRQPRIO_CRITICAL:
+#ifdef CONFIG_KVM_E500MC
+	case BOOKE_IRQPRIO_DBELL_CRIT:
+#endif
 		allowed = vcpu->arch.shared->msr & MSR_CE;
 		allowed = allowed && !crit;
 		msr_mask = MSR_GS | MSR_ME;
@@ -342,6 +345,9 @@  static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
 		keep_irq = true;
 		/* fall through */
 	case BOOKE_IRQPRIO_EXTERNAL:
+#ifdef CONFIG_KVM_E500MC
+	case BOOKE_IRQPRIO_DBELL:
+#endif
 		allowed = vcpu->arch.shared->msr & MSR_EE;
 		allowed = allowed && !crit;
 		msr_mask = MSR_GS | MSR_CE | MSR_ME | MSR_DE;
diff --git a/arch/powerpc/kvm/e500_emulate.c b/arch/powerpc/kvm/e500_emulate.c
index 98b6c1c..29d5604 100644
--- a/arch/powerpc/kvm/e500_emulate.c
+++ b/arch/powerpc/kvm/e500_emulate.c
@@ -14,16 +14,74 @@ 
 
 #include <asm/kvm_ppc.h>
 #include <asm/disassemble.h>
+#include <asm/dbell.h>
 
 #include "booke.h"
 #include "e500.h"
 
+#define XOP_MSGSND  206
+#define XOP_MSGCLR  238
 #define XOP_TLBIVAX 786
 #define XOP_TLBSX   914
 #define XOP_TLBRE   946
 #define XOP_TLBWE   978
 #define XOP_TLBILX  18
 
+#ifdef CONFIG_KVM_E500MC
+static int dbell2prio(ulong param)
+{
+	int msg = param & PPC_DBELL_TYPE(-1);
+	int prio = -1;
+
+	switch (msg) {
+	case PPC_DBELL_TYPE(PPC_DBELL):
+		prio = BOOKE_IRQPRIO_DBELL;
+		break;
+	case PPC_DBELL_TYPE(PPC_DBELL_CRIT):
+		prio = BOOKE_IRQPRIO_DBELL_CRIT;
+		break;
+	default:
+		break;
+	}
+
+	return prio;
+}
+
+static int kvmppc_e500_emul_msgclr(struct kvm_vcpu *vcpu, int rb)
+{
+	ulong param = vcpu->arch.gpr[rb];
+	int prio = dbell2prio(param);
+
+	if (prio < 0)
+		return EMULATE_FAIL;
+
+	clear_bit(prio, &vcpu->arch.pending_exceptions);
+	return EMULATE_DONE;
+}
+
+static int kvmppc_e500_emul_msgsnd(struct kvm_vcpu *vcpu, int rb)
+{
+	ulong param = vcpu->arch.gpr[rb];
+	int prio = dbell2prio(rb);
+	int pir = param & 0x3fff;
+	int i;
+	struct kvm_vcpu *cvcpu;
+
+	if (prio < 0)
+		return EMULATE_FAIL;
+
+	kvm_for_each_vcpu(i, cvcpu, vcpu->kvm) {
+		int cpir = cvcpu->arch.shared->pir;
+		if ((param & PPC_DBELL_MSG_BRDCAST) || (cpir == pir)) {
+			set_bit(prio, &cvcpu->arch.pending_exceptions);
+			kvm_vcpu_kick(cvcpu);
+		}
+	}
+
+	return EMULATE_DONE;
+}
+#endif
+
 int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
                            unsigned int inst, int *advance)
 {
@@ -36,6 +94,16 @@  int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	case 31:
 		switch (get_xop(inst)) {
 
+#ifdef CONFIG_KVM_E500MC
+		case XOP_MSGSND:
+			emulated = kvmppc_e500_emul_msgsnd(vcpu, get_rb(inst));
+			break;
+
+		case XOP_MSGCLR:
+			emulated = kvmppc_e500_emul_msgclr(vcpu, get_rb(inst));
+			break;
+#endif
+
 		case XOP_TLBRE:
 			emulated = kvmppc_e500_emul_tlbre(vcpu);
 			break;