From 95c1255967b46efa3e7add1eafd9e35862bfc677 Mon Sep 17 00:00:00 2001 From: Soby Mathew Date: Mon, 14 Nov 2016 17:19:35 +0000 Subject: [PATCH] Fix normal memory bakery lock implementation This patch fixes an issue in the normal memory bakery lock implementation. During assertion of lock status, there is a possibility that the assertion could fail. This is because the previous update done to the lock status by the owning CPU when not participating in cache coherency could result in stale data in the cache due to cache maintenance operations not propagating to all the caches. This patch fixes this issue by doing an extra read cache maintenance operation prior to the assertion. Fixes ARM-software/tf-issues#402 Change-Id: I0f38a7c52476a4f58e17ebe0141d256d198be88d Signed-off-by: Soby Mathew --- lib/locks/bakery/bakery_lock_normal.c | 29 +++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/lib/locks/bakery/bakery_lock_normal.c b/lib/locks/bakery/bakery_lock_normal.c index efc1b57e..5a2fb071 100644 --- a/lib/locks/bakery/bakery_lock_normal.c +++ b/lib/locks/bakery/bakery_lock_normal.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015, ARM Limited and Contributors. All rights reserved. + * Copyright (c) 2015-2016, ARM Limited and Contributors. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: @@ -90,6 +90,22 @@ extern void *__PERCPU_BAKERY_LOCK_SIZE__; #define read_cache_op(addr, cached) if (cached) \ dccivac((uintptr_t)addr) +/* Helper function to check if the lock is acquired */ +static inline int is_lock_acquired(const bakery_info_t *my_bakery_info, + int is_cached) +{ + /* + * Even though lock data is updated only by the owning cpu and + * appropriate cache maintenance operations are performed, + * if the previous update was done when the cpu was not participating + * in coherency, then there is a chance that cache maintenance + * operations were not propagated to all the caches in the system. + * Hence do a `read_cache_op()` prior to read. + */ + read_cache_op(my_bakery_info, is_cached); + return !!(bakery_ticket_number(my_bakery_info->lock_data)); +} + static unsigned int bakery_get_ticket(bakery_lock_t *lock, unsigned int me, int is_cached) { @@ -104,12 +120,8 @@ static unsigned int bakery_get_ticket(bakery_lock_t *lock, my_bakery_info = get_bakery_info(me, lock); assert(my_bakery_info); - /* - * Prevent recursive acquisition. - * Since lock data is written to and cleaned by the owning cpu, it - * doesn't require any cache operations prior to reading the lock data. - */ - assert(!bakery_ticket_number(my_bakery_info->lock_data)); + /* Prevent recursive acquisition.*/ + assert(!is_lock_acquired(my_bakery_info, is_cached)); /* * Tell other contenders that we are through the bakery doorway i.e. @@ -222,7 +234,8 @@ void bakery_lock_release(bakery_lock_t *lock) unsigned int is_cached = read_sctlr_el3() & SCTLR_C_BIT; my_bakery_info = get_bakery_info(plat_my_core_pos(), lock); - assert(bakery_ticket_number(my_bakery_info->lock_data)); + + assert(is_lock_acquired(my_bakery_info, is_cached)); my_bakery_info->lock_data = 0; write_cache_op(my_bakery_info, is_cached); -- 2.30.2