Re: [Xen-devel] [PATCH v4 12/17] xen/arm: ITS: Initialize LPI irq descriptors and route

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Ian,

On Fri, Jul 10, 2015 at 9:00 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@xxxxxxxxx wrote:
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 3ebadcf..92d2be9 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -68,11 +68,18 @@ enum gic_version gic_hw_version(void)
>>     return gic_hw_ops->info->hw_version;
>>  }
>>
>> +/* Only validates PPIs/SGIs/SPIs supported */
>>  unsigned int gic_number_lines(void)
>>  {
>>      return gic_hw_ops->info->nr_lines;
>>  }
>>
>> +/* Validates PPIs/SGIs/SPIs/LPIs supported */
>> +bool_t gic_is_valid_irq(unsigned int irq)
>> +{
>> +    return ((irq < gic_hw_ops->info->nr_lines) && is_lpi(irq));
>> +}
>> +
>>  unsigned int gic_nr_id_bits(void)
>>  {
>>      return gic_hw_ops->info->nr_id_bits;
>> @@ -148,7 +155,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const 
>> cpumask_t *cpu_mask,
>>  {
>>      ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
>>      /* Can't route interrupts that don't exist */
>> -    ASSERT(desc->irq < gic_number_lines() || is_lpi(desc->irq));
>> +    ASSERT(gic_is_valid_irq(desc->irq));
>
>
> The fact that I commented on this earlier is another artefact in the way
> this series presents functionality in an essentially arbitrary order.
>
> I notice that you appear to have reintroduced the logic but when you
> moved this code from here into gic_is_valid_irq. AFAICT that function
> can never return true, but then I'm rather confused about how this
> series can have been tested.
>
>>      ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>>      ASSERT(spin_is_locked(&desc->lock));
>>
>> @@ -160,6 +167,17 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const 
>> cpumask_t *cpu_mask,
>>  /* Program the GIC to route an interrupt to a guest
>>   *   - desc.lock must be held
>>   */
>> +int gic_route_lpi_to_guest(struct domain *d, unsigned int virq,
>> +                           struct irq_desc *desc, unsigned int priority)
>> +{
>> +    ASSERT(spin_is_locked(&desc->lock));
>
> ASSERT(virq >= SOME_DEFINE_FOR_MIN_LPI);
>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index 3806d98..c8ea627 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -62,12 +62,21 @@ hw_irq_controller no_irq_type = {
>>  };
>>
>>  static irq_desc_t irq_desc[NR_IRQS];
>> +#ifdef CONFIG_ARM_64
>> +static irq_desc_t irq_desc_lpi[NR_GIC_LPI];
>
> http://xenbits.xen.org/people/ianc/vits/draftG.html#irq-descriptors
> contains: "Therefore a second dynamically allocated array will be added
> to cover the range 8192..nr_lpis"
>
> IOW this should be dynamically allocated and therefore NR_GIC_LPI (which
> I think I already commented on earlier) should go away, since the limit
> should come from the h/w.
>> @@ -267,9 +285,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int 
>> irq, int is_fiq)
>>          set_bit(_IRQ_INPROGRESS, &desc->status);
>>          desc->arch.eoi_cpu = smp_processor_id();
>>
>> +#ifdef CONFIG_ARM_64
>> +        if ( is_lpi(irq) )
>> +            vgic_vcpu_inject_lpi(info->d, irq);
>> +        else
>> +#endif
>>          /* the irq cannot be a PPI, we only support delivery of SPIs to
>>           * guests */
>> -        vgic_vcpu_inject_spi(info->d, info->virq);
>> +            vgic_vcpu_inject_spi(info->d, info->virq);
>
> Comment should be reindented too.
>
>> +    if ( !is_lpi(irq) )
>> +    {
>> +        rank = vgic_rank_irq(v, irq);
>> +
>> +        ASSERT(spin_is_locked(&rank->lock));
>> +        priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8,
>>                                                irq, DABT_WORD)], 0, irq & 
>> 0x3);
>> +    }
>> +    if ( is_lpi(irq) && gic_lpi_supported() )
>
> Should be "else if ( gic_lpi_supported() )"
>
>> +    vdev = vits_find_device(&d->arch.vits->dev_root, devid);
>> +    if ( !vdev )
>> +    {
>> +        dprintk(XENLOG_WARNING,
>> +                "LPI %d received for dev 0x%x not assigned..dropping\n",
>> +                irq, devid);
>> +        return;
>> +    }
>
> This isn't used again and the whole R-B tree should go away.
>
>> +    p = irq_to_pending(d->vcpu[0], vitt_entry.vlpi);
>
> Perhaps given that we know this is a vlpi, we could skipping going via
> vcpu[0] and just go right to the entry in the d->pending_lpi array (via
> a suitable accessor of course)
>
>> +    p->desc = desc;
>
> This should have happened during routing, not now.
>
> Oh. I see what is going on, your vgic_vcpu_inject_lpi appears to be
> taking an IRQ, and not a vpli, or at least to be confused about what it
> should do with the number which it calls irq.
>
> Therefore you find yourself needing to lookup the irqdesc, and look in
> the vitt etc, none of which belongs here.
>
> For interrupts coming from a plpi all of that lookup should happen based
> on the its_device which is contained in the irq_guest and other info
> from there, before calling this function. You can get the irq_guest
> which you can get because you have a PLPI in your hand and can look up
> the irq_guest via the irq_desc which you get using the plpi number.
>

AUI, you are refering to

http://xenbits.xen.org/people/ianc/vits/draftG.html#virtual-lpi-injection

If we want to extract, its_device, and VITT information before calling
vgic_vcpu_inject_lpi(),
 it cannot be done in do_IRQ() as irq.c cannot have vITS related code.

So one option is to call vits_foo() function which will validated plpi and get
its_device / VITT info and call vgic_vcpu_inject_lpi(). So vITS will
call back vgic function.
OR
Introduce new vgic function for doing this and call vgic_vcpu_inject_lpi().

> For interrupts coming from the VITS INT command the command itself
> contains a vDevice and vEvent, which can then be looked up in the ITT to
> get a vpli which can be passed to vgic_vcpu_inject_lpi.
>
> Thus this function has no need to lookup an irq_desc, nor a dev id, nor
> to do an rb tree lookup.
>
> I think both of these cases are adequately explained, with pseudocode,
> in the draftG vits design doc. Please ask if you are unsure.
>
> Ian.
>

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