From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 09/21] xen/arm: Release IRQ routed to a domain when it's destroying Date: Wed, 06 Aug 2014 17:01:07 +0100 Message-ID: <53E25143.3060909@linaro.org> References: <1406818852-31856-1-git-send-email-julien.grall@linaro.org> <1406818852-31856-10-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XF3eM-0000XB-8B for xen-devel@lists.xenproject.org; Wed, 06 Aug 2014 16:01:14 +0000 Received: by mail-wg0-f42.google.com with SMTP id l18so2825105wgh.13 for ; Wed, 06 Aug 2014 09:01:12 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, tim@xen.org, ian.campbell@citrix.com, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org 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. 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. Regards, -- Julien Grall