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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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.

Regards,

-- 
Julien Grall

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