Re: [PATCH v6 15/24] xsplice: Add support for exception tables.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
> @@ -48,19 +49,23 @@ static void __init swap_ex(void *a, void *b, int size)
>  }
>  #endif
>  
> -void __init sort_exception_tables(void)
> +void __INIT sort_exception_table(struct exception_table_entry *start,
> +                          struct exception_table_entry *stop)
>  {
> -    sort(__start___ex_table, __stop___ex_table - __start___ex_table,
> -         sizeof(struct exception_table_entry), cmp_ex, swap_ex);
> -    sort(__start___pre_ex_table,
> -         __stop___pre_ex_table - __start___pre_ex_table,
> +    sort(start, stop - start,
>           sizeof(struct exception_table_entry), cmp_ex, swap_ex);

This reminds me that Xen's heapsort implementation is buggy.

By shear luck, it does end up in the correct order, but in O(N^2) time.

I will submit a separate bugfix patch.

>  }
>  
> -static inline unsigned long
> -search_one_table(const struct exception_table_entry *first,
> -                 const struct exception_table_entry *last,
> -                 unsigned long value)
> +void __init sort_exception_tables(void)
> +{
> +    sort_exception_table(__start___ex_table, __stop___ex_table);
> +    sort_exception_table(__start___pre_ex_table, __stop___pre_ex_table);
> +}
> +
> +unsigned long
> +search_one_extable(const struct exception_table_entry *first,
> +                   const struct exception_table_entry *last,
> +                   unsigned long value)
>  {
>      const struct exception_table_entry *mid;
>      long diff;
> @@ -85,7 +90,7 @@ search_exception_table(unsigned long addr)
>      const struct virtual_region *region = find_text_region(addr);
>  
>      if ( region && region->ex )
> -        return search_one_table(region->ex, region->ex_end-1, addr);
> +        return search_one_extable(region->ex, region->ex_end-1, addr);
>  
>      return 0;
>  }
> @@ -94,7 +99,7 @@ unsigned long
>  search_pre_exception_table(struct cpu_user_regs *regs)
>  {
>      unsigned long addr = (unsigned long)regs->eip;
> -    unsigned long fixup = search_one_table(
> +    unsigned long fixup = search_one_extable(
>          __start___pre_ex_table, __stop___pre_ex_table-1, addr);
>      if ( fixup )
>      {
> diff --git a/xen/arch/x86/test/xen_hello_world_func.c b/xen/arch/x86/test/xen_hello_world_func.c
> index 1ad002a..7e239ca 100644
> --- a/xen/arch/x86/test/xen_hello_world_func.c
> +++ b/xen/arch/x86/test/xen_hello_world_func.c
> @@ -5,9 +5,20 @@
>  
>  #include <xen/types.h>
>  
> +static unsigned long *non_canonical_addr = (unsigned long *)(1UL<<48);

I would recommend a more visible hex constant, both for the unlikely
case of patching being broken and Xen actually barfing, and because this
particular non-canonical address will no longer be non-canonical when 5
level paging appears.

how about 0xdead000000000000ULL ?

> +
>  /* Our replacement function for xen_extra_version. */
>  const char *xen_hello_world(void)
>  {
> +    unsigned long tmp = 0xdeadbeef;

No need to initialise.

> +    int rc;
> +    /*
> +     * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
> +     * exceptions will be caught and processed properly.
> +     */
> +    rc = __get_user(tmp, non_canonical_addr);
> +    BUG_ON(rc != -EFAULT);
> +
>      return "Hello World";
>  }
>  
> diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> index 087cb94..31ddd5d 100644
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -525,6 +525,31 @@ static int prepare_payload(struct payload *payload,
>                                        sizeof(*region->frame[i].bugs);
>      }
>  
> +#ifdef CONFIG_X86
> +    sec = xsplice_elf_sec_by_name(elf, ".ex_table");
> +    if ( sec )
> +    {
> +        struct exception_table_entry *s, *e;
> +
> +        if ( !sec->sec->sh_size ||
> +             (sec->sec->sh_size % sizeof(*region->ex)) )
> +        {
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
> +                    elf->name, sizeof(*region->ex),
> +                    sec->sec->sh_size);
> +            return -EINVAL;
> +        }
> +
> +        s = sec->load_addr;
> +        e = sec->load_addr + sec->sec->sh_size;
> +
> +        sort_exception_table(s ,e);
> +
> +        region->ex = (const struct exception_table_entry *)s;
> +        region->ex_end = (const struct exception_table_entry *)e;

These casts should not be needed at all.

> +    }
> +#endif
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-x86/uaccess.h b/xen/include/asm-x86/uaccess.h
> index 947470d..2c839a9 100644
> --- a/xen/include/asm-x86/uaccess.h
> +++ b/xen/include/asm-x86/uaccess.h
> @@ -277,5 +277,7 @@ extern struct exception_table_entry __stop___pre_ex_table[];
>  
>  extern unsigned long search_exception_table(unsigned long);
>  extern void sort_exception_tables(void);
> +extern void sort_exception_table(struct exception_table_entry *start,
> +                                 struct exception_table_entry *stop);
>  
>  #endif /* __X86_UACCESS_H__ */
> diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
> index ca78eae..6113061 100644
> --- a/xen/include/xen/xsplice.h
> +++ b/xen/include/xen/xsplice.h
> @@ -37,6 +37,15 @@ struct xsplice_patch_func_internal {
>      } u;
>  };
>  
> +/*
> + * We use alternative and exception table code - which by default are __init
> + * only, however we need them during runtime. These macros allows us to build
> + * the image with these functions built-in. (See the #else below).
> + */
> +#define __INITCONST
> +#define __INITDATA
> +#define __INIT
> +

This isn't very nice, but is probably the best we can do for 4.7

It would be nice to have things like __maybe_init(CONFIG_XSPLICE) , but
then negations get hard (and perhaps this needs more thought).

No major issues, so with the identified things fixed, Reviewed-by:
Andrew Cooper <andrew.cooper3@citrix.com>

>  /* Convenience define for printk. */
>  #define XSPLICE "xsplice: "
>  
> @@ -96,6 +105,14 @@ void arch_xsplice_mask(void);
>  void arch_xsplice_unmask(void);
>  #else
>  
> +/*
> + * If not compiling with xSplice certain functionality should stay as
> + * __init.
> + */
> +#define __INITCONST    __initconst
> +#define __INITDATA     __initdata
> +#define __INIT         __init
> +
>  #include <xen/errno.h> /* For -EOPNOTSUPP */
>  static inline int xsplice_op(struct xen_sysctl_xsplice_op *op)
>  {


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