Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752428AbdLEDFY (ORCPT ); Mon, 4 Dec 2017 22:05:24 -0500 Received: from ozlabs.org ([103.22.144.67]:34003 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752344AbdLEDFT (ORCPT ); Mon, 4 Dec 2017 22:05:19 -0500 Date: Tue, 5 Dec 2017 14:05:12 +1100 From: Paul Mackerras To: Benjamin Herrenschmidt Cc: Laurent Vivier , linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org, Paolo Bonzini , Thomas Huth , Sam Bobroff Subject: Re: [PATCH] powerpc/xive: store server for masked interrupt in kvmppc_xive_set_xive() Message-ID: <20171205030512.daghjczlensy7252@rohan> References: <20171123090633.873-1-lvivier@redhat.com> <1511469493.2466.47.camel@kernel.crashing.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1511469493.2466.47.camel@kernel.crashing.org> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2503 Lines: 70 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: > > 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 > > --- > > 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? > > /* > > * 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. > > > /* 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.