Re: [Crash-utility] [PATCH] Add ELF core dump support for s390x

OK, first the simple stuff... The change to is_shared_object() doesn't make sense to me. That function is only used when loading extension modules. You make the following change in the ELFCLASS32 section. So if a 32-bit s390 crash session attempts to load a 32-bit s390 extension module, your patch will cause it to fall through and fail: --- a/symbols.c +++ b/symbols.c @@ -2710,7 +2710,7 @@ is_shared_object(char *file) break; case EM_S390: - if (machine_type("S390")) + if (machine_type("S390X")) return TRUE; break; How can that work?

Anyway, on to the the bigger picture... --- a/netdump.h +++ b/netdump.h @@ -68,6 +68,12 @@ struct vmcore_data { ulong switch_stack; uint num_prstatus_notes; void *nt_prstatus_percpu[NR_CPUS]; + void *nt_fpregset_percpu[NR_CPUS]; + void *nt_s390_timer_percpu[NR_CPUS]; + void *nt_s390_todcmp_percpu[NR_CPUS]; + void *nt_s390_todpreg_percpu[NR_CPUS]; + void *nt_s390_ctrs_percpu[NR_CPUS]; + void *nt_s390_prefix_percpu[NR_CPUS]; struct xen_kdump_data *xen_kdump_data; void *vmcoreinfo; uint size_vmcoreinfo; This is bit hard to swallow. A while back I went through and purged several prior static declarations of data structures that used arrays indexed with NR_CPUS -- replacing them with pointers that dynamically allocated the arrays when the actual number of cpus was known. I really don't want to ever *add* any new instances of static arrays indexed with NR_CPUS. Now you could argue that the currently-existing nt_prstatus_percpu[NR_CPUS] is there, but that should probably be changed to malloc() NR_CPUS-worth during the dumpfile discovery phase, and then do a realloc() later on when the actual number of cpus is determined. Anyway, the x86_64 and ia64 arches are up to 4096 NR_CPUS, so your patch adds ~200k of useless static data for those architectures. So I suggest suggest that all of your new NR_CPU-indexed fields -- none of which are of any interest to the other architectures -- should be moved into s390x.c, and then just a single pointer to the collective data could be put into the vmcore_data structure. (i.e., similar to the xen_kdump_data pointer). Let's keep the vmcore_data structure as generic as possible.

Also BTW, there's a lot work d one reading/saving the new note data, but once that's accomplished, I don't see where the data is ever used? What's the point of saving it if it's not referred to somewhere later on? You couldn't even look at it unless you ran a gdb on the crash session. Or am I missing something?

I see the temporary bt->machdep reference to the static s390x_cpu data structure in get_netdump_regs_s390x(). But I would think that if you're going to the trouble to save all of that data, that you might want to put the s390x_cpu structure in s390x, and then maybe dump its data in s390x_dump_machdep_table() via "help -m". Now, the NT_S390_XXX defines -- were did they originate exactly?

If I google them -- all I get is a reference to your 2-hour old crash-utility mailing list post!

Anyway, I haven't tried compiling it, but given that they are all #define'd in the "#ifdef S390X" section of defs.h, then netdump.c couldn't possibly compile for any other arch.

Also, the NT_S390_XXX numerical values are probably safe and would never be misconstrued, but the NT_FPREGSET note is the only new note section that could conceivably show up in some other architecture, so that should be restricted to s390x only.

What do you think?