| Message ID | 1458064616-23101-8-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote: > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 31d2115..b62c91f 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -16,6 +16,7 @@ > * GNU General Public License for more details. > */ > > +#include <xen/bug_ex_symbols.h> how about just <xen/virtual_region.h> ? It contains more than just bugframes. > diff --git a/xen/common/bug_ex_symbols.c b/xen/common/bug_ex_symbols.c > new file mode 100644 > index 0000000..77bb72b > --- /dev/null > +++ b/xen/common/bug_ex_symbols.c > @@ -0,0 +1,119 @@ > +/* > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > + * > + */ > + > +#include <xen/bug_ex_symbols.h> > +#include <xen/config.h> > +#include <xen/kernel.h> > +#include <xen/init.h> > +#include <xen/spinlock.h> > + > +extern char __stext[]; There is no such symbol. _stext comes in via kernel.h > + > +struct virtual_region kernel_text = { How about just "compiled" ? This is more than just .text. > + .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 > +}; > + > +/* > + * The kernel_inittext should only be used when system_state > + * is booting. Otherwise all accesses should be ignored. > + */ > +static bool_t ignore_if_active(unsigned int flag, unsigned long priv) > +{ > + return (system_state >= SYS_STATE_active); > +} > + > +/* > + * 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 can live in .init.data and be taken off the linked list in init_done(), which performs other bits of cleanup relating to .init > + > +/* > + * 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(). It should still be possible to lock this properly. We expect no contention, at which point acquiring and releasing the locks will always hit fastpaths, but it will avoid accidental corruption if something goes wrong. In each of register or deregister, take the lock, then confirm whether the current region is in a list or not, by looking at r->list. With the single virtual_region_lock held, that can safely avoid repeatedly adding the region to the region list. > + * > + */ > +LIST_HEAD(virtual_region_list); > + > +int register_virtual_region(struct virtual_region *r) > +{ > + ASSERT(!local_irq_is_enabled()); > + > + list_add_tail(&r->list, &virtual_region_list); > + return 0; > +} > + > +void unregister_virtual_region(struct virtual_region *r) > +{ > + ASSERT(!local_irq_is_enabled()); > + > + list_del_init(&r->list); > +} > + > +void __init setup_virtual_regions(void) > +{ > + ssize_t sz; > + unsigned int i, idx; > + 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 > + }; > + > +#ifdef CONFIG_X86 > + sort_exception_tables(); > +#endif Any reason why this needs moving out of setup.c ? > diff --git a/xen/include/xen/bug_ex_symbols.h b/xen/include/xen/bug_ex_symbols.h > new file mode 100644 > index 0000000..6f3401b > --- /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 > +#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); Why do we need infrastructure like this? A virtual region is either active and in use (in which case it should be on the list and fully complete), or not in use and never available to query. If it was only to deal with .init, I would recommend dropping it all. > + > + 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 > + > + unsigned long priv; /* To be used by above funcionts if need to. */ > +}; > + > +extern struct list_head virtual_region_list; > + > +extern void setup_virtual_regions(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: > + */ > diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h > index 548b64d..8cf7af7 100644 > --- a/xen/include/xen/kernel.h > +++ b/xen/include/xen/kernel.h > @@ -65,12 +65,14 @@ > 1; \ > }) > > + Spurious change. ~Andrew
On Tue, Mar 15, 2016 at 07:24:30PM +0000, Andrew Cooper wrote: > On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote: > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > > index 31d2115..b62c91f 100644 > > --- a/xen/arch/arm/traps.c > > +++ b/xen/arch/arm/traps.c > > @@ -16,6 +16,7 @@ > > * GNU General Public License for more details. > > */ > > > > +#include <xen/bug_ex_symbols.h> > > how about just <xen/virtual_region.h> ? It contains more than just > bugframes. /me nods. > > > diff --git a/xen/common/bug_ex_symbols.c b/xen/common/bug_ex_symbols.c > > new file mode 100644 > > index 0000000..77bb72b > > --- /dev/null > > +++ b/xen/common/bug_ex_symbols.c > > @@ -0,0 +1,119 @@ > > +/* > > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > > + * > > + */ > > + > > +#include <xen/bug_ex_symbols.h> > > +#include <xen/config.h> > > +#include <xen/kernel.h> > > +#include <xen/init.h> > > +#include <xen/spinlock.h> > > + > > +extern char __stext[]; > > There is no such symbol. _stext comes in via kernel.h Argh. > > > + > > +struct virtual_region kernel_text = { > > How about just "compiled" ? This is more than just .text. > > > + .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 > > +}; > > + > > +/* > > + * The kernel_inittext should only be used when system_state > > + * is booting. Otherwise all accesses should be ignored. > > + */ > > +static bool_t ignore_if_active(unsigned int flag, unsigned long priv) > > +{ > > + return (system_state >= SYS_STATE_active); > > +} > > + > > +/* > > + * 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 can live in .init.data and be taken off the linked list in > init_done(), which performs other bits of cleanup relating to .init Unfortunatly at that point of time it is SMP - so if we clean it up we need to use a spin_lock. > > > + > > +/* > > + * 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(). > > It should still be possible to lock this properly. We expect no > contention, at which point acquiring and releasing the locks will always > hit fastpaths, but it will avoid accidental corruption if something goes > wrong. > > In each of register or deregister, take the lock, then confirm whether > the current region is in a list or not, by looking at r->list. With the > single virtual_region_lock held, that can safely avoid repeatedly adding > the region to the region list. Yeah. I don't know why I was thinking we can't. Ah, I was thinking about traversing the list - and we don't want the spin_lock as this is in the do_traps or other code that really really should not take any spinlocks. But if the adding/removing is done under a spinlock then that is OK. Let me do that. > > > + * > > + */ > > +LIST_HEAD(virtual_region_list); > > + > > +int register_virtual_region(struct virtual_region *r) > > +{ > > + ASSERT(!local_irq_is_enabled()); > > + > > + list_add_tail(&r->list, &virtual_region_list); > > + return 0; > > +} > > + > > +void unregister_virtual_region(struct virtual_region *r) > > +{ > > + ASSERT(!local_irq_is_enabled()); > > + > > + list_del_init(&r->list); > > +} > > + > > +void __init setup_virtual_regions(void) > > +{ > > + ssize_t sz; > > + unsigned int i, idx; > > + 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 > > + }; > > + > > +#ifdef CONFIG_X86 > > + sort_exception_tables(); > > +#endif > > Any reason why this needs moving out of setup.c ? None at all. > > > diff --git a/xen/include/xen/bug_ex_symbols.h b/xen/include/xen/bug_ex_symbols.h > > new file mode 100644 > > index 0000000..6f3401b > > --- /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 > > +#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); > > Why do we need infrastructure like this? A virtual region is either > active and in use (in which case it should be on the list and fully > complete), or not in use and never available to query. > > If it was only to deal with .init, I would recommend dropping it all. That was the reason.
On 15/03/16 19:34, Konrad Rzeszutek Wilk wrote: > On Tue, Mar 15, 2016 at 07:24:30PM +0000, Andrew Cooper wrote: >> On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote: >>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>> index 31d2115..b62c91f 100644 >>> --- a/xen/arch/arm/traps.c >>> +++ b/xen/arch/arm/traps.c >>> @@ -16,6 +16,7 @@ >>> * GNU General Public License for more details. >>> */ >>> >>> +#include <xen/bug_ex_symbols.h> >> how about just <xen/virtual_region.h> ? It contains more than just >> bugframes. > /me nods. >>> diff --git a/xen/common/bug_ex_symbols.c b/xen/common/bug_ex_symbols.c >>> new file mode 100644 >>> index 0000000..77bb72b >>> --- /dev/null >>> +++ b/xen/common/bug_ex_symbols.c >>> @@ -0,0 +1,119 @@ >>> +/* >>> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. >>> + * >>> + */ >>> + >>> +#include <xen/bug_ex_symbols.h> >>> +#include <xen/config.h> >>> +#include <xen/kernel.h> >>> +#include <xen/init.h> >>> +#include <xen/spinlock.h> >>> + >>> +extern char __stext[]; >> There is no such symbol. _stext comes in via kernel.h > Argh. > >>> + >>> +struct virtual_region kernel_text = { >> How about just "compiled" ? This is more than just .text. >> >>> + .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 >>> +}; >>> + >>> +/* >>> + * The kernel_inittext should only be used when system_state >>> + * is booting. Otherwise all accesses should be ignored. >>> + */ >>> +static bool_t ignore_if_active(unsigned int flag, unsigned long priv) >>> +{ >>> + return (system_state >= SYS_STATE_active); >>> +} >>> + >>> +/* >>> + * 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 can live in .init.data and be taken off the linked list in >> init_done(), which performs other bits of cleanup relating to .init > Unfortunatly at that point of time it is SMP - so if we clean it up > we need to use a spin_lock. > >>> + >>> +/* >>> + * 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(). >> It should still be possible to lock this properly. We expect no >> contention, at which point acquiring and releasing the locks will always >> hit fastpaths, but it will avoid accidental corruption if something goes >> wrong. >> >> In each of register or deregister, take the lock, then confirm whether >> the current region is in a list or not, by looking at r->list. With the >> single virtual_region_lock held, that can safely avoid repeatedly adding >> the region to the region list. > Yeah. I don't know why I was thinking we can't. Ah, I was thinking about > traversing the list - and we don't want the spin_lock as this is in > the do_traps or other code that really really should not take any spinlocks. > > But if the adding/removing is done under a spinlock then that is OK. > > Let me do that. Actually, that isn't sufficient. Sorry for misleaing you. You have to exclude modifications to the list against other cpus waking it in an exception handler, which might include NMI and MCE context. Now I think about it, going lockless here is probably a bonus, as we don't want to be messing around with locks in fatal contexts. In which case, it would be better to use a single linked list and cmpxchg to insert/remove elements. It generally wants to be walked forwards, and will only have a handful of elements, so searching forwards to delete will be ok. ~Andrew
On 15/03/16 19:51, Andrew Cooper wrote: > On 15/03/16 19:34, Konrad Rzeszutek Wilk wrote: >> On Tue, Mar 15, 2016 at 07:24:30PM +0000, Andrew Cooper wrote: >>> On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote: >>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>>> index 31d2115..b62c91f 100644 >>>> --- a/xen/arch/arm/traps.c >>>> +++ b/xen/arch/arm/traps.c >>>> @@ -16,6 +16,7 @@ >>>> * GNU General Public License for more details. >>>> */ >>>> >>>> +#include <xen/bug_ex_symbols.h> >>> how about just <xen/virtual_region.h> ? It contains more than just >>> bugframes. >> /me nods. >>>> diff --git a/xen/common/bug_ex_symbols.c b/xen/common/bug_ex_symbols.c >>>> new file mode 100644 >>>> index 0000000..77bb72b >>>> --- /dev/null >>>> +++ b/xen/common/bug_ex_symbols.c >>>> @@ -0,0 +1,119 @@ >>>> +/* >>>> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. >>>> + * >>>> + */ >>>> + >>>> +#include <xen/bug_ex_symbols.h> >>>> +#include <xen/config.h> >>>> +#include <xen/kernel.h> >>>> +#include <xen/init.h> >>>> +#include <xen/spinlock.h> >>>> + >>>> +extern char __stext[]; >>> There is no such symbol. _stext comes in via kernel.h >> Argh. >> >>>> + >>>> +struct virtual_region kernel_text = { >>> How about just "compiled" ? This is more than just .text. >>> >>>> + .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 >>>> +}; >>>> + >>>> +/* >>>> + * The kernel_inittext should only be used when system_state >>>> + * is booting. Otherwise all accesses should be ignored. >>>> + */ >>>> +static bool_t ignore_if_active(unsigned int flag, unsigned long priv) >>>> +{ >>>> + return (system_state >= SYS_STATE_active); >>>> +} >>>> + >>>> +/* >>>> + * 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 can live in .init.data and be taken off the linked list in >>> init_done(), which performs other bits of cleanup relating to .init >> Unfortunatly at that point of time it is SMP - so if we clean it up >> we need to use a spin_lock. >> >>>> + >>>> +/* >>>> + * 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(). >>> It should still be possible to lock this properly. We expect no >>> contention, at which point acquiring and releasing the locks will always >>> hit fastpaths, but it will avoid accidental corruption if something goes >>> wrong. >>> >>> In each of register or deregister, take the lock, then confirm whether >>> the current region is in a list or not, by looking at r->list. With the >>> single virtual_region_lock held, that can safely avoid repeatedly adding >>> the region to the region list. >> Yeah. I don't know why I was thinking we can't. Ah, I was thinking about >> traversing the list - and we don't want the spin_lock as this is in >> the do_traps or other code that really really should not take any spinlocks. >> >> But if the adding/removing is done under a spinlock then that is OK. >> >> Let me do that. > Actually, that isn't sufficient. Sorry for misleaing you. > > You have to exclude modifications to the list against other cpus waking > it in an exception handler, which might include NMI and MCE context. > > Now I think about it, going lockless here is probably a bonus, as we > don't want to be messing around with locks in fatal contexts. In which > case, it would be better to use a single linked list and cmpxchg to > insert/remove elements. It generally wants to be walked forwards, and > will only have a handful of elements, so searching forwards to delete > will be ok. Actually, knowing that the list is only ever walked forwards by the exception handlers, and with some regular spinlocks around mutation, dudicious use of list_add_tail_rcu() and list_del_rcu() should suffice (I think), and will definitely be better than handrolling a single linked list. ~Andrew
>>> On 15.03.16 at 21:02, <andrew.cooper3@citrix.com> wrote: > On 15/03/16 19:51, Andrew Cooper wrote: >> On 15/03/16 19:34, Konrad Rzeszutek Wilk wrote: >>> On Tue, Mar 15, 2016 at 07:24:30PM +0000, Andrew Cooper wrote: >>>> On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote: >>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>>>> index 31d2115..b62c91f 100644 >>>>> --- a/xen/arch/arm/traps.c >>>>> +++ b/xen/arch/arm/traps.c >>>>> @@ -16,6 +16,7 @@ >>>>> * GNU General Public License for more details. >>>>> */ >>>>> >>>>> +#include <xen/bug_ex_symbols.h> >>>> how about just <xen/virtual_region.h> ? It contains more than just >>>> bugframes. >>> /me nods. >>>>> diff --git a/xen/common/bug_ex_symbols.c b/xen/common/bug_ex_symbols.c >>>>> new file mode 100644 >>>>> index 0000000..77bb72b >>>>> --- /dev/null >>>>> +++ b/xen/common/bug_ex_symbols.c >>>>> @@ -0,0 +1,119 @@ >>>>> +/* >>>>> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. >>>>> + * >>>>> + */ >>>>> + >>>>> +#include <xen/bug_ex_symbols.h> >>>>> +#include <xen/config.h> >>>>> +#include <xen/kernel.h> >>>>> +#include <xen/init.h> >>>>> +#include <xen/spinlock.h> >>>>> + >>>>> +extern char __stext[]; >>>> There is no such symbol. _stext comes in via kernel.h >>> Argh. >>> >>>>> + >>>>> +struct virtual_region kernel_text = { >>>> How about just "compiled" ? This is more than just .text. >>>> >>>>> + .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 >>>>> +}; >>>>> + >>>>> +/* >>>>> + * The kernel_inittext should only be used when system_state >>>>> + * is booting. Otherwise all accesses should be ignored. >>>>> + */ >>>>> +static bool_t ignore_if_active(unsigned int flag, unsigned long priv) >>>>> +{ >>>>> + return (system_state >= SYS_STATE_active); >>>>> +} >>>>> + >>>>> +/* >>>>> + * 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 can live in .init.data and be taken off the linked list in >>>> init_done(), which performs other bits of cleanup relating to .init >>> Unfortunatly at that point of time it is SMP - so if we clean it up >>> we need to use a spin_lock. >>> >>>>> + >>>>> +/* >>>>> + * 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(). >>>> It should still be possible to lock this properly. We expect no >>>> contention, at which point acquiring and releasing the locks will always >>>> hit fastpaths, but it will avoid accidental corruption if something goes >>>> wrong. >>>> >>>> In each of register or deregister, take the lock, then confirm whether >>>> the current region is in a list or not, by looking at r->list. With the >>>> single virtual_region_lock held, that can safely avoid repeatedly adding >>>> the region to the region list. >>> Yeah. I don't know why I was thinking we can't. Ah, I was thinking about >>> traversing the list - and we don't want the spin_lock as this is in >>> the do_traps or other code that really really should not take any spinlocks. >>> >>> But if the adding/removing is done under a spinlock then that is OK. >>> >>> Let me do that. >> Actually, that isn't sufficient. Sorry for misleaing you. >> >> You have to exclude modifications to the list against other cpus waking >> it in an exception handler, which might include NMI and MCE context. >> >> Now I think about it, going lockless here is probably a bonus, as we >> don't want to be messing around with locks in fatal contexts. In which >> case, it would be better to use a single linked list and cmpxchg to >> insert/remove elements. It generally wants to be walked forwards, and >> will only have a handful of elements, so searching forwards to delete >> will be ok. > > Actually, knowing that the list is only ever walked forwards by the > exception handlers, and with some regular spinlocks around mutation, > dudicious use of list_add_tail_rcu() and list_del_rcu() should suffice > (I think), and will definitely be better than handrolling a single > linked list. Good that I went to the end of this sub-thread, before replying to suggest just this. Jan
>>> On 15.03.16 at 18:56, <konrad.wilk@oracle.com> wrote: > lookup. Was this meant to be part of $subject? > @@ -1077,27 +1080,39 @@ 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 ) > + list_for_each_entry( region, &virtual_region_list, list ) > { > - while ( unlikely(bug == stop_frames[id]) ) > - ++id; > + unsigned int i; > > - if ( ((vaddr_t)bug_loc(bug)) == pc ) > - break; > - } > + if ( region->skip && region->skip(CHECKING_BUG_FRAME, region->priv) ) > + continue; > + > + if ( pc < region->start || pc > region->end ) > + continue; > > - if ( !stop_frames[id] ) > + for ( id = 0; id < BUGFRAME_NR; id++ ) > + { > + const struct bug_frame *b = NULL; Pointless initializer. > --- a/xen/arch/x86/extable.c > +++ b/xen/arch/x86/extable.c > @@ -1,6 +1,8 @@ > > +#include <xen/bug_ex_symbols.h> > #include <xen/config.h> In cases like this please take the opportunity to get rid of the explicit inclusion of xen/config.h. > --- /dev/null > +++ b/xen/common/bug_ex_symbols.c > @@ -0,0 +1,119 @@ > +/* > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > + * > + */ > + > +#include <xen/bug_ex_symbols.h> > +#include <xen/config.h> > +#include <xen/kernel.h> > +#include <xen/init.h> > +#include <xen/spinlock.h> > + > +extern char __stext[]; > + > +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? 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). > +void __init setup_virtual_regions(void) > +{ > + ssize_t sz; > + unsigned int i, idx; > + 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 > + }; > + > +#ifdef CONFIG_X86 > + sort_exception_tables(); > +#endif > + > + /* N.B. idx != i */ > + for ( idx = 0, i = 1; stop_frames[i]; i++, idx++ ) Irrespective of the comment - why two loop variables when they get incremented in lockstep. > + { > + struct bug_frame *s; > + > + s = (struct bug_frame *)stop_frames[i-1]; Bogus cast, the more that it discards constness. > @@ -95,10 +96,28 @@ static unsigned int get_symbol_offset(unsigned long pos) > return name - symbols_names; > } > > +bool_t __is_active_kernel_text(unsigned long addr, symbols_lookup_t *cb) No new (and even more so global) symbols with double underscores at their beginning please. > @@ -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. > 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). > --- /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? > +#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? Jan
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 31d2115..b62c91f 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -16,6 +16,7 @@ * GNU General Public License for more details. */ +#include <xen/bug_ex_symbols.h> #include <xen/config.h> #include <xen/stdbool.h> #include <xen/init.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,39 @@ 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 ) + list_for_each_entry( region, &virtual_region_list, list ) { - while ( unlikely(bug == stop_frames[id]) ) - ++id; + unsigned int i; - if ( ((vaddr_t)bug_loc(bug)) == pc ) - break; - } + if ( region->skip && region->skip(CHECKING_BUG_FRAME, region->priv) ) + continue; + + if ( pc < region->start || pc > region->end ) + continue; - if ( !stop_frames[id] ) + for ( id = 0; id < BUGFRAME_NR; id++ ) + { + const struct bug_frame *b = NULL; + + 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; + } + } + } + } + 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..6e083a8 100644 --- a/xen/arch/x86/extable.c +++ b/xen/arch/x86/extable.c @@ -1,6 +1,8 @@ +#include <xen/bug_ex_symbols.h> #include <xen/config.h> #include <xen/init.h> +#include <xen/list.h> #include <xen/perfc.h> #include <xen/sort.h> #include <xen/spinlock.h> @@ -80,8 +82,18 @@ 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; + + list_for_each_entry( region, &virtual_region_list, list ) + { + if ( region->skip && region->skip(CHECKING_EXCEPTION, region->priv) ) + continue; + + if ( (addr >= region->start) && (addr < region->end) ) + 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 a8bf2c9..115e6fd 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1,3 +1,4 @@ +#include <xen/bug_ex_symbols.h> #include <xen/config.h> #include <xen/init.h> #include <xen/lib.h> @@ -614,8 +615,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) load_system_tables(); 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 564a107..eeada97 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -24,6 +24,7 @@ * Gareth Hughes <gareth@valinux.com>, May 2000 */ +#include <xen/bug_ex_symbols.h> #include <xen/config.h> #include <xen/init.h> #include <xen/sched.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,35 @@ 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 ) + list_for_each_entry( region, &virtual_region_list, list ) { - while ( unlikely(bug == stop_frames[id]) ) - ++id; - if ( bug_loc(bug) == eip ) - break; + unsigned int i; + + if ( region->skip && region->skip(CHECKING_BUG_FRAME, region->priv) ) + continue; + + if ( regs->eip < region->start || regs->eip > region->end ) + continue; + + for ( id = 0; id < BUGFRAME_NR; id++ ) + { + const struct bug_frame *b = NULL; + + 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 82625a5..76d7b07 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -1,4 +1,5 @@ obj-y += bitmap.o +obj-y += bug_ex_symbols.o obj-$(CONFIG_CORE_PARKING) += core_parking.o obj-y += cpu.o obj-y += cpupool.o diff --git a/xen/common/bug_ex_symbols.c b/xen/common/bug_ex_symbols.c new file mode 100644 index 0000000..77bb72b --- /dev/null +++ b/xen/common/bug_ex_symbols.c @@ -0,0 +1,119 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include <xen/bug_ex_symbols.h> +#include <xen/config.h> +#include <xen/kernel.h> +#include <xen/init.h> +#include <xen/spinlock.h> + +extern char __stext[]; + +struct virtual_region kernel_text = { + .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 +}; + +/* + * The kernel_inittext should only be used when system_state + * is booting. Otherwise all accesses should be ignored. + */ +static bool_t ignore_if_active(unsigned int flag, unsigned long priv) +{ + return (system_state >= SYS_STATE_active); +} + +/* + * 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 +}; + +/* + * 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); + +int register_virtual_region(struct virtual_region *r) +{ + ASSERT(!local_irq_is_enabled()); + + list_add_tail(&r->list, &virtual_region_list); + return 0; +} + +void unregister_virtual_region(struct virtual_region *r) +{ + ASSERT(!local_irq_is_enabled()); + + list_del_init(&r->list); +} + +void __init setup_virtual_regions(void) +{ + ssize_t sz; + unsigned int i, idx; + 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 + }; + +#ifdef CONFIG_X86 + sort_exception_tables(); +#endif + + /* N.B. idx != i */ + for ( idx = 0, i = 1; stop_frames[i]; i++, idx++ ) + { + struct bug_frame *s; + + s = (struct bug_frame *)stop_frames[i-1]; + sz = stop_frames[i] - s; + + kernel_text.frame[idx].n_bugs = sz; + kernel_text.frame[idx].bugs = s; + + kernel_inittext.frame[idx].n_bugs = sz; + kernel_inittext.frame[idx].bugs = s; + } + + register_virtual_region(&kernel_text); + register_virtual_region(&kernel_inittext); +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/common/symbols.c b/xen/common/symbols.c index a59c59d..2cc416e 100644 --- a/xen/common/symbols.c +++ b/xen/common/symbols.c @@ -10,6 +10,7 @@ * compression (see tools/symbols.c for a more complete description) */ +#include <xen/bug_ex_symbols.h> #include <xen/config.h> #include <xen/symbols.h> #include <xen/kernel.h> @@ -95,10 +96,28 @@ static unsigned int get_symbol_offset(unsigned long pos) return name - symbols_names; } +bool_t __is_active_kernel_text(unsigned long addr, symbols_lookup_t *cb) +{ + struct virtual_region *region; + + list_for_each_entry( region, &virtual_region_list, list ) + { + if ( region->skip && region->skip(CHECKING_SYMBOL, region->priv) ) + continue; + + if ( addr >= region->start && addr < region->end ) + { + if ( cb && region->symbols_lookup ) + *cb = region->symbols_lookup; + return 1; + } + } + return 0; +} + bool_t is_active_kernel_text(unsigned long addr) { - return (is_kernel_text(addr) || - (system_state < SYS_STATE_active && is_kernel_inittext(addr))); + return __is_active_kernel_text(addr, NULL); } const char *symbols_lookup(unsigned long addr, @@ -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; 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); + /* do a binary search on the sorted symbols_addresses array */ low = 0; high = symbols_num_syms; diff --git a/xen/include/xen/bug_ex_symbols.h b/xen/include/xen/bug_ex_symbols.h new file mode 100644 index 0000000..6f3401b --- /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 +#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 + + unsigned long priv; /* To be used by above funcionts if need to. */ +}; + +extern struct list_head virtual_region_list; + +extern void setup_virtual_regions(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: + */ diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h index 548b64d..8cf7af7 100644 --- a/xen/include/xen/kernel.h +++ b/xen/include/xen/kernel.h @@ -65,12 +65,14 @@ 1; \ }) + extern char _start[], _end[], start[]; #define is_kernel(p) ({ \ char *__p = (char *)(unsigned long)(p); \ (__p >= _start) && (__p < _end); \ }) +/* For symbols_lookup usage. */ extern char _stext[], _etext[]; #define is_kernel_text(p) ({ \ char *__p = (char *)(unsigned long)(p); \ diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h index 1fa0537..fe9ed8f 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,
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. Furthermore there are also overrides via the .skip function. There are three possible flags that can be passed in - depending on what kind of search is being done. A return of 1 means skip this region. If the .skip is NULL the region will be considered. The ->lookup_symbol will only be used if ->skip returns 1 for CHECKING_SYMBOLS (and of course if it points to a function). Otherwise the default internal symbol lookup mechanism is used. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Stefano Stabellini <stefano.stabellini@citrix.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- --- xen/arch/arm/traps.c | 45 ++++++++++----- xen/arch/x86/extable.c | 16 +++++- xen/arch/x86/setup.c | 3 +- xen/arch/x86/traps.c | 46 +++++++++------ xen/common/Makefile | 1 + xen/common/bug_ex_symbols.c | 119 +++++++++++++++++++++++++++++++++++++++ xen/common/symbols.c | 29 +++++++++- xen/include/xen/bug_ex_symbols.h | 74 ++++++++++++++++++++++++ xen/include/xen/kernel.h | 2 + xen/include/xen/symbols.h | 9 +++ 10 files changed, 307 insertions(+), 37 deletions(-) create mode 100644 xen/common/bug_ex_symbols.c create mode 100644 xen/include/xen/bug_ex_symbols.h