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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.03.12 at 19:42, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Didn't expect it there, and thus overlooked it - I'm sorry.

> To summarize:
> 
> Numbering follows on from XEN_ELFNOTE_CRASH_* (0x100000x) and
> XEN_ELFNOTE_DUMPCORE_* (0x200000x)

Then please call the new ones XEN_ELFNOTE_* too, fold the whole
stuff into public/elfnote.h, and generate the data as normal notes.
Or is there a reason not to? Even more so that part of what you do
here is really overlapping with XEN_ELFNOTE_DUMPCORE_* (which
you could simply re-use).

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

If going with "proper" notes, this will become mute anyway.

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

Yet here it doesn't even make sense - you're not going to have a
string table of 4Gb or more in size, and hence you can as well use
uint32_t uniformly.

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

Then pass a bool_t instead of the ID. You're wanting two values from
the console code only after all.

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

No, the separation can be completely transparent to the console code,
just leveraging that the data type is always uint64_t.

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

When installing a package (or even manually via "make install-xen")
this can't happen, so someone must have broken this by copying
stuff manually. Which I don't think the code ought to be prepared
to deal with. You just can't have provisions for all sort of admin
mistakes - it won't scale.

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

I'm not following - Xen always resides in memory below 4G, so no
symbol in the symbol table can possibly be inaccessible (with the
pages tables for Xen's code and data being static and hence below
4G too, eventual page table walks wouldn't represent a problem
either).

Jan

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