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

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

On Fri, Jul 10, 2015 at 7:16 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> Implements hw_irq_controller api's required
>> to handle LPI's
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> ---
>> 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

>
>> +static void its_host_irq_end(struct irq_desc *desc)
>> +{
>> +    /* Lower the priority */
>> +    gicv3_eoi_irq(desc);
>> +    /* Deactivate */
>> +    gicv3_dir_irq(desc);
>> +}
>> +
>> +static void its_guest_irq_end(struct irq_desc *desc)
>> +{
>> +    gicv3_eoi_irq(desc);
>> +}
>
> Exposing those two gicv3 functions is a bit unfortunate, but I think it
> will do for now.
>
> Exposing gicv3_[host|guest]_irq_end might have been nicer, since you
> could just insert them into your its_[host|guest]_lpi_type instead of
> duplicating them.
>
> Eventually we may want to refactor such that register_its_ops gives back
> the ack/eoi hooks to use.
>
>> +static void its_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
>> +{
>
> Please add
>
>      /* Not yet supported */
>
>> @@ -104,7 +126,8 @@ static void gic_set_irq_properties(struct irq_desc *desc,
>>                                     const cpumask_t *cpu_mask,
>>                                     unsigned int priority)
>>  {
>> -   gic_hw_ops->set_irq_properties(desc, cpu_mask, priority);
>> +    if ( desc->irq < gic_number_lines() )
>
> Should this be is_lpi as in other similar places?
>
>> +        gic_hw_ops->set_irq_properties(desc, cpu_mask, priority);
>>  }
>>
>>  /* Program the GIC to route an interrupt to the host (i.e. Xen)
>> @@ -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@citrix.com> 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

>
>> ocessor), priority);
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index 2dd43ee..ba8528a 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -35,7 +35,13 @@ static DEFINE_SPINLOCK(local_irqs_type_lock);
>>  struct irq_guest
>>  {
>>      struct domain *d;
>> -    unsigned int virq;
>> +    union
>> +    {
>> +        /* virq refer to virtual irq in case of spi */
>> +        unsigned int virq;
>> +        /* virq refer to event ID in case of lpi */
>
> "refers" in both cases
>
> And I'd say "to the ..." not just "to ..." and "in the case of..." too.
>
>> +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
>
> BTW, while checking the field name I spotted "struct msi_desc
> *msi_desc" in the main struct irq_desc.
>
> Since MSIs are effectively the same as LPIs as a future cleanup I think
> we should s/its_device/msi_desc/g and use this field instead of adding a
> second redundant type and pointer to it. THis is not a blocker for 4.6
> though.
>
>> +struct its_device *get_irq_device(struct irq_desc *desc)
>> +{
>> +    ASSERT(spin_is_locked(&desc->lock));
>> +
>> +    return desc->arch.dev;
>> +}
>> +
>> +void set_irq_device(struct irq_desc *desc, struct its_device *dev)
>> +{
>> +    ASSERT(spin_is_locked(&desc->lock));
>> +    desc->arch.dev = dev;
>> +}
>
> Please add _its to the names of both of these functions, ie..g
> set_irq_its_device.
>
>> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
>> index b5e09bd..e8d244f 100644
>> --- a/xen/include/asm-arm/gic-its.h
>> +++ b/xen/include/asm-arm/gic-its.h
>> @@ -161,6 +161,10 @@ typedef union {
>>   * The ITS view of a device.
>>   */
>>  struct its_device {
>> +    /* Physical ITS */
>> +    struct its_node         *its;
>> +    /* Number of Physical LPIs assigned */
>> +    int                     nr_lpis;
>>      /* Physical Device id */
>>      u32                     device_id;
>>      /* RB-tree entry */
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index e9d5f36..44c2317 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -20,6 +20,9 @@
>>
>>  #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.