diff mbox

[v4,07/34] arm/x86: Use struct virtual_region to do bug, symbol, and (x86) exception tables

Message ID 1458064616-23101-8-git-send-email-konrad.wilk@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk March 15, 2016, 5:56 p.m. UTC
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

Comments

Andrew Cooper March 15, 2016, 7:24 p.m. UTC | #1
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
Konrad Rzeszutek Wilk March 15, 2016, 7:34 p.m. UTC | #2
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.
Andrew Cooper March 15, 2016, 7:51 p.m. UTC | #3
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
Andrew Cooper March 15, 2016, 8:02 p.m. UTC | #4
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
Jan Beulich March 16, 2016, 10:33 a.m. UTC | #5
>>> 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
Jan Beulich March 18, 2016, 1:07 p.m. UTC | #6
>>> 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 mbox

Patch

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,