Hello Dave,
On Thu, 2010-02-11 at 11:13 -0500, Dave Anderson wrote:
> 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?
Ok, that was probably a left over from my former work on the snap
extension module. That chunk is not needed for the EFL core dump
support. Sorry for that.
> 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.
Ok, so I will try to replace the arrays with one **void nt_s390
pointer.
> 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?
You are missing something. The data is copied to the s390x_cpu structure
in case of a backtrace of an active CPU.
> 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?
We have already brought the new s390 note sections upstream in the
binutils package. This is the place where new note sections are defined
first. In the next merge window for the Linux kernel, we also will bring
the new defines upstream to the Linux kernel (include/linux/elf.h).
> If I google them -- all I get is a reference to your 2-hour old
> crash-utility mailing list post!
Be patient.
> 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.
Yes, I noticed that 10 seconds after I hit the send button for that
patch. The defines have to be outside of #ifdef S390X or when I change
the patch regarding your wishes, I can probably move them to s390x.c
> 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?
Good points, I try to change my patch and move more stuff into s390x.c.
Michael
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
