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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 5 Sep 2022, at 17:55, Julien Grall <julien@xxxxxxx> 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@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.

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