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:49, Rahul Singh wrote:
> Hi Julien,
Hi Rahul,

> > On 5 Sep 2022, at 5:37 pm, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > 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?
> >    
> 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,

--
Julien Grall