[patch][5/5] powerpc V2: Add the general support for Embedded Floating-Point instructions

[Zhu Ebony-r57400]

> -----Original Message-----
> From: Kumar Gala [mailto:galak at kernel.crashing.org] 
> Sent: Thursday, February 08, 2007 3:33 PM
> To: Zhu Ebony-r57400
> Cc: paulus at samba.org; linuxppc-dev at ozlabs.org
> Subject: Re: [patch][5/5] powerpc V2: Add the general support 
> for Embedded Floating-Point instructions
> 
> 
> On Feb 7, 2007, at 9:55 PM, ebony.zhu at freescale.com wrote:
> 
> > Add the general support for Embedded Floating-Point instructions to 
> > fully comply with IEEE-754.
> >
> > Signed-off-by:Ebony Zhu <ebony.zhu at freescale.com>
> > ---
> >  arch/powerpc/kernel/head_fsl_booke.S |    4
> >  arch/powerpc/kernel/traps.c          |   57 +++++
> >  arch/powerpc/math-emu/Makefile       |   25 ++
> >  arch/powerpc/math-emu/sfp-machine.h  |    2
> >  arch/powerpc/math-emu/spe.h          |    1
> >  arch/powerpc/sysdev/Makefile         |    1
> >  arch/powerpc/sysdev/sigfpe_handler.c |  361 +++++++++++++++++++++++
> > +++++++++++
> >  7 files changed, 442 insertions(+), 9 deletions(-)
> 
> I thought we were going to have some general Kconfig option 
> to enable all this?  EMDEDDED_FP_IEEE or something like that

So that users have chance to enable/disable fully IEEE compliance?
Segher
had mentioned this, and it sounds reasonable. 

> 
> >
> > diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/ 
> > kernel/head_fsl_booke.S
> > index 66877bd..0d05db0 100644
> > --- a/arch/powerpc/kernel/head_fsl_booke.S
> > +++ b/arch/powerpc/kernel/head_fsl_booke.S
> > @@ -705,7 +705,7 @@ #else
> >  #endif /* CONFIG_SPE */
> >
> >  	/* SPE Floating Point Round */
> > -	EXCEPTION(0x2050, SPEFloatingPointRound, unknown_exception,  
> > EXC_XFER_EE)
> > +	EXCEPTION(0x2050, SPEFloatingPointRound,  
> > SPEFloatingPointException_Round, EXC_XFER_EE)
> >
> >  	/* Performance Monitor */
> >  	EXCEPTION(0x2060, PerformanceMonitor,  
> > performance_monitor_exception, EXC_XFER_STD)
> > @@ -840,6 +840,8 @@ load_up_spe:
> >  	oris	r5,r5,MSR_SPE at h
> >  	mtmsr	r5			/* enable use of SPE now */
> >  	isync
> > +	li	r5,(SPEFSCR_FINVE | SPEFSCR_FDBZE | SPEFSCR_FUNFE |  
> > SPEFSCR_FOVFE)
> > +	mtspr   SPRN_SPEFSCR,r5
> 
> We should do this via INIT_THREAD, is there a reason that you 
> want to  
> set these always?

I just thought it's the first time that an SPE instruction is
encountered, so I
enable the exceptions here.

> 
> >  /*
> >   * For SMP, we don't do lazy SPE switching because it just gets too
> >   * horrendously complex, especially when a task switches 
> from one CPU
> > diff --git a/arch/powerpc/kernel/traps.c 
> b/arch/powerpc/kernel/traps.c
> > index 6915b91..30ab0f7 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -986,9 +986,16 @@ #endif /* CONFIG_FSL_BOOKE */
> >  #ifdef CONFIG_SPE
> >  void SPEFloatingPointException(struct pt_regs *regs)
> >  {
> > +	extern int spedata_handler(struct pt_regs *regs);
> >  	unsigned long spefscr;
> >  	int fpexc_mode;
> >  	int code = 0;
> > +	int err;
> > +	
> > +	preempt_disable();
> > +	if (regs->msr & MSR_SPE)
> > +		giveup_spe(current);
> > +	preempt_enable();
> 
> use flush_spe_to_thread(current);

OK. It works, and I'll change it.

> 
> >
> >  	spefscr = current->thread.spefscr;
> >  	fpexc_mode = current->thread.fpexc_mode;
> > @@ -1013,9 +1020,55 @@ void SPEFloatingPointException(struct pt
> >  		code = FPE_FLTRES;
> >
> >  	current->thread.spefscr = spefscr;
> > +	err = spedata_handler(regs);
> > +	if (err == 0) {
> > +		regs->nip += 4;		/* skip emulated instruction */
> > +		emulate_single_step(regs);
> > +		return;
> > +	}
> 
> Take a look at the path I put up that reworks the error 
> handling from  
> do_mathemu() we need to be doing something similar (in parsing  
> spefscr/fpexc_mode to setup code properly)

It's new in trap.c? I'll look into it.


> 
> > -	_exception(SIGFPE, regs, code, regs->nip);
> > -	return;
> > +	if (err == -EFAULT) {
> > +		/* got an error reading the instruction */
> > +		_exception(SIGSEGV, regs, SEGV_ACCERR, regs->nip);
> > +	} else if (err == -EINVAL) {
> > +		/* didn't recognize the instruction */
> > +		printk(KERN_ERR "unrecognized spe instruction "
> > +		       "in %s at %lx\n", current->comm, regs->nip);
> 
> We should probably just SIGSEGV in this case since will never make  
> forward progress once we hit this case.

Won't it hit the case that the instruction is not an spe instruction?


> >
> > diff --git a/arch/powerpc/math-emu/Makefile 
> b/arch/powerpc/math-emu/ 
> > Makefile
> > index 29bc912..2da11ba 100644
> > --- a/arch/powerpc/math-emu/Makefile
> > +++ b/arch/powerpc/math-emu/Makefile
> > @@ -1,16 +1,29 @@
> >
> > -obj-y				:= math.o fmr.o lfd.o stfd.o
> > -
> > -obj-$(CONFIG_MATH_EMULATION)	+= fabs.o fadd.o 
> fadds.o fcmpo.o  
> > fcmpu.o \
> > +obj-y				:= fabs.o fneg.o 
> types.o udivmodti4.o
> 
> This isn't right, we don't want to always build these files.
> 
fabs.o/fneg.o should be built in the case
that CONFIG_MATH_EMULATION or CONFIG_SPE or both are
enabled. Because I reused fabs.c and fneg.c to implement instruction
efdabs and
efdneg. Therefore, these two files should be build once we have chance
to build the files in directory math-emu. Types.c and udivmodti4.c
contain the function
that both math emulation instructions and spe instructions may call, so
they always need
to be built.

B.R
Ebony