Re: [PATCH v3 09/24] xen/arm: route_irq_to_guest: Check validity of the IRQ

[Julien Grall <julien.grall@linaro.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