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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/03/12 16:35, Jan Beulich wrote:
>>>> On 12.03.12 at 15:53, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> Changes since v1:
>>  *  Fix up naming conventions and static qualifiers.
>>  *  Remove XENLOG_WARNINGs, as in the case of a crash, they are little or no
>>     use. Instead, mark entries as invalid so as to reduce confusion to the
>>     crashdump kernel trying to parse the table.
>>  *  Change strtab setup so a failure at that point wont fail the entire kexec
>>     setup.
>>  *  Change the modifications to console.c to prevent marking conring and
>>     conring_size as global symbols.
>>  *  Add more details into crashtables.h clarifying certain information.
> While you indeed added a few things, I don't see any of my remarks
> against v1 having got addressed.

There is a new comment section at the top, explaining the basis for
numbering, and why unsigned longs are not a problem.

To summarize:

Numbering follows on from XEN_ELFNOTE_CRASH_* (0x100000x) and
XEN_ELFNOTE_DUMPCORE_* (0x200000x)

unsigned longs are more complicated.  The justification was that Xen
would only ever use the size it was compiled for, and this would be
calculable from the core file class (ELFCLASS{32,64})  Any crashdump
environment designed for the same bitness as Xen would be fine, and any
general crashdump environment would have to implement all possible
sizes, based on the class.

I will see if there is a neat way to expose explicit structures for both
possible bitnesses of Xen.

>>  *  Add emacs local variables at the end of crashtables.h
> I'd rather like to see them go away in the other public headers too.

I am ambivalent, but will maintain consistency with the others.

>> +#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)
> Didn't even notice in v1 that here you're using "unsigned long" too.

Same justification as above.

>> +        switch( crash_val64tab[i].id )
>> +        {
>> +        case XEN_VALTAB_MAX_PAGE:
>> +            crash_val64tab[i].val = (uint64_t)(max_page);
> Pointless cast and parentheses.

True

>> +            break;
>> +        case XEN_VALTAB_CONRING_SIZE:
>> +            console_write_val64tab(&crash_val64tab[i]);
>             console_write_val64tab(crash_val64tab[i].id, 
> &crash_val64tab[i].val);
>
> would eliminate the need to expose the crashval types to console.h.
> I'd further suggest folding the two console related functions.

But the crashval #DEFINES would still need to be exposed and they are
all in the same header file.

The symtab and valtab functions are separate because they refer to
separate tables.  If I were to fold the functions, I would either need
an extra parameter and nested switch statements, or settle with a
dependence between ID values.  The first is an ugly hack, and the second
works against the design principles of separating the tables in the
first place.

> And my reservations remain: Why is this any better than the consumer
> looking at the symbol table (as it would continue to need to do for all
> other information it might be after).

There are several different reasons.  In the 'min' case, it is true that
all relevant information is in the symbol table, but there is
information required for 'all' which wont be in the symbol table.  We
have had cases in the past where users have updated Xen without updating
the symbol file, resulting in confusing or wrong crash dump analyses. 
Whilst in an ideal world, we wouldn't support this, that argument
doesn't fly politically.

Finally, and most importantly along the line of 32bit crashdump support
on 64bit Xen; It is problematic to find out (through the symbol file and
page table walks) that an value you need is located in memory outside
the first 64GB.  By entering the value into the value table, it is
explicitly being made accessible to a 32bit crash kernel.

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