|
[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 16:11, Jan Beulich wrote:
>>>> 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?
Yes
>> +
>> +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.
ASSERT() it is then. Having a bug at this point stalling the crash path
would just be silly, and the crash kernel does have to deal with the
possibility that all of Xen memory is completely corrupted.
>> + 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.
Ok
>> +
>> 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.
Why not? Is this about the potential safety of making the symbols
visible outside of console.o?
Having functions around the codebase to call to fill in values is not
scalable in terms of code; It would either require one function per
intended entry into the crash table, or one function per .c file which
then has to re-switch on the table id.
Both of these options require that new entries for the crash table
require new code in two files rather than just one.
>> 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.
Ok
>> +
>> +#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?
You are correct. I have missed the final logical step (I think when
merging my much finer grain development patches).
>> +
>> +#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
Ditto re. merging, although I realized this mistake right after the
emails got sent.
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |