Re: [TF-A] Insufficient ordering guarantees in bakery_lock_get() and bakery_lock_release() in bakery_lock_coherent.c
Hi Soby,
Thanks for your response.
it is needed to ensure the ordering of the succeeding sev().
Agree. Thanks for the clarification.
Was this an issue that actually manifested on a hardware or is this
something that you caught while reviewing the code? Noticed it while reviewing code and I have not observed it on hardware.
Thanks -Raghu
On November 26, 2019 at 8:55 AM, Soby Mathew Soby.Mathew@arm.com wrote:
On 26/11/2019 16:30, Raghupathy Krishnamurthy via TF-A wrote: Hello!
Reposting this from (https://developer.trustedfirmware.org/T589).
bakery_lock_get() uses a dmbld() after lock acquisition which is insufficient in a lock acquire situation. With just dmbld(), stores in a critical section can be reordered before the dmbld() and indeed before the lock acquisition has taken place. similarly, bakery_lock_release() only uses dmbst(). A load in the critical section could be reordered after the dmbst() and write to the lock data structure releasing the lock. This is likely less of a problem but lock release needs to provide release semantics, and dmbst() is insufficient. For ex: A load in the critical section of CPU1 can be reordered after the store for the lock release, and it could read from a store that is executed on CPU2 in the same critical section, since CPU2 saw the store for the lock release first, and raced into the critical section.
Hi Raghu, You are right on this. The dmbld() and dmbst() does not provide sufficient guarantees in the cases you mention.
Was this an issue that actually manifested on a hardware or is this something that you caught while reviewing the code ?
Also the dsb() after the write to the lock seems unnecessary. Am I missing something here ? It looks like the same issue is present even in bakery_lock_normal.
If you are referring to the dsb() at this line : https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/lib/locks/b...
it is needed to ensure the ordering of the succeeding sev().
Best Regards Soby Mathew
Thanks Raghu
On 26/11/2019 18:18, Raghupathy Krishnamurthy wrote:
Hi Soby,
Thanks for your response.
it is needed to ensure the ordering of the succeeding sev().
Agree. Thanks for the clarification.
Was this an issue that actually manifested on a hardware or is this
something that you caught while reviewing the code? Noticed it while reviewing code and I have not observed it on hardware.
Thanks -Raghu
On November 26, 2019 at 8:55 AM, Soby Mathew Soby.Mathew@arm.com wrote:
On 26/11/2019 16:30, Raghupathy Krishnamurthy via TF-A wrote:
Hello!
Reposting this from (https://developer.trustedfirmware.org/T589).
bakery_lock_get() uses a dmbld() after lock acquisition which is insufficient in a lock acquire situation. With just dmbld(), stores in a critical section can be reordered before the dmbld() and indeed before the lock acquisition has taken place. similarly, bakery_lock_release() only uses dmbst(). A load in the critical section could be reordered after the dmbst() and write to the lock data structure releasing the lock. This is likely less of a problem but lock release needs to provide release semantics, and dmbst() is insufficient. For ex: A load in the critical section of CPU1 can be reordered after the store for the lock release, and it could read from a store that is executed on CPU2 in the same critical section, since CPU2 saw the store for the lock release first, and raced into the critical section.
Hi Raghu, You are right on this. The dmbld() and dmbst() does not provide sufficient guarantees in the cases you mention.
Was this an issue that actually manifested on a hardware or is this something that you caught while reviewing the code ?
Also the dsb() after the write to the lock seems unnecessary. Am I missing something here ? It looks like the same issue is present even in bakery_lock_normal.
If you are referring to the dsb() at this line : https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/lib/locks/b...
it is needed to ensure the ordering of the succeeding sev().
Best Regards Soby Mathew
Thanks Raghu
Thanks Raghu for your confirmation. Would be you be able to submit a patch for the same ?
Best Regards Soby Mathew
Sure. Will try and upload one soon.
Raghu
On Dec 2, 2019, at 2:56 AM, Soby Mathew Soby.Mathew@arm.com wrote:
On 26/11/2019 18:18, Raghupathy Krishnamurthy wrote:
Hi Soby,
Thanks for your response.
it is needed to ensure the ordering of the succeeding sev().
Agree. Thanks for the clarification.
Was this an issue that actually manifested on a hardware or is this
something that you caught while reviewing the code? Noticed it while reviewing code and I have not observed it on hardware.
Thanks -Raghu
On November 26, 2019 at 8:55 AM, Soby Mathew Soby.Mathew@arm.com wrote:
On 26/11/2019 16:30, Raghupathy Krishnamurthy via TF-A wrote:
Hello!
Reposting this from (https://developer.trustedfirmware.org/T589).
bakery_lock_get() uses a dmbld() after lock acquisition which is insufficient in a lock acquire situation. With just dmbld(), stores in a critical section can be reordered before the dmbld() and indeed before the lock acquisition has taken place. similarly, bakery_lock_release() only uses dmbst(). A load in the critical section could be reordered after the dmbst() and write to the lock data structure releasing the lock. This is likely less of a problem but lock release needs to provide release semantics, and dmbst() is insufficient. For ex: A load in the critical section of CPU1 can be reordered after the store for the lock release, and it could read from a store that is executed on CPU2 in the same critical section, since CPU2 saw the store for the lock release first, and raced into the critical section.
Hi Raghu, You are right on this. The dmbld() and dmbst() does not provide sufficient guarantees in the cases you mention.
Was this an issue that actually manifested on a hardware or is this something that you caught while reviewing the code ?
Also the dsb() after the write to the lock seems unnecessary. Am I missing something here ? It looks like the same issue is present even in bakery_lock_normal.
If you are referring to the dsb() at this line : https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/lib/locks/b...
it is needed to ensure the ordering of the succeeding sev().
Best Regards Soby Mathew
Thanks Raghu
Thanks Raghu for your confirmation. Would be you be able to submit a patch for the same ?
Best Regards Soby Mathew
tf-a@lists.trustedfirmware.org
-
Raghupathy Krishnamurthy -
Soby Mathew