Re: [PATCH for-4.5 3/8] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 02/19/2014 11:35 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the
>> IRQ is correctly setup.
>>
>> As IRQ can be shared between devices, if the devices are not assigned to the
>> same domain or Xen, this could result to IRQ route to the domain instead of
>> Xen ...
>>
>> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Hopefully, none of the supported platforms have UARTs (the only device
> 
>                                                      ^shared?

Hmmm ... I don't remember what I was trying to say here :/.

Anyway, this part was for argue to push it for Xen 4.4. It doesn't make
sense anymore. I will remove it.

> 
> Other than wondering if EBUSY might be more natural than EADDRINUSE and
> some grammar nits (below) I think this patch looks good.

Right, I will use EBUSY for the next version.

> 
>>     currently used by Xen). It would be nice to have this patch for Xen 4.4 to
>>     avoid waste of time for developer.
>>
>>     The downside of this patch is if someone wants to support a such platform
>>     (eg IRQ shared between device assigned to different domain/XEN), it will
>>     end up to a error message and a panic.
>> ---
>>  xen/arch/arm/domain_build.c |    8 ++++++--
>>  xen/arch/arm/gic.c          |   40 +++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 45 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 47b781b..1fc359a 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -712,8 +712,12 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
>>          }
>>  
>>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
>> -        /* Don't check return because the IRQ can be use by multiple device */
>> -        gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>> +        res = gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>> +        if ( res )
>> +        {
>> +            printk(XENLOG_ERR "Unable to route the IRQ %u to dom0\n", irq.irq);
> 
> "Unable to route IRQ %u..." and I think you want to use d->domain_id
> rather than hardcoding 0.

I will fix it. At the same time, the error message when Xen is unable to
map the range also use "dom0". I will send a separate patch for that.

Cheers,

-- 
Julien Grall