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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.03.16 at 21:18, <konrad.wilk@oracle.com> wrote:
> It would be good to only have __init related exceptions on the __inittext
> (and also ditch the __init exception tables once the boot is completed)
> but I am not exactly sure how to automatically make the macros resolve
> what sections it should go in. That is a further TODO though.

I'm pretty confident that this can't be reasonably addressed without
the compiler's help, by extending the section attribute to also allow
for templates rather than only fixed names.

>> > @@ -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.
> 
> I believe we need it. That is the contents on the stack can be garbage
> and the __is_active_kernel_text won't update symbol_lookup unless
> it finds a match. Ah, and also the compiler is unhappy:
> 
> symbols.c: In function ‘symbols_lookup’:
> symbols.c:136:8: error: ‘symbol_lookup’ may be used uninitialized in this 
> function [-Werror=maybe-uninitialized]
>      if (symbol_lookup)
>         ^
> cc1: all warnings being treated as errors

That's unfortunate, since ...

>> >      namebuf[KSYM_NAME_LEN] = 0;
>> >      namebuf[0] = 0;
>> >  
>> > -    if (!is_active_kernel_text(addr))
>> > +    if (!__is_active_kernel_text(addr, &symbol_lookup))
>> >          return NULL;

... this return ensures that ...

>> > +    if (symbol_lookup)
>> > +        return (symbol_lookup)(addr, symbolsize, offset, namebuf);

... this won't be reached with symbol_lookup uninitialized.

>> Note that there are few coding style issues here (missing blanks,
>> superfluous parentheses).
> 
> That file uses a different StyleGuide. The Linux one. Which reminds me
> that __is_active_kernel_text needs to adhere to different StyleGuide.

Linux style would not be using spaces for indentation. The file is
really a bad mixture of styles, but yes, you're right that you don't
violate that pre-existing mixture.

>> > --- /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?
> 
> 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.

> --- 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.

> --- /dev/null
> +++ b/xen/common/virtual_region.c
> @@ -0,0 +1,151 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + *
> + */
> +
> +#include <xen/config.h>

No new explicit inclusion of xen/config.h please. We're actually in
the process of getting rid of such elsewhere.

> +#include <xen/init.h>
> +#include <xen/kernel.h>
> +#include <xen/rcupdate.h>
> +#include <xen/spinlock.h>
> +#include <xen/virtual_region.h>
> +
> +static struct virtual_region compiled = {

I think "compiled" is a bad name, as patch modules are equally
compiled - "core" or "builtin" perhaps?

> +struct virtual_region* search_virtual_regions(unsigned long addr)

Misplaced *. And probably should return a pointer to const.

> +{
> +    struct virtual_region *region;
> +
> +    list_for_each_entry_rcu( region, &virtual_region_list, list )

Where's the rcu_read_lock() use of which the comment preceding
the #define of this mandates?

> +static void __unregister_virtual_region(struct virtual_region *r)

As I think I have said before - no double underscore prefixes on
new identifiers please.

> +{
> +    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).

> +void __init setup_virtual_regions(void)
> +{
> +    ssize_t sz;
> +    unsigned int i;
> +    static const struct bug_frame *const stop_frames[] = {

This (now sitting in an __init function) should then become __initconstrel.

> +        __start_bug_frames,

This element makes the array name bogus.

> +        __stop_bug_frames_0,
> +        __stop_bug_frames_1,
> +        __stop_bug_frames_2,
> +#ifdef CONFIG_X86
> +        __stop_bug_frames_3,
> +#endif
> +        NULL
> +    };
> +
> +    /* N.B. idx != i */

Stale comment?

> +    for ( i = 1; stop_frames[i]; i++ )
> +    {
> +        const struct bug_frame *s;
> +
> +        s = stop_frames[i-1];
> +        sz = stop_frames[i] - s;
> +
> +        compiled.frame[i-1].n_bugs = sz;
> +        compiled.frame[i-1].bugs = s;
> +
> +        compiled_init.frame[i-1].n_bugs = sz;
> +        compiled_init.frame[i-1].bugs = s;
> +    }

Many times "[i - 1]" please.

> +    register_virtual_region(&compiled_init);
> +    register_virtual_region(&compiled);

Is there a particular reason not to do the main region first? Probably
it's benign, but if there is a reason, I think a comment would be
warranted.

> --- 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,

Stray parentheses.

> --- /dev/null
> +++ b/xen/include/xen/virtual_region.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + *
> + */
> +
> +#ifndef __BUG_EX_SYMBOL_LIST__
> +#define __BUG_EX_SYMBOL_LIST__

No longer in line with the header name, and missing a XEN_ portion.

> +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.
> +     */

Here or elsewhere I'm sure I've made the comment before: This is
a single line comment.

> +    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];
> +
> +#ifdef CONFIG_X86
> +    struct exception_table_entry *ex;
> +    struct exception_table_entry *ex_end;
> +#endif

Would there be any harm omitting the #ifdef and leaving the
pointers be NULL on ARM?

> +extern struct virtual_region *search_virtual_regions(unsigned long addr);
> +extern void setup_virtual_regions(void);
> +extern void unregister_init_virtual_region(void);
> +extern int register_virtual_region(struct virtual_region *r);
> +extern void unregister_virtual_region(struct virtual_region *r);

While a matter of taste, I personally would prefer if "extern" was
omitted on function declarations.

Jan

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