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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel