[PATCH] powerpc/xive: store server for masked interrupt in kvmppc_xive_set_xive()

This is needed to map kvmppc_xive_set_xive() behavior
to kvmppc_xics_set_xive().

As we store the server, kvmppc_xive_get_xive() can return
the good value and we can also allow kvmppc_xive_int_on().

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 arch/powerpc/kvm/book3s_xive.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index bf457843e032..2781b8733038 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c

@@ -584,10 +584,14 @@ int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
 	 *       we could initialize interrupts with valid default
 	 */
 
-	if (new_act_prio != MASKED &&
-	    (state->act_server != server ||
-	     state->act_priority != new_act_prio))
-		rc = xive_target_interrupt(kvm, state, server, new_act_prio);
+	if (state->act_server != server ||
+	    state->act_priority != new_act_prio) {
+		if (new_act_prio != MASKED)
+			rc = xive_target_interrupt(kvm, state, server,
+						   new_act_prio);
+		if (!rc)
+			state->act_server = server;
+	}
 
 	/*
 	 * Perform the final unmasking of the interrupt source

@@ -646,14 +650,6 @@ int kvmppc_xive_int_on(struct kvm *kvm, u32 irq)
 
 	pr_devel("int_on(irq=0x%x)\n", irq);
 
-	/*
-	 * Check if interrupt was not targetted
-	 */
-	if (state->act_priority == MASKED) {
-		pr_devel("int_on on untargetted interrupt\n");
-		return -EINVAL;
-	}
-
 	/* If saved_priority is 0xff, do nothing */
 	if (state->saved_priority == MASKED)
 		return 0;

-- 
2.13.6

On Thu, 2017-11-23 at 10:06 +0100, Laurent Vivier wrote:

That leads to another problem with this code. My current implementation
is such that is a target queue is full, it will pick another target.
But here, we still update act_server to the passed-in server and
not the actual target...

So my thinking here was that act_priority was never going to be MASKED
except if the interrupt had never been targetted anywhere at machine
startup time. Thus if act_priority is masked, the act_server field
cannot be trusted.

 	/* If saved_priority is 0xff, do nothing */
 	if (state->saved_priority == MASKED)
 		return 0;

On Fri, Nov 24, 2017 at 07:38:13AM +1100, Benjamin Herrenschmidt wrote:

On Thu, 2017-11-23 at 10:06 +0100, Laurent Vivier wrote:

quoted

This is needed to map kvmppc_xive_set_xive() behavior
to kvmppc_xics_set_xive().

As we store the server, kvmppc_xive_get_xive() can return
the good value and we can also allow kvmppc_xive_int_on().

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 arch/powerpc/kvm/book3s_xive.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index bf457843e032..2781b8733038 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c

@@ -584,10 +584,14 @@ int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
 	 *       we could initialize interrupts with valid default
 	 */
 
-	if (new_act_prio != MASKED &&
-	    (state->act_server != server ||
-	     state->act_priority != new_act_prio))
-		rc = xive_target_interrupt(kvm, state, server, new_act_prio);
+	if (state->act_server != server ||
+	    state->act_priority != new_act_prio) {
+		if (new_act_prio != MASKED)
+			rc = xive_target_interrupt(kvm, state, server,
+						   new_act_prio);
+		if (!rc)
+			state->act_server = server;
+	}

That leads to another problem with this code. My current implementation
is such that is a target queue is full, it will pick another target.
But here, we still update act_server to the passed-in server and
not the actual target...

So does that amount to a NAK?

quoted

 	/*
 	 * Perform the final unmasking of the interrupt source

@@ -646,14 +650,6 @@ int kvmppc_xive_int_on(struct kvm *kvm, u32 irq)
 
 	pr_devel("int_on(irq=0x%x)\n", irq);
 
-	/*
-	 * Check if interrupt was not targetted
-	 */
-	if (state->act_priority == MASKED) {
-		pr_devel("int_on on untargetted interrupt\n");
-		return -EINVAL;
-	}
-

So my thinking here was that act_priority was never going to be MASKED
except if the interrupt had never been targetted anywhere at machine
startup time. Thus if act_priority is masked, the act_server field
cannot be trusted.

quoted

 	/* If saved_priority is 0xff, do nothing */
 	if (state->saved_priority == MASKED)
 		return 0;

How do you think this should be fixed?

Laurent, are you reworking the patch at the moment?

Paul.

On 05/12/2017 04:05, Paul Mackerras wrote:

On Fri, Nov 24, 2017 at 07:38:13AM +1100, Benjamin Herrenschmidt wrote:

quoted

On Thu, 2017-11-23 at 10:06 +0100, Laurent Vivier wrote:

quoted

This is needed to map kvmppc_xive_set_xive() behavior
to kvmppc_xics_set_xive().

As we store the server, kvmppc_xive_get_xive() can return
the good value and we can also allow kvmppc_xive_int_on().

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 arch/powerpc/kvm/book3s_xive.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index bf457843e032..2781b8733038 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c

@@ -584,10 +584,14 @@ int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
 	 *       we could initialize interrupts with valid default
 	 */
 
-	if (new_act_prio != MASKED &&
-	    (state->act_server != server ||
-	     state->act_priority != new_act_prio))
-		rc = xive_target_interrupt(kvm, state, server, new_act_prio);
+	if (state->act_server != server ||
+	    state->act_priority != new_act_prio) {
+		if (new_act_prio != MASKED)
+			rc = xive_target_interrupt(kvm, state, server,
+						   new_act_prio);
+		if (!rc)
+			state->act_server = server;
+	}

That leads to another problem with this code. My current implementation
is such that is a target queue is full, it will pick another target.
But here, we still update act_server to the passed-in server and
not the actual target...

So does that amount to a NAK?

quoted

quoted

 	/*
 	 * Perform the final unmasking of the interrupt source

@@ -646,14 +650,6 @@ int kvmppc_xive_int_on(struct kvm *kvm, u32 irq)
 
 	pr_devel("int_on(irq=0x%x)\n", irq);
 
-	/*
-	 * Check if interrupt was not targetted
-	 */
-	if (state->act_priority == MASKED) {
-		pr_devel("int_on on untargetted interrupt\n");
-		return -EINVAL;
-	}
-

So my thinking here was that act_priority was never going to be MASKED
except if the interrupt had never been targetted anywhere at machine
startup time. Thus if act_priority is masked, the act_server field
cannot be trusted.

quoted

 	/* If saved_priority is 0xff, do nothing */
 	if (state->saved_priority == MASKED)
 		return 0;

How do you think this should be fixed?

Laurent, are you reworking the patch at the moment?

Not for the moment.

The easy way is to forbid to set interrupt value to the MASKED one with
xive_set_xive. I think it's allowed by the specs.

I've got another bug in the XICS emulation: when we migrate a guest
under stress, the pending interrupt is lost and the guest hangs on the
destination side. I'm trying to understand why.

Thanks,
Laurent