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

[Tom Rini]

On Thu, Apr 07, 2005 at 10:18:29PM +0200, Joakim Tjernlund wrote:
> > 
> > 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.
> 
> Yes, I don't want the patch applied as is. This is just how the patch
> progressed as I wen't along. I wan't to hash out what to do next
> before messing with files outside head_8xx.S.
> Should it be configurable or not?

My 2 cents is yes, under advanced options, to disable the fixup.  We can
later judge if there's really been an impact and go from there.

> > [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).
> 
> Not sure. -1 is more "logical" than 0xf0, but 0xf0 saves an instruction
> in the DTLB handlers. Comments welcome.

Less instructions the better.

> > [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).
> 
> NP.
> 
> > - Is there (aside from "eww, self modifying code") a good reason to have
> >   NO_SELF_MODIFYING_CODE ?
> 
> The self modifying code version is smaller, eaiser to maintain and possibly faster.
> I would like to remove the other alternative if acceptable.

Sounds like a plan to me.

> > - 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?).
> 
> I just started out this way and added user space later, the KERNEL_SPACE_ONLY
> should be removed.

ok.

> we don't avoid all of these instructions in kernel space, but the bigger point is that we don't
> need to avoid them if we do this workaround instead. Then 8xx can use the same code as the rest of
> the PPC CPU:s
> 
> I am just showing the options with these defines and I hope we can agree on
> removing some or all of these choises.
> 
> Before I invest any more time on this, is this something we want in the kernel?

Yes, I think it is.

> BTW, have you tested the patch? Does it work if you remove the 8xx special handling in
> copy_tofrom_user()?

I have not.  I've been very busy, and leave for 2 weeks vacation Monday.

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