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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Sat, 2015-07-11 at 20:10 +0530, Vijay Kilari wrote:
> On Fri, Jul 10, 2015 at 7:16 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> > On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@xxxxxxxxx wrote:
> >> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> >>
> >> Implements hw_irq_controller api's required
> >> to handle LPI's
> >>
> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> >> ---
> >> v4: - Implement separate hw_irq_controller for LPIs
> >>     - Drop setting LPI affinity
> >>     - virq and vid are moved under union
> >>     - Introduced inv command handling
> >>     - its_device is stored in irq_desc
> >> ---
> >>  xen/arch/arm/gic-v3-its.c         |  132 
> >> +++++++++++++++++++++++++++++++++++++
> >>  xen/arch/arm/gic-v3.c             |    5 +-
> >>  xen/arch/arm/gic.c                |   32 +++++++--
> >>  xen/arch/arm/irq.c                |   40 ++++++++++-
> >>  xen/include/asm-arm/gic-its.h     |    4 ++
> >>  xen/include/asm-arm/gic.h         |   13 ++++
> >>  xen/include/asm-arm/gic_v3_defs.h |    1 +
> >>  xen/include/asm-arm/irq.h         |    8 ++-
> >>  8 files changed, 227 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> >> index b421a6f..b98d396 100644
> >> --- a/xen/arch/arm/gic-v3-its.c
> >> +++ b/xen/arch/arm/gic-v3-its.c
> >> @@ -295,6 +295,19 @@ post:
> >>      its_wait_for_range_completion(its, cmd, next_cmd);
> >>  }
> >>
> >> +static void its_send_inv(struct its_device *dev, struct its_collection 
> >> *col,
> >> +                         u32 event_id)
> >> +{
> >> +    its_cmd_block cmd;
> >> +
> >> +    memset(&cmd, 0x0, sizeof(its_cmd_block));
> >> +    cmd.inv.cmd = GITS_CMD_INV;
> >> +    cmd.inv.devid = dev->device_id;
> >> +    cmd.inv.event = event_id;
> >> +
> >> +    its_send_single_command(dev->its, &cmd, col);
> >> +}
> >
> > This ought to be in the prior patch doing such things I think.
> >
> > Oh I see, you didn't have struct its_device defined back then. I think
> > you can just reorder patches #3 and #4 to solve that.
> 
>   INV is used only in this patch in lpi_set_config().
> So introduced in this patch

And the other patch introduces every (almost) every other cmd handler.
Having one patch do a bulk add of most commands and then other commands
dribbled in later as they are used just makes the series harder to
follow.

> >> @@ -114,11 +137,12 @@ 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));
> >
> > As discussed in <1436284206.25646.258.camel@xxxxxxxxxx> please make some
> > sort of is_valid_irq(irq) helper to encapsulate this logic.
> 
>   I have added it patch#12. I remove this change from this patch

Please fix the ordering of the series so that you don't need to do
things like this. It just wastes review bandwidth since people reading
patch #5 have no idea what is going to happen in #12 and in any case bad
or redundant code shouldn't be added only to be removed later unless
there is really no option (a rare occurrence)

> >> +unsigned int irq_to_vid(struct irq_desc *desc)
> >> +{
> >> +    return irq_get_guest_info(desc)->vid;
> >> +}
> >> +
> >> +unsigned int irq_to_virq(struct irq_desc *desc)
> >> +{
> >> +    return irq_get_guest_info(desc)->virq;
> >> +}
> >
> > Please assert that irq_desc->arch.its_device is (non-)NULL as
> > appropriate in these two cases.
> 
>    These two functions are accessing irq_guest structure not arch.its_device

->vid and ->virq are members of a union. The distinguishing feature
which tells us which one is valid is whether or not
irq_desc->arch.its_device is NULL or not.

Therefore an assertion in each function should be added to catch cases
where people try to get the vid of an SPI or the virq of an LPI.

> >>  #define NR_GIC_LOCAL_IRQS  NR_LOCAL_IRQS
> >>  #define NR_GIC_SGI         16
> >> +#define FIRST_GIC_LPI      8192
> >> +#define NR_GIC_LPI         4096
> >> +#define MAX_LPI            (FIRST_GIC_LPI + NR_GIC_LPI)
> >
> > MAX_LPI and NR_GIC_LPI should be obtained from the hardware at init time
> > and put somewhere, like a global nr_lpis perhaps, to be used throughout.
> 
>  This MAX_LPI and NR_GIC_LPI is Xen limitation where in we
> are allocating irq_descriptors statically upto NR_GIC_LPI.

As I said later on, please make this allocation dynamic as described in
the design doc. The static LPI descriptor array used in this series is
not acceptable.

Ian.


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