From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v2 09/21] xen/arm: Release IRQ routed to a domain when it's destroying Date: Wed, 6 Aug 2014 17:53:21 +0100 Message-ID: References: <1406818852-31856-1-git-send-email-julien.grall@linaro.org> <1406818852-31856-10-git-send-email-julien.grall@linaro.org> <53E25143.3060909@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XF4U1-00045e-Rq for xen-devel@lists.xenproject.org; Wed, 06 Aug 2014 16:54:38 +0000 In-Reply-To: <53E25143.3060909@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: xen-devel@lists.xenproject.org, tim@xen.org, ian.campbell@citrix.com, stefano.stabellini@citrix.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Wed, 6 Aug 2014, Julien Grall wrote: > Hi Stefano, > > On 08/06/2014 04:49 PM, Stefano Stabellini wrote: > > On Thu, 31 Jul 2014, Julien Grall wrote: > >> Xen has to release IRQ routed to a domain in order to reuse later. Currently > >> only SPIs can be routed to the guest so we only need to browse SPIs for a > >> specific domain. > >> > >> Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has > >> not being EOIed). Xen will have to reset the IRQ in order to be able to reuse > >> the IRQ later. > >> > >> Introduce 2 new functions for release an IRQ routed to a domain: > >> - release_guest_irq: upper level to retrieve the IRQ, call the GIC > >> code and release the action > >> - gic_remove_guest_irq: Check if we can remove the IRQ, and reset > >> it if necessary > >> > >> Signed-off-by: Julien Grall > >> > >> --- > >> Changes in v2: > >> - Drop the desc->handler = &no_irq_type in release_irq as it's > >> buggy the IRQ is routed to Xen > >> - Add release_guest_irq and gic_remove_guest_irq > >> --- > >> xen/arch/arm/gic.c | 36 ++++++++++++++++++++++++++++++++++ > >> xen/arch/arm/irq.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ > >> xen/arch/arm/vgic.c | 16 +++++++++++++++ > >> xen/include/asm-arm/gic.h | 4 ++++ > >> xen/include/asm-arm/irq.h | 2 ++ > >> 5 files changed, 106 insertions(+) > >> > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > >> index 8ef8764..22f331a 100644 > >> --- a/xen/arch/arm/gic.c > >> +++ b/xen/arch/arm/gic.c > >> @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned int virq, > >> p->desc = desc; > >> } > >> > >> +/* This function only works with SPIs for now */ > >> +int gic_remove_irq_from_guest(struct domain *d, unsigned int virq, > >> + struct irq_desc *desc) > >> +{ > >> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq); > > > > Use vgic_get_target_vcpu to get the target vcpu of virq. You can pass > > d->vcpu[0] as first argument to vgic_get_target_vcpu. > > Why do I need to add vgic_get_target_vcpu? This function is only able to > handle SPIs which is shared between VCPU. OK, in that case ASSERT(virq >= 32 && virq < nr_lines). I am fine either way. Also see below. > It might be interesting to introduce spi_to_pending function. > > > I am sorry that doing that is going to add a dependency on my gic series. > > I'm already depends on your series :). > > >> + ASSERT(spin_is_locked(&desc->lock)); > >> + ASSERT(test_bit(_IRQ_GUEST, &desc->status)); > >> + ASSERT(p->desc == desc); > >> + > >> + /* If the IRQ is removed when the domain is dying, we only need to > >> + * EOI the IRQ if it has not been done by the guest > >> + */ > >> + if ( d->is_dying ) > >> + { > >> + desc->handler->shutdown(desc); > >> + if ( test_bit(_IRQ_INPROGRESS, &desc->status) ) > >> + gic_hw_ops->deactivate_irq(desc); > >> + clear_bit(_IRQ_INPROGRESS, &desc->status); > >> + goto end; > >> + } > >> + > >> + /* TODO: Handle eviction from LRs. For now, deny remove if the IRQ > >> + * is inflight and not disabled. > >> + */ > >> + if ( test_bit(_IRQ_INPROGRESS, &desc->status) || > >> + test_bit(_IRQ_DISABLED, &desc->status) ) > >> + return -EBUSY; > >> + > >> +end: > >> + desc->handler = &no_irq_type; > >> + p->desc = NULL; > >> + > >> + return 0; > >> +} > >> + > >> int gic_irq_xlate(const u32 *intspec, unsigned int intsize, > >> unsigned int *out_hwirq, > >> unsigned int *out_type) > >> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > >> index 7eeb8dd..ba33571 100644 > >> --- a/xen/arch/arm/irq.c > >> +++ b/xen/arch/arm/irq.c > >> @@ -292,6 +292,7 @@ void release_irq(unsigned int irq, const void *dev_id) > >> if ( !desc->action ) > >> { > >> desc->handler->shutdown(desc); > >> + > >> clear_bit(_IRQ_GUEST, &desc->status); > >> } > > > > > > spurious change > > Will drop the line. > > > > >> @@ -479,6 +480,53 @@ out: > >> return retval; > >> } > >> > >> +int release_guest_irq(struct domain *d, unsigned int virq) > >> +{ > >> + struct irq_desc *desc; > >> + struct irq_guest *info; > >> + unsigned long flags; > >> + struct pending_irq *p; > >> + int ret; > >> + > >> + if ( virq >= vgic_num_irqs(d) ) > >> + return -EINVAL; > >> + > >> + p = irq_to_pending(d->vcpu[0], virq); > >> + if ( !p->desc ) > >> + return -EINVAL; > > > > Same here: call vgic_get_target_vcpu. > > Also if this function is supposed to work only with SPIs, you should add > > a comment or explicitly check for it. > > route_irq_to_guest already check if we are able to route an IRQ or not. > For non-SPIs the function will bailout. > > So, here, it's impossible to have p->desc set to another value than NULL > for non-SPIs. > > Or Xen is buggy will likely fail in another place. If you do: p = irq_to_pending(d->vcpu[0], virq); you are actually introducing more code that cannot cope with non-SGIs. So you should either: 1) esplicitely check for it (add an ASSERT) 2) replace it with code that can cope with non-SGIs, such us irq_to_pending(vgic_get_target_vcpu(d->vcpu[0], virq), virq) I would prefer 2), but 1) is also an option