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

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-07-15 at 10:26 +0200, Julien Grall wrote:

> >>> @@ -149,7 +173,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned
> >>> int virq,
> >>>             test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> >>>            goto out;
> >>>
> >>> -    desc->handler = gic_hw_ops->gic_guest_irq_type;
> >>> +    desc->handler = get_guest_hw_irq_controller(desc->irq);
> >>>        set_bit(_IRQ_GUEST, &desc->status);
> >>>
> >>>        gic_set_irq_properties(desc, cpumask_of(v_target->processor),
> >>> 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 */
> >>> +        unsigned int vid;
> >>
> >>
> >> Why can't we store the event ID in the irq_guest? As said on v3, this is not
> >
> > Are you referring to irq_desc in above statement?
> 
> Yes sorry.

I'm afraid I don't follow your suggestion here, are you suggesting that
the vid field added above should be moved to irq_desc?

But the vid _is_ domain specific, it is the virtual event ID which is
per-domain (it's the thing looked up in the ITT to get a vLPI to be
injected). I think it is a pretty direct analogue of the virq field used
for non-LPI irq_guest structs.

If we had need for the physical event id then that would like belong in
the irq_desc.

Your proposal on v3 looks to be around moving the its_device pointer to
the irq_desc, which appears to have been done here, along with turning
the virq+vid into a union as requested there too.

> >> It has been suggested by Ian to move col_id in the its_device in the
> >> previous version [4]. Any reason to not doing it?
> >
> > In round robin fashion each plpi is attached to col_id. So storing
> > in its_device is not possible. In linux latest col_id is stored in its_device
> > structure for which set_affinity is called.

Are you saying that in Linux all Events/LPIs associated with a given ITS
device are routed to the same collection?

> You could do round robin on its_device... It would be exactly the same 

Routing all LPIs associated with a given its_device to the same
collection is not exactly the same as round robin-ing all LPIs from the
device over the collections.

> and save 2 byte if not more with the alignment per irq_desc.

If this is a concern then I would say we would either want a separate
array of per-pLPI information which we do not want in irq_desc because
it is irq specific, or do add a pointer to its_desc which points to an
array of per-event information.

Ian.