Re: [Xen-devel] [PATCH 4/4] xen/arm: gic: Relax barrier when sending an SGI

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 23 Oct 2018, Julien Grall wrote:

> When sending an SGI to another CPU, we require a barrier to ensure that
> any pending stores to normal memory are made visible to the recipient
> before the interrupt arrives.
> 
> For GICv2, rather than using dsb(sy) before writel_gicd, we can instead
> use dsb(ishst), since we just need to ensure that any pending normal
> writes are visible within the inner-shareable domain before we poke the
> GIC.
> 
> With this observation, we can then further weaken the barrier to a
> dmb(ishst), since other CPUs in the inner-shareable domain must observe
> the write to the distributor before the SGI is generated.
> 
> A DMB instruction can be used to ensure the relative order of only
> memory accesses before and after the barrier. Since writes to system
> registers are not memory operations, barrier DMB is not sufficient for
> observalibility of memory accesses that occur before ICC_SGI1R_EL1
> (GICv3).
> 
> For GICv3, a DSB instruction ensures that no instructions that appear in
> program order after the DSB instruction, can execute until the DSB
> instruction has completed.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/gic-v2.c |  6 ++++++
>  xen/arch/arm/gic-v3.c |  6 ++++++
>  xen/arch/arm/gic.c    | 18 ------------------
>  3 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index e7eb01f30a..1a744c576f 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -455,6 +455,12 @@ static void gicv2_send_SGI(enum gic_sgi sgi, enum 
> gic_sgi_mode irqmode,
>      unsigned int mask = 0;
>      cpumask_t online_mask;
>  
> +    /*
> +     * Ensure that stores to Normal memory are visible to the other CPUs
> +     * before they observe us issuing the IPI.
> +     */
> +    dmb(ishst);
> +
>      switch ( irqmode )
>      {
>      case SGI_TARGET_OTHERS:
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 2952335d05..a0a1a45ba7 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -986,6 +986,12 @@ static void gicv3_send_sgi_list(enum gic_sgi sgi, const 
> cpumask_t *cpumask)
>  static void gicv3_send_sgi(enum gic_sgi sgi, enum gic_sgi_mode mode,
>                             const cpumask_t *cpumask)
>  {
> +    /*
> +     * Ensure that stores to Normal memory are visible to the other CPUs
> +     * before issuing the IPI.
> +     */
> +    wmb();

NIT: do we want to use dsb(st) instead of wmb() for consistency with
gic-v2.c and to make the difference more obvious?

In any case

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


FYI I committed the first three patches.


>      switch ( mode )
>      {
>      case SGI_TARGET_OTHERS:
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 0108e9603c..077b941b79 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -300,12 +300,6 @@ void send_SGI_mask(const cpumask_t *cpumask, enum 
> gic_sgi sgi)
>  {
>      ASSERT(sgi < 16); /* There are only 16 SGIs */
>  
> -   /*
> -    * Ensure that stores to Normal memory are visible to the other CPUs
> -    * before issuing the IPI.
> -    * Matches the read barrier in do_sgi.
> -    */
> -    dsb(sy);
>      gic_hw_ops->send_SGI(sgi, SGI_TARGET_LIST, cpumask);
>  }
>  
> @@ -318,12 +312,6 @@ void send_SGI_self(enum gic_sgi sgi)
>  {
>      ASSERT(sgi < 16); /* There are only 16 SGIs */
>  
> -   /*
> -    * Ensure that stores to Normal memory are visible to the other CPUs
> -    * before issuing the IPI.
> -    * Matches the read barrier in do_sgi.
> -    */
> -    dsb(sy);
>      gic_hw_ops->send_SGI(sgi, SGI_TARGET_SELF, NULL);
>  }
>  
> @@ -331,12 +319,6 @@ void send_SGI_allbutself(enum gic_sgi sgi)
>  {
>     ASSERT(sgi < 16); /* There are only 16 SGIs */
>  
> -   /*
> -    * Ensure that stores to Normal memory are visible to the other CPUs
> -    * before issuing the IPI.
> -    * Matches the read barrier in do_sgi.
> -    */
> -   dsb(sy);
>     gic_hw_ops->send_SGI(sgi, SGI_TARGET_OTHERS, NULL);
>  }
>  
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel