fpswa logging redux

I've recently had some requests from people running apps on our ia64 boxes for more detailed fpswa fault information. Currently, the ia64 kernel will print up to a certain number of fpswa fault messages per tick, with ip information about where the fault came from. The user can also choose to be notified of faults via a SIGFPE signal, but the si_info field of the siginfo structure doesn't contain enough information about the fault to be useful.

I'd like to fully decode the information in the isr about what type of fault occured (whether it be a software assist fault, some sort of IEEE filter fault, etc.) and print it along with the ip when faults occur. While I'm at it, I may as well the proper SIGFPE subcode into si_info in case of signal delivery is selected.

Comments? David & Bjorn, do you think this would be useful enough to get into the tree?

Thanks,
Jesse

I have a couple of Perl scripts for decoding isr and psr. They read hex values (with or without the leading 0x) from stdin and pretty print the result.

--- ia64_isr ---

#!/usr/bin/perl -w
use strict;
use integer;

my @fields = (

	[ 0, 16, "Code" ],
	[ 16, 8, "Vector" ],
	[ 24, 8, "rv" ],
	[ 32, 1, "x" ],
	[ 33, 1, "w" ],
	[ 34, 1, "r" ],
	[ 35, 1, "na" ],
	[ 36, 1, "sp" ],
	[ 37, 1, "rs" ],
	[ 39, 1, "ni" ],
	[ 40, 1, "so" ],
	[ 41, 2, "ei" ],
	[ 43, 1, "ed" ],
	[ 44, 20, "rv" ],
	);

while (defined($_ = <STDIN>)) {
	chomp();
	s/^0[xX]//;
	my $w0 = hex(substr($_, 0, -8));
	my $w1 = hex(substr($_, -8));
	printf("isr %s\n", $_);
	&ia64_isr($w1, 0, 32);
	&ia64_isr($w0, 32, 64);
}

sub ia64_isr()

{
	my ($w, $s, $e) = @_;
	my $v;
	foreach (@fields) {
		next if ($_->[0] >= $e || $_->[0] < $s);
		# use integer implies signed shift, I want unsigned
		no integer;
		$v = ($w >> ($_->[0] - $s)) & (~0 >> (32 - $_->[1]));
		printf("%2d %2d %12s %x\n", $_->[0], $_->[1], $_->[2], $v);
	}
}

--- ia64_psr ---

#!/usr/bin/perl -w
use strict;
use integer;

my @fields = (

	[ 0, 6, "User mask" ],
	[ 0, 1, "rv" ],
	[ 1, 1, "be" ],
	[ 2, 1, "up" ],
	[ 3, 1, "ac" ],
	[ 4, 1, "mfl" ],
	[ 5, 1, "mfh" ],
	[ 0, 24, "System mask" ],
	[ 6, 6, "rv" ],
	[ 13, 1, "ic" ],
	[ 14, 1, "i" ],
	[ 15, 1, "pk" ],
	[ 16, 1, "rv" ],
	[ 17, 1, "dt" ],
	[ 18, 1, "dfl" ],
	[ 19, 1, "dfh" ],
	[ 20, 1, "sp" ],
	[ 21, 1, "pp" ],
	[ 22, 1, "di" ],
	[ 23, 1, "si" ],
	[ 24, 1, "db" ],
	[ 25, 1, "lp" ],
	[ 26, 1, "tb" ],
	[ 27, 1, "rt" ],
	[ 28, 4, "rv" ],
	[ 32, 2, "cpl" ],
	[ 34, 1, "is" ],
	[ 35, 1, "mc" ],
	[ 36, 1, "it" ],
	[ 37, 1, "id" ],
	[ 38, 1, "da" ],
	[ 39, 1, "dd" ],
	[ 40, 1, "ss" ],
	[ 41, 2, "ri" ],
	[ 43, 1, "ed" ],
	[ 44, 1, "bn" ],
	[ 45, 1, "ia" ],
	[ 46, 18, "rv" ]
	);

while (defined($_ = <STDIN>)) {
	chomp();
	s/^0[xX]//;
	my $w0 = hex(substr($_, 0, -8));
	my $w1 = hex(substr($_, -8));
	printf("psr %s\n", $_);
	&ia64_psr($w1, 0, 32);
	&ia64_psr($w0, 32, 64);
}

sub ia64_psr()

{
	my ($w, $s, $e) = @_;
	my $v;
	foreach (@fields) {
		next if ($_->[0] >= $e || $_->[0] < $s);
		# use integer implies signed shift, I want unsigned
		no integer;
		$v = ($w >> ($_->[0] - $s)) & (~0 >> (32 - $_->[1]));
		printf("%2d %2d %12s %x\n", $_->[0], $_->[1], $_->[2], $v);
	}
}

It's the isr code field I'm particularly interested in. For fp faults and traps, I'd like to decode what type of fault/trap occured and add it to the printk, e.g.

Dec 11 17:11:05 morale kernel: a.out(1901): floating-point assist trap : ip@00000000000752
Dec 11 17:11:05 morale kernel: a.out(1901): floating-point assist fault : software assist ip@00000000000c01

Jesse

The general approach is that the kernel prints raw values and user space decodes them, unless there is some overriding reason to do it in kernel. IOW, print the isr in the kernel but decode it using the scripts or equivalent.

But if we do that the si_code field of siginfo won't have useful information. My thought was that if we decode isr in enough detail to fill in si_code correctly, we may as well make the printks more useful as well. I think all this stuff would be in arch/ia64/kernel/traps.c, with the possible exception of another SIGFPE code for software assist faults (and maybe denormal/unnormal).

Thanks,
Jesse

Jesse> I'd like to fully decode the information in the isr about Jesse> what type of fault occured (whether it be a software assist Jesse> fault, some sort of IEEE filter fault, etc.) and print it Jesse> along with the ip when faults occur. While I'm at it, I may Jesse> as well the proper SIGFPE subcode into si_info in case of Jesse> signal delivery is selected.

Like Keith, I'd like to keep the amount of decoding in the kernel as minimal as possible. We already have si_isr in the siginfo structure, so apps/libraries can decode things themselves. Having said that, we ought to decode things sufficiently so that si_code can be set to the correct value (where this makes sense). Hopefully, that won't add tons of code and it would help application portability.

	--david

How about something like this then? It just prints out the isr in addition to the ip for the case of printk, and decodes the right si_code for signals. If this approach looks ok, I can add trap decoding too.

Thanks,
Jesse

diff -Naur -X /home/jbarnes/dontdiff linux-2.4.19-ia64/arch/ia64/kernel/traps.c linux-2.4.19-ia64-fpswa/arch/ia64/kernel/traps.c
@@ -45,6 +45,34 @@
 
 static fpswa_interface_t *fpswa_interface;
 
+/*
+ * fp fault isr.code <-> siginfo.si_code mapping
+ */
+struct fp_fault_info fp_fault_info[] = {
+	{ 1<<0, FPE_FLTINV },
+	{ 1<<1, FPE_DENORM },
+	{ 1<<2, FPE_FLTDIV },
+	{ 1<<3, FPE_SWASST },
+	{ 1<<4, FPE_FLTINV },
+	{ 1<<5, FPE_DENORM },
+	{ 1<<6, FPE_FLTDIV },
+	{ 1<<7, FPE_SWASST },
+};
+
+static inline int
+fp_fault_si_code(__u64 isr)
+{
+	int i, ret = 0;
+	int nr_entries;
+	nr_entries = sizeof(fp_fault_info)/sizeof(struct fp_fault_info);
+	
+	for (i = 0; i < nr_entries; i++) {
+		if (isr & fp_fault_info[i].isr_code_bit)
+			ret |= fp_fault_info[i].si_code;
+	}
+	return ret;
+}
+
 void __init
 trap_init (void)
 {

@@ -336,8 +364,9 @@
 		fpu_swa_count = 0;
 	if ((++fpu_swa_count < 5) && !(current->thread.flags & IA64_THREAD_FPEMU_NOPRINT)) {
 		last_time = jiffies;
-		printk(KERN_WARNING "%s(%d): floating-point assist fault at ip %016lx\n",
-		       current->comm, current->pid, regs->cr_iip + ia64_psr(regs)->ri);
+		printk(KERN_WARNING "%s(%d): floating-point assist %s ",
+		       current->comm, current->pid, fp_fault ? "fault:" : "trap");
+		printk("ip=%016lx, isr=%16lx\n", regs->cr_iip + ia64_psr(regs)->ri, isr);
 	}
 
 	exception = fp_emulate(fp_fault, bundle, &regs->cr_ipsr, &regs->ar_fpsr, &isr, &regs->pr,

@@ -556,6 +585,8 @@
 			siginfo.si_signo = SIGFPE;
 			siginfo.si_errno = 0;
 			siginfo.si_code = FPE_FLTINV;
+			if (vector == 32) /* decode fault into si_code */
+				siginfo.si_code = fp_fault_si_code(isr);
 			siginfo.si_addr = (void *) (regs->cr_iip + ia64_psr(regs)->ri);
 			siginfo.si_flags = __ISR_VALID;
 			siginfo.si_isr = isr;

diff -Naur -X /home/jbarnes/dontdiff linux-2.4.19-ia64/include/asm-ia64/fpu.h linux-2.4.19-ia64-fpswa/include/asm-ia64/fpu.h
@@ -54,6 +54,11 @@
 
 # ifndef __ASSEMBLY__
 
+struct fp_fault_info {
+	unsigned long isr_code_bit;
+	int si_code; /* SIGFPE si_code */
+};
+
 struct ia64_fpreg {
 	union {
 		unsigned long bits[2];

diff -Naur -X /home/jbarnes/dontdiff linux-2.4.19-ia64/include/asm-ia64/siginfo.h linux-2.4.19-ia64-fpswa/include/asm-ia64/siginfo.h
@@ -172,6 +172,8 @@
 #define __FPE_DECERR	(__SI_FAULT|11)	/* packed decimal error */
 #define __FPE_INVASC	(__SI_FAULT|12)	/* invalid ASCII digit */
 #define __FPE_INVDEC	(__SI_FAULT|13)	/* invalid decimal digit */
+#define FPE_SWASST	(__SI_FAULT|14) /* software assist fault */
+#define FPE_DENORM	(__SI_FAULT|15) /* denormal/unnormal operand */
 #define NSIGFPE		13
 
 /*

Jesse> How about something like this then? It just prints out the Jesse> isr in addition to the ip for the case of printk, and decodes Jesse> the right si_code for signals. If this approach looks ok, I Jesse> can add trap decoding too.

Basically OK with me, however:

o What's the point of declaring fp_fault_info in a header file? It's used only in one place.

o si_code is NOT a bitmask; it makes no sense at all for fp_fault_si_code() to OR multiple values together; this makes me highly suspicious that there is a misunderstanding somewhere...

o Who defined FP_SWASST and FPE_DENORM? I don't recall seeing them in the ia64 psABI and they definitely look ia64-specific, so their names should at least be prefixed by a double-underscore (siginfo.h).

o If you add stuff to asm/siginfo.h, don't forget to update glibc (in sysdeps/unix/sysv/linux/ia64/bits/siginfo.h).

	--david