Re: [Xen-devel] [PATCH v4 07/27] ARM: GICv3 ITS: introduce host LPI array

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 04/03/2017 09:28 PM, Andre Przywara wrote:
>  /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
>  void gicv3_its_dt_init(const struct dt_device_node *node)
>  {
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> index 8b3660a..d3ee141 100644
> --- a/xen/arch/arm/gic-v3-lpi.c
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -20,25 +20,59 @@
[...]

> +/*
> + * There could be a lot of LPIs on the host side, and they always go to
> + * a guest. So having a struct irq_desc for each of them would be wasteful
> + * and useless.
> + * Instead just store enough information to find the right VCPU to inject
> + * those LPIs into, which just requires the virtual LPI number.
> + * To avoid a global lock on this data structure, this is using a lockless
> + * approach relying on the architectural atomicty of native data types:
s/atomicty/atomicity/

[...]

> +/*
> + * Allocate a block of 32 LPIs on the given host ITS for device "devid",
> + * starting with "eventid". Put them into the respective ITT by issuing a
> + * MAPTI command for each of them.
> + */
> +int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t *first_lpi)
> +{
[...]

> +    /* If we hit an unallocated chunk, we initialize it and use entry 0. */
> +    if ( !lpi_data.host_lpis[chunk] )
> +    {
> +        union host_lpi *new_chunk;
> +
> +        /* TODO: NUMA locality for quicker IRQ path? */
> +        new_chunk = xmalloc_bytes(PAGE_SIZE);
NIT: As suggested on v2, this could be xenheap_alloc_page(0);

> +        if ( !new_chunk )
> +        {
> +            spin_unlock(&lpi_data.host_lpis_lock);
> +            return -ENOMEM;
> +        }
> +
> +        for ( i = 0; i < HOST_LPIS_PER_PAGE; i += LPI_BLOCK )
> +            new_chunk[i].dom_id = DOMID_INVALID;
As host_lpis could be read without lock, I would add a barrier here to 
make sure new_chunk[*].dom_id have been written before setting up the page.
A similar barrier would be needed in the do_lpi(...) path.

[...]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 836a103..d04bd04 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -220,6 +220,8 @@ enum gic_version {
>      GIC_V3,
>  };
>
> +#define INVALID_LPI     0
Again, I think INVALID_LPI should be moved in irq.h to stay with the 
definition of LPI_OFFSET. It was supposed to be fixed in v3...
Cheers,

--
Julien Grall

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