Re: [RFC PATCH v3 13/18] xen/arm: ITS: Add irq descriptors for LPIs

[Vijay Kilari <vijay.kilari@gmail.com>]

On Mon, Jun 29, 2015 at 6:41 PM, Julien Grall <julien.grall@citrix.com> wrote:
> On 29/06/15 13:58, Ian Campbell wrote:
>> On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@gmail.com wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> Add irq descriptors for LPIs and route
>>
>> This seems to also do interrupt injection for LPIs and more. Please
>> check that your commit messages are accurately describing the contents
>> of the patch. If it turns into a long list of unrelated sounding things
>> then that might suggest the patch should be split up.
>>
>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>> ---
>>>  xen/arch/arm/gic-v3.c         |    8 +++-
>>>  xen/arch/arm/gic.c            |   17 +++++++-
>>>  xen/arch/arm/irq.c            |   38 +++++++++++++----
>>>  xen/arch/arm/vgic-v3-its.c    |    9 +++++
>>>  xen/arch/arm/vgic.c           |   90 ++++++++++++++++++++++++++++++++++++++---
>>>  xen/include/asm-arm/domain.h  |    2 +
>>>  xen/include/asm-arm/gic-its.h |    6 +++
>>>  xen/include/asm-arm/gic.h     |    3 ++
>>>  xen/include/asm-arm/vgic.h    |    1 +
>>>  9 files changed, 157 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index 737646c..793f2f0 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -899,9 +899,13 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p,
>>>
>>>      val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT) | grp;
>>>      val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
>>> -    val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
>>>
>>> -   if ( p->desc != NULL )
>>> +    if ( is_lpi(p->irq) )
>>> +        val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
>>> +    else
>>> +        val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
>>
>> Is there supposed to be something different between these two cases? (Or
>> am I missing it?)
>>
>>> +
>>> +   if ( p->desc != NULL && !(is_lpi(p->irq)) )
>>>         val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
>>>                             << GICH_LR_PHYSICAL_SHIFT);
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index cfc9c42..091f7e5 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -124,18 +124,31 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>>>                            unsigned int priority)
>>>  {
>>>      ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
>>> -    ASSERT(desc->irq < gic_number_lines());/* Can't route interrupts that don't exist */
>>> +    /* Can't route interrupts that don't exist */
>>> +    ASSERT(desc->irq < gic_number_lines() && is_lpi(desc->irq));
>>
>> ||, surely? Otherwise doesn't this hit for every possible irq?
>
> Aside that, the change in the ASSERT is wrong. The goal of the helper
> gic_number_lines is to return the number of IRQs (i.e PPIs, LPIs, SPIs)
> present in the hardware. We use in few places to ensure the validity of
> the IRQ.
>
> Although, this will require some extra care in places where an interrupt
> is assigned to a domain in order to only allow SPIs.

 Today, gic_number_lines returns number of SPI+PPI supported.
If we want to include number LPIs supported, then we cannot
compare against with irq number because of hole in irq numbers from
1024 - 8196.

Regards
Vijay