[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



----- "Michael Holzheu" <holzheu linux vnet ibm com> wrote:

> Hi Dave,
> 
> Next patch...
> 
> We will have ELF core dumps on s390x in the near future:
> 
> This patch enables crash for reading s390x (64 bit) ELF core dumps. The
> following new ELF note sections are added by this patch:
> 
> * NT_FPREGSET:     Floating point registers - all architectures
> * NT_S390_TIMER:   S390 CPU timer
> * NT_S390_TODCMP:  S390 TOD clock comparator
> * NT_S390_TODPREG: S390 TOD programmable register
> * NT_S390_CTRS:    S390 control registers
> * NT_S390_PREFIX:  S390 prefix register
> 
> The mapping of a s390 CPU to the correct note number is done via the prefix
> register of the CPUs. This is implemented in s390x_get_note_nr(). The
> function gets the prefix register for the logical CPU number from the lowcore
> pointer array (global variable lowcore_ptr) and searches the matching prefix
> register note.
> 
> If CPU information is found in the dump for a running task, it is converted
> into a generic s390x_cpu structure which is then attached to "bt->machdep".
> If "bt->machdep" is set, the s390x "machdep->get_stack_frame()" function uses
> that register information.

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?

Dave





> 
> ---
>  defs.h    |   66 ++++++++++++++++++++++++
>  netdump.c |  164
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  netdump.h |    6 ++
>  s390x.c   |   34 ++++++++++--
>  symbols.c |    2 
>  5 files changed, 264 insertions(+), 8 deletions(-)
> 
> --- a/defs.h
> +++ b/defs.h
> @@ -2747,6 +2747,71 @@ struct efi_memory_desc_t {
>  #define _SECTION_SIZE_BITS	28
>  #define _MAX_PHYSMEM_BITS	42
>  
> +/*
> + * S390 CPU timer ELF note
> + */
> +#ifndef NT_S390_TIMER
> +#define NT_S390_TIMER 0x301
> +#endif
> +
> +/*
> + * S390 TOD clock comparator ELF note
> + */
> +#ifndef NT_S390_TODCMP
> +#define NT_S390_TODCMP 0x302
> +#endif
> +
> +/*
> + * S390 TOD programmable register ELF note
> + */
> +#ifndef NT_S390_TODPREG
> +#define NT_S390_TODPREG 0x303
> +#endif
> +
> +/*
> + * S390 control registers ELF note
> + */
> +#ifndef NT_S390_CTRS
> +#define NT_S390_CTRS 0x304
> +#endif
> +
> +/*
> + * S390 prefix ELF note
> + */
> +#ifndef NT_S390_PREFIX
> +#define NT_S390_PREFIX 0x305
> +#endif
> +
> +/*
> + * S390 floating point register ELF Note
> + */
> +#ifndef NT_FPREGSET
> +#define NT_FPREGSET 0x2
> +#endif
> +
> +struct nt_s390_fpregset_64 {
> +	uint32_t	fpc;
> +	uint32_t	pad;
> +	uint64_t	fprs[16];
> +} __attribute__ ((packed));
> +
> +/*
> + * s390x CPU info
> + */
> +struct s390x_cpu
> +{
> +	uint64_t	gprs[16];
> +	uint64_t	ctrs[16];
> +	uint32_t	acrs[16];
> +	uint64_t	fprs[16];
> +	uint32_t	fpc;
> +	uint64_t	psw[2];
> +	uint32_t	prefix;
> +	uint64_t	timer;
> +	uint64_t	todcmp;
> +	uint32_t	todpreg;
> +};
> +
>  #endif  /* S390X */
>  
>  #ifdef PLATFORM
> @@ -4196,6 +4261,7 @@ void get_netdump_regs(struct bt_info *, 
>  int is_partial_netdump(void);
>  void get_netdump_regs_x86(struct bt_info *, ulong *, ulong *);
>  void get_netdump_regs_x86_64(struct bt_info *, ulong *, ulong *);
> +void get_netdump_regs_s390x(struct bt_info *, ulong *, ulong *);
>  struct vmcore_data;
>  struct vmcore_data *get_kdump_vmcore_data(void);
>  int read_kdump(int, void *, int, ulong, physaddr_t);
> --- a/netdump.c
> +++ b/netdump.c
> @@ -213,6 +213,11 @@ is_netdump(char *file, ulong source_quer
>  				goto bailout;
>  			break;
>  
> +		case EM_S390:
> +			if (machine_type_mismatch(file, "S390X", NULL,
> +			    source_query))
> +				goto bailout;
> +			break;
>  		case EM_386:
>  			if (machine_type_mismatch(file, "X86", NULL,
>  			    source_query))
> @@ -1858,6 +1863,72 @@ dump_Elf64_Nhdr(Elf64_Off offset, int st
>  			}
>  		}
>  		break;
> +	case NT_FPREGSET:
> +		netdump_print("(NT_FPREGSET)");
> +		if (store) {
> +			for (i = 0; i < NR_CPUS; i++) {
> +				if (nd->nt_fpregset_percpu[i])
> +					continue;
> +				nd->nt_fpregset_percpu[i] = (void *)note;
> +				break;
> +			}
> +		}
> +		break;
> +	case NT_S390_TIMER:
> +		netdump_print("(NT_S390_TIMER)\n");
> +		if (store) {
> +			for (i = 0; i < NR_CPUS; i++) {
> +				if (nd->nt_s390_timer_percpu[i])
> +					continue;
> +				nd->nt_s390_timer_percpu[i] = (void *)note;
> +				break;
> +			}
> +		}
> +		break;
> +	case NT_S390_TODCMP:
> +		netdump_print("(NT_S390_TODCMP)\n");
> +		if (store) {
> +			for (i = 0; i < NR_CPUS; i++) {
> +				if (nd->nt_s390_todcmp_percpu[i])
> +					continue;
> +				nd->nt_s390_todcmp_percpu[i] = (void *)note;
> +				break;
> +			}
> +		}
> +		break;
> +	case NT_S390_TODPREG:
> +		netdump_print("(NT_S390_TODPREG)\n");
> +		if (store) {
> +			for (i = 0; i < NR_CPUS; i++) {
> +				if (nd->nt_s390_todpreg_percpu[i])
> +					continue;
> +				nd->nt_s390_todpreg_percpu[i] = (void *)note;
> +				break;
> +			}
> +		}
> +		break;
> +	case NT_S390_CTRS:
> +		netdump_print("(NT_S390_CTRS)\n");
> +		if (store) {
> +			for (i = 0; i < NR_CPUS; i++) {
> +				if (nd->nt_s390_ctrs_percpu[i])
> +					continue;
> +				nd->nt_s390_ctrs_percpu[i] = (void *)note;
> +				break;
> +			}
> +		}
> +		break;
> +	case NT_S390_PREFIX:
> +		netdump_print("(NT_S390_PREFIX)\n");
> +		if (store) {
> +			for (i = 0; i < NR_CPUS; i++) {
> +				if (nd->nt_s390_prefix_percpu[i])
> +					continue;
> +				nd->nt_s390_prefix_percpu[i] = (void *)note;
> +				break;
> +			}
> +		}
> +		break;
>  	case NT_PRPSINFO:
>  		netdump_print("(NT_PRPSINFO)\n");
>  		if (store)
> @@ -2082,6 +2153,9 @@ get_netdump_regs(struct bt_info *bt, ulo
>  		return get_netdump_regs_x86_64(bt, eip, esp);
>  		break;
>  
> +	case EM_S390:
> +		get_netdump_regs_s390x(bt, eip, esp);
> +		break;
>  	default:
>  		error(FATAL, 
>  		   "support for ELF machine type %d not available\n",
> @@ -2381,6 +2455,96 @@ get_netdump_regs_ppc64(struct bt_info *b
>  	machdep->get_stack_frame(bt, eip, esp);
>  }
>  
> +static void *
> +get_note_desc(Elf64_Nhdr *note)
> +{
> +	void *ptr = note;
> +
> +	return ptr + roundup(sizeof(*note) + note->n_namesz, 4);
> +}
> +
> +static int
> +s390x_get_note_nr(struct bt_info *bt)
> +{
> +	unsigned int cpu = bt->tc->processor;
> +	unsigned long lowcore_ptr, prefix;
> +	uint32_t *nt_prefix;
> +	unsigned int i;
> +
> +	lowcore_ptr = symbol_value("lowcore_ptr");
> +	readmem(lowcore_ptr + cpu * sizeof(long), KVADDR,
> +		&prefix, sizeof(long), "lowcore_ptr", FAULT_ON_ERROR);
> +	for (i = 0; i < nd->num_prstatus_notes; i++) {
> +		nt_prefix = get_note_desc(nd->nt_s390_prefix_percpu[i]);
> +		if (*nt_prefix == prefix)
> +			return i;
> +	}
> +	error(FATAL, "cannot determine NT_PRSTATUS ELF note for task:
> %lx\n",
> +	      bt->task);
> +}
> +
> +static void
> +s390x_note_to_s390x_cpu(struct s390x_cpu *s390x_cpu, int note_nr)
> +{
> +	struct nt_s390_fpregset_64 *nt_s390_fpregset;
> +	Elf64_Nhdr *note;
> +	char *ptr;
> +
> +	/* Copy prstatus note */
> +	note = (Elf64_Nhdr *) nd->nt_prstatus_percpu[note_nr];
> +	ptr = get_note_desc(note) + MEMBER_OFFSET("elf_prstatus",
> "pr_reg");
> +	memcpy(&s390x_cpu->psw, ptr, sizeof(s390x_cpu->psw));
> +	memcpy(&s390x_cpu->gprs, ptr + 16, sizeof(s390x_cpu->gprs));
> +	memcpy(&s390x_cpu->acrs, ptr + 16 + 16 * 8,
> sizeof(s390x_cpu->acrs));
> +
> +	/* Copy s390 floating point register note */
> +	nt_s390_fpregset = get_note_desc(nd->nt_fpregset_percpu[note_nr]);
> +	memcpy(&s390x_cpu->fpc, &nt_s390_fpregset->fpc,
> +	       sizeof(s390x_cpu->fpc));
> +	memcpy(&s390x_cpu->fprs, &nt_s390_fpregset->fprs,
> +	       sizeof(s390x_cpu->fprs));
> +
> +	/* Copy s390 additional notes */
> +	memcpy(&s390x_cpu->timer,
> +	       get_note_desc(nd->nt_s390_timer_percpu[note_nr]),
> +	       sizeof(s390x_cpu->timer));
> +	memcpy(&s390x_cpu->todcmp,
> +	       get_note_desc(nd->nt_s390_todcmp_percpu[note_nr]),
> +	       sizeof(s390x_cpu->todcmp));
> +	memcpy(&s390x_cpu->todpreg,
> +	       get_note_desc(nd->nt_s390_todpreg_percpu[note_nr]),
> +	       sizeof(s390x_cpu->todpreg));
> +	memcpy(&s390x_cpu->ctrs,
> +	       get_note_desc(nd->nt_s390_ctrs_percpu[note_nr]),
> +	       sizeof(s390x_cpu->ctrs));
> +	memcpy(&s390x_cpu->prefix,
> +	       get_note_desc(nd->nt_s390_prefix_percpu[note_nr]),
> +	       sizeof(s390x_cpu->prefix));
> +}
> +
> +void
> +get_netdump_regs_s390x(struct bt_info *bt, ulong *eip, ulong *esp)
> +{
> +	static struct s390x_cpu s390x_cpu;
> +	unsigned int note_nr;
> +	Elf64_Nhdr *note;
> +
> +	bt->machdep = NULL;
> +	if (nd->num_prstatus_notes == 0)
> +		goto out;
> +	if (!(is_task_active(bt->task) &&
> +	      (kt->cpu_flags[bt->tc->processor] & ONLINE)))
> +		goto out;
> +	note_nr = s390x_get_note_nr(bt);
> +	if (!nd->nt_prstatus_percpu[note_nr])
> +		error(FATAL, "cannot determine NT_PRSTATUS ELF note for "
> +		      "task: %lx\n", bt->task);
> +	s390x_note_to_s390x_cpu(&s390x_cpu, note_nr);
> +	bt->machdep = &s390x_cpu;
> +out:
> +	machdep->get_stack_frame(bt, eip, esp);
> +}
> +
>  int 
>  is_partial_netdump(void)
>  {
> --- 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;
> --- a/s390x.c
> +++ b/s390x.c
> @@ -56,7 +56,6 @@ static ulong s390x_processor_speed(void)
>  static int s390x_eframe_search(struct bt_info *);
>  static void s390x_back_trace_cmd(struct bt_info *);
>  static void s390x_dump_irq(int);
> -static void s390x_get_stack_frame(struct bt_info *, ulong *, ulong
> *);
>  static int s390x_dis_filter(ulong, char *);
>  static void s390x_cmd_mach(void);
>  static int s390x_get_smp_cpus(void);
> @@ -64,6 +63,7 @@ static void s390x_display_machine_stats(
>  static void s390x_dump_line_number(ulong);
>  static struct line_number_hook s390x_line_number_hooks[];
>  static int s390x_is_uvaddr(ulong, struct task_context *);
> +static void s390x_get_stack_frame(struct bt_info *, ulong *, ulong
> *);
>  
>  
>  /*
> @@ -571,15 +571,36 @@ s390x_has_cpu(struct bt_info *bt)
>   * read lowcore for cpu
>   */
>  static void
> -s390x_get_lowcore(int cpu, char* lowcore)
> +s390x_get_lowcore(struct bt_info *bt, char* lowcore)
>  {
>  	unsigned long lowcore_array,lowcore_ptr;
> +	struct s390x_cpu *s390x_cpu;
> +	int cpu = bt->tc->processor;
>  
>  	lowcore_array = symbol_value("lowcore_ptr");
>  	readmem(lowcore_array + cpu * S390X_WORD_SIZE,KVADDR,
> -		&lowcore_ptr, sizeof(long), "lowcore_ptr", FAULT_ON_ERROR);
> -	readmem(lowcore_ptr, KVADDR, lowcore, LOWCORE_SIZE, "lowcore", 
> +		&lowcore_ptr, sizeof(long), "lowcore_ptr",
>  		FAULT_ON_ERROR);
> +	readmem(lowcore_ptr, KVADDR, lowcore, LOWCORE_SIZE, "lowcore",
> FAULT_ON_ERROR);
> +
> +	if (!bt->machdep)
> +		return;
> +
> +	s390x_cpu = bt->machdep;
> +	/* Copy ELF register information to defined places in lowcore */
> +	memcpy(lowcore + 4864, &s390x_cpu->psw, sizeof(s390x_cpu->psw));
> +	memcpy(lowcore + 4736, &s390x_cpu->gprs, sizeof(s390x_cpu->gprs));
> +	memcpy(lowcore + 4928, &s390x_cpu->acrs, sizeof(s390x_cpu->acrs));
> +
> +	memcpy(lowcore + 4892, &s390x_cpu->fpc, sizeof(s390x_cpu->fpc));
> +	memcpy(lowcore + 4608, &s390x_cpu->fprs, sizeof(s390x_cpu->fprs));
> +
> +	memcpy(lowcore + 4888, &s390x_cpu->prefix,
> sizeof(s390x_cpu->prefix));
> +	memcpy(lowcore + 4992, &s390x_cpu->ctrs, sizeof(s390x_cpu->ctrs));
> +
> +	memcpy(lowcore + 4900, &s390x_cpu->todpreg,
> sizeof(s390x_cpu->todpreg));
> +	memcpy(lowcore + 4904, &s390x_cpu->timer,
> sizeof(s390x_cpu->timer));
> +	memcpy(lowcore + 4912, &s390x_cpu->todcmp,
> sizeof(s390x_cpu->todcmp));
>  }
>  
>  /*
> @@ -627,7 +648,7 @@ s390x_back_trace_cmd(struct bt_info *bt)
>  			fprintf(fp,"(active)\n");
>  			return;
>  		}
> -		s390x_get_lowcore(cpu,lowcore);
> +		s390x_get_lowcore(bt, lowcore);
>  		psw_flags = ULONG(lowcore + OFFSET(s390_lowcore_psw_save_area));
>  
>  		if(psw_flags & 0x1000000000000ULL){
> @@ -908,7 +929,7 @@ s390x_get_stack_frame(struct bt_info *bt
>  	char lowcore[LOWCORE_SIZE];
>  
>  	if(s390x_has_cpu(bt))
> -		s390x_get_lowcore(s390x_cpu_of_task(bt->task),lowcore);
> +		s390x_get_lowcore(bt, lowcore);
>  
>  	/* get the stack pointer */
>  	if(esp){
> @@ -1139,5 +1160,4 @@ try_closest:
>  		}
>  	}
>  }
> -
>  #endif 
> --- 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;
>  		}
> 
> 
> --
> Crash-utility mailing list
> Crash-utility redhat com
> https://www.redhat.com/mailman/listinfo/crash-utility


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]