[POWERPC] Handles float point exception

[alfredo]

Summary

Enables FPE on ppc by default. Note: user application still needs to set, with feenableexcept() , which exception will trigger a SIGFPE.

Test Plan

On both 32 and 64 bits environments, the last lines of test program bellow should be like:

Handler called 5 times. Caught signal 8
si_signo 8
si_code 3
si_addr 0x0x100110a0
Caught SIGFPE due to FPE_FLTDIV, as expected
Clearing exceptions
## handler called too many times, disabling FP exceptions ##
y = inf
Exited normally.

Test code:

#include <fenv.h>
#include <signal.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define CNT_MAX 5
int cnt = 0;

void handler(int sig, siginfo_t *info, void *uap)
{
	cnt++;

        printf("Handler called %d times. Caught signal %d\n", cnt, sig);
        printf("si_signo %d\n", info->si_signo);
        printf("si_code %d\n", info->si_code);
        printf("si_addr 0x%p\n", info->si_addr);

        if (info->si_signo == SIGFPE  &&
                        info->si_code == FPE_FLTDIV) {
                printf("Caught SIGFPE due to FPE_FLTDIV, as expected\n");
        } else
                printf("Caught unexpected signal.\n");

	printf("Clearing exceptions\n");
	feclearexcept(FE_ALL_EXCEPT);

	if (cnt >= CNT_MAX) {
		printf("## handler called too many times, disabling FP exceptions ##\n");
		fedisableexcept(FE_INVALID | FE_DIVBYZERO | FE_OVERFLOW  | FE_UNDERFLOW);
		if (cnt > CNT_MAX + 5) {
			printf("ERROR!! Didn't stop triggering exceptions!\n");
			exit(1);
		}
	}


}

int main(void)
{
        struct sigaction sa;

        sa.sa_sigaction = handler;

        sigemptyset(&sa.sa_mask);
        sa.sa_flags = SA_SIGINFO; /* Restart functions if
                                 interrupted by handler */

        if (sigaction(SIGFPE, &sa, NULL) == -1)
        {
                printf("error\n");
        }

        int mask = feenableexcept(FE_INVALID   |
                   FE_DIVBYZERO |
                   FE_OVERFLOW  |
                   FE_UNDERFLOW);
        printf("mask: %x\n", mask);

        volatile double x = 0.0;
        volatile double y = 1.0 / x;
        printf("y = %f\n", y);
	if (cnt == CNT_MAX) {
		printf("Exited normally.\n");
	}
	else {
		printf("ERROR!!!!\n");
	}
        return y;
}

[jhibbits]

sys/powerpc/include/psl.h

49

This belongs in trap.hh, as EXC_PGM_FPE, or similar name, since it's part of SRR1, not MSR.

Actually, I think this really should belong in spr.h, as SRR1_PGM_FPE, and the other EXC_PGM* #defines moved there, too. But that's not how it's done now, so just put it in trap.h.

[luporl]

Looks good.
I just have some style related comments and 2 questions about the changes.

sys/powerpc/include/psl.h

91

Do we want to enable FP exceptions by default?

sys/powerpc/powerpc/trap.c

417–431

This brace should be in the line above.
See style(9).

423

Variable declarations should be all in the beginning of a block, separated of other statements by a new line, and function call initializers should not be used (e.g. mfmsr()).
See style(9).

443

I may be missing something, but why is enable_fpu() needed here?

[renato.riolino_eldorado.org.br]

sys/powerpc/include/psl.h

91

It's still not "enabled". By default none of the exception enabled registers are enable. User must call feenableexcept() if he wants to receive a FP exception.

sys/powerpc/powerpc/trap.c

443

enable_fpu() loads the floating point registers from the PCB. Without it the application can't capture the SIGFPE.

[renato.riolino_eldorado.org.br]

Fixes pointed by jhibbits and luporl and a test of endianess when checking the type of fp exception.

[luporl]

sys/powerpc/powerpc/trap.c

419–430

I think this whole union could be replaced by an uint64_t.
The only part where you use it as a double is in the inline assembly, which should work fine with uint64_t too.
Then you wouldn't need to worry about the endianness too.
(BTW, currently all kernel PPC code assume BE, but starting to take LE into account is nice).

[renato.riolino_eldorado.org.br]

Using a uint64_t instead of a union as suggested by luporl

[jhibbits]

sys/powerpc/powerpc/trap.c

430

This block won't work as you think. FPE_FLTDIV, et al, are integers, not bitfields. Only one reason is allowed.

[jhibbits]

sys/powerpc/powerpc/trap.c

441

Is there a paired save_fpu() anywhere? Otherwise we're overwriting the FPRs with whatever was last saved, which could be nothing.

[renato.riolino_eldorado.org.br]

Treating fpe exceptions as integers instead of bitfields and calling save_fpu() when detected the signal as been FPE.

[renato.riolino_eldorado.org.br]

Fixed order for calling save_fpu() and enable_fpu()

[jhibbits]

sys/powerpc/powerpc/trap.c

417–431

I don't think this is quite right. We need to be able to pass up SIGFPE on emulated FPU as well.

[renato.riolino_eldorado.org.br]

Passing SIGFPE on emulated FPU as well.

[jhibbits]

sys/powerpc/powerpc/trap.c

421–425

This will panic with an illegal instruction on cores with no FPU. Isn't the FPSCR already available?

I think this should also probably go into a FPU-specific header (machine/fpu.h, maybe even powerpc/fpu.c for implementation), so that it's easy to also use other FPU interfaces, such as the SPE for powerpcspe arch.

[renato.riolino_eldorado.org.br]

Moved exception type check from trap.c to fpu.c

[jhibbits]

sys/powerpc/powerpc/exec_machdep.c

1165 ↗

(On Diff #69111)

Shouldn't this be a save_fpu() instead?

sys/powerpc/powerpc/fpu.c

222

I think there's a misunderstanding here:

• We need to support SIGFPE for both emulated and physical FPU. This check only handles physical.
• 'sig' in exec_machdep.c is set by fpu_emulate(), otherwise it's set directly by ppc_instr_emulate(), and the FPU registers are already updated. Therefore
• We already have in the PCB the fpscr from above. There's no need to extract it again here.

[alfredo]

I'm going to continue with revision and tests. Thank you @renato.riolino_eldorado.org.br for the work investigating it and providing the patch!

[alfredo]

Updated after jhibbits comments.
I don't think this patch is ready. Test shows signal being delivered to application, but I cannot catch/handle it. Need more tests.

[alfredo]

code refactory (still in progress)

[alfredo]

Code refactored, FPE exception is now identified without instruction emulate
Cleared ssr1 bits FE0 and FE1 makes user signal handler work instead of trap with SIGILL , but I don't think it's definitive solution.

system instability doesn't occur on bare metal (confirmed by @afscoelho_gmail.com ). It might point to a bug in QEMU msr handling

[bdragon]

I can tell you off the top of my head: Yes, qemu MSR handling *is* busted (at least in TCG) -- mtmsr(d) in qemu does not do anything remotely close to what the ISA describes it as. It just immediately whacks the entire value into the MSR, no bit masking no nothing.

I encountered this myself working on the LE stuff. As you can imagine, the endianness suddenly changing in between instructions because I was relying on some of the bits actually being ignored was... problematic to say the least.

I had to work around this on my end with hacks like https://github.com/freebsd/freebsd/commit/4a8fe4362a432ba8ddeb0f13148fe144a26e7b2a -- if there are any MSR bits you rely on that are only supposed to be updated during rfi(d), you will need to ensure not to change them prematurely with qemu.

[alfredo]

Fix "random process" crash due to leaked FPSCR bits, code cleanup and updated test plan example.

[jhibbits]

Is there any more work needed on this?

[alfredo]

Added cleanup of FPSCR after saving fpu state

[alfredo]

In D23623#604427, @jhibbits wrote:

Is there any more work needed on this?

I did one more change: in trap.c moved cleanup_fpscr() that was after get_fpu_exception() to save_fpu(), so kernel will continue with a clean FPSCR after this point.
Final behavior look the same, but any input is welcome.

[alfredo]

It should be good now. User is able to change floating point stat inside handler if wish.
Updated test program to exercise trap reocorring a few times before disabling exceptions inside handler and letting program to finish "gracefully".

[alfredo]

Add support for signal handling on 32 bits