Re: [PATCH] [RFC] workaround buggy dcbX instructions in 8xx

[Tom Rini <trini@kernel.crashing.org>]

On Wed, Apr 06, 2005 at 05:22:57PM +0200, Joakim Tjernlund wrote:

> All cache instructions in 8xx are somewhat buggy as they
> do not update the DAR register when causing a DTLB Miss/Error
[snip]
> ===== head_8xx.S 1.21 vs edited =====
[snip]
> +#define CONFIG_8xx_DCBxFIXED

If this is configurable, it needs to be done in the Kconfig like, well a
real config option.  The arguement for doing it as a config option is
that it is possible to avoid these instructions in userland (I _think_
with a properly configured gcc, all you need to do is remove the
memset.S file from glibc), and avoid the (theoretical) slow-down.

That said, it should also probably be an 'advanced' option that defaults
to the fixup.

[snip]
> +#ifdef CONFIG_8xx_DCBxFIXED
> +/* These macros are used to tag DAR with a known value so that the
> + * DataTLBError can recognize a buggy dcbx instruction and workaround
> + * the problem.
> + */
> +#define TAG_VAL 0x00f0	/*  -1 may also be used */

Is there an advantage of using -1?  If it just "or we could use",
perhaps we should just comment about it, and always define TAG_VAL to
0x00f0 (which will make the rest of the patch a bit cleaner).

[snip]
> +#ifdef CONFIG_8xx_DCBxFIXED
> +/* This is the workaround procedure to calculate the data EA for buggy dcbx,dcbi instructions
> + * by decoding the registers used by the dcbx instruction and adding them.
> + * DAR is set to the calculated address and r10 also holds the EA on exit.
> + */
> +//#define INSTR_CHECK /* define to verify if it is a dcbx instr. Should not be needed. */
> +//#define NO_SELF_MODIFYING_CODE /* define if you don't want to use self modifying code */
> +//#define DEBUG_DCBX_INSTRUCTIONS /* for debugging only. Needs INSTR_CHECK defined as well. */
> +//#define KERNEL_SPACE_ONLY /* define if user space do NOT contain dcbx instructions. */

Aside from preferring #undef FOO to /* #define FOO *//* Comment */ and
detesting // comments:
- Perhaps just one debug symbol (combine INSTR_CHECK and
  DEBUG_DCBX_INSTRUCTIONS).
- Is there (aside from "eww, self modifying code") a good reason to have
  NO_SELF_MODIFYING_CODE ?
- Since today, IIRC, we avoid these instructions in the kernel anyhow,
  is there a reason for KERNEL_SPACE_ONLY ?  In my mind at least, I
  could see a why for userspace-only, but would think an all/nothing
  approach is probably most sane (if you can avoid in space A why not
  B?).

-- 
Tom Rini
http://gate.crashing.org/~trini/