|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 05/17] xen/arm: ITS: implement hw_irq_controller for LPIs
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
>
>> +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@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
>
>> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |