Re: [PATCH v4 05/17] xen/arm: ITS: implement hw_irq_controller for LPIs

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 15/07/2015 09:16, Vijay Kilari wrote:
> On Tue, Jul 14, 2015 at 2:48 AM, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi,
>>
>> On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index c41e82e..4f3801b 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> +static inline hw_irq_controller *get_host_hw_irq_controller(unsigned int
>>> irq)
>>> +{
>>> +    if ( is_lpi(irq) )
>>> +        return its_hw_ops->lpi_host_irq_type;
>>> +    else
>>> +        return gic_hw_ops->gic_host_irq_type;
>>> +}
>>
>>
>> This is not what I asked on v3 [1]. The ITS hardware controller shouldn't be
>> exposed to the common GIC. We have to keep a clean and comprehensible
>> interface.
>>
>> What I asked is to replace the gic_host_irq_type variable by a new callback
>> which will return the correct hw_irq_controller.
>>
>> For GICv2, it will return the same hw_irq_controller as today. For GICv3, it
>> will check is the IRQ is an LPI and return the correct controller.
>>
>> FWIW, it was "ack" by Ian [2].
>
>   If we don't want to expose any ITS interfaces to common gic code, then we
> have to register callbacks to GICv3 driver.

Why? In fine, the ITS is an integral part of the GICv3, so you could 
directly call the ITS code within the GICv3 without any callback.

Actually, you already do that in some place. So I don't see why you 
can't do it there...

>>> @@ -149,7 +173,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned
>>> int virq,
>>>             test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>>>            goto out;
>>>
>>> -    desc->handler = gic_hw_ops->gic_guest_irq_type;
>>> +    desc->handler = get_guest_hw_irq_controller(desc->irq);
>>>        set_bit(_IRQ_GUEST, &desc->status);
>>>
>>>        gic_set_irq_properties(desc, cpumask_of(v_target->processor),
>>> priority);
>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>> index 2dd43ee..ba8528a 100644
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -35,7 +35,13 @@ static DEFINE_SPINLOCK(local_irqs_type_lock);
>>>    struct irq_guest
>>>    {
>>>        struct domain *d;
>>> -    unsigned int virq;
>>> +    union
>>> +    {
>>> +        /* virq refer to virtual irq in case of spi */
>>> +        unsigned int virq;
>>> +        /* virq refer to event ID in case of lpi */
>>> +        unsigned int vid;
>>
>>
>> Why can't we store the event ID in the irq_guest? As said on v3, this is not
>
> Are you referring to irq_desc in above statement?

Yes sorry.

>
>> domain specific [3]. Furthermore, you add support to route LPI in Xen (see
>> gic_route_irq_to_xen) where you will clearly need the event ID.
>>
>>>    void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>>>    {
>>>        if ( desc != NULL )
>>> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
>>> index b5e09bd..e8d244f 100644
>>> --- a/xen/include/asm-arm/gic-its.h
>>> +++ b/xen/include/asm-arm/gic-its.h
>>> @@ -161,6 +161,10 @@ typedef union {
>>>     * The ITS view of a device.
>>>     */
>>>    struct its_device {
>>> +    /* Physical ITS */
>>> +    struct its_node         *its;
>>> +    /* Number of Physical LPIs assigned */
>>> +    int                     nr_lpis;
>>
>>
>> Why didn't you add this field directly in the patch #4? It would be more
>> logical.
>>
>>>    /*
>>>     * ITS registers, offsets from ITS_base
>>> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
>>> index 34b492b..55e219f 100644
>>> --- a/xen/include/asm-arm/irq.h
>>> +++ b/xen/include/asm-arm/irq.h
>>> @@ -17,6 +17,8 @@ struct arch_pirq
>>>    struct arch_irq_desc {
>>>        int eoi_cpu;
>>>        unsigned int type;
>>> +    struct its_device *dev;
>>> +    u16 col_id;
>>
>>
>> It has been suggested by Ian to move col_id in the its_device in the
>> previous version [4]. Any reason to not doing it?
>
> In round robin fashion each plpi is attached to col_id. So storing
> in its_device is not possible. In linux latest col_id is stored in its_device
> structure for which set_affinity is called.

You could do round robin on its_device... It would be exactly the same 
and save 2 byte if not more with the alignment per irq_desc.

Don't forget that 1 byte in the irq_desc means 1KB added in Xen binary. 
These bytes saved could be used to store the event ID.

That remind me, these 2 new fields should only be defined when GICv3 is 
used (#ifdef HAS_GICV3).

I'm would be fine if you skip the former for 4.6, but the latter is 
mandatory. ITS code shouldn't be compiled on arm32.

Regards,

-- 
Julien Grall