Skip to content

userspace: get rid of app section placeholders #13924

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

Merged
merged 3 commits into from
Mar 4, 2019
Merged

userspace: get rid of app section placeholders #13924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
615cbfe
tests: userspace: remove unused partition
Mar 2, 2019
51278ee
userspace: clarify memory domain assertions
Mar 2, 2019
d48b883
userspace: get rid of app section placeholders
Feb 28, 2019
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
34 changes: 32 additions & 2 deletions include/app_memory/app_memdomain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,37 @@ struct z_app_region {
#define Z_APP_BSS_START(id) data_smem_##id##_bss_start
#define Z_APP_BSS_SIZE(id) data_smem_##id##_bss_size

/* If a partition is declared with K_APPMEM_PARTITION, but never has any
* data assigned to its contents, then no symbols with its prefix will end
* up in the symbol table. This prevents gen_app_partitions.py from detecting
* that the partition exists, and the linker symbols which specify partition
* bounds will not be generated, resulting in build errors.
*
* What this inline assembly code does is define a symbol with no data.
* This should work for all arches that produce ELF binaries, see
* https://sourceware.org/binutils/docs/as/Section.html
*
* We don't know what active flags/type of the pushed section were, so we are
* specific: "aw" indicates section is allocatable and writable,
* and "@progbits" indicates the section has data.
*/
#ifdef CONFIG_ARM
/* ARM has a quirk in that '@' denotes a comment, so we have to send
* %progbits to the assembler instead.
*/
#define Z_PROGBITS_SYM "\%"
#else
#define Z_PROGBITS_SYM "@"
#endif

#define Z_APPMEM_PLACEHOLDER(name) \
__asm__ ( \
".pushsection " STRINGIFY(K_APP_DMEM_SECTION(name)) \
",\"aw\"," Z_PROGBITS_SYM "progbits\n\t" \
".global " STRINGIFY(name) "_placeholder\n\t" \
STRINGIFY(name) "_placeholder:\n\t" \
".popsection\n\t")

/**
* @brief Define an application memory partition with linker support
*
Expand Down Expand Up @@ -94,8 +125,7 @@ struct z_app_region {
.bss_start = &Z_APP_BSS_START(name), \
.bss_size = (size_t) &Z_APP_BSS_SIZE(name) \
}; \
K_APP_BMEM(name) char name##_placeholder;

Z_APPMEM_PLACEHOLDER(name);
#else

#define K_APP_BMEM(ptn)
Expand Down
11 changes: 11 additions & 0 deletions include/misc/libc-hooks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,23 @@ __syscall size_t _zephyr_fwrite(const void *_MLIBC_RESTRICT ptr, size_t size,
#endif /* CONFIG_NEWLIB_LIBC */

#ifdef CONFIG_USERSPACE
#if defined(CONFIG_NEWLIB_LIBC) || (CONFIG_MINIMAL_LIBC_MALLOC_ARENA_SIZE > 0)
#define Z_MALLOC_PARTITION_EXISTS 1

/* Memory partition containing the libc malloc arena */
extern struct k_mem_partition z_malloc_partition;
#endif

#if defined(CONFIG_NEWLIB_LIBC) || defined(CONFIG_STACK_CANARIES)
/* Minimal libc has no globals. We do put the stack canary global in the
* libc partition since it is not worth placing in a partition of its own.
*/
#define Z_LIBC_PARTITION_EXISTS 1

/* C library globals, except the malloc arena */
extern struct k_mem_partition z_libc_partition;
#endif
#endif /* CONFIG_USERSPACE */

#include <syscalls/libc-hooks.h>

Expand Down
9 changes: 6 additions & 3 deletions kernel/mem_domain.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ void k_mem_domain_init(struct k_mem_domain *domain, u8_t num_parts,
for (i = 0U; i < num_parts; i++) {
__ASSERT(parts[i] != NULL, "");
__ASSERT((parts[i]->start + parts[i]->size) >
parts[i]->start, "");
parts[i]->start,
"invalid partition %p size %d",
parts[i], parts[i]->size);

Copy link
Member

Choose a reason for hiding this comment

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

Questions, would this ASSERT never happens because the size would be at least 1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that someone might not want to use the APPMEM partition macros, and just use the k_mem_partition and k_mem_domain directly? And, then, initialize a memory domain in that way? Then we should ASSERT that the size is not zero. Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ioannisg I'm going to get rid of these changes to mem_domain.c in the next version of this patch

The goal of the next version of this patch will be to print a comprehensible error message if a 0-size partition is defined, instead of the mess of linker errors we get if the placeholder is removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and this is done now.

#if defined(CONFIG_EXECUTE_XOR_WRITE) || \
defined(CONFIG_MPU_REQUIRES_NON_OVERLAPPING_REGIONS)
Expand Down Expand Up @@ -153,7 +155,8 @@ void k_mem_domain_add_partition(struct k_mem_domain *domain,

__ASSERT(domain != NULL, "");
__ASSERT(part != NULL, "");
__ASSERT((part->start + part->size) > part->start, "");
__ASSERT((part->start + part->size) > part->start,
"invalid partition %p size %d", part, part->size);

#if defined(CONFIG_EXECUTE_XOR_WRITE) || \
defined(CONFIG_MPU_REQUIRES_NON_OVERLAPPING_REGIONS)
Expand Down Expand Up @@ -201,7 +204,7 @@ void k_mem_domain_remove_partition(struct k_mem_domain *domain,
}

/* Assert if not found */
__ASSERT(p_idx < max_partitions, "");
__ASSERT(p_idx < max_partitions, "no matching partition found");

/* Handle architecture-specific remove
* only if it is the current thread.
Expand Down
3 changes: 3 additions & 0 deletions kernel/userspace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@
#include <init.h>
#include <stdbool.h>
#include <app_memory/app_memdomain.h>
#include <misc/libc-hooks.h>

#ifdef Z_LIBC_PARTITION_EXISTS
K_APPMEM_PARTITION_DEFINE(z_libc_partition);
#endif

#define LOG_LEVEL CONFIG_KERNEL_LOG_LEVEL
#include <logging/log.h>
Expand Down
5 changes: 1 addition & 4 deletions lib/libc/minimal/source/stdlib/malloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,9 @@
#include <logging/log.h>
LOG_MODULE_DECLARE(os);

#ifdef CONFIG_USERSPACE
K_APPMEM_PARTITION_DEFINE(z_malloc_partition);
#endif

#if (CONFIG_MINIMAL_LIBC_MALLOC_ARENA_SIZE > 0)
#ifdef CONFIG_USERSPACE
K_APPMEM_PARTITION_DEFINE(z_malloc_partition);
#define POOL_SECTION K_APP_DMEM_SECTION(z_malloc_partition)
#else
#define POOL_SECTION .data
Expand Down
8 changes: 6 additions & 2 deletions subsys/testsuite/ztest/src/ztest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,15 @@ void main(void)
{
#ifdef CONFIG_USERSPACE
struct k_mem_partition *parts[] = {
&ztest_mem_partition,
#ifdef Z_LIBC_PARTITION_EXISTS
/* C library globals, stack canary storage, etc */
&z_libc_partition,
#endif
#ifdef Z_MALLOC_PARTITION_EXISTS
/* Required for access to malloc arena */
&z_malloc_partition
&z_malloc_partition,
#endif
&ztest_mem_partition
};

/* Ztests just have one memory domain with one partition.
Expand Down
9 changes: 4 additions & 5 deletions tests/kernel/mem_protect/userspace/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ K_SEM_DEFINE(expect_fault_sem, 0, 1);

/*
* Create partitions. part0 is for all variables to run
* ztest and this test suite. part1 and part2 are for
* ztest and this test suite. part1 is for
* subsequent test specifically for this new implementation.
*/
FOR_EACH(K_APPMEM_PARTITION_DEFINE, part0, part1, part2);
FOR_EACH(K_APPMEM_PARTITION_DEFINE, part0, part1);

/*
* Create memory domains. dom0 is for the ztest and this
Expand Down Expand Up @@ -650,14 +650,13 @@ static void shared_mem_thread(void)
*/
static void access_other_memdomain(void)
{
struct k_mem_partition *parts[] = {&part0, &part2};
struct k_mem_partition *parts[] = {&part0};
/*
* Following tests the ability for a thread to access data
* in a domain that it is denied.
*/

/* initialize domain dom1 with partition part2 */
k_mem_domain_init(&dom1, 2, parts);
k_mem_domain_init(&dom1, ARRAY_SIZE(parts), parts);

/* remove current thread from domain dom0 and add to dom1 */
k_mem_domain_remove_thread(k_current_get());
Expand Down