[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 4 of 4] KEXEC: Introduce new crashtables interface



>>> On 09.03.12 at 15:42, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -82,6 +85,21 @@ paddr_t crashinfo_maxaddr_bits = 64;
>  /* Pointers to keep track of the crash heap region. */
>  static void *crash_heap_current = NULL, *crash_heap_end = NULL;
>  
> +char * StringTab = NULL;
> +size_t StringTabSize = 0;

static? Naming convention?

> +
> +static val64tab_t ValTab[] =

const? Naming convention?

> +{
> +    { XEN_VALTAB_TOP_OF_MEM, 0 },
> +    { XEN_VALTAB_CONRING_SIZE, 0 }
> +};
> +
> +static sym64tab_t SymTab[] =

const? Naming convention?

> +{
> +    { XEN_SYMTAB_CONRING, 0 }
> +};
> +
> +
>  /*
>   * Parse command lines in the format
>   *
> @@ -262,12 +280,116 @@ void kexec_crash_save_cpu(void)
>      elf_core_save_regs(&prstatus->pr_reg, xencore);
>  }
>  
> +/* Round val up to a machine word alignment */
> +size_t round_up(size_t val)
> +{
> +    return ((val + (sizeof(unsigned long)-1)) &
> +            ~((sizeof (unsigned long))-1));
> +}
> +
> +
> +extern xen_commandline_t saved_cmdline;
> +static int setup_stringtable(void)
> +{
> +    char * ptr;
> +    size_t sl = sizeof(unsigned long);
> +
> +#define STRING(x) #x
> +#define INT2STR(x) STRING(x)
> +
> +#define XEN_STR_VERSION INT2STR(XEN_VERSION) "." INT2STR(XEN_SUBVERSION) \
> +        XEN_EXTRAVERSION
> +#define XEN_STR_COMPILED_BY XEN_COMPILE_BY "@" XEN_COMPILE_HOST "." \
> +        XEN_COMPILE_DOMAIN
> +
> +    size_t size =
> +        sl + round_up(sizeof(XEN_STR_VERSION)) +
> +        sl + round_up(sizeof(XEN_CHANGESET)) +
> +        sl + round_up(sizeof(XEN_COMPILE_DATE)) +
> +        sl + round_up(sizeof(XEN_STR_COMPILED_BY)) +
> +        sl + round_up(sizeof(XEN_COMPILER)) +
> +        sl + round_up(strlen(saved_cmdline)+1);
> +
> +    StringTab = xmalloc_bytes(size);
> +    if ( ! StringTab )
> +        return -ENOMEM;
> +
> +    StringTabSize = size;
> +    memset(StringTab, 0, size);
> +
> +    ptr = StringTab;
> +
> +#define WRITE_STRTAB_ENTRY_CONST(v, s)                      \
> +    *(unsigned long*)ptr = (v); ptr += sl;                  \
> +    memcpy(ptr, s, sizeof(s)); ptr += round_up(sizeof(s))
> +
> +#define WRITE_STRTAB_ENTRY_VAR(v, s)                        \
> +    *(unsigned long*)ptr = (v); ptr += sl;                  \
> +    memcpy(ptr, s, strlen(s)); ptr += round_up(strlen(s)+1)
> +
> +    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_VERSION, XEN_STR_VERSION);
> +    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_CSET, XEN_CHANGESET);
> +    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_COMPILE_DATE, XEN_COMPILE_DATE);
> +    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_COMPILED_BY, XEN_STR_COMPILED_BY);
> +    WRITE_STRTAB_ENTRY_CONST(XEN_STRINGTAB_COMPILER, XEN_COMPILER);
> +    WRITE_STRTAB_ENTRY_VAR(XEN_STRINGTAB_CMDLINE, saved_cmdline);
> +
> +#undef WRITE_STRTAB_ENTRY_VAR
> +#undef WRITE_STRTAB_ENTRY_CONST
> +#undef XEN_STR_COMPILED_BY
> +#undef XEN_STR_VERSION
> +#undef INT2STR
> +#undef STRING
> +
> +    return 0;
> +}
> +
> +static void write_val64tab(void)
> +{
> +    int i;
> +    for ( i=0; i<ARRAY_SIZE(ValTab); ++i )
> +    {
> +        switch( ValTab[i].id )
> +        {
> +        case XEN_VALTAB_TOP_OF_MEM:
> +            ValTab[i].val = (uint64_t)top_of_mem;
> +            break;
> +        case XEN_VALTAB_CONRING_SIZE:
> +            ValTab[i].val = (uint64_t)conring_size;
> +            break;
> +        default:
> +            printk(XENLOG_WARNING "Unrecognised ValTab ID 0x%016"PRIx64"\n",
> +                   ValTab[i].id);

Sorry, but this is a BUG() or an ASSERT(), but not a warning.

> +            break;
> +        }
> +    }
> +}
> +
> +static void write_sym64tab(void)
> +{
> +    int i;
> +    for ( i=0; i<ARRAY_SIZE(SymTab); ++i )
> +    {
> +        switch( SymTab[i].id )
> +        {
> +        case XEN_SYMTAB_CONRING:
> +            SymTab[i].addr = (uint64_t)__pa(conring);
> +            break;
> +        default:
> +            printk(XENLOG_WARNING "Unrecognised SymTab ID 0x%016"PRIx64"\n",
> +                   SymTab[i].id);

Same here.

> +            break;
> +        }
> +    }
> +}
> +
>  /* Set up the single Xen-specific-info crash note. */
>  crash_xen_info_t *kexec_crash_save_info(void)
>  {
>      int cpu = smp_processor_id();
>      crash_xen_info_t info;
>      crash_xen_info_t *out = (crash_xen_info_t *)ELFNOTE_DESC(xen_crash_note);
> +    char * details;
>  
>      BUG_ON(!cpumask_test_and_set_cpu(cpu, &crash_saved_cpus));
>  
> @@ -284,6 +406,17 @@ crash_xen_info_t *kexec_crash_save_info(
>      /* Copy from guaranteed-aligned local copy to possibly-unaligned dest. */
>      memcpy(out, &info, sizeof(info));
>  
> +    details = ELFNOTE_DESC(xen_stringtab);
> +    memcpy(details, StringTab, StringTabSize);
> +
> +    details = ELFNOTE_DESC(xen_valtab);
> +    write_val64tab();
> +    memcpy(details, ValTab, sizeof(ValTab));
> +
> +    details = ELFNOTE_DESC(xen_symtab);
> +    write_sym64tab();
> +    memcpy(details, SymTab, sizeof(SymTab));
> +
>      return out;
>  }
>  
> @@ -365,7 +498,10 @@ static size_t sizeof_cpu_notes(const uns
>      /* CPU0 also presents the crash_xen_info note. */
>      if ( ! cpu )
>          bytes = bytes +
> -            sizeof_note("Xen", sizeof(crash_xen_info_t));
> +            sizeof_note("Xen", sizeof(crash_xen_info_t)) +
> +            sizeof_note("Xen", StringTabSize) +
> +            sizeof_note("Xen", sizeof(ValTab)) +
> +            sizeof_note("Xen", sizeof(SymTab));
>  
>      return bytes;
>  }
> @@ -449,6 +585,21 @@ static int kexec_init_cpu_notes(const un
>                  xen_crash_note = note = ELFNOTE_NEXT(note);
>                  setup_note(note, "Xen", XEN_ELFNOTE_CRASH_INFO,
>                             sizeof(crash_xen_info_t));
> +
> +                /* Set up Xen StringTab note. */
> +                xen_stringtab = note = ELFNOTE_NEXT(note);
> +                setup_note(note, "Xen", XEN_CRASHTABLE_STRINGTAB,
> +                           StringTabSize);
> +
> +                /* Set up Xen ValTab note. */
> +                xen_valtab = note = ELFNOTE_NEXT(note);
> +                setup_note(note, "Xen", XEN_CRASHTABLE_VAL64TAB,
> +                           sizeof(ValTab));
> +
> +                /* Set up Xen SymTab note. */
> +                xen_symtab = note = ELFNOTE_NEXT(note);
> +                setup_note(note, "Xen", XEN_CRASHTABLE_SYM64TAB,
> +                           sizeof(SymTab));
>              }
>          }
>      }
> @@ -505,6 +656,9 @@ static int __init kexec_init(void)
>  
>      register_keyhandler('C', &crashdump_trigger_keyhandler);
>  
> +    if ( setup_stringtable() )
> +        return -ENOMEM;

Given that this is an extension, I don't think failing to set this up
should be fatal in any way. Just don't write those new notes when
you can't set them up.

> +
>      if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
>      {
>          size_t crash_heap_size;
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -59,8 +59,8 @@ size_param("conring_size", opt_conring_s
>  #define _CONRING_SIZE 16384
>  #define CONRING_IDX_MASK(i) ((i)&(conring_size-1))
>  static char __initdata _conring[_CONRING_SIZE];
> -static char *__read_mostly conring = _conring;
> -static uint32_t __read_mostly conring_size = _CONRING_SIZE;
> +char *__read_mostly conring = _conring;
> +uint32_t __read_mostly conring_size = _CONRING_SIZE;

Please don't. If anything, add a call into this file to have the note
fields filled.

>  static uint32_t conringc, conringp;
>  
>  static int __read_mostly sercon_handle = -1;
> --- /dev/null
> +++ b/xen/include/public/crashtables.h
> @@ -0,0 +1,77 @@
> +/******************************************************************************
> + * crashtables.h
> + *
> + * Definitions used for the Xen ELF crash notes.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
> and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (c) 2011,2012 Andrew Cooper, Citrix Ltd.
> + */
> +
> +#ifndef __XEN_PUBLIC_CRASHTABLES_H__
> +#define __XEN_PUBLIC_CRASHTABLES_H__
> +
> +#include "xen.h"
> +
> +/*
> + * Xen string crash table.
> + * Note is a list in the form of:
> + *     Machine word (32/64bit) String ID.
> + *     Null terminating string, 0-extended to word alignment.
> + */
> +#define XEN_CRASHTABLE_STRINGTAB            0x3000000

The number looks as if it wasn't picked arbitrarily, so a comment should
say what it was derived from. Or if it is truly (and compatibly) arbitrary,
imo this should be stated here too.

> +
> +#define XEN_STRINGTAB_VERSION 0
> +#define XEN_STRINGTAB_CSET 1
> +#define XEN_STRINGTAB_COMPILE_DATE 2
> +#define XEN_STRINGTAB_COMPILED_BY 3
> +#define XEN_STRINGTAB_COMPILER 4
> +#define XEN_STRINGTAB_CMDLINE 5
> +
> +/*
> + * Xen value crash table.
> + * Note is a list in the form of:
> + *     Machine word (32/64bit) Value ID.
> + *     64bit value.
> + */
> +#define XEN_CRASHTABLE_VAL64TAB             0x3000001
> +
> +typedef struct { unsigned long id; uint64_t val; } val64tab_t;

You're saying you're doing this to improve 32-/64-bit interaction. How
is using 'unsigned long' here (and below) fitting into this picture?

> +
> +#define XEN_VALTAB_TOP_OF_MEM 0
> +#define XEN_VALTAB_CONRING_SIZE 1
> +
> +/*
> + * Xen symbol crash table
> + * Note is a list in the form of:
> + *     Machine word (32/64bit) Symbol ID.
> + *     Symbol Address (64bit pointers).
> + */
> +#define XEN_CRASHTABLE_SYM64TAB             0x3000002
> +
> +typedef struct { unsigned long id; uint64_t addr; } sym64tab_t;
> +
> +#define XEN_SYMTAB_CONRING 0
> +
> +
> +#endif /* __XEN_PUBLIC_CRASHTABLES_H__ */
> +
> +/*
> +TODO: reintroduce local vars here.  emacs doesn't get on well with patch 
> files claiming to be C files
> + */

???

Jan

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.