Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spin lock deadlock detection #1670

Merged
merged 2 commits into from
Jul 15, 2017
Merged

Spin lock deadlock detection #1670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98afff7
core: attempt to detect deadlock on spin locks
jenswi-linaro Jul 7, 2017
413a61f
core: pager: more details in deadlock report
jenswi-linaro Jul 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions core/arch/arm/include/kernel/spinlock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,47 @@ void __cpu_spin_unlock(unsigned int *lock);
/* returns 0 on locking success, non zero on failure */
unsigned int __cpu_spin_trylock(unsigned int *lock);

static inline void cpu_spin_lock(unsigned int *lock)
static inline void cpu_spin_lock_no_dldetect(unsigned int *lock)
{
assert(thread_foreign_intr_disabled());
__cpu_spin_lock(lock);
spinlock_count_incr();
}

#ifdef CFG_TEE_CORE_DEBUG
#define cpu_spin_lock(lock) \
cpu_spin_lock_dldetect(__func__, __LINE__, lock)

static inline void cpu_spin_lock_dldetect(const char *func, const int line,
unsigned int *lock)
{
unsigned int retries = 0;
unsigned int reminder = 0;

assert(thread_foreign_intr_disabled());

while (__cpu_spin_trylock(lock)) {
retries++;
if (!retries) {
/* wrapped, time to report */
trace_printf(func, line, TRACE_ERROR, true,
"possible spinlock deadlock reminder %u",
reminder);
if (reminder < UINT_MAX)
reminder++;
}
}

spinlock_count_incr();
}
#else
static inline void cpu_spin_lock(unsigned int *lock)
{
cpu_spin_lock_no_dldetect(lock);
}
#endif


static inline bool cpu_spin_trylock(unsigned int *lock)
{
unsigned int rc;
Expand All @@ -82,14 +116,35 @@ static inline void cpu_spin_unlock(unsigned int *lock)
spinlock_count_decr();
}

static inline uint32_t cpu_spin_lock_xsave(unsigned int *lock)
static inline uint32_t cpu_spin_lock_xsave_no_dldetect(unsigned int *lock)
{
uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL);

cpu_spin_lock(lock);
return exceptions;
}


#ifdef CFG_TEE_CORE_DEBUG
#define cpu_spin_lock_xsave(lock) \
cpu_spin_lock_xsave_dldetect(__func__, __LINE__, lock)

static inline uint32_t cpu_spin_lock_xsave_dldetect(const char *func,
const int line,
unsigned int *lock)
{
uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL);

cpu_spin_lock_dldetect(func, line, lock);
return exceptions;
}
#else
static inline uint32_t cpu_spin_lock_xsave(unsigned int *lock)
{
return cpu_spin_lock_xsave_no_dldetect(lock);
}
#endif

static inline void cpu_spin_unlock_xrestore(unsigned int *lock,
uint32_t exceptions)
{
Expand Down
2 changes: 1 addition & 1 deletion core/arch/arm/kernel/trace_ext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void trace_ext_puts(const char *str)

if (mmu_enabled && !cpu_spin_trylock(&puts_lock)) {
was_contended = true;
cpu_spin_lock(&puts_lock);
cpu_spin_lock_no_dldetect(&puts_lock);
}

console_flush();
Expand Down
44 changes: 36 additions & 8 deletions core/arch/arm/mm/tee_pager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,38 @@ static tee_mm_entry_t *pager_alias_area;
*/
static uintptr_t pager_alias_next_free;

static uint32_t pager_lock(void)
#ifdef CFG_TEE_CORE_DEBUG
#define pager_lock(ai) pager_lock_dldetect(__func__, __LINE__, ai)

static uint32_t pager_lock_dldetect(const char *func, const int line,
struct abort_info *ai)
{
uint32_t exceptions = thread_mask_exceptions(THREAD_EXCP_ALL);
unsigned int retries = 0;
unsigned int reminder = 0;

while (!cpu_spin_trylock(&pager_spinlock)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

! or not ! ?

Copy link
Contributor

@igoropaniuk igoropaniuk Jul 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etienne-lms

/* returns 0 on locking success, non zero on failure */
unsigned int __cpu_spin_trylock(unsigned int *lock);
....
static inline bool cpu_spin_trylock(unsigned int *lock)
{
	unsigned int rc;

	assert(thread_foreign_intr_disabled());
	rc = __cpu_spin_trylock(lock);
	if (!rc)
		spinlock_count_incr();
	return !rc;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually understand why cpu_spin_trylock() == !__cpu_spin_trylock(), which can throw into confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it is confusing. It stems from strex returning 1 in <Rd> on failure and 0 on success and that __cpu_spin_trylock() tries to do the bare minimum.

The code here is correct though.

retries++;
if (!retries) {
/* wrapped, time to report */
trace_printf(func, line, TRACE_ERROR, true,
"possible spinlock deadlock reminder %u",
reminder);
if (reminder < UINT_MAX)
reminder++;
if (ai)
abort_print(ai);
}
}

return exceptions;
}
#else
static uint32_t pager_lock(struct abort_info __unused *ai)
{
return cpu_spin_lock_xsave(&pager_spinlock);
}
#endif

static void pager_unlock(uint32_t exceptions)
{
Expand Down Expand Up @@ -328,7 +356,7 @@ static struct tee_pager_area *alloc_area(struct pgt *pgt,

static void area_insert_tail(struct tee_pager_area *area)
{
uint32_t exceptions = pager_lock();
uint32_t exceptions = pager_lock(NULL);

TAILQ_INSERT_TAIL(&tee_pager_area_head, area, link);

Expand Down Expand Up @@ -719,7 +747,7 @@ static void init_tbl_info_from_pgt(struct core_mmu_table_info *ti,
static void transpose_area(struct tee_pager_area *area, struct pgt *new_pgt,
vaddr_t new_base)
{
uint32_t exceptions = pager_lock();
uint32_t exceptions = pager_lock(NULL);

/*
* If there's no pgt assigned to the old area there's no pages to
Expand Down Expand Up @@ -805,7 +833,7 @@ static void rem_area(struct tee_pager_area_head *area_head,
struct tee_pager_pmem *pmem;
uint32_t exceptions;

exceptions = pager_lock();
exceptions = pager_lock(NULL);

TAILQ_REMOVE(area_head, area, link);

Expand Down Expand Up @@ -874,7 +902,7 @@ bool tee_pager_set_uta_area_attr(struct user_ta_ctx *utc, vaddr_t base,
f |= TEE_MATTR_PW;
f = get_area_mattr(f);

exceptions = pager_lock();
exceptions = pager_lock(NULL);

while (s) {
s2 = MIN(CORE_MMU_PGDIR_SIZE - (b & CORE_MMU_PGDIR_MASK), s);
Expand Down Expand Up @@ -1220,7 +1248,7 @@ bool tee_pager_handle_fault(struct abort_info *ai)
* page, instead we use the aliased mapping to populate the page
* and once everything is ready we map it.
*/
exceptions = pager_lock();
exceptions = pager_lock(ai);

stat_handle_fault();

Expand Down Expand Up @@ -1417,7 +1445,7 @@ void tee_pager_pgt_save_and_release_entries(struct pgt *pgt)
{
struct tee_pager_pmem *pmem;
struct tee_pager_area *area;
uint32_t exceptions = pager_lock();
uint32_t exceptions = pager_lock(NULL);

if (!pgt->num_used_entries)
goto out;
Expand Down Expand Up @@ -1460,7 +1488,7 @@ void tee_pager_release_phys(void *addr, size_t size)
area != find_area(&tee_pager_area_head, end - SMALL_PAGE_SIZE))
panic();

exceptions = pager_lock();
exceptions = pager_lock(NULL);

for (va = begin; va < end; va += SMALL_PAGE_SIZE)
unmaped |= tee_pager_release_one_phys(area, va);
Expand Down