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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Rahul,

> On 6 Sep 2022, at 10:55, Rahul Singh <Rahul.Singh@xxxxxxx> wrote:
> 
> From: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> 
> Backport Linux commit a76a37777f2c. Introduce __iomb() in the smmu-v3.c
> file with other Linux compatibility definitions.
> 
> 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>

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Cheers
Bertrand

> ---
> Changes in v4:
> - rename iomb() to __iomb()
> Changes in v3:
> - rename __iomb() to iomb() and also move it from common file to
>   smmu-v3.c file
> Changes in v2:
> - fix commit msg
> - add __iomb changes also from the origin patch
> ---
> xen/drivers/passthrough/arm/smmu-v3.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index 64d39bb4d3..229b9a4b0d 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -107,6 +107,8 @@ typedef paddr_t           dma_addr_t;
> typedef paddr_t               phys_addr_t;
> typedef unsigned int          gfp_t;
> 
> +#define __iomb()             dmb(osh)
> +
> #define platform_device               device
> 
> #define GFP_KERNEL            0
> @@ -951,7 +953,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 +965,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
>