From patchwork Thu May 7 07:09:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeremy Kerr X-Patchwork-Id: 469298 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 29A091402A5 for ; Thu, 7 May 2015 17:09:45 +1000 (AEST) Received: from ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id F06431A0AC3 for ; Thu, 7 May 2015 17:09:44 +1000 (AEST) X-Original-To: skiboot@lists.ozlabs.org Delivered-To: skiboot@lists.ozlabs.org Received: from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 57CB11A0276 for ; Thu, 7 May 2015 17:09:42 +1000 (AEST) Received: by ozlabs.org (Postfix, from userid 1023) id 445731402A5; Thu, 7 May 2015 17:09:42 +1000 (AEST) MIME-Version: 1.0 Message-Id: <1430982580.86528.316644000972.1.gpush@pablo> In-Reply-To: <1430982158.53861.596589456916.1.gpush@pablo> To: skiboot@lists.ozlabs.org From: Jeremy Kerr Date: Thu, 07 May 2015 15:09:40 +0800 Subject: [Skiboot] [PATCH v2] core: Prevent adding new regions after mem_region_add_dt_reserved X-BeenThere: skiboot@lists.ozlabs.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Mailing list for skiboot development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Skiboot" If we reserve any memory after mem_region_add_dt_reserved, that reservation won't appear in the device tree. Ensure that we can't add new regions after this point. This allows us to relax the locking in mem_region_add_dt_reserved, as we only need to protect against updates to the region list (which can't happen after the finalise). With the relaxed locking, we can use malloc & free too. Also, add a testcase for the finalise, including some basic reserved-ranges property checks. Signed-off-by: Jeremy Kerr --- v2: fix printf format warning --- core/mem_region.c | 26 ++- core/test/Makefile.check | 2 core/test/run-mem_region_reservations.c | 181 ++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 11 deletions(-) diff --git a/core/mem_region.c b/core/mem_region.c index 5a496aa..9c37b97 100644 --- a/core/mem_region.c +++ b/core/mem_region.c @@ -34,6 +34,8 @@ struct lock mem_region_lock = LOCK_UNLOCKED; static struct list_head regions = LIST_HEAD_INIT(regions); +static bool mem_regions_finalised = false; + unsigned long top_of_ram = SKIBOOT_BASE + SKIBOOT_SIZE; static struct mem_region skiboot_os_reserve = { @@ -618,6 +620,12 @@ static bool add_region(struct mem_region *region) { struct mem_region *r; + if (mem_regions_finalised) { + prerror("MEM: add_region(%s@0x%llx) called after finalise!\n", + region->name, region->start); + return false; + } + /* First split any regions which intersect. */ list_for_each(®ions, r, list) if (!maybe_split(r, region->start) || @@ -859,6 +867,7 @@ void mem_region_release_unused(void) struct mem_region *r; lock(&mem_region_lock); + assert(!mem_regions_finalised); printf("Releasing unused memory:\n"); list_for_each(®ions, r, list) { @@ -912,7 +921,12 @@ void mem_region_add_dt_reserved(void) names_len = 0; ranges_len = 0; + /* Finalise the region list, so we know that the regions list won't be + * altered after this point. The regions' free lists may change after + * we drop the lock, but we don't access those. */ lock(&mem_region_lock); + mem_regions_finalised = true; + unlock(&mem_region_lock); /* First pass: calculate length of property data */ list_for_each(®ions, region, list) { @@ -922,15 +936,8 @@ void mem_region_add_dt_reserved(void) ranges_len += 2 * sizeof(uint64_t); } - /* Allocate property data with mem_alloc; malloc() acquires - * mem_region_lock */ - names = mem_alloc(&skiboot_heap, names_len, - __alignof__(*names), __location__); - ranges = mem_alloc(&skiboot_heap, ranges_len, - __alignof__(*ranges), __location__); - - name = names; - range = ranges; + name = names = malloc(names_len); + range = ranges = malloc(ranges_len); printf("Reserved regions:\n"); /* Second pass: populate property data */ @@ -950,7 +957,6 @@ void mem_region_add_dt_reserved(void) range[1] = cpu_to_fdt64(region->len); range += 2; } - unlock(&mem_region_lock); dt_add_property(dt_root, "reserved-names", names, names_len); dt_add_property(dt_root, "reserved-ranges", ranges, ranges_len); diff --git a/core/test/Makefile.check b/core/test/Makefile.check index 457b61c..3044ecc 100644 --- a/core/test/Makefile.check +++ b/core/test/Makefile.check @@ -1,5 +1,5 @@ # -*-Makefile-*- -CORE_TEST := core/test/run-device core/test/run-mem_region core/test/run-malloc core/test/run-malloc-speed core/test/run-mem_region_init core/test/run-mem_region_release_unused core/test/run-mem_region_release_unused_noalloc core/test/run-trace core/test/run-msg core/test/run-pel core/test/run-pool core/test/run-timer +CORE_TEST := core/test/run-device core/test/run-mem_region core/test/run-malloc core/test/run-malloc-speed core/test/run-mem_region_init core/test/run-mem_region_release_unused core/test/run-mem_region_release_unused_noalloc core/test/run-mem_region_reservations core/test/run-trace core/test/run-msg core/test/run-pel core/test/run-pool core/test/run-timer CORE_TEST_NOSTUB := core/test/run-console-log CORE_TEST_NOSTUB += core/test/run-console-log-buf-overrun diff --git a/core/test/run-mem_region_reservations.c b/core/test/run-mem_region_reservations.c new file mode 100644 index 0000000..b33827f --- /dev/null +++ b/core/test/run-mem_region_reservations.c @@ -0,0 +1,181 @@ +/* Copyright 2013-2015 IBM Corp. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#define BITS_PER_LONG (sizeof(long) * 8) +/* Don't include this, it's PPC-specific */ +#define __CPU_H +static unsigned int cpu_max_pir = 1; +struct cpu_thread { + unsigned int chip_id; +}; + +#include + +static void *__malloc(size_t size, const char *location __attribute__((unused))) +{ + return malloc(size); +} + +static void *__realloc(void *ptr, size_t size, const char *location __attribute__((unused))) +{ + return realloc(ptr, size); +} + +static void *__zalloc(size_t size, const char *location __attribute__((unused))) +{ + return calloc(size, 1); +} + +static inline void __free(void *p, const char *location __attribute__((unused))) +{ + return free(p); +} + +#include + +/* We need mem_region to accept __location__ */ +#define is_rodata(p) true +#include "../mem_region.c" + +/* But we need device tree to make copies of names. */ +#undef is_rodata +#define is_rodata(p) false + +#include "../device.c" +#include +#include + +void lock(struct lock *l) +{ + l->lock_val++; +} + +void unlock(struct lock *l) +{ + l->lock_val--; +} + +#define TEST_HEAP_ORDER 12 +#define TEST_HEAP_SIZE (1ULL << TEST_HEAP_ORDER) + +static void add_mem_node(uint64_t start, uint64_t len) +{ + struct dt_node *mem; + u64 reg[2]; + char *name; + + name = (char*)malloc(sizeof("memory@") + STR_MAX_CHARS(reg[0])); + assert(name); + + /* reg contains start and length */ + reg[0] = cpu_to_be64(start); + reg[1] = cpu_to_be64(len); + + sprintf(name, "memory@%llx", (long long)start); + + mem = dt_new(dt_root, name); + dt_add_property_string(mem, "device_type", "memory"); + dt_add_property(mem, "reg", reg, sizeof(reg)); + free(name); +} + +void add_chip_dev_associativity(struct dt_node *dev __attribute__((unused))) +{ +} + +static struct { + const char *name; + uint64_t addr; + bool found; +} test_regions[] = { + { "test.1", 0x1000, false }, + { "test.2", 0x2000, false }, + { "test.3", 0x4000, false }, +}; + +int main(void) +{ + const struct dt_property *names, *ranges; + struct mem_region *r; + unsigned int i, l, c; + uint64_t *rangep; + const char *name; + void *buf; + + /* Use malloc for the heap, so valgrind can find issues. */ + skiboot_heap.start = (unsigned long)malloc(TEST_HEAP_SIZE); + skiboot_heap.len = TEST_HEAP_SIZE; + skiboot_os_reserve.len = skiboot_heap.start; + + dt_root = dt_new_root(""); + dt_add_property_cells(dt_root, "#address-cells", 2); + dt_add_property_cells(dt_root, "#size-cells", 2); + + buf = malloc(1024*1024); + add_mem_node((unsigned long)buf, 1024*1024); + + /* Now convert. */ + mem_region_init(); + + /* create our reservations */ + for (i = 0; i < ARRAY_SIZE(test_regions); i++) + mem_reserve(test_regions[i].name, test_regions[i].addr, 0x1000); + + /* release unused */ + mem_region_release_unused(); + + /* and create reservations */ + mem_region_add_dt_reserved(); + + /* ensure we can't create further reservations */ + r = new_region("test.4", 0x5000, 0x1000, NULL, REGION_RESERVED); + assert(!add_region(r)); + + /* check dt properties */ + names = dt_find_property(dt_root, "reserved-names"); + ranges = dt_find_property(dt_root, "reserved-ranges"); + + assert(names && ranges); + + /* walk through names & ranges properies, ensuring that the test + * regions are all present */ + for (name = names->prop, rangep = (uint64_t *)ranges->prop, c = 0; + name < names->prop + names->len; + name += l, rangep += 2) { + uint64_t addr; + + addr = dt_get_number(rangep, 2); + l = strlen(name) + 1; + + for (i = 0; i < ARRAY_SIZE(test_regions); i++) { + if (strcmp(test_regions[i].name, name)) + continue; + assert(test_regions[i].addr == addr); + assert(!test_regions[i].found); + test_regions[i].found = true; + c++; + } + } + + assert(c == ARRAY_SIZE(test_regions)); + + dt_free(dt_root); + free((void *)(long)skiboot_heap.start); + free(buf); + return 0; +}