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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 07/07/15 12:00, Vijay Kilari wrote:
> On Mon, Jun 29, 2015 at 6:41 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>> On 29/06/15 13:58, Ian Campbell wrote:
>>> On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@xxxxxxxxx wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxxxxxxx>
>>>> ---
>>>>  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.

I don't see any problem... All the SPI/PPI reported by the GIC may not
be implemented. This ASSERT is here for a sanity check of the IRQ value.

If the caller decides to pass an unused IRQ then it's own problem.

There is not much reason to not extend gic_number_lines. The common GIC
code should not care whether the IRQ is a LPI, SPI...

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel