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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-07-07 at 12:16 +0100, Julien Grall wrote:
> 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...

I'd rather we refactored these checks into some sort of is_valid_irq()
helper which could check vs. nr_lines (which remains the
GICD_TYPER.ITLinesNumber based thing) and the number of LPIs separately.

Combining the two risks considering interrupt in the 1025..8191 range as
valid and open coding anything more complex than the existing single <
check in all the relevant places isn't going to scale.

Ian.


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