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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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