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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.16 at 18:56, <konrad.wilk@xxxxxxxxxx> 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

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