Re: CVS commit: [matt-nb5-mips64] src/sys/arch/mips/mips

[Simon Burge <simonb%NetBSD.org@localhost>]

Hi Cliff,

A couple of things with these changes:

> Module Name:    src
> Committed By:   cliff
> Date:           Thu Jun 10 00:32:11 UTC 2010
> 
> Modified Files:
> 
>         src/sys/arch/mips/include [matt-nb5-mips64]: locore.h
> 
> Log Message:
> 
> - add lsw_bus_error to struct locoresw, provides hook to call
> for chip-specific bus error handling/decode from e.g. trap()

and

> Module Name:  src
> Committed By: cliff
> Date:         Thu Jun 10 00:33:51 UTC 2010
> 
> Modified Files:
> 
>       src/sys/arch/mips/mips [matt-nb5-mips64]: trap.c
> 
> Log Message:
> 
> - in trap(), if traptype is bus error, call chip-specific bus error
> handler in locoresw: (*mips_locoresw.lsw_bus_error)(cause)

1: It's not obvious to me if you're trying to provide for a replacement
bus error handler (as the commit seems to imply) or an "assistant" to
the current bus error handler (which is what the code does).

2: With:

        if ((TRAPTYPE(cause) == 6) || (TRAPTYPE(cause) == 7))
                (void)(*mips_locoresw.lsw_bus_error)(cause);

please please avoid magic numbers - the intent of this isn't obvious at
all.

3: It appears that only sbmips actually sets the lsw_bus_error handler,
so a bus error on any other arch would NULL-deference and panic?

4: With:

#ifdef MIPS3_PLUS
#define TRAPTYPE(x) (((x) & MIPS3_CR_EXC_CODE) >> MIPS_CR_EXC_CODE_SHIFT)
#else
#define TRAPTYPE(x) (((x) & MIPS1_CR_EXC_CODE) >> MIPS_CR_EXC_CODE_SHIFT)
#endif

This looks like it assumes MIPS1 or MIPS3+ at compile time, but we can
have one kernel that can run on both.  This needs to be a runtime thing.
Maybe create a macro/inline function to extract the EXC part of a cause
reg in mips/include/cpureg.h too?

5: Is this worth generalising?  Someone might want to add other CPU
specific trap error handlers so it might be better doing something like:

        if (mips_locoresw.lsw_trap_error)
                (void)(*mips_locoresw.lsw_trap_error)(status, cause, vaddr, 
opc);

and letting the handler determine which exception codes to deal with.
This isn't in time critical code (it either panics or drops to ddb/kgdb)
to the if () check doesn't hurt.  This would also fix 3 above.

Cheers,
Simon.