Re: [PATCH v5 12/30] ARM: GICv3: introduce separate pending_irq structs for LPIs

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

Hi Andre,

On 06/04/17 00:19, Andre Przywara wrote:
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 142eb64..5128f13 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1451,6 +1451,9 @@ static int vgic_v3_domain_init(struct domain *d)
>
>      vgic_v3_its_init_domain(d);
>
> +    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.
> @@ -1524,14 +1527,33 @@ static int vgic_v3_domain_init(struct domain *d)
>  static void vgic_v3_domain_free(struct domain *d)
>  {
>      vgic_v3_its_free_domain(d);
> +    radix_tree_destroy(&d->arch.vgic.pend_lpi_tree, NULL);
>      xfree(d->arch.vgic.rdist_regions);
>  }
>
> +/*
> + * 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 (or no LPIs are supported).

I asked few questions on the previous version about when lpi_to_pending 
returns NULL and the fact that none of the vGIC code is able to handle 
that. I was hoping a bit of documentation/ASSERT in the code rather than 
hiding the problem as you seem to do in this version.

Looking at the whole code, I see that you insert the pending_irq in the 
radix when on MAPTI. But never remove it. However the command DISCARD or 
MAPD (V=0) will remove the mapping from the ITT.

Which means it is never possible to re-use the same vLPI again. However 
if you remove from the radix tree you may end up having the LPI still in 
LR but lpi_to_pending return NULL and make Xen segfault.

So what will protect Xen against segfault? What is the plan?


> 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..7c86f5b 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -134,6 +134,8 @@ struct vgic_ops {
>      void (*domain_free)(struct domain *d);
>      /* vGIC sysreg/cpregs emulate */
>      bool (*emulate_reg)(struct cpu_user_regs *regs, union hsr hsr);
> +    /* lookup the struct pending_irq for a given LPI interrupt */
> +    struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi);
>      /* Maximum number of vCPU supported */
>      const unsigned int max_vcpus;
>  };
>

Cheers,

-- 
Julien Grall

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