Re: [PATCH v2 03/10] xen/arm: smmuv3: Ensure queue is read after updating prod pointer

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 2 Sep 2022, Rahul Singh wrote:
> From: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> 
> Backport Linux commit a76a37777f2c. This is the clean backport without
> any changes.
> 
> Reading the 'prod' MMIO register in order to determine whether or
> not there is valid data beyond 'cons' for a given queue does not
> provide sufficient dependency ordering, as the resulting access is
> address dependent only on 'cons' and can therefore be speculated
> ahead of time, potentially allowing stale data to be read by the
> CPU.
> 
> Use readl() instead of readl_relaxed() when updating the shadow copy
> of the 'prod' pointer, so that all speculated memory reads from the
> corresponding queue can occur only from valid slots.
> 
> Signed-off-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> Link: 
> https://lore.kernel.org/r/1601281922-117296-1-git-send-email-wangzhou1@xxxxxxxxxxxxx
> [will: Use readl() instead of explicit barrier. Update 'cons' side to match.]
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> a76a37777f2c
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> ---
> Changes in v2:
>  - fix commit msg
>  - add _iomb changes also from the origin patch
> ---
>  xen/arch/arm/include/asm/system.h     |  1 +
>  xen/drivers/passthrough/arm/smmu-v3.c | 11 +++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/system.h 
> b/xen/arch/arm/include/asm/system.h
> index 65d5c8e423..fe27cf8c5e 100644
> --- a/xen/arch/arm/include/asm/system.h
> +++ b/xen/arch/arm/include/asm/system.h
> @@ -29,6 +29,7 @@
>  #endif
>  
>  #define smp_wmb()       dmb(ishst)
> +#define __iomb()        dmb(osh)

We don't have any other #define starting with __ in system.h.
I wonder if we should call this macro differently or simply iomb().


>  #define smp_mb__before_atomic()    smp_mb()
>  #define smp_mb__after_atomic()     smp_mb()
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index 64d39bb4d3..cee13d1fc7 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -951,7 +951,7 @@ static void queue_sync_cons_out(struct arm_smmu_queue *q)
>        * Ensure that all CPU accesses (reads and writes) to the queue
>        * are complete before we update the cons pointer.
>        */
> -     mb();
> +     __iomb();
>       writel_relaxed(q->llq.cons, q->cons_reg);
>  }
>  
> @@ -963,8 +963,15 @@ static void queue_inc_cons(struct arm_smmu_ll_queue *q)
>  
>  static int queue_sync_prod_in(struct arm_smmu_queue *q)
>  {
> +     u32 prod;
>       int ret = 0;
> -     u32 prod = readl_relaxed(q->prod_reg);
> +
> +     /*
> +      * We can't use the _relaxed() variant here, as we must prevent
> +      * speculative reads of the queue before we have determined that
> +      * prod has indeed moved.
> +      */
> +     prod = readl(q->prod_reg);
>  
>       if (Q_OVF(prod) != Q_OVF(q->llq.prod))
>               ret = -EOVERFLOW;
> -- 
> 2.25.1
>