From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v3 09/24] xen/arm: route_irq_to_guest: Check
 validity of the IRQ
Date: Fri, 20 Feb 2015 17:21:22 +0000
Message-ID: <54E76D12.20201@linaro.org>
References: <1421159133-31526-1-git-send-email-julien.grall@linaro.org>	
	<1421159133-31526-10-git-send-email-julien.grall@linaro.org>
	<1424448005.30924.327.camel@citrix.com>
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.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <julien.grall@linaro.org>) id 1YOrGy-0003vy-EP
	for xen-devel@lists.xenproject.org; Fri, 20 Feb 2015 17:21:52 +0000
Received: by mail-wi0-f180.google.com with SMTP id h11so4406659wiw.1
	for <xen-devel@lists.xenproject.org>;
	Fri, 20 Feb 2015 09:21:50 -0800 (PST)
In-Reply-To: <1424448005.30924.327.camel@citrix.com>
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: Ian Campbell <ian.campbell@citrix.com>
Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com
List-Id: xen-devel@lists.xenproject.org

Hi Ian,

On 20/02/15 16:00, Ian Campbell wrote:
> 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".

I think "configured in" is better here. I will change to "number of IRQs
configured in the vGIC".

>> 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?

No, I wanted to mean when a IRQ type is not set.

I will replace the last sentence by "This can happen when:"

> 
>>     - 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>.

Right, this sentence is not clear. What I wanted to mean is we won't
support any IRQ not described in the DT or which belong to a specific
domain.

But with an upcoming patch from Parth, the IRQ configuration
(edge/level) will be deferred until the guest has enabled this IRQ.

>> 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.

For now we retrieve it from an array. But Vijay's plan to replace the
array by a radix tree.

I will move the whole block (if () ... desc = ) after the vGIC because I
think they should be tight.

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

Yes. I think I have multiple place where 32 is open-coded. I will
replace them.

> 
>> +    {
>> +        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.

I don't remember why I made this change. I will drop it.

Regards,

-- 
Julien Grall