Re: [PATCH v4 09/27] ARM: GICv3: introduce separate pending_irq structs for LPIs

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

Hi Andre,

On 03/04/17 21:28, Andre Przywara wrote:
> For the same reason that allocating a struct irq_desc for each
> possible LPI is not an option, having a struct pending_irq for each LPI
> is also not feasible. We only care about mapped LPIs, so we can get away
> with having struct pending_irq's only for them.
> Maintain a radix tree per domain where we drop the pointer to the
> respective pending_irq. The index used is the virtual LPI number.
> The memory for the actual structures has been allocated already per
> device at device mapping time.
> Teach the existing VGIC functions to find the right pointer when being
> given a virtual LPI number.
> We also take care of checking for a NULL pointer in the VCPU exit path,
> should an LPI have been removed from the tree for any reason.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic.c           | 12 ++++++++++++
>  xen/arch/arm/vgic-v3.c       | 21 +++++++++++++++++++++
>  xen/arch/arm/vgic.c          |  5 +++++
>  xen/include/asm-arm/domain.h |  2 ++
>  xen/include/asm-arm/vgic.h   |  1 +
>  5 files changed, 41 insertions(+)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 9522c6c..3ed6f81 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -461,7 +461,19 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>
>      gic_hw_ops->read_lr(i, &lr_val);
>      irq = lr_val.virq;
> +

Spurious change.

>      p = irq_to_pending(v, irq);
> +    /* An LPI might have been unmapped, in which case we just clean up here. */
> +    if ( !p )

I cannot find any code removing a pending_irq from the radix tree, so it 
is hard to know how p will not be freed whilst in use here. Is there any 
lock?

> +    {
> +        ASSERT(is_lpi(irq));
> +
> +        gic_hw_ops->clear_lr(i);
> +        clear_bit(i, &this_cpu(lr_mask));
> +
> +        return;
> +    }
> +
>      if ( lr_val.state & GICH_LR_ACTIVE )
>      {
>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 3c7161b..95fa0ba 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -330,6 +330,23 @@ read_unknown:
>      return 1;
>  }
>
> +/*
> + * Looks up a virtual LPI number in our tree of mapped LPIs. This will return
> + * the corresponding struct pending_irq, which we also use to store the
> + * enabled and pending bit plus the priority.
> + * Returns NULL if an LPI cannot be found.

See my question above regarding the behavior of this function. It is not 
clear to me what is protecting the pending_irq to be freed whilst in use.

> + */
> +struct pending_irq *lpi_to_pending(struct domain *d, unsigned int lpi)

This function ought to be a vgic_ops callback. I will not say it again.

> +{
> +    struct pending_irq *pirq;
> +
> +    read_lock(&d->arch.vgic.pend_lpi_tree_lock);
> +    pirq = radix_tree_lookup(&d->arch.vgic.pend_lpi_tree, lpi);
> +    read_unlock(&d->arch.vgic.pend_lpi_tree_lock);
> +
> +    return pirq;
> +}
> +
>  static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>                                            uint32_t gicr_reg,
>                                            register_t r)
> @@ -1452,6 +1469,9 @@ static int vgic_v3_domain_init(struct domain *d)
>      spin_lock_init(&d->arch.vgic.its_devices_lock);
>      d->arch.vgic.its_devices = RB_ROOT;
>
> +    rwlock_init(&d->arch.vgic.pend_lpi_tree_lock);
> +    radix_tree_init(&d->arch.vgic.pend_lpi_tree);
> +
>      /*
>       * Domain 0 gets the hardware address.
>       * Guests get the virtual platform layout.
> @@ -1525,6 +1545,7 @@ static int vgic_v3_domain_init(struct domain *d)
>  static void vgic_v3_domain_free(struct domain *d)
>  {
>      gicv3_its_unmap_all_devices(d);
> +    radix_tree_destroy(&d->arch.vgic.pend_lpi_tree, NULL);

You may have thousands LPIs mapped, how long will you expect this to take?

>      xfree(d->arch.vgic.rdist_regions);
>  }
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 67d75a6..28f6f66 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -431,10 +431,15 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
>  struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>  {
>      struct pending_irq *n;
> +

Spurious change.

>      /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
>       * are used for SPIs; the rests are used for per cpu irqs */
>      if ( irq < 32 )
>          n = &v->arch.vgic.pending_irqs[irq];
> +#ifdef CONFIG_HAS_ITS

I really don't want to see #ifdef CONFIG_HAS_ITS nor vGIC specific 
function called directly from the common code. Please introduce proper 
vgic_ops.

> +    else if ( is_lpi(irq) )
> +        n = lpi_to_pending(v->domain, irq);
> +#endif
>      else
>          n = &v->domain->arch.vgic.pending_irqs[irq - 32];
>      return n;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 503a3cf..6ee7538 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -111,6 +111,8 @@ struct arch_domain
>          uint32_t rdist_stride;              /* Re-Distributor stride */
>          struct rb_root its_devices;         /* Devices mapped to an ITS */
>          spinlock_t its_devices_lock;        /* Protects the its_devices tree */
> +        struct radix_tree_root pend_lpi_tree; /* Stores struct pending_irq's */
> +        rwlock_t pend_lpi_tree_lock;        /* Protects the pend_lpi_tree */
>  #endif
>      } vgic;
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 894c3f1..69ef160 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -300,6 +300,7 @@ extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
>  extern void vgic_clear_pending_irqs(struct vcpu *v);
>  extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
>  extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq);
> +extern struct pending_irq *lpi_to_pending(struct domain *d, unsigned int irq);
>  extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
>  extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
>  extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
>

Cheers,

-- 
Julien Grall

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