[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.