Re: [Xen-devel] [PATCH v5 03/28] arm/x86: Use struct virtual_region to do bug, symbol, and (x86) exception tables lookup.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> @@ -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_text(pc);

find_text_region() maybe? And "virtual_region" then perhaps could
also become "text_region"?

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

I can't spot what code below needs this.

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

And I continue to be unhappy about these #ifdef-s, the more that
I think that ARM is more the exception than the norm in not needing
these tables. Couldn't the arch pass the two pointers to
setup_virtual_regions() instead?

> +struct virtual_region* search_for_text(unsigned long addr)

I think I had said before that this should return a pointer to const.
And the * also is still misplaced.

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

Inconsistent style: Either you need another blank ahead of the
opening paren, or you don't need any immediately inside.

> +int register_virtual_region(struct virtual_region *r)
> +{
> +    ASSERT(!local_irq_is_enabled());
> +
> +    list_add_tail_rcu(&r->list, &virtual_region_list);
> +
> +    return 0;
> +}

Is it really useful for this function to return non-void?

> --- /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__

Still inconsistent with the header's name.

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

Still wrongly a multi 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];
> +
> +    struct exception_table_entry *ex;
> +    struct exception_table_entry *ex_end;

So you nicely constified "bugs", but what about these two?

Jan


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