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

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index e6004d2..53554e6 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -895,7 +895,7 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p,
>       val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
>       val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT;
>
> -   if ( p->desc != NULL )
> +   if ( p->desc != NULL && !(is_lpi(p->irq)) )

It seems that you replaced all the p->desc != NULL by "p->desc != NULL 
&& !is_lpi(p->irq).

Why don't you avoid to set p->desc in this case?

You may also want to put some explanation in the commit message to 
explain why you don't have to set the GICH_LR.HW bit for LPIs.

>          val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK)
>                              << GICH_LR_PHYSICAL_SHIFT);
>
> 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 */

This comment seems wrong. The function doesn't validate the IRQ but 
return the number of Lines (i.e PPIs/SGIs/SPIs).

>   unsigned int gic_number_lines(void)
>   {
>       return gic_hw_ops->info->nr_lines;
>   }

[...]

>   int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>                              struct irq_desc *desc, unsigned int priority)
>   {
> @@ -454,7 +472,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>           if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
>                test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
>           {
> -            if ( p->desc == NULL )
> +            if ( p->desc == NULL  || is_lpi(irq) )
>               {
>                    lr_val.state |= GICH_LR_PENDING;
>                    gic_hw_ops->write_lr(i, &lr_val);
> @@ -677,7 +695,7 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>           /* Reading IRQ will ACK it */
>           irq = gic_hw_ops->read_irq();
>
> -        if ( likely(irq >= 16 && irq < 1020) )
> +        if ( (likely(irq >= 16 && irq < 1020)) || is_lpi(irq) )

Please move the is_lpi(irq) in likely.

>           {
>               local_irq_enable();
>               do_IRQ(regs, irq, is_fiq);

[...]

> @@ -208,7 +226,7 @@ int request_irq(unsigned int irq, unsigned int irqflags,
>        * which interrupt is which (messes up the interrupt freeing
>        * logic etc).
>        */
> -    if ( irq >= nr_irqs )
> +    if ( irq >= nr_irqs && !is_lpi(irq) )

Technically nr_irqs should contain the total number of IRQ and not only 
the number of SPI/PPI/SGI.

Either modify nr_irqs or use gic_is_valid_irq which would do the same here.

>           return -EINVAL;
>       if ( !handler )
>           return -EINVAL;
> @@ -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);
>           goto out_no_end;
>       }
>
> @@ -436,7 +459,8 @@ err:
>   bool_t is_assignable_irq(unsigned int irq)
>   {
>       /* For now, we can only route SPIs to the guest */
> -    return ((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines()));
> +    return (((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines())) ||
> +              is_lpi(irq));

If you modify the function, please also modify the comment which become 
invalid now.

[...]

> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index bbcc7bb..4649b07 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -625,6 +625,15 @@ err:
>       return 0;
>   }
>
> +uint8_t vgic_its_get_priority(struct vcpu *v, uint32_t pid)
> +{
> +    uint8_t priority;
> +
> +    priority =  readb_relaxed(v->domain->arch.vits->prop_page + pid);

Why do you use readb_relaxed here? This should only be used for Device MMIO.

Although, you need to ensure that the value will be correctly 
synchronize if another CPU is writing in prop_page which is protected by 
prop_lock.

> +    priority &= LPI_PRIORITY_MASK;
> +
> +    return priority;
> +}
>   static int vgic_v3_gits_lpi_mmio_read(struct vcpu *v, mmio_info_t *info)
>   {
>       uint32_t offset;

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 69bf1ff..537ed3d 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -226,6 +226,9 @@ extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mas
>   extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
>                                     struct irq_desc *desc,
>                                     unsigned int priority);
> +extern int gic_route_lpi_to_guest(struct domain *d, unsigned int virq,
> +                                   struct irq_desc *desc,
> +                                   unsigned int priority);
>
>   /* Remove an IRQ passthrough to a guest */
>   int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> @@ -282,8 +285,10 @@ extern void send_SGI_allbutself(enum gic_sgi sgi);
>   /* print useful debug info */
>   extern void gic_dump_info(struct vcpu *v);
>
> -/* Number of interrupt lines */
> +/* Number of interrupt lines (SPIs)*/

This is not really true. The interrupts lines is equals to PPIs + SGIs + 
SPIs.

>   extern unsigned int gic_number_lines(void);
> +/* Check if irq is valid SPI or LPI */

This comment is not true. This function is used to check that an IRQ is 
valid in general, not only for SPI or LPI.

> +bool_t gic_is_valid_irq(unsigned int irq);
>   /* Number of interrupt id bits supported */
>   extern unsigned int gic_nr_id_bits(void);
>   /* LPI support info */

Regards,

-- 
Julien Grall