From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v3 09/24] xen/arm: route_irq_to_guest: Check
 validity of the IRQ
Date: Fri, 20 Feb 2015 16:00:05 +0000
Message-ID: <1424448005.30924.327.camel@citrix.com>
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: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Ian.Campbell@citrix.com>) id 1YOqfl-0007sX-Ve
	for xen-devel@lists.xenproject.org; Fri, 20 Feb 2015 16:43:26 +0000
In-Reply-To: <1421159133-31526-10-git-send-email-julien.grall@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@linaro.org>
Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com
List-Id: xen-devel@lists.xenproject.org

On Tue, 2015-01-13 at 14:25 +0000, 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.

I think you mean the "number of IRQs handled by the vGIC" (or configured
in?) and it would just be "an SPI".

> 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:
                                ^routing?

>     - the device has been blacklist for the current platform

"blacklisted".

>     - the IRQ has not been describe in the device tree

"described".

> I think we can safely assume that a user won't never ask to route
> as such IRQ to the guest.

Can we now ;-) Does this mean the code doesn't check for and abort on
these cases?

<later>Having read further I think you do catch it, so I think you can
remove that sentence, or at least append "...but we check for this
anyway"</later>.

> 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)
> +{
> +    /* 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.

"assignable"

> + */
>  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);

I can't remember if this is expensive, but you could safely do it
further down after more of the sanity checks.

> +
> +    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 )

NR_LOCAL_IRQS?

> +    {
> +        dprintk(XENLOG_G_ERR, "IRQ can only be routed to a virtual SPIs");

Just "SPI".

> -            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);

Is the file/line really needed here? The messages seem reasonably unique
already.