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

[Julien Grall <julien@xxxxxxx>]

On 05/09/2022 17:30, Rahul Singh wrote:
> From: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
>
> Backport Linux commit a76a37777f2c. Rename __iomb to iomb() while
> merging to get in sync with other Xen 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>
> ---
> Changes in v3:
>   - rename __iomb() to iomb() and also move it from common file to
>     smmu-v3.c file
Hmmm... Quoting Bertrand:

"We need the __iomb as “linux compatibility” in fact so I would suggest 
for now to only introduce it at the beginning of smmu-v3.c with other 
linux compatibility stuff to prevent adding this to Xen overall."
Which I also agreed. I couldn't a more recent conversation explaining 
your approach. Can you outline why you didn't follow the approached 
discussed?
Cheers,

> 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..e632c75b21 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;
--
Julien Grall