Message ID | bd55c05d7197b72cb4597c7412fc4911fa2281d1.1662394710.git.rahul.singh@arm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xen/arm: smmuv3: Merge Linux fixes to Xen | expand |
On 05/09/2022 17:30, Rahul Singh wrote: > From: Zhou Wang <wangzhou1@hisilicon.com> > > 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@hisilicon.com> > Link: https://lore.kernel.org/r/1601281922-117296-1-git-send-email-wangzhou1@hisilicon.com > [will: Use readl() instead of explicit barrier. Update 'cons' side to match.] > Signed-off-by: Will Deacon <will@kernel.org> > Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a76a37777f2c > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > 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;
Hi Julien, > On 5 Sep 2022, at 5:37 pm, Julien Grall <julien@xen.org> wrote: > > > > On 05/09/2022 17:30, Rahul Singh wrote: >> From: Zhou Wang <wangzhou1@hisilicon.com> >> 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@hisilicon.com> >> Link: https://lore.kernel.org/r/1601281922-117296-1-git-send-email-wangzhou1@hisilicon.com >> [will: Use readl() instead of explicit barrier. Update 'cons' side to match.] >> Signed-off-by: Will Deacon <will@kernel.org> >> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a76a37777f2c >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> 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? > I am really sorry that I missed the naming and when I made the patch in my mind there was a comment from Stefano to rename the __iomb() to iomb(). I will send only this patch after fixing or do you want me to send the whole series? Regards, Rahul
On 05/09/2022 17:49, Rahul Singh wrote: > Hi Julien, Hi Rahul, > >> On 5 Sep 2022, at 5:37 pm, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 05/09/2022 17:30, Rahul Singh wrote: >>> From: Zhou Wang <wangzhou1@hisilicon.com> >>> 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@hisilicon.com> >>> Link: https://lore.kernel.org/r/1601281922-117296-1-git-send-email-wangzhou1@hisilicon.com >>> [will: Use readl() instead of explicit barrier. Update 'cons' side to match.] >>> Signed-off-by: Will Deacon <will@kernel.org> >>> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a76a37777f2c >>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>> --- >>> 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? >> > > I am really sorry that I missed the naming and when I made the patch in my mind there was a comment from Stefano to > rename the __iomb() to iomb(). I will send only this patch after fixing or do you want me to send the whole series? I would be fine if you only resend this patch. Also, looking at the other patches, you added the Acked-by before your Signed-off-by. In general, the tags are ordered chronologically, so this should be inverted. I can deal with that on commit once Bertrand confirmed he is happy with the series. Cheers,
Hi Julien, > On 5 Sep 2022, at 17:55, Julien Grall <julien@xen.org> wrote: > > > > On 05/09/2022 17:49, Rahul Singh wrote: >> Hi Julien, > > Hi Rahul, > >>> On 5 Sep 2022, at 5:37 pm, Julien Grall <julien@xen.org> wrote: >>> >>> >>> >>> On 05/09/2022 17:30, Rahul Singh wrote: >>>> From: Zhou Wang <wangzhou1@hisilicon.com> >>>> 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@hisilicon.com> >>>> Link: https://lore.kernel.org/r/1601281922-117296-1-git-send-email-wangzhou1@hisilicon.com >>>> [will: Use readl() instead of explicit barrier. Update 'cons' side to match.] >>>> Signed-off-by: Will Deacon <will@kernel.org> >>>> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a76a37777f2c >>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>>> --- >>>> 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? >>> >> I am really sorry that I missed the naming and when I made the patch in my mind there was a comment from Stefano to >> rename the __iomb() to iomb(). I will send only this patch after fixing or do you want me to send the whole series? > > I would be fine if you only resend this patch. > > Also, looking at the other patches, you added the Acked-by before your Signed-off-by. In general, the tags are ordered chronologically, so this should be inverted. I can deal with that on commit once Bertrand confirmed he is happy with the series. With the acked-by needing to be moved and the iomb part, it will make your life easier if Rahul just resend the serie so Rahul please send a v4. Cheers Bertrand > > Cheers, > > -- > Julien Grall
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;