diff mbox

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

Message ID 20160324024942.GB13266@localhost.localdomain (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk March 24, 2016, 2:49 a.m. UTC
> >> > +#ifdef CONFIG_X86
> >> > +#include <asm/uaccess.h>
> >> > +#endif
> >> 
> >> Why?
> > 
> > Otherwise the compilation will fail on ARM as they do not have exceptions
> > (and no asm/uaccess.h file)
> 
> Well, the question was for the #include, not the #ifdef.

Ah, yes. And with the 'ex' being pointers it matters no.

> 
> > --- a/xen/common/symbols.c
> > +++ b/xen/common/symbols.c
> > @@ -17,6 +17,7 @@
> >  #include <xen/lib.h>
> >  #include <xen/string.h>
> >  #include <xen/spinlock.h>
> > +#include <xen/virtual_region.h>
> >  #include <public/platform.h>
> >  #include <xen/guest_access.h>
> >  
> > @@ -97,8 +98,7 @@ static unsigned int get_symbol_offset(unsigned long pos)
> >  
> >  bool_t is_active_kernel_text(unsigned long addr)
> >  {
> > -    return (is_kernel_text(addr) ||
> > -            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));
> > +    return !!search_virtual_regions(addr);
> 
> search_virtual_regions() doesn't sound like it would be looking for
> text addresses only.

I am not sure what would be a better name - as it
(search_virtual_regions) is used by three other callers?

search_for_addr? 

.. snip..
> > +static void __unregister_virtual_region(struct virtual_region *r)
> 
> > +{
> > +    unsigned long flags;
> > +
> > +    spin_lock_irqsave(&virtual_region_lock, flags);
> > +    list_del_rcu(&r->list);
> > +    spin_unlock_irqrestore(&virtual_region_lock, flags);
> > +    /*
> > +     * We do not need to invoke call_rcu.
> > +     *
> > +     * This is due to the fact that on the deletion we have made sure
> > +     * to use spinlocks (to guard against somebody else calling
> > +     * unregister_virtual_region) and list_deletion spiced with an memory
> > +     * barrier - which will flush out the cache lines in other CPUs.
> 
> I don't think barriers do any kind of cache flushing on remote CPUs
> (not even on the local one).

I am not sure what I had been thinking. The only thing it does is a
memory barrier.

.. snip..

I believe I've addressed the review comments you had:
From 4a2690ba815db7edd5fe075c8d7e9e2ac62a0020 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 10 Mar 2016 16:35:50 -0500
Subject: [PATCH] arm/x86: Use struct virtual_region to do bug, symbol, and
 (x86) exception tables lookup.

During execution of the hypervisor we have two regions of
executable code - stext -> _etext, and _sinittext -> _einitext.

The later is not needed after bootup.

We also have various built-in macros and functions to search
in between those two swaths depending on the state of the system.

That is either for bug_frames, exceptions (x86) or symbol
names for the instruction.

With xSplice in the picture - we need a mechansim for new payloads
to searched as well for all of this.

Originally we had extra 'if (xsplice)...' but that gets
a bit tiring and does not hook up nicely.

This 'struct virtual_region' and virtual_region_list provide a
mechanism to search for the bug_frames, exception table,
and symbol names entries without having various calls in
other sub-components in the system.

Code which wishes to participate in bug_frames and exception table
entries search has to only use two public APIs:
 - register_virtual_region
 - unregister_virtual_region

to let the core code know.

If the ->lookup_symbol is not then the default internal symbol lookup
mechanism is used.

Suggested-by: Andrew Cooper <andrew.cooper3@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>

v4: New patch.
v5:
 - Rename to virtual_region.
 - Ditch the 'skip' function.
 - Remove the _stext.
 - Use RCU lists.
 - Add a search function.
 - Remove extern, add rcu_read_lock. remove __ from name.
---
---
 xen/arch/arm/setup.c             |   4 +
 xen/arch/arm/traps.c             |  39 ++++++----
 xen/arch/x86/extable.c           |  12 ++-
 xen/arch/x86/setup.c             |   6 ++
 xen/arch/x86/traps.c             |  40 ++++++----
 xen/common/Makefile              |   1 +
 xen/common/symbols.c             |  11 ++-
 xen/common/virtual_region.c      | 160 +++++++++++++++++++++++++++++++++++++++
 xen/include/xen/symbols.h        |   9 +++
 xen/include/xen/virtual_region.h |  48 ++++++++++++
 10 files changed, 293 insertions(+), 37 deletions(-)
 create mode 100644 xen/common/virtual_region.c
 create mode 100644 xen/include/xen/virtual_region.h

Comments

Jan Beulich March 24, 2016, 9:20 a.m. UTC | #1
>>> On 24.03.16 at 03:49, <konrad.wilk@oracle.com> wrote:
>> > --- a/xen/common/symbols.c
>> > +++ b/xen/common/symbols.c
>> > @@ -17,6 +17,7 @@
>> >  #include <xen/lib.h>
>> >  #include <xen/string.h>
>> >  #include <xen/spinlock.h>
>> > +#include <xen/virtual_region.h>
>> >  #include <public/platform.h>
>> >  #include <xen/guest_access.h>
>> >  
>> > @@ -97,8 +98,7 @@ static unsigned int get_symbol_offset(unsigned long pos)
>> >  
>> >  bool_t is_active_kernel_text(unsigned long addr)
>> >  {
>> > -    return (is_kernel_text(addr) ||
>> > -            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));
>> > +    return !!search_virtual_regions(addr);
>> 
>> search_virtual_regions() doesn't sound like it would be looking for
>> text addresses only.
> 
> I am not sure what would be a better name - as it
> (search_virtual_regions) is used by three other callers?
> 
> search_for_addr? 

How would that make clear that you're after .text addresses only?
Part of the problem of course is that only .text regions get registered,
yet the function names all don't reflect this. Perhaps generally
s/virtual/text/ ?

Jan
diff mbox

Patch

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6d205a9..09ff1ea 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -34,6 +34,7 @@ 
 #include <xen/keyhandler.h>
 #include <xen/cpu.h>
 #include <xen/pfn.h>
+#include <xen/virtual_region.h>
 #include <xen/vmap.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/acpi.h>
@@ -860,6 +861,9 @@  void __init start_xen(unsigned long boot_phys_offset,
 
     system_state = SYS_STATE_active;
 
+    /* Must be done past setting system_state. */
+    unregister_init_virtual_region();
+
     domain_unpause_by_systemcontroller(dom0);
 
     /* Switch on to the dynamically allocated stack for the idle vcpu
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 31d2115..69ccb47 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -31,6 +31,7 @@ 
 #include <xen/softirq.h>
 #include <xen/domain_page.h>
 #include <xen/perfc.h>
+#include <xen/virtual_region.h>
 #include <public/sched.h>
 #include <public/xen.h>
 #include <asm/debugger.h>
@@ -101,6 +102,8 @@  integer_param("debug_stack_lines", debug_stack_lines);
 
 void init_traps(void)
 {
+    setup_virtual_regions();
+
     /* Setup Hyp vector base */
     WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2);
 
@@ -1077,27 +1080,33 @@  void do_unexpected_trap(const char *msg, struct cpu_user_regs *regs)
 
 int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
 {
-    const struct bug_frame *bug;
+    const struct bug_frame *bug = NULL;
     const char *prefix = "", *filename, *predicate;
     unsigned long fixup;
-    int id, lineno;
-    static const struct bug_frame *const stop_frames[] = {
-        __stop_bug_frames_0,
-        __stop_bug_frames_1,
-        __stop_bug_frames_2,
-        NULL
-    };
+    int id = -1, lineno;
+    struct virtual_region *region;
 
-    for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug )
+    region = search_for_addr(pc);
+    if ( region )
     {
-        while ( unlikely(bug == stop_frames[id]) )
-            ++id;
+        for ( id = 0; id < BUGFRAME_NR; id++ )
+        {
+            const struct bug_frame *b;
+            unsigned int i;
 
-        if ( ((vaddr_t)bug_loc(bug)) == pc )
-            break;
+            for ( i = 0, b = region->frame[id].bugs;
+                  i < region->frame[id].n_bugs; b++, i++ )
+            {
+                if ( ((vaddr_t)bug_loc(b)) == pc )
+                {
+                    bug = b;
+                    goto found;
+                }
+            }
+        }
     }
-
-    if ( !stop_frames[id] )
+ found:
+    if ( !bug )
         return -ENOENT;
 
     /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
index 89b5bcb..14cd533 100644
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -1,10 +1,12 @@ 
 
-#include <xen/config.h>
 #include <xen/init.h>
+#include <xen/list.h>
 #include <xen/perfc.h>
+#include <xen/rcupdate.h>
 #include <xen/sort.h>
 #include <xen/spinlock.h>
 #include <asm/uaccess.h>
+#include <xen/virtual_region.h>
 
 #define EX_FIELD(ptr, field) ((unsigned long)&(ptr)->field + (ptr)->field)
 
@@ -80,8 +82,12 @@  search_one_table(const struct exception_table_entry *first,
 unsigned long
 search_exception_table(unsigned long addr)
 {
-    return search_one_table(
-        __start___ex_table, __stop___ex_table-1, addr);
+    struct virtual_region *region = search_for_addr(addr);
+
+    if ( region && region->ex )
+        return search_one_table(region->ex, region->ex_end-1, addr);
+
+    return 0;
 }
 
 unsigned long
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ee65f55..c8a5adb 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -26,6 +26,7 @@ 
 #include <xen/pfn.h>
 #include <xen/nodemask.h>
 #include <xen/tmem_xen.h>
+#include <xen/virtual_region.h>
 #include <xen/watchdog.h>
 #include <public/version.h>
 #include <compat/platform.h>
@@ -514,6 +515,9 @@  static void noinline init_done(void)
 
     system_state = SYS_STATE_active;
 
+    /* MUST be done prior to removing .init data. */
+    unregister_init_virtual_region();
+
     domain_unpause_by_systemcontroller(hardware_domain);
 
     /* Zero the .init code and data. */
@@ -616,6 +620,8 @@  void __init noreturn __start_xen(unsigned long mbi_p)
     smp_prepare_boot_cpu();
     sort_exception_tables();
 
+    setup_virtual_regions();
+
     /* Full exception support from here on in. */
 
     loader = (mbi->flags & MBI_LOADERNAME)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6fbb1cf..3708309 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -48,6 +48,7 @@ 
 #include <xen/kexec.h>
 #include <xen/trace.h>
 #include <xen/paging.h>
+#include <xen/virtual_region.h>
 #include <xen/watchdog.h>
 #include <asm/system.h>
 #include <asm/io.h>
@@ -1132,18 +1133,12 @@  static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
 
 void do_invalid_op(struct cpu_user_regs *regs)
 {
-    const struct bug_frame *bug;
+    const struct bug_frame *bug = NULL;
     u8 bug_insn[2];
     const char *prefix = "", *filename, *predicate, *eip = (char *)regs->eip;
     unsigned long fixup;
-    int id, lineno;
-    static const struct bug_frame *const stop_frames[] = {
-        __stop_bug_frames_0,
-        __stop_bug_frames_1,
-        __stop_bug_frames_2,
-        __stop_bug_frames_3,
-        NULL
-    };
+    int id = -1, lineno;
+    struct virtual_region *region;
 
     DEBUGGER_trap_entry(TRAP_invalid_op, regs);
 
@@ -1160,16 +1155,29 @@  void do_invalid_op(struct cpu_user_regs *regs)
          memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
         goto die;
 
-    for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug )
+    region = search_for_addr(regs->eip);
+    if ( region )
     {
-        while ( unlikely(bug == stop_frames[id]) )
-            ++id;
-        if ( bug_loc(bug) == eip )
-            break;
+        for ( id = 0; id < BUGFRAME_NR; id++ )
+        {
+            const struct bug_frame *b;
+            unsigned int i;
+
+            for ( i = 0, b = region->frame[id].bugs;
+                  i < region->frame[id].n_bugs; b++, i++ )
+            {
+                if ( bug_loc(b) == eip )
+                {
+                    bug = b;
+                    goto found;
+                }
+            }
+        }
     }
-    if ( !stop_frames[id] )
-        goto die;
 
+ found:
+    if ( !bug )
+        goto die;
     eip += sizeof(bug_insn);
     if ( id == BUGFRAME_run_fn )
     {
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 77de27e..e43ec49 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -51,6 +51,7 @@  obj-y += time.o
 obj-y += timer.o
 obj-y += trace.o
 obj-y += version.o
+obj-y += virtual_region.o
 obj-y += vm_event.o
 obj-y += vmap.o
 obj-y += vsprintf.o
diff --git a/xen/common/symbols.c b/xen/common/symbols.c
index a59c59d..23b142d 100644
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -17,6 +17,7 @@ 
 #include <xen/lib.h>
 #include <xen/string.h>
 #include <xen/spinlock.h>
+#include <xen/virtual_region.h>
 #include <public/platform.h>
 #include <xen/guest_access.h>
 
@@ -97,8 +98,7 @@  static unsigned int get_symbol_offset(unsigned long pos)
 
 bool_t is_active_kernel_text(unsigned long addr)
 {
-    return (is_kernel_text(addr) ||
-            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));
+    return !!search_for_addr(addr);
 }
 
 const char *symbols_lookup(unsigned long addr,
@@ -108,13 +108,18 @@  const char *symbols_lookup(unsigned long addr,
 {
     unsigned long i, low, high, mid;
     unsigned long symbol_end = 0;
+    struct virtual_region *region;
 
     namebuf[KSYM_NAME_LEN] = 0;
     namebuf[0] = 0;
 
-    if (!is_active_kernel_text(addr))
+    region = search_for_addr(addr);
+    if (!region)
         return NULL;
 
+    if (region->symbols_lookup)
+        return region->symbols_lookup(addr, symbolsize, offset, namebuf);
+
         /* do a binary search on the sorted symbols_addresses array */
     low = 0;
     high = symbols_num_syms;
diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
new file mode 100644
index 0000000..2ff2d7b
--- /dev/null
+++ b/xen/common/virtual_region.c
@@ -0,0 +1,160 @@ 
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#include <xen/init.h>
+#include <xen/kernel.h>
+#include <xen/rcupdate.h>
+#include <xen/spinlock.h>
+#include <xen/virtual_region.h>
+
+#ifdef CONFIG_X86
+#include <asm/uaccess.h>
+#endif
+
+static struct virtual_region core = {
+    .list = LIST_HEAD_INIT(core.list),
+    .start = (unsigned long)_stext,
+    .end = (unsigned long)_etext,
+#ifdef CONFIG_X86
+    .ex = (struct exception_table_entry *)__start___ex_table,
+    .ex_end = (struct exception_table_entry *)__stop___ex_table,
+#endif
+};
+
+/* Becomes irrelevant when __init sections are cleared. */
+static struct virtual_region core_init __initdata = {
+    .list = LIST_HEAD_INIT(core_init.list),
+    .start = (unsigned long)_sinittext,
+    .end = (unsigned long)_einittext,
+#ifdef CONFIG_X86
+    /* Even if they are __init their exception entry still gets stuck here. */
+    .ex = (struct exception_table_entry *)__start___ex_table,
+    .ex_end = (struct exception_table_entry *)__stop___ex_table,
+#endif
+};
+
+/*
+ * RCU locking. Additions are done either at startup (when there is only
+ * one CPU) or when all CPUs are running without IRQs.
+ *
+ * Deletions are big tricky. We do it when xSplicing (all CPUs running
+ * without IRQs) or during bootup (when clearing the init).
+ *
+ * Hence we use list_del_rcu (which sports an memory fence) and a spinlock
+ * on deletion.
+ *
+ * All readers of virtual_region_list MUST use list list_for_each_entry_rcu.
+ *
+ */
+static LIST_HEAD(virtual_region_list);
+static DEFINE_SPINLOCK(virtual_region_lock);
+static DEFINE_RCU_READ_LOCK(rcu_virtual_region_lock);
+
+struct virtual_region* search_for_addr(unsigned long addr)
+{
+    struct virtual_region *region;
+
+    rcu_read_lock(&rcu_virtual_region_lock);
+
+    list_for_each_entry_rcu( region, &virtual_region_list, list )
+    {
+        if ( addr >= region->start && addr < region->end )
+        {
+            rcu_read_unlock(&rcu_virtual_region_lock);
+            return region;
+        }
+    }
+
+    rcu_read_unlock(&rcu_virtual_region_lock);
+    return NULL;
+}
+
+int register_virtual_region(struct virtual_region *r)
+{
+    ASSERT(!local_irq_is_enabled());
+
+    list_add_tail_rcu(&r->list, &virtual_region_list);
+
+    return 0;
+}
+
+static void remove_virtual_region(struct virtual_region *r)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&virtual_region_lock, flags);
+    list_del_rcu(&r->list);
+    spin_unlock_irqrestore(&virtual_region_lock, flags);
+    /*
+     * We do not need to invoke call_rcu.
+     *
+     * This is due to the fact that on the deletion we have made sure
+     * to use spinlocks (to guard against somebody else calling
+     * unregister_virtual_region) and list_deletion spiced with
+     * memory barrier.
+     *
+     * That protects us from corrupting the list as the readers all
+     * use list_for_each_entry_rcu which is safe against concurrent
+     * deletions.
+     */
+}
+
+void unregister_virtual_region(struct virtual_region *r)
+{
+    /* Expected to be called from xSplice - which has IRQs disabled. */
+    ASSERT(!local_irq_is_enabled());
+
+    remove_virtual_region(r);
+}
+
+void unregister_init_virtual_region(void)
+{
+    BUG_ON(system_state != SYS_STATE_active);
+
+    remove_virtual_region(&core_init);
+}
+
+void __init setup_virtual_regions(void)
+{
+    ssize_t sz;
+    unsigned int i;
+    static const struct bug_frame *const __initconstrel bug_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
+    };
+
+    for ( i = 1; bug_frames[i]; i++ )
+    {
+        const struct bug_frame *s;
+
+        s = bug_frames[i - 1];
+        sz = bug_frames[i] - s;
+
+        core.frame[i - 1].n_bugs = sz;
+        core.frame[i - 1].bugs = s;
+
+        core_init.frame[i - 1].n_bugs = sz;
+        core_init.frame[i - 1].bugs = s;
+    }
+
+    register_virtual_region(&core_init);
+    register_virtual_region(&core);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h
index 1fa0537..f58e611 100644
--- a/xen/include/xen/symbols.h
+++ b/xen/include/xen/symbols.h
@@ -5,6 +5,15 @@ 
 
 #define KSYM_NAME_LEN 127
 
+/*
+ * Typedef for the callback functions that symbols_lookup
+ * can call if virtual_region_list has an callback for it.
+ */
+typedef const char *symbols_lookup_t(unsigned long addr,
+                                     unsigned long *symbolsize,
+                                     unsigned long *offset,
+                                     char *namebuf);
+
 /* Lookup an address. */
 const char *symbols_lookup(unsigned long addr,
                            unsigned long *symbolsize,
diff --git a/xen/include/xen/virtual_region.h b/xen/include/xen/virtual_region.h
new file mode 100644
index 0000000..04640cc
--- /dev/null
+++ b/xen/include/xen/virtual_region.h
@@ -0,0 +1,48 @@ 
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_VIRTUAL_REGION_LIST__
+#define __XEN_VIRTUAL_REGION_LIST__
+
+#include <xen/list.h>
+#include <xen/symbols.h>
+
+struct virtual_region
+{
+    struct list_head list;
+    unsigned long start;        /* Virtual address start. */
+    unsigned long end;          /* Virtual address start. */
+
+    /*
+     * If this is NULL the default lookup mechanism is used.
+     */
+    symbols_lookup_t *symbols_lookup;
+
+    struct {
+        const struct bug_frame *bugs; /* The pointer to array of bug frames. */
+        ssize_t n_bugs;         /* The number of them. */
+    } frame[BUGFRAME_NR];
+
+    struct exception_table_entry *ex;
+    struct exception_table_entry *ex_end;
+};
+
+struct virtual_region *search_for_addr(unsigned long addr);
+void setup_virtual_regions(void);
+void unregister_init_virtual_region(void);
+int register_virtual_region(struct virtual_region *r);
+void unregister_virtual_region(struct virtual_region *r);
+
+#endif /* __XEN_VIRTUAL_REGION_LIST__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */