[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 07/34] arm/x86: Use struct virtual_region to do bug, symbol, and (x86) exception tables
. snip.. > > +struct virtual_region kernel_text = { > > static > > > + .list = LIST_HEAD_INIT(kernel_text.list), > > + .start = (unsigned long)_stext, > > + .end = (unsigned long)_etext, > > +#ifdef CONFIG_X86 > > + .ex = (struct exception_table_entry *)__start___ex_table, > > + .ex_end = (struct exception_table_entry *)__stop___ex_table, > > +#endif > > Is this together with ... > > > +/* > > + * Becomes irrelevant when __init sections are cleared. > > + */ > > +struct virtual_region kernel_inittext = { > > + .list = LIST_HEAD_INIT(kernel_inittext.list), > > + .skip = ignore_if_active, > > + .start = (unsigned long)_sinittext, > > + .end = (unsigned long)_einittext, > > +#ifdef CONFIG_X86 > > + /* Even if they are __init their exception entry still gets stuck > > here. */ > > + .ex = (struct exception_table_entry *)__start___ex_table, > > + .ex_end = (struct exception_table_entry *)__stop___ex_table, > > +#endif > > ... this really a good idea? I.e. are there not going to be any > odd side effects because of that redundancy? None. If the EIP falls within _stext and _etext then its 'ex' table will be scanned. If _inittext and _einittext then this one. Both of them end up scanning the same exact exception table (which is kind silly) - but there are no side-effect. It would be good to only have __init related exceptions on the __inittext (and also ditch the __init exception tables once the boot is completed) but I am not exactly sure how to automatically make the macros resolve what sections it should go in. That is a further TODO though. > > Also note that the comment preceding this object is a single line one. > > > +/* > > + * No locking. Additions are done either at startup (when there is only > > + * one CPU) or when all CPUs are running without IRQs. > > + * > > + * Deletions are big tricky. We MUST make sure all but one CPU > > + * are running cpu_relax(). > > + * > > + */ > > +LIST_HEAD(virtual_region_list); > > I wonder whether this wouldn't better be static, with the iterator > that the various parties need getting put here as an out-of-line > function (instead of getting open coded in a couple of places). There are three users of this list: * search_exception_table - which ends up calling search_one_table if within start->end and if region->ex is defined. * do_invalid_trap (x86 and ARM) - where both scan start->end. * symbols_lookup - where we scan start->end. The last two could be unified in a: struct virtual_region search_virtual_region(unsigned long addr); And the first one can use this and then check if region->ex is set as well. [trying it out] .. snip.. > > @@ -108,13 +127,17 @@ const char *symbols_lookup(unsigned long addr, > > { > > unsigned long i, low, high, mid; > > unsigned long symbol_end = 0; > > + symbols_lookup_t symbol_lookup = NULL; > > Pointless initializer. I believe we need it. That is the contents on the stack can be garbage and the __is_active_kernel_text won't update symbol_lookup unless it finds a match. Ah, and also the compiler is unhappy: symbols.c: In function ‘symbols_lookup’: symbols.c:136:8: error: ‘symbol_lookup’ may be used uninitialized in this function [-Werror=maybe-uninitialized] if (symbol_lookup) ^ cc1: all warnings being treated as errors > > > namebuf[KSYM_NAME_LEN] = 0; > > namebuf[0] = 0; > > > > - if (!is_active_kernel_text(addr)) > > + if (!__is_active_kernel_text(addr, &symbol_lookup)) > > return NULL; > > > > + if (symbol_lookup) > > + return (symbol_lookup)(addr, symbolsize, offset, namebuf); > > Note that there are few coding style issues here (missing blanks, > superfluous parentheses). That file uses a different StyleGuide. The Linux one. Which reminds me that __is_active_kernel_text needs to adhere to different StyleGuide. > > > --- /dev/null > > +++ b/xen/include/xen/bug_ex_symbols.h > > @@ -0,0 +1,74 @@ > > +/* > > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > > + * > > + */ > > + > > +#ifndef __BUG_EX_SYMBOL_LIST__ > > +#define __BUG_EX_SYMBOL_LIST__ > > + > > +#include <xen/config.h> > > +#include <xen/list.h> > > +#include <xen/symbols.h> > > + > > +#ifdef CONFIG_X86 > > +#include <asm/uaccess.h> > > +#endif > > Why? Otherwise the compilation will fail on ARM as they do not have exceptions (and no asm/uaccess.h file) > > > +#include <asm/bug.h> > > + > > +struct virtual_region > > +{ > > + struct list_head list; > > + > > +#define CHECKING_SYMBOL (1<<1) > > +#define CHECKING_BUG_FRAME (1<<2) > > +#define CHECKING_EXCEPTION (1<<3) > > + /* > > + * Whether to skip this region for particular searches. The flag > > + * can be CHECKING_[SYMBOL|BUG_FRAMES|EXCEPTION]. > > + * > > + * If the function returns 1 this region will be skipped. > > + */ > > + bool_t (*skip)(unsigned int flag, unsigned long priv); > > + > > + unsigned long start; /* Virtual address start. */ > > + unsigned long end; /* Virtual address start. */ > > + > > + /* > > + * If ->skip returns false for CHECKING_SYMBOL we will use > > + * 'symbols_lookup' callback to retrieve the name of the > > + * addr between start and end. If this is NULL the > > + * default lookup mechanism is used (the skip value is > > + * ignored). > > + */ > > + symbols_lookup_t symbols_lookup; > > + > > + struct { > > + struct bug_frame *bugs; /* The pointer to array of bug frames. */ > > + ssize_t n_bugs; /* The number of them. */ > > + } frame[BUGFRAME_NR]; > > + > > +#ifdef CONFIG_X86 > > + struct exception_table_entry *ex; > > + struct exception_table_entry *ex_end; > > +#endif > > The bug frame and exception related data are kind of odd to be > placed in a structure with this name. Would that not better be > accessed through ... > > > + unsigned long priv; /* To be used by above funcionts if need > > to. */ > > ... this by the interested parties? This had been re-worked with Andrew's suggestions and yours. Pls see: From d542ae2b6de421c5402888519d4d11a09abe8d46 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> Date: Thu, 10 Mar 2016 16:35:50 -0500 Subject: [PATCH] arm/x86: Use struct virtual_region to do bug, symbol, and (x86) exception tables lookup. During execution of the hypervisor we have two regions of executable code - stext -> _etext, and _sinittext -> _einitext. The later is not needed after bootup. We also have various built-in macros and functions to search in between those two swaths depending on the state of the system. That is either for bug_frames, exceptions (x86) or symbol names for the instruction. With xSplice in the picture - we need a mechansim for new payloads to searched as well for all of this. Originally we had extra 'if (xsplice)...' but that gets a bit tiring and does not hook up nicely. This 'struct virtual_region' and virtual_region_list provide a mechanism to search for the bug_frames, exception table, and symbol names entries without having various calls in other sub-components in the system. Code which wishes to participate in bug_frames and exception table entries search has to only use two public APIs: - register_virtual_region - unregister_virtual_region to let the core code know. If the ->lookup_symbol is not then the default internal symbol lookup mechanism is used. Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> --- Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx> Cc: Julien Grall <julien.grall@xxxxxxx> Cc: Keir Fraser <keir@xxxxxxx> Cc: Jan Beulich <jbeulich@xxxxxxxx> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> v4: New patch. v5: - Rename to virtual_region. - Ditch the 'skip' function. - Remove the _stext. - Use RCU lists. - Add a search function. --- --- xen/arch/arm/setup.c | 4 ++ xen/arch/arm/traps.c | 39 ++++++---- xen/arch/x86/extable.c | 12 +++- xen/arch/x86/setup.c | 6 ++ xen/arch/x86/traps.c | 40 ++++++----- xen/common/Makefile | 1 + xen/common/symbols.c | 11 ++- xen/common/virtual_region.c | 151 +++++++++++++++++++++++++++++++++++++++ xen/include/xen/symbols.h | 9 +++ xen/include/xen/virtual_region.h | 56 +++++++++++++++ 10 files changed, 292 insertions(+), 37 deletions(-) create mode 100644 xen/common/virtual_region.c create mode 100644 xen/include/xen/virtual_region.h diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 6d205a9..09ff1ea 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -34,6 +34,7 @@ #include <xen/keyhandler.h> #include <xen/cpu.h> #include <xen/pfn.h> +#include <xen/virtual_region.h> #include <xen/vmap.h> #include <xen/libfdt/libfdt.h> #include <xen/acpi.h> @@ -860,6 +861,9 @@ void __init start_xen(unsigned long boot_phys_offset, system_state = SYS_STATE_active; + /* Must be done past setting system_state. */ + unregister_init_virtual_region(); + domain_unpause_by_systemcontroller(dom0); /* Switch on to the dynamically allocated stack for the idle vcpu diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 31d2115..97e40bb 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -31,6 +31,7 @@ #include <xen/softirq.h> #include <xen/domain_page.h> #include <xen/perfc.h> +#include <xen/virtual_region.h> #include <public/sched.h> #include <public/xen.h> #include <asm/debugger.h> @@ -101,6 +102,8 @@ integer_param("debug_stack_lines", debug_stack_lines); void init_traps(void) { + setup_virtual_regions(); + /* Setup Hyp vector base */ WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2); @@ -1077,27 +1080,33 @@ void do_unexpected_trap(const char *msg, struct cpu_user_regs *regs) int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc) { - const struct bug_frame *bug; + const struct bug_frame *bug = NULL; const char *prefix = "", *filename, *predicate; unsigned long fixup; - int id, lineno; - static const struct bug_frame *const stop_frames[] = { - __stop_bug_frames_0, - __stop_bug_frames_1, - __stop_bug_frames_2, - NULL - }; + int id = -1, lineno; + struct virtual_region *region; - for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug ) + region = search_virtual_regions(pc); + if ( region ) { - while ( unlikely(bug == stop_frames[id]) ) - ++id; + for ( id = 0; id < BUGFRAME_NR; id++ ) + { + const struct bug_frame *b; + unsigned int i; - if ( ((vaddr_t)bug_loc(bug)) == pc ) - break; + for ( i = 0, b = region->frame[id].bugs; + i < region->frame[id].n_bugs; b++, i++ ) + { + if ( ((vaddr_t)bug_loc(b)) == pc ) + { + bug = b; + goto found; + } + } + } } - - if ( !stop_frames[id] ) + found: + if ( !bug ) return -ENOENT; /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c index 89b5bcb..d0f1361 100644 --- a/xen/arch/x86/extable.c +++ b/xen/arch/x86/extable.c @@ -1,10 +1,12 @@ -#include <xen/config.h> #include <xen/init.h> +#include <xen/list.h> #include <xen/perfc.h> +#include <xen/rcupdate.h> #include <xen/sort.h> #include <xen/spinlock.h> #include <asm/uaccess.h> +#include <xen/virtual_region.h> #define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field + (ptr)->field) @@ -80,8 +82,12 @@ search_one_table(const struct exception_table_entry *first, unsigned long search_exception_table(unsigned long addr) { - return search_one_table( - __start___ex_table, __stop___ex_table-1, addr); + struct virtual_region *region = search_virtual_regions(addr); + + if ( region && region->ex ) + return search_one_table(region->ex, region->ex_end-1, addr); + + return 0; } unsigned long diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 1876a28..20cd9b3 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -26,6 +26,7 @@ #include <xen/pfn.h> #include <xen/nodemask.h> #include <xen/tmem_xen.h> +#include <xen/virtual_region.h> #include <xen/watchdog.h> #include <public/version.h> #include <compat/platform.h> @@ -514,6 +515,9 @@ static void noinline init_done(void) system_state = SYS_STATE_active; + /* MUST be done prior to removing .init data. */ + unregister_init_virtual_region(); + domain_unpause_by_systemcontroller(hardware_domain); /* Zero the .init code and data. */ @@ -616,6 +620,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) smp_prepare_boot_cpu(); sort_exception_tables(); + setup_virtual_regions(); + /* Full exception support from here on in. */ loader = (mbi->flags & MBI_LOADERNAME) diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 6fbb1cf..f90d6ac 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -48,6 +48,7 @@ #include <xen/kexec.h> #include <xen/trace.h> #include <xen/paging.h> +#include <xen/virtual_region.h> #include <xen/watchdog.h> #include <asm/system.h> #include <asm/io.h> @@ -1132,18 +1133,12 @@ static int emulate_forced_invalid_op(struct cpu_user_regs *regs) void do_invalid_op(struct cpu_user_regs *regs) { - const struct bug_frame *bug; + const struct bug_frame *bug = NULL; u8 bug_insn[2]; const char *prefix = "", *filename, *predicate, *eip = (char *)regs->eip; unsigned long fixup; - int id, lineno; - static const struct bug_frame *const stop_frames[] = { - __stop_bug_frames_0, - __stop_bug_frames_1, - __stop_bug_frames_2, - __stop_bug_frames_3, - NULL - }; + int id = -1, lineno; + struct virtual_region *region; DEBUGGER_trap_entry(TRAP_invalid_op, regs); @@ -1160,16 +1155,29 @@ void do_invalid_op(struct cpu_user_regs *regs) memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) ) goto die; - for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug ) + region = search_virtual_regions(regs->eip); + if ( region ) { - while ( unlikely(bug == stop_frames[id]) ) - ++id; - if ( bug_loc(bug) == eip ) - break; + for ( id = 0; id < BUGFRAME_NR; id++ ) + { + const struct bug_frame *b; + unsigned int i; + + for ( i = 0, b = region->frame[id].bugs; + i < region->frame[id].n_bugs; b++, i++ ) + { + if ( bug_loc(b) == eip ) + { + bug = b; + goto found; + } + } + } } - if ( !stop_frames[id] ) - goto die; + found: + if ( !bug ) + goto die; eip += sizeof(bug_insn); if ( id == BUGFRAME_run_fn ) { diff --git a/xen/common/Makefile b/xen/common/Makefile index 77de27e..e43ec49 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -51,6 +51,7 @@ obj-y += time.o obj-y += timer.o obj-y += trace.o obj-y += version.o +obj-y += virtual_region.o obj-y += vm_event.o obj-y += vmap.o obj-y += vsprintf.o diff --git a/xen/common/symbols.c b/xen/common/symbols.c index a59c59d..bba0f2e 100644 --- a/xen/common/symbols.c +++ b/xen/common/symbols.c @@ -17,6 +17,7 @@ #include <xen/lib.h> #include <xen/string.h> #include <xen/spinlock.h> +#include <xen/virtual_region.h> #include <public/platform.h> #include <xen/guest_access.h> @@ -97,8 +98,7 @@ static unsigned int get_symbol_offset(unsigned long pos) bool_t is_active_kernel_text(unsigned long addr) { - return (is_kernel_text(addr) || - (system_state < SYS_STATE_active && is_kernel_inittext(addr))); + return !!search_virtual_regions(addr); } const char *symbols_lookup(unsigned long addr, @@ -108,13 +108,18 @@ const char *symbols_lookup(unsigned long addr, { unsigned long i, low, high, mid; unsigned long symbol_end = 0; + struct virtual_region *region; namebuf[KSYM_NAME_LEN] = 0; namebuf[0] = 0; - if (!is_active_kernel_text(addr)) + region = search_virtual_regions(addr); + if (!region) return NULL; + if (region->symbols_lookup) + return region->symbols_lookup(addr, symbolsize, offset, namebuf); + /* do a binary search on the sorted symbols_addresses array */ low = 0; high = symbols_num_syms; diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c new file mode 100644 index 0000000..618a312 --- /dev/null +++ b/xen/common/virtual_region.c @@ -0,0 +1,151 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include <xen/config.h> +#include <xen/init.h> +#include <xen/kernel.h> +#include <xen/rcupdate.h> +#include <xen/spinlock.h> +#include <xen/virtual_region.h> + +static struct virtual_region compiled = { + .list = LIST_HEAD_INIT(compiled.list), + .start = (unsigned long)_stext, + .end = (unsigned long)_etext, +#ifdef CONFIG_X86 + .ex = (struct exception_table_entry *)__start___ex_table, + .ex_end = (struct exception_table_entry *)__stop___ex_table, +#endif +}; + +/* Becomes irrelevant when __init sections are cleared. */ +static struct virtual_region compiled_init __initdata = { + .list = LIST_HEAD_INIT(compiled_init.list), + .start = (unsigned long)_sinittext, + .end = (unsigned long)_einittext, +#ifdef CONFIG_X86 + /* Even if they are __init their exception entry still gets stuck here. */ + .ex = (struct exception_table_entry *)__start___ex_table, + .ex_end = (struct exception_table_entry *)__stop___ex_table, +#endif +}; + +/* + * RCU locking. Additions are done either at startup (when there is only + * one CPU) or when all CPUs are running without IRQs. + * + * Deletions are big tricky. We do it when xSplicing (all CPUs running + * without IRQs) or during bootup (when clearing the init). + * + * Hence we use list_del_rcu (which sports an memory fence) and a spinlock + * on deletion. + * + * All readers of virtual_region_list MUST use list list_for_each_entry_rcu. + * + */ +static LIST_HEAD(virtual_region_list); +static DEFINE_SPINLOCK(virtual_region_lock); + +struct virtual_region* search_virtual_regions(unsigned long addr) +{ + struct virtual_region *region; + + list_for_each_entry_rcu( region, &virtual_region_list, list ) + { + if ( addr >= region->start && addr < region->end ) + return region; + } + + return NULL; +} + +int register_virtual_region(struct virtual_region *r) +{ + ASSERT(!local_irq_is_enabled()); + + list_add_tail_rcu(&r->list, &virtual_region_list); + + return 0; +} + +static void __unregister_virtual_region(struct virtual_region *r) +{ + unsigned long flags; + + spin_lock_irqsave(&virtual_region_lock, flags); + list_del_rcu(&r->list); + spin_unlock_irqrestore(&virtual_region_lock, flags); + /* + * We do not need to invoke call_rcu. + * + * This is due to the fact that on the deletion we have made sure + * to use spinlocks (to guard against somebody else calling + * unregister_virtual_region) and list_deletion spiced with an memory + * barrier - which will flush out the cache lines in other CPUs. + * + * That protects us from corrupting the list as the readers all + * use list_for_each_entry_rcu which is safe against concurrent + * deletions. + */ +} + +void unregister_virtual_region(struct virtual_region *r) +{ + /* Expected to be called from xSplice - which has IRQs disabled. */ + ASSERT(!local_irq_is_enabled()); + + __unregister_virtual_region(r); +} + +void unregister_init_virtual_region(void) +{ + BUG_ON(system_state != SYS_STATE_active); + + __unregister_virtual_region(&compiled_init); +} + +void __init setup_virtual_regions(void) +{ + ssize_t sz; + unsigned int i; + static const struct bug_frame *const stop_frames[] = { + __start_bug_frames, + __stop_bug_frames_0, + __stop_bug_frames_1, + __stop_bug_frames_2, +#ifdef CONFIG_X86 + __stop_bug_frames_3, +#endif + NULL + }; + + /* N.B. idx != i */ + for ( i = 1; stop_frames[i]; i++ ) + { + const struct bug_frame *s; + + s = stop_frames[i-1]; + sz = stop_frames[i] - s; + + compiled.frame[i-1].n_bugs = sz; + compiled.frame[i-1].bugs = s; + + compiled_init.frame[i-1].n_bugs = sz; + compiled_init.frame[i-1].bugs = s; + } + + register_virtual_region(&compiled_init); + register_virtual_region(&compiled); +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h index 1fa0537..fa353f8 100644 --- a/xen/include/xen/symbols.h +++ b/xen/include/xen/symbols.h @@ -5,6 +5,15 @@ #define KSYM_NAME_LEN 127 +/* + * Typedef for the callback functions that symbols_lookup + * can call if virtual_region_list has an callback for it. + */ +typedef const char *(symbols_lookup_t)(unsigned long addr, + unsigned long *symbolsize, + unsigned long *offset, + char *namebuf); + /* Lookup an address. */ const char *symbols_lookup(unsigned long addr, unsigned long *symbolsize, diff --git a/xen/include/xen/virtual_region.h b/xen/include/xen/virtual_region.h new file mode 100644 index 0000000..bf15fe9 --- /dev/null +++ b/xen/include/xen/virtual_region.h @@ -0,0 +1,56 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#ifndef __BUG_EX_SYMBOL_LIST__ +#define __BUG_EX_SYMBOL_LIST__ + +#include <xen/config.h> +#include <xen/list.h> +#include <xen/symbols.h> + +#ifdef CONFIG_X86 +#include <asm/uaccess.h> +#endif +#include <asm/bug.h> + +struct virtual_region +{ + struct list_head list; + unsigned long start; /* Virtual address start. */ + unsigned long end; /* Virtual address start. */ + + /* + * If this is NULL the default lookup mechanism is used. + */ + symbols_lookup_t *symbols_lookup; + + struct { + const struct bug_frame *bugs; /* The pointer to array of bug frames. */ + ssize_t n_bugs; /* The number of them. */ + } frame[BUGFRAME_NR]; + +#ifdef CONFIG_X86 + struct exception_table_entry *ex; + struct exception_table_entry *ex_end; +#endif +}; + +extern struct virtual_region *search_virtual_regions(unsigned long addr); +extern void setup_virtual_regions(void); +extern void unregister_init_virtual_region(void); +extern int register_virtual_region(struct virtual_region *r); +extern void unregister_virtual_region(struct virtual_region *r); + +#endif /* __BUG_EX_SYMBOL_LIST__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ -- 2.5.0 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |