From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v3 09/24] xen/arm: route_irq_to_guest: Check validity of the IRQ Date: Wed, 28 Jan 2015 17:55:14 +0000 Message-ID: References: <1421159133-31526-1-git-send-email-julien.grall@linaro.org> <1421159133-31526-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 1YGWq4-0007jz-85 for xen-devel@lists.xenproject.org; Wed, 28 Jan 2015 17:55:40 +0000 In-Reply-To: <1421159133-31526-10-git-send-email-julien.grall@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 List-Id: xen-devel@lists.xenproject.org On Tue, 13 Jan 2015, Julien Grall wrote: > Currently Xen only supports SPIs routing for guest, add a function > is_assignable_irq to check if we can assign a given IRQ to the guest. > > Secondly, make sure the vIRQ is not the greater that the number of IRQs handle > to the vGIC and it's an SPIs. > > Thirdly, when the IRQ is already assigned to the domain, check the user > is not asking to use a different vIRQ than the one already bound. > > Finally, desc->arch.type which contains the IRQ type (i.e level/edge) must > be correctly configured before. The IRQ type won't be configure when: > - the device has been blacklist for the current platform > - the IRQ has not been describe in the device tree > > I think we can safely assume that a user won't never ask to route > as such IRQ to the guest. > > Also, use XENLOG_G_ERR in the error message within the function as it will > be later called from a guest. > > Signed-off-by: Julien Grall > > --- > Changes in v3: > - Fix typo in commit message and comment > - Add a check that the vIRQ is an SPI > - Check if the user is not asking for a different vIRQ when the > IRQ is already assigned to the guest > > Changes in v2: > - Rename is_routable_irq into is_assignable_irq > - Check if the IRQ is not greater than the number handled by the > number of IRQs handled by the gic > - Move is_assignable_irq in irq.c rather than defining in the > header irq.h > - Retrieve the irq descriptor after checking the validity of the > IRQ > - vgic_num_irqs has been moved in a separate patch > - Fix the irq check against vgic_num_irqs > - Use virq instead of irq for vGIC sanity check > --- > xen/arch/arm/irq.c | 58 +++++++++++++++++++++++++++++++++++++++++++---- > xen/include/asm-arm/irq.h | 2 ++ > 2 files changed, 56 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 830832c..af408ac 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -379,6 +379,15 @@ err: > return rc; > } > > +bool_t is_assignable_irq(unsigned int irq) static inline? > +{ > + /* For now, we can only route SPIs to the guest */ > + return ((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())); > +} > + > +/* Route an IRQ to a specific guest. > + * For now only SPIs are assignabled to the guest. > + */ > int route_irq_to_guest(struct domain *d, unsigned int virq, > unsigned int irq, const char * devname) > { > @@ -388,6 +397,29 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, > unsigned long flags; > int retval = 0; > > + if ( !is_assignable_irq(irq) ) > + { > + dprintk(XENLOG_G_ERR, "the IRQ%u is not routable\n", irq); > + return -EINVAL; > + } > + > + desc = irq_to_desc(irq); > + > + if ( virq >= vgic_num_irqs(d) ) > + { > + dprintk(XENLOG_G_ERR, > + "the vIRQ number %u is too high for domain %u (max = %u)\n", > + irq, d->domain_id, vgic_num_irqs(d)); > + return -EINVAL; > + } > + > + /* Only routing to virtual SPIs is supported */ > + if ( virq < 32 ) > + { > + dprintk(XENLOG_G_ERR, "IRQ can only be routed to a virtual SPIs"); > + return -EINVAL; > + } > + > action = xmalloc(struct irqaction); > if ( !action ) > return -ENOMEM; > @@ -408,8 +440,18 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, > > spin_lock_irqsave(&desc->lock, flags); > > + if ( desc->arch.type == DT_IRQ_TYPE_INVALID ) > + { > + dprintk(XENLOG_G_ERR, "IRQ %u has not been configured\n", > + irq); > + retval = -EIO; > + goto out; > + } > + > /* If the IRQ is already used by someone > - * - If it's the same domain -> Xen doesn't need to update the IRQ desc > + * - If it's the same domain -> Xen doesn't need to update the IRQ desc. > + * For safety check if we are not trying to assign the IRQ to a > + * different vIRQ. > * - Otherwise -> For now, don't allow the IRQ to be shared between > * Xen and domains. > */ > @@ -418,13 +460,21 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, > struct domain *ad = irq_get_domain(desc); > > if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad ) > + { > + if ( irq_get_guest_info(desc)->virq != virq ) > + { > + dprintk(XENLOG_G_ERR, "d%u: IRQ %u is already assigned to vIRQ %u\n", > + d->domain_id, irq, irq_get_guest_info(desc)->virq); > + retval = -EPERM; I don't think that EPERM is the right error for this. Maybe EBUSY? > + } Should we return error for this too? Maybe EEXIST? > goto out; > + } > > if ( test_bit(_IRQ_GUEST, &desc->status) ) > - printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n", > - irq, ad->domain_id); > + dprintk(XENLOG_G_ERR, "IRQ %u is already used by domain %u\n", > + irq, ad->domain_id); > else > - printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n", irq); > + dprintk(XENLOG_G_ERR, "IRQ %u is already used by Xen\n", irq); > retval = -EBUSY; > goto out; > } > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index f00eb11..71b39e7 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -37,6 +37,8 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq); > > #define domain_pirq_to_irq(d, pirq) (pirq) > > +bool_t is_assignable_irq(unsigned int irq); > + > void init_IRQ(void); > void init_secondary_IRQ(void); > > -- > 2.1.4 >